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@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. I trust you know what you are doing copying the remapped view from plane_state to linked_state, so Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
