| Message ID | 20230105174001.2.Ic07cba4ab9a7bd3618a9e4258b8f92ea7d10ae5a@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable" | expand |
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)
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 --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.
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(-)