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@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxxxx> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx> > > --- > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
