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

From: "Michel Dänzer" <michel.daenzer@mailbox.org>
To: "Brian Norris" <briannorris@chromium.org>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Sean Paul" <seanpaul@chromium.org>
Cc: "kernelci.org bot" <bot@kernelci.org>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Sandy Huang <hjc@rock-chips.com>,
	linux-rockchip@lists.infradead.org, stable@vger.kernel.org
Subject: Re: [PATCH 2/2] drm/rockchip: vop: Leave vblank enabled in self-refresh
Date: Fri, 6 Jan 2023 12:42:54 +0100	[thread overview]
Message-ID: <9455bc5b-2074-4f48-71a7-5c816ee19a78@mailbox.org> (raw)
In-Reply-To: <20230105174001.2.Ic07cba4ab9a7bd3618a9e4258b8f92ea7d10ae5a@changeid>

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)

-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer