Re: [PATCH] drm/i915: Detect invalid scanout pitches

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 19, 2013 at 04:50:34PM +0100, Chris Wilson wrote:
> Report back the user error of attempting to setup a CRTC with an invalid
> framebuffer pitch. This is trickier than it should be as on gen4, there
> is a restriction that tiled surfaces must have a stride less than 16k -
> which is less than the largest supported CRTC size.
> 
> v2: Fix the limits for gen3
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=65099
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 39e7b1b..68cab9f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1871,6 +1871,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  	struct drm_i915_gem_object *obj;
>  	int plane = intel_crtc->plane;
>  	unsigned long linear_offset;
> +	int pitch_limit;
>  	u32 dspcntr;
>  	u32 reg;
>  
> @@ -1886,6 +1887,27 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  	intel_fb = to_intel_framebuffer(fb);
>  	obj = intel_fb->obj;
>  
> +	if (IS_VLV(dev)) {

That won't compile.

> +		pitch_limit = 32*1024;
> +	} else if (IS_GEN4(dev)) {
> +		if (obj->tiling_mode)
> +			pitch_limit = 16*1024;
> +		else
> +			pitch_limit = 32*1024;
> +	} else if (IS_GEN3(dev)) {
> +		if (obj->tiling_mode)
> +			pitch_limit = 8*1024;
> +		else
> +			pitch_limit = 16*1024;
> +	} else
> +		pitch_limit = 8*1024;
> +
> +
> +	if (fb->pitches[0] > pitch_limit) {
> +		DRM_DEBUG_KMS("Invalid pitch (%d) for scanout\n", fb->pitches[0]);
> +		return -EINVAL;
> +	}

We already have a bit of pitch checking in intel_framebuffer_init().
In fact there's a FIXME about pre-ilk limits there.

Assuming all the planes on a specific piece of hardware have the same
pitch limits, I'd like the checks to be live in
intel_framebuffer_init() so that the issue gets caught as early as
possible. For stricter per-plane limits we obviously need the checks
in update_plane.

What I can gather from BSpec is this:
gen2: linear/tiled 8k, (maybe DSPC tiled max 4k?)
gen3: linear ?, tiled 8k
gen4: linear ?, tiled 16k
ctg: linear ?, tiled 16k
ilk+: 32k all the way

Looking at your patch you have 16k,32k,32k for the ?s in my list.
Otherwise your numbers seem to agree with my findings.

So, to me it looks like all the planes do share the same limits (DSPC on
gen2 might be a minor exception), so I think we could move all of these
checks to intel_framebuffer_init().

> +
>  	reg = DSPCNTR(plane);
>  	dspcntr = I915_READ(reg);
>  	/* Mask out pixel format bits in case we change it */
> @@ -1983,6 +2005,11 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> +	if (fb->pitches[0] > 32*1024) {
> +		DRM_DEBUG_KMS("Invalid pitch (%d) for scanout\n", fb->pitches[0]);
> +		return -EINVAL;
> +	}
> +
>  	intel_fb = to_intel_framebuffer(fb);
>  	obj = intel_fb->obj;
>  
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC