diff mbox series

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

Message ID 20200110183228.8199-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/6] drm/i915: Make a copy of the ggtt view for slave plane | expand

Commit Message

Ville Syrjälä Jan. 10, 2020, 6:32 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@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@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@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@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Chris Wilson Jan. 10, 2020, 6:54 p.m. UTC | #1 
Quoting Ville Syrjala (2020-01-10 18:32:23)
> From: Ville Syrjälä <ville.syrjala@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@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@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@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()?

What happens for equivalent of intel_plane_needs_remap()?
-Chris
Ville Syrjälä Jan. 13, 2020, 1:15 p.m. UTC | #2 
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@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@chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst@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@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.
Chris Wilson Jan. 13, 2020, 1:22 p.m. UTC | #3 
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@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@chris-wilson.co.uk>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@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@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@chris-wilson.co.uk>
-Chris
Juha-Pekka Heikkila Jan. 13, 2020, 2:54 p.m. UTC | #4 
On 10.1.2020 20.32, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@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@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));
>   
> 

This fixes also https://gitlab.freedesktop.org/drm/intel/issues/199

Though, that is listed also for older platforms but I see it happening 
only on icl/tgl.

/Juha-Pekka
diff mbox series

Patch

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));