[Intel-gfx] [PATCH 1/6] drm/i915: Make a copy of the ggtt view for slave plane

[Chris Wilson]

Quoting Ville Syrjälä (2020-01-13 13:15:16)
> On Fri, Jan 10, 2020 at 06:54:13PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2020-01-10 18:32:23)
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > intel_prepare_plane_fb() will always pin plane_state->hw.fb whenever
> > > it is present. We copy that from the master plane to the slave plane,
> > > but we fail to copy the corresponding ggtt view. Thus when it comes time
> > > to pin the slave plane's fb we use some stale ggtt view left over from
> > > the last time the plane was used as a non-slave plane. If that previous
> > > use involved 90/270 degree rotation or remapping we'll try to shuffle
> > > the pages of the new fb around accordingingly. However the new
> > > fb may be backed by a bo with less pages than what the ggtt view
> > > rotation/remapped info requires, and so we we trip a GEM_BUG().
> > > 
> > > Steps to reproduce on icl:
> > > 1. plane 1: whatever
> > >    plane 6: largish !NV12 fb + 90 degree rotation
> > > 2. plane 1: smallish NV12 fb
> > >    plane 6: make invisible so it gets slaved to plane 1
> > > 3. GEM_BUG()
> > > 
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/951
> > > Fixes: 1f594b209fe1 ("drm/i915: Remove special case slave handling during hw programming, v3.")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 59c375879186..fafb67689dee 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -12366,6 +12366,7 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > >                 /* Copy parameters to slave plane */
> > >                 linked_state->ctl = plane_state->ctl | PLANE_CTL_YUV420_Y_PLANE;
> > >                 linked_state->color_ctl = plane_state->color_ctl;
> > > +               linked_state->view = plane_state->view;
> > >                 memcpy(linked_state->color_plane, plane_state->color_plane,
> > >                        sizeof(linked_state->color_plane));
> > 
> > So this bit is just copying across the results of
> > intel_plane_compute_gtt()?
> 
> Yep. Actually we copied some of it already (.color_plane[])
> but this part was missing.
> 
> > 
> > What happens for equivalent of intel_plane_needs_remap()?
> 
> The master plane makes the remap vs. not decision and fills
> .color_plane[] and .view accordingly for both chroma and luma.
> Though at the moment intel_plane_needs_remap() is a bit
> incomplete as it only considers the luma stride limit. The
> chroma stride limit is not really documented (at least on
> pre-icl) and I've been too lazy to reverse engineer it.

I trust you know what you are doing copying the remapped view from
plane_state to linked_state, so

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris