[PATCH 1/3] drm/i915; Protect intel_gen4_compute_offset_xtiled() against unknown pixel format

[PATCH 1/3] drm/i915; Protect intel_gen4_compute_offset_xtiled() against unknown pixel format

[Chris Wilson]

If the pixel format is not known to the core helpers, it fills in the
fb->bits_per_pixel field as 0. This causes a fatal divide-by-zero OOPS
when we then try to calculate the tiled offset. This would not be a
problem, but that the core helpers do not know about the YUV planar
formats we use for sprites.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8986172..de09f47 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2019,6 +2019,9 @@ unsigned long intel_gen4_compute_offset_xtiled(int *x, int *y,
 {
 	int tile_rows, tiles;
 
+	if (bpp == 0) /* XXX unknown pixel format! */
+		return 0;
+
 	tile_rows = *y / 8;
 	*y %= 8;
 	tiles = *x / (512/bpp);
-- 
1.7.10.4

[PATCH 2/3] drm/i915: The sprite scaler on Ironlake also support YUV planes

[Chris Wilson]

This fixes a regression from

commit 57779d06367a915ee03e6cb918d7575f0a46e419
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Wed Oct 31 17:50:14 2012 +0200

    drm/i915: Fix display pixel format handling

(which even says that they are supported on Ironlake, and then promptly
rejects then...)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index de09f47..211a097 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8332,7 +8332,7 @@ int intel_framebuffer_init(struct drm_device *dev,
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_YVYU:
 	case DRM_FORMAT_VYUY:
-		if (INTEL_INFO(dev)->gen < 6)
+		if (INTEL_INFO(dev)->gen < 5)
 			return -EINVAL;
 		break;
 	default:
-- 
1.7.10.4

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

[PATCH 3/3] drm/i915: Add DEBUG messages to all intel_create_user_framebuffer error paths

[Chris Wilson]

This proves to be very useful when investigating why code suddenly
started failing.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 211a097..50d6580 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8293,19 +8293,30 @@ int intel_framebuffer_init(struct drm_device *dev,
 {
 	int ret;
 
-	if (obj->tiling_mode == I915_TILING_Y)
+	if (obj->tiling_mode == I915_TILING_Y) {
+		DRM_DEBUG("hardware does not support tiling Y\n");
 		return -EINVAL;
+	}
 
-	if (mode_cmd->pitches[0] & 63)
+	if (mode_cmd->pitches[0] & 63) {
+		DRM_DEBUG("pitch (%d) must be at least 64 byte aligned\n",
+			  mode_cmd->pitches[0]);
 		return -EINVAL;
+	}
 
 	/* FIXME <= Gen4 stride limits are bit unclear */
-	if (mode_cmd->pitches[0] > 32768)
+	if (mode_cmd->pitches[0] > 32768) {
+		DRM_DEBUG("pitch (%d) must be at less than 32768\n",
+			  mode_cmd->pitches[0]);
 		return -EINVAL;
+	}
 
 	if (obj->tiling_mode != I915_TILING_NONE &&
-	    mode_cmd->pitches[0] != obj->stride)
+	    mode_cmd->pitches[0] != obj->stride) {
+		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
+			  mode_cmd->pitches[0], obj->stride);
 		return -EINVAL;
+	}
 
 	/* Reject formats not supported by any plane early. */
 	switch (mode_cmd->pixel_format) {
@@ -8316,8 +8327,10 @@ int intel_framebuffer_init(struct drm_device *dev,
 		break;
 	case DRM_FORMAT_XRGB1555:
 	case DRM_FORMAT_ARGB1555:
-		if (INTEL_INFO(dev)->gen > 3)
+		if (INTEL_INFO(dev)->gen > 3) {
+			DRM_DEBUG("invalid format: 0x%08x\n", mode_cmd->pixel_format);
 			return -EINVAL;
+		}
 		break;
 	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_ABGR8888:
@@ -8325,18 +8338,22 @@ int intel_framebuffer_init(struct drm_device *dev,
 	case DRM_FORMAT_ARGB2101010:
 	case DRM_FORMAT_XBGR2101010:
 	case DRM_FORMAT_ABGR2101010:
-		if (INTEL_INFO(dev)->gen < 4)
+		if (INTEL_INFO(dev)->gen < 4) {
+			DRM_DEBUG("invalid format: 0x%08x\n", mode_cmd->pixel_format);
 			return -EINVAL;
+		}
 		break;
 	case DRM_FORMAT_YUYV:
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_YVYU:
 	case DRM_FORMAT_VYUY:
-		if (INTEL_INFO(dev)->gen < 5)
+		if (INTEL_INFO(dev)->gen < 5) {
+			DRM_DEBUG("invalid format: 0x%08x\n", mode_cmd->pixel_format);
 			return -EINVAL;
+		}
 		break;
 	default:
-		DRM_DEBUG_KMS("unsupported pixel format 0x%08x\n", mode_cmd->pixel_format);
+		DRM_DEBUG("unsupported pixel format 0x%08x\n", mode_cmd->pixel_format);
 		return -EINVAL;
 	}
 
-- 
1.7.10.4

Re: [PATCH 3/3] drm/i915: Add DEBUG messages to all intel_create_user_framebuffer error paths

[Ville Syrjälä]

On Tue, Dec 18, 2012 at 10:13:14PM +0000, Chris Wilson wrote:
> This proves to be very useful when investigating why code suddenly
> started failing.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 211a097..50d6580 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8293,19 +8293,30 @@ int intel_framebuffer_init(struct drm_device *dev,
>  {
>  	int ret;
>  
> -	if (obj->tiling_mode == I915_TILING_Y)
> +	if (obj->tiling_mode == I915_TILING_Y) {
> +		DRM_DEBUG("hardware does not support tiling Y\n");

Shouldn't all of these be DRM_DEBUG_KMS()?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: Add DEBUG messages to all intel_create_user_framebuffer error paths

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 1158 bytes --]

On Wed, 19 Dec 2012 13:47:41 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Dec 18, 2012 at 10:13:14PM +0000, Chris Wilson wrote:
> > This proves to be very useful when investigating why code suddenly
> > started failing.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   33 +++++++++++++++++++++++++--------
> >  1 file changed, 25 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 211a097..50d6580 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8293,19 +8293,30 @@ int intel_framebuffer_init(struct drm_device *dev,
> >  {
> >  	int ret;
> >  
> > -	if (obj->tiling_mode == I915_TILING_Y)
> > +	if (obj->tiling_mode == I915_TILING_Y) {
> > +		DRM_DEBUG("hardware does not support tiling Y\n");
> 
> Shouldn't all of these be DRM_DEBUG_KMS()?

I choose DRM_DEBUG because these are user errors, which I think has been
the prevailing choice in the past.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/3] drm/i915: Add DEBUG messages to all intel_create_user_framebuffer error paths

[Ville Syrjälä]

On Wed, Dec 19, 2012 at 11:52:11AM +0000, Chris Wilson wrote:
> On Wed, 19 Dec 2012 13:47:41 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Dec 18, 2012 at 10:13:14PM +0000, Chris Wilson wrote:
> > > This proves to be very useful when investigating why code suddenly
> > > started failing.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   33 +++++++++++++++++++++++++--------
> > >  1 file changed, 25 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 211a097..50d6580 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8293,19 +8293,30 @@ int intel_framebuffer_init(struct drm_device *dev,
> > >  {
> > >  	int ret;
> > >  
> > > -	if (obj->tiling_mode == I915_TILING_Y)
> > > +	if (obj->tiling_mode == I915_TILING_Y) {
> > > +		DRM_DEBUG("hardware does not support tiling Y\n");
> > 
> > Shouldn't all of these be DRM_DEBUG_KMS()?
> 
> I choose DRM_DEBUG because these are user errors, which I think has been
> the prevailing choice in the past.

drm_crtc.c uses DRM_DEBUG_KMS for user errors too.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/3] drm/i915: The sprite scaler on Ironlake also support YUV planes

[Ville Syrjälä]

On Tue, Dec 18, 2012 at 10:13:13PM +0000, Chris Wilson wrote:
> This fixes a regression from
> 
> commit 57779d06367a915ee03e6cb918d7575f0a46e419
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Wed Oct 31 17:50:14 2012 +0200
> 
>     drm/i915: Fix display pixel format handling
> 
> (which even says that they are supported on Ironlake, and then promptly
> rejects then...)

Sorry about that. I do remember changing the condition when changing the
commit message, but I must have made an error when resolving conflicts
during one of the rebases the patch went through.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index de09f47..211a097 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8332,7 +8332,7 @@ int intel_framebuffer_init(struct drm_device *dev,
>  	case DRM_FORMAT_UYVY:
>  	case DRM_FORMAT_YVYU:
>  	case DRM_FORMAT_VYUY:
> -		if (INTEL_INFO(dev)->gen < 6)
> +		if (INTEL_INFO(dev)->gen < 5)
>  			return -EINVAL;
>  		break;
>  	default:
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/3] drm/i915: The sprite scaler on Ironlake also support YUV planes

[Daniel Vetter]

On Wed, Dec 19, 2012 at 02:00:59PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 18, 2012 at 10:13:13PM +0000, Chris Wilson wrote:
> > This fixes a regression from
> > 
> > commit 57779d06367a915ee03e6cb918d7575f0a46e419
> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Date:   Wed Oct 31 17:50:14 2012 +0200
> > 
> >     drm/i915: Fix display pixel format handling
> > 
> > (which even says that they are supported on Ironlake, and then promptly
> > rejects then...)
> 
> Sorry about that. I do remember changing the condition when changing the
> commit message, but I must have made an error when resolving conflicts
> during one of the rebases the patch went through.
> 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Applied this one to -fixes. Still in limbo about all the other ones ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915; Protect intel_gen4_compute_offset_xtiled() against unknown pixel format

[Daniel Vetter]

On Tue, Dec 18, 2012 at 10:13:12PM +0000, Chris Wilson wrote:
> If the pixel format is not known to the core helpers, it fills in the
> fb->bits_per_pixel field as 0. This causes a fatal divide-by-zero OOPS
> when we then try to calculate the tiled offset. This would not be a
> problem, but that the core helpers do not know about the YUV planar
> formats we use for sprites.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Do we still need this one with the s/fb->bits_per_pixel/pixel_size/ change
in one of the later patches?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8986172..de09f47 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2019,6 +2019,9 @@ unsigned long intel_gen4_compute_offset_xtiled(int *x, int *y,
>  {
>  	int tile_rows, tiles;
>  
> +	if (bpp == 0) /* XXX unknown pixel format! */
> +		return 0;
> +
>  	tile_rows = *y / 8;
>  	*y %= 8;
>  	tiles = *x / (512/bpp);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915: Add DEBUG messages to all intel_create_user_framebuffer error paths

[Daniel Vetter]

On Tue, Dec 18, 2012 at 10:13:14PM +0000, Chris Wilson wrote:
> This proves to be very useful when investigating why code suddenly
> started failing.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Picked up for -fixes, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915; Protect intel_gen4_compute_offset_xtiled() against unknown pixel format

[Chris Wilson]

On Tue, 8 Jan 2013 12:13:50 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Dec 18, 2012 at 10:13:12PM +0000, Chris Wilson wrote:
> > If the pixel format is not known to the core helpers, it fills in the
> > fb->bits_per_pixel field as 0. This causes a fatal divide-by-zero OOPS
> > when we then try to calculate the tiled offset. This would not be a
> > problem, but that the core helpers do not know about the YUV planar
> > formats we use for sprites.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Do we still need this one with the s/fb->bits_per_pixel/pixel_size/ change
> in one of the later patches?

I don't believe so, we fixed up the only known issue in the sprite code,
and for the time being we only have rgb primary display planes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre