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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel