Re: [PATCH 2/2] drm/amd/display: use drm_crtc_set_vblank_offdelay()

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Tue, Jul 09, 2024 at 11:32:11AM +0200, Daniel Vetter wrote:
> On Mon, Jul 08, 2024 at 04:29:07PM -0400, Hamza Mahfooz wrote:
> > Hook up drm_crtc_set_vblank_offdelay() in amdgpu_dm, so that we can
> > enable PSR more quickly for displays that support it.
> > 
> > Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 30 ++++++++++++++-----
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index fdbc9b57a23d..ee6c31e9d3c4 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -8231,7 +8231,7 @@ static int amdgpu_dm_encoder_init(struct drm_device *dev,
> >  
> >  static void manage_dm_interrupts(struct amdgpu_device *adev,
> >  				 struct amdgpu_crtc *acrtc,
> > -				 bool enable)
> > +				 struct dm_crtc_state *acrtc_state)
> >  {
> >  	/*
> >  	 * We have no guarantee that the frontend index maps to the same
> > @@ -8239,12 +8239,25 @@ static void manage_dm_interrupts(struct amdgpu_device *adev,
> >  	 *
> >  	 * TODO: Use a different interrupt or check DC itself for the mapping.
> >  	 */
> > -	int irq_type =
> > -		amdgpu_display_crtc_idx_to_irq_type(
> > -			adev,
> > -			acrtc->crtc_id);
> > +	int irq_type = amdgpu_display_crtc_idx_to_irq_type(adev,
> > +							   acrtc->crtc_id);
> > +	struct dc_crtc_timing *timing;
> > +	int offdelay;
> > +
> > +	if (acrtc_state) {
> > +		timing = &acrtc_state->stream->timing;
> > +
> > +		/* at least 2 frames */
> > +		offdelay = 2000 / div64_u64(div64_u64((timing->pix_clk_100hz *
> > +						       (uint64_t)100),
> > +						      timing->v_total),
> > +					    timing->h_total) + 1;
> 
> Yeah, _especially_ when you have a this short timeout your really have to
> instead fix the vblank driver code properly so you can enable
> vblank_disable_immediate. This is just cheating :-)

Michel mentioned on irc that DC had immediate vblank disabling, but this
was reverted with f64e6e0b6afe ("Revert "drm/amdgpu/display: set
vblank_disable_immediate for DC"").

I haven't looked at the details of the bug report, but stuttering is
exactly what happens when the driver's vblank code has these races. Going
for a very low timeout instead of zero just means it's a bit harder to hit
the issue, and much, much harder to debug properly.

So yeah even more reasons to look at the underlying root-cause here I
think.
-Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch