Re: [Intel-gfx] [PATCH 1/2] drm/i915/psr: Use full update In case of area calculation fails

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Fri, 2022-05-06 at 18:40 +0000, Souza, Jose wrote:
> On Fri, 2022-05-06 at 18:28 +0000, Hogander, Jouni wrote:
> > On Fri, 2022-05-06 at 15:29 +0000, Souza, Jose wrote:
> > > On Fri, 2022-05-06 at 08:48 +0300, Jouni Högander wrote:
> > > > Currently we have some corner cases where area calculation
> > > > fails.  For
> > > > these sel fetch are calculation ends up having update area as
> > > > y1 =
> > > > 0,
> > > > y2 = 4. Instead of these values safer option is full update.
> > > 
> > > Aren't you able to reproduce this scenarios with IGT? So why not
> > > probably fix the calculations?
> > 
> > There were some discussion with Ville Syrjälä that the proper fix
> > for
> > this would be to move psr update area calculation into where other
> > calculations for planes are done. Currently we don't have e.g.
> > proper
> > offset information available here. I have this in my tasklist, but
> > been
> > busy with other tracks.
> 
> Okay so please add some of that to the commit description.

Added some of this into commit message in new version, please check.

> 
> > I'm also concerned generally on the first loop possibly ending up
> > with
> > y1=-1,y2=-1 values due to other reasons as well. So using that
> > full_update prevents this posibility completely.
> > 
> > If I forget how I originally found this problem with bigfb I think
> > this
> > backup using full update if something goes wrong is generally a
> > good
> > idea. Currently it's just using y1=0,y2=4.
> > 
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > Tested-by: Mark Pearson <markpearson@lenovo.com>
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_psr.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 06db407e2749..8c099d24de86 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1770,6 +1770,9 @@ int intel_psr2_sel_fetch_update(struct
> > > > intel_atomic_state *state,
> > > >  clip_area_update(&pipe_clip, &damaged_area);
> > > >  }
> > > > 
> 
> Add a TODO and a "drm_DEBUG_ONCE()"(check drm_WARN_ONCE) here so we
> get a warning about this at least once and this is not forgot.

I left the warn out. There is some case during boot-up at least in
Fedora35. I.e. This warning would be there always.

How about if I just file own bug for this in gitlab and assign it to
myself?

> 
> > > > +if (pipe_clip.y1 == -1)
> > > > +full_update = true;
> > > > +
> > > >  if (full_update)
> > > >  goto skip_sel_fetch_set_loop;
> > > >