Re: [PATCH] drm/i915: Check fb stride against plane max stride

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Monday, September 24, 2018 6:31:56 AM PDT Ville Syrjälä wrote:
> On Fri, Sep 21, 2018 at 03:17:45PM -0700, Dhinakaran Pandiyan wrote:
> > On Tuesday, September 18, 2018 7:02:43 AM PDT Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > commit 4e0b83a567e2 ("drm/i915: Extract per-platform plane->check()
> > > functions") removed the plane max stride check for sprite planes.
> > > I was going to add it back when introducing GTT remapping for the
> > > display, but after further thought it seems better to re-introduce
> > > it separately.
> > > 
> > > So let's add the max stride check back. And let's do it in a nicer
> > > form than what we had before and do it for all plane types (easy
> > > now that we have the ->max_stride() plane vfunc).
> > > 
> > > Only sprite planes really need this for now since primary planes
> > > are capable of scanning out the current max fb size we allow, and
> > > cursors have more stringent stride checks elsewhere.
> > > 

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Fixes: 4e0b83a567e2 ("drm/i915: Extract per-platform plane->check()
> > > functions") Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 22 ++++++++++++++++++++++
> > >  3 files changed, 37 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c index eb25037d7b38..1eb99d5ec221
> > > 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3151,6 +3151,10 @@ int skl_check_plane_surface(struct
> > > intel_plane_state
> > > *plane_state) plane_state->color_plane[0].stride = intel_fb_pitch(fb, 0,
> > > rotation); plane_state->color_plane[1].stride = intel_fb_pitch(fb, 1,
> > > rotation);
> > > 
> > > +	ret = intel_plane_check_stride(plane_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > 
> > >  	if (!plane_state->base.visible)
> > >  	
> > >  		return 0;
> > > 
> > > @@ -3286,10 +3290,15 @@ int i9xx_check_plane_surface(struct
> > > intel_plane_state *plane_state) int src_x = plane_state->base.src.x1 >>
> > > 16;
> > > 
> > >  	int src_y = plane_state->base.src.y1 >> 16;
> > >  	u32 offset;
> > > 
> > > +	int ret;
> > > 
> > >  	intel_fill_fb_ggtt_view(&plane_state->view, fb, rotation);
> > >  	plane_state->color_plane[0].stride = intel_fb_pitch(fb, 0, rotation);
> > 
> > Is there a good reason to not  inline the code ?
> > 
> > 	if (color_plane[0].stride > plane->max_stride())
> > 	
> > 		return -EINVAL;
> 
> Consistency. Easier to just call a single function that does things in a
> consistent fashion rather than duplicating the same check everywhere.
> Also keeps the debug print consistent.
> 
> > > +	ret = intel_plane_check_stride(plane_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > 
> > >  	intel_add_fb_offsets(&src_x, &src_y, plane_state, 0);
> > >  	
> > >  	if (INTEL_GEN(dev_priv) >= 4)
> > > 
> > > @@ -9685,10 +9694,15 @@ static int intel_cursor_check_surface(struct
> > > intel_plane_state *plane_state) unsigned int rotation =
> > > plane_state->base.rotation;
> > > 
> > >  	int src_x, src_y;
> > >  	u32 offset;
> > > 
> > > +	int ret;
> > > 
> > >  	intel_fill_fb_ggtt_view(&plane_state->view, fb, rotation);
> > >  	plane_state->color_plane[0].stride = intel_fb_pitch(fb, 0, rotation);
> > > 
> > > +	ret = intel_plane_check_stride(plane_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > 
> > >  	src_x = plane_state->base.src_x >> 16;
> > >  	src_y = plane_state->base.src_y >> 16;
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h index bf1c38728a59..a34c2f1f9159
> > > 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -2140,6 +2140,7 @@ unsigned int skl_plane_max_stride(struct
> > > intel_plane
> > > *plane, unsigned int rotation);
> > > 
> > >  int skl_plane_check(struct intel_crtc_state *crtc_state,
> > >  
> > >  		    struct intel_plane_state *plane_state);
> > > 
> > > +int intel_plane_check_stride(const struct intel_plane_state
> > > *plane_state);
> > > 
> > >  int intel_plane_check_src_coordinates(struct intel_plane_state
> > > 
> > > *plane_state); int chv_plane_check_rotation(const struct
> > > intel_plane_state
> > > *plane_state);
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c index d4c8e10fc90b..5fd2f7bf3927
> > > 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -230,6 +230,28 @@ void intel_pipe_update_end(struct intel_crtc_state
> > > *new_crtc_state) #endif
> > > 
> > >  }
> > > 
> > > +int intel_plane_check_stride(const struct intel_plane_state
> > > *plane_state)
> > > +{
> > > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> > > +	unsigned int rotation = plane_state->base.rotation;
> > > +	u32 stride, max_stride;
> > > +
> > > +	/* FIXME other color planes? */
> > 
> > Doesn't the color plane 0 have the max stride always?
> 
> Each color plane could have a different maximum. Depends on the hw.
> Not that it's actually documented for the skl+ aux surface. Would
> probably need to test what is the max, or just pick a sensible
> limit that still seems to work.
> 
> > > +	stride = plane_state->color_plane[0].stride;
> > > +	max_stride = plane->max_stride(plane, fb->format->format,
> > > +				       fb->modifier, rotation);
> > > +
> > > +	if (stride > max_stride) {
> > > +		DRM_DEBUG_KMS("[FB:%d] stride (%d) exceeds [PLANE:%d:%s] max stride
> > > (%d)\n", +			      fb->base.id, stride,
> > > +			      plane->base.base.id, plane->base.name, max_stride);
> > 
> > Include the color plane number in the debug print?
> 
> I suppose.
> 
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > 
> > >  int intel_plane_check_src_coordinates(struct intel_plane_state
> > > 
> > > *plane_state) {
> > > 
> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx