| Message ID | 20220506054834.2822650-2-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for selective fetch area calculation | expand |
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? > > 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); > } > > + if (pipe_clip.y1 == -1) > + full_update = true; > + > if (full_update) > goto skip_sel_fetch_set_loop; >
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. 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); > > } > > > > +if (pipe_clip.y1 == -1) > > +full_update = true; > > + > > if (full_update) > > goto skip_sel_fetch_set_loop; > >
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. > > 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. > > > +if (pipe_clip.y1 == -1) > > > +full_update = true; > > > + > > > if (full_update) > > > goto skip_sel_fetch_set_loop; > > > >
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; > > > >
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); } + if (pipe_clip.y1 == -1) + full_update = true; + if (full_update) goto skip_sel_fetch_set_loop;