diff mbox series

[2/2] drm/rockchip: vop: Leave vblank enabled in self-refresh

Message ID 20230105174001.2.Ic07cba4ab9a7bd3618a9e4258b8f92ea7d10ae5a@changeid
State New
Headers
Series [1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable" |

Commit Message

Brian Norris Jan. 6, 2023, 1:40 a.m. UTC 
  If we disable vblank when entering self-refresh, vblank APIs (like
DRM_IOCTL_WAIT_VBLANK) no longer work. But user space is not aware when
we enter self-refresh, so this appears to be an API violation -- that
DRM_IOCTL_WAIT_VBLANK fails with EINVAL whenever the display is idle and
enters self-refresh.

The downstream driver used by many of these systems never used to
disable vblank for PSR, and in fact, even upstream, we didn't do that
until radically redesigning the state machine in commit 6c836d965bad
("drm/rockchip: Use the helpers for PSR").

Thus, it seems like a reasonable API fix to simply restore that
behavior, and leave vblank enabled.

Note that this appears to potentially unbalance the
drm_crtc_vblank_{off,on}() calls in some cases, but:
(a) drm_crtc_vblank_on() documents this as OK and
(b) if I do the naive balancing, I find state machine issues such that
    we're not in sync properly; so it's easier to take advantage of (a).

Backporting notes:
Marking as 'Fixes' commit 6c836d965bad ("drm/rockchip: Use the helpers
for PSR"), but it probably depends on commit bed030a49f3e
("drm/rockchip: Don't fully disable vop on self refresh") as well.

We also need the previous patch ("drm/atomic: Allow vblank-enabled +
self-refresh "disable""), of course.

Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/dri-devel/Y5itf0+yNIQa6fU4@sirena.org.uk/
Reported-by: "kernelci.org bot" <bot@kernelci.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Michel Dänzer Jan. 6, 2023, 11:42 a.m. UTC | #1 
On 1/6/23 02:40, Brian Norris wrote:
> If we disable vblank when entering self-refresh, vblank APIs (like
> DRM_IOCTL_WAIT_VBLANK) no longer work. But user space is not aware when
> we enter self-refresh, so this appears to be an API violation -- that
> DRM_IOCTL_WAIT_VBLANK fails with EINVAL whenever the display is idle and
> enters self-refresh.
> 
> The downstream driver used by many of these systems never used to
> disable vblank for PSR, and in fact, even upstream, we didn't do that
> until radically redesigning the state machine in commit 6c836d965bad
> ("drm/rockchip: Use the helpers for PSR").
> 
> Thus, it seems like a reasonable API fix to simply restore that
> behavior, and leave vblank enabled.
> 
> Note that this appears to potentially unbalance the
> drm_crtc_vblank_{off,on}() calls in some cases, but:
> (a) drm_crtc_vblank_on() documents this as OK and
> (b) if I do the naive balancing, I find state machine issues such that
>     we're not in sync properly; so it's easier to take advantage of (a).
> 
> Backporting notes:
> Marking as 'Fixes' commit 6c836d965bad ("drm/rockchip: Use the helpers
> for PSR"), but it probably depends on commit bed030a49f3e
> ("drm/rockchip: Don't fully disable vop on self refresh") as well.
> 
> We also need the previous patch ("drm/atomic: Allow vblank-enabled +
> self-refresh "disable""), of course.
> 
> Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
> Cc: <stable@vger.kernel.org>
> Link: https://lore.kernel.org/dri-devel/Y5itf0+yNIQa6fU4@sirena.org.uk/
> Reported-by: "kernelci.org bot" <bot@kernelci.org>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index fa1f4ee6d195..c541967114b4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -719,11 +719,11 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>  	mutex_lock(&vop->vop_lock);
>  
> -	drm_crtc_vblank_off(crtc);
> -
>  	if (crtc->state->self_refresh_active)
>  		goto out;
>  
> +	drm_crtc_vblank_off(crtc);
> +
>  	/*
>  	 * Vop standby will take effect at end of current frame,
>  	 * if dsp hold valid irq happen, it means standby complete.

The out label immediately unlocks vop->vop_lock again, seems a bit pointless. :)

AFAICT the self_refresh_active case should just return immediately, the out label would also send any pending atomic commit completion event, which seems wrong now that the vblank interrupt is left enabled. (I also wonder if drm_crtc_vblank_off doesn't take care of that already, at least amdgpu doesn't do this explicitly AFAICT)
Brian Norris Jan. 7, 2023, 1:21 a.m. UTC | #2 
On Fri, Jan 06, 2023 at 12:42:54PM +0100, Michel Dänzer wrote:
> On 1/6/23 02:40, Brian Norris wrote:
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -719,11 +719,11 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
> >  
> >  	mutex_lock(&vop->vop_lock);
> >  
> > -	drm_crtc_vblank_off(crtc);
> > -
> >  	if (crtc->state->self_refresh_active)
> >  		goto out;
> >  
> > +	drm_crtc_vblank_off(crtc);
> > +
> >  	/*
> >  	 * Vop standby will take effect at end of current frame,
> >  	 * if dsp hold valid irq happen, it means standby complete.
> 
> The out label immediately unlocks vop->vop_lock again, seems a bit pointless. :)

Oops, of course. Will change that in v2.

> AFAICT the self_refresh_active case should just return immediately,
> the out label would also send any pending atomic commit completion
> event, which seems wrong now that the vblank interrupt is left
> enabled.

If I return immediately and drop that completion, I hit a warning:

[    4.423876] WARNING: CPU: 5 PID: 58 at drivers/gpu/drm/drm_atomic_helper.c:2460 drm_atomic_helper_commit_hw_done+0xe0/0xe8
...
[    4.424036] Call trace:
[    4.424039]  drm_atomic_helper_commit_hw_done+0xe0/0xe8
[    4.424045]  drm_atomic_helper_commit_tail_rpm+0x58/0x7c
...

I plan to leave it as-is for v2.

> (I also wonder if drm_crtc_vblank_off doesn't take care of
> that already, at least amdgpu doesn't do this explicitly AFAICT)

I'm not sure.

Brian
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fa1f4ee6d195..c541967114b4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -719,11 +719,11 @@  static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 
 	mutex_lock(&vop->vop_lock);
 
-	drm_crtc_vblank_off(crtc);
-
 	if (crtc->state->self_refresh_active)
 		goto out;
 
+	drm_crtc_vblank_off(crtc);
+
 	/*
 	 * Vop standby will take effect at end of current frame,
 	 * if dsp hold valid irq happen, it means standby complete.