[PATCH 0/6] drm/i915: Framebuffer layout fixes and sanity checks

[PATCH 0/6] drm/i915: Framebuffer layout fixes and sanity checks

[ville.syrjala]

Add all kinds of framebuffer layout sanity checks to the code.

Also the framebuffer offset wasn't properly handled, and code dealing
with the primary plane pixel format was quite broken.

[PATCH 1/6] drm/i915: Fix display pixel format handling

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Fix support for all RGB/BGR pixel formats (except the 16:16:16:16 float
format).

Fix intel_init_framebuffer() to match hardware and driver limitations:
* RGB332 is not supported at all
* CI8 is supported
* XRGB1555 & co. are supported on Gen3 and earlier
* XRGB210101010 & co. are supported from Gen4 onwards
* BGR formats are supported from Gen4 onwards
* YUV formats are supported from Gen6 onwards (driver limitation)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   17 ++++--
 drivers/gpu/drm/i915/intel_display.c |   98 ++++++++++++++++++++++------------
 2 files changed, 76 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2d49b95..845e5cb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2855,12 +2855,19 @@
 #define   DISPPLANE_GAMMA_ENABLE		(1<<30)
 #define   DISPPLANE_GAMMA_DISABLE		0
 #define   DISPPLANE_PIXFORMAT_MASK		(0xf<<26)
+#define   DISPPLANE_YUV422			(0x0<<26)
 #define   DISPPLANE_8BPP			(0x2<<26)
-#define   DISPPLANE_15_16BPP			(0x4<<26)
-#define   DISPPLANE_16BPP			(0x5<<26)
-#define   DISPPLANE_32BPP_NO_ALPHA		(0x6<<26)
-#define   DISPPLANE_32BPP			(0x7<<26)
-#define   DISPPLANE_32BPP_30BIT_NO_ALPHA	(0xa<<26)
+#define   DISPPLANE_BGRA555			(0x3<<26)
+#define   DISPPLANE_BGRX555			(0x4<<26)
+#define   DISPPLANE_BGRX565			(0x5<<26)
+#define   DISPPLANE_BGRX888			(0x6<<26)
+#define   DISPPLANE_BGRA888			(0x7<<26)
+#define   DISPPLANE_RGBX101010			(0x8<<26)
+#define   DISPPLANE_RGBA101010			(0x9<<26)
+#define   DISPPLANE_BGRX101010			(0xa<<26)
+#define   DISPPLANE_RGBX161616			(0xc<<26)
+#define   DISPPLANE_RGBX888			(0xe<<26)
+#define   DISPPLANE_RGBA888			(0xf<<26)
 #define   DISPPLANE_STEREO_ENABLE		(1<<25)
 #define   DISPPLANE_STEREO_DISABLE		0
 #define   DISPPLANE_SEL_PIPE_SHIFT		24
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ee61ad1..7cf639c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1843,24 +1843,38 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	dspcntr = I915_READ(reg);
 	/* Mask out pixel format bits in case we change it */
 	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
-	switch (fb->bits_per_pixel) {
-	case 8:
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_C8:
 		dspcntr |= DISPPLANE_8BPP;
 		break;
-	case 16:
-		if (fb->depth == 15)
-			dspcntr |= DISPPLANE_15_16BPP;
-		else
-			dspcntr |= DISPPLANE_16BPP;
-		break;
-	case 24:
-	case 32:
-		dspcntr |= DISPPLANE_32BPP_NO_ALPHA;
+	case DRM_FORMAT_XRGB1555:
+	case DRM_FORMAT_ARGB1555:
+		dspcntr |= DISPPLANE_BGRX555;
+		break;
+	case DRM_FORMAT_RGB565:
+		dspcntr |= DISPPLANE_BGRX565;
+		break;
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ARGB8888:
+		dspcntr |= DISPPLANE_BGRX888;
+		break;
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_ABGR8888:
+		dspcntr |= DISPPLANE_RGBX888;
+		break;
+	case DRM_FORMAT_XRGB2101010:
+	case DRM_FORMAT_ARGB2101010:
+		dspcntr |= DISPPLANE_BGRX101010;
+		break;
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_ABGR2101010:
+		dspcntr |= DISPPLANE_RGBX101010;
 		break;
 	default:
-		DRM_ERROR("Unknown color depth %d\n", fb->bits_per_pixel);
+		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
 		return -EINVAL;
 	}
+
 	if (INTEL_INFO(dev)->gen >= 4) {
 		if (obj->tiling_mode != I915_TILING_NONE)
 			dspcntr |= DISPPLANE_TILED;
@@ -1917,27 +1931,31 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
 	dspcntr = I915_READ(reg);
 	/* Mask out pixel format bits in case we change it */
 	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
-	switch (fb->bits_per_pixel) {
-	case 8:
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_C8:
 		dspcntr |= DISPPLANE_8BPP;
 		break;
-	case 16:
-		if (fb->depth != 16)
-			return -EINVAL;
-
-		dspcntr |= DISPPLANE_16BPP;
-		break;
-	case 24:
-	case 32:
-		if (fb->depth == 24)
-			dspcntr |= DISPPLANE_32BPP_NO_ALPHA;
-		else if (fb->depth == 30)
-			dspcntr |= DISPPLANE_32BPP_30BIT_NO_ALPHA;
-		else
-			return -EINVAL;
+	case DRM_FORMAT_RGB565:
+		dspcntr |= DISPPLANE_BGRX565;
+		break;
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ARGB8888:
+		dspcntr |= DISPPLANE_BGRX888;
+		break;
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_ABGR8888:
+		dspcntr |= DISPPLANE_RGBX888;
+		break;
+	case DRM_FORMAT_XRGB2101010:
+	case DRM_FORMAT_ARGB2101010:
+		dspcntr |= DISPPLANE_BGRX101010;
+		break;
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_ABGR2101010:
+		dspcntr |= DISPPLANE_RGBX101010;
 		break;
 	default:
-		DRM_ERROR("Unknown color depth %d\n", fb->bits_per_pixel);
+		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
 		return -EINVAL;
 	}
 
@@ -6638,24 +6656,36 @@ int intel_framebuffer_init(struct drm_device *dev,
 	if (mode_cmd->pitches[0] & 63)
 		return -EINVAL;
 
+	/* Reject formats not supported by any plane early. */
 	switch (mode_cmd->pixel_format) {
-	case DRM_FORMAT_RGB332:
+	case DRM_FORMAT_C8:
 	case DRM_FORMAT_RGB565:
 	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_ARGB8888:
+		break;
+	case DRM_FORMAT_XRGB1555:
+	case DRM_FORMAT_ARGB1555:
+		if (INTEL_INFO(dev)->gen > 3)
+			return -EINVAL;
+		break;
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_ABGR8888:
 	case DRM_FORMAT_XRGB2101010:
 	case DRM_FORMAT_ARGB2101010:
-		/* RGB formats are common across chipsets */
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_ABGR2101010:
+		if (INTEL_INFO(dev)->gen < 4)
+			return -EINVAL;
 		break;
 	case DRM_FORMAT_YUYV:
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_YVYU:
 	case DRM_FORMAT_VYUY:
+		if (INTEL_INFO(dev)->gen < 6)
+			return -EINVAL;
 		break;
 	default:
-		DRM_DEBUG_KMS("unsupported pixel format %u\n",
-				mode_cmd->pixel_format);
+		DRM_DEBUG_KMS("unsupported pixel format 0x%08x\n", mode_cmd->pixel_format);
 		return -EINVAL;
 	}
 
-- 
1.7.3.4

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

[PATCH 2/6] drm/i915: Check framebuffer stride more thoroughly

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make sure the the framebuffer stride is smaller than the maximum
accepted by any plane.

Also when using a tiled memory make sure the object stride matches
the framebuffer stride.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7cf639c..8fea475 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6643,6 +6643,17 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
 	.create_handle = intel_user_framebuffer_create_handle,
 };
 
+static unsigned int intel_max_fb_stride(const struct drm_device *dev)
+{
+	/* FIXME: BSpec for pre-Gen5 is a bit unclear on stride limits */
+	if (INTEL_INFO(dev)->gen <= 3)
+		return 8192;
+	else if (INTEL_INFO(dev)->gen <= 4)
+		return 16384;
+	else
+		return 32768;
+}
+
 int intel_framebuffer_init(struct drm_device *dev,
 			   struct intel_framebuffer *intel_fb,
 			   struct drm_mode_fb_cmd2 *mode_cmd,
@@ -6656,6 +6667,13 @@ int intel_framebuffer_init(struct drm_device *dev,
 	if (mode_cmd->pitches[0] & 63)
 		return -EINVAL;
 
+	if (mode_cmd->pitches[0] > intel_max_fb_stride(dev))
+		return -EINVAL;
+
+	if (obj->tiling_mode != I915_TILING_NONE &&
+	    mode_cmd->pitches[0] != obj->stride)
+		return -EINVAL;
+
 	/* Reject formats not supported by any plane early. */
 	switch (mode_cmd->pixel_format) {
 	case DRM_FORMAT_C8:
-- 
1.7.3.4

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

[PATCH 3/6] drm/i915: Zero initialize mode_cmd

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Zero initialize the mode_cmd structure when creating the kernel
framebuffer. Avoids having uninitialized data in offsets[0] for
instance.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_fb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index bf86907..07404ac 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -65,7 +65,7 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct fb_info *info;
 	struct drm_framebuffer *fb;
-	struct drm_mode_fb_cmd2 mode_cmd;
+	struct drm_mode_fb_cmd2 mode_cmd = {};
 	struct drm_i915_gem_object *obj;
 	struct device *device = &dev->pdev->dev;
 	int size, ret;
-- 
1.7.3.4

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

[PATCH 4/6] drm/i915: Check the framebuffer offset

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The framebuffer offset must be aligned to (macro)pixel size.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8fea475..9df15ee 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6660,6 +6660,7 @@ int intel_framebuffer_init(struct drm_device *dev,
 			   struct drm_i915_gem_object *obj)
 {
 	int ret;
+	unsigned int align = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
 
 	if (obj->tiling_mode == I915_TILING_Y)
 		return -EINVAL;
@@ -6699,6 +6700,7 @@ int intel_framebuffer_init(struct drm_device *dev,
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_YVYU:
 	case DRM_FORMAT_VYUY:
+		align <<= 1;
 		if (INTEL_INFO(dev)->gen < 6)
 			return -EINVAL;
 		break;
@@ -6707,6 +6709,9 @@ int intel_framebuffer_init(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	if (mode_cmd->offsets[0] % align)
+		return -EINVAL;
+
 	ret = drm_framebuffer_init(dev, &intel_fb->base, &intel_fb_funcs);
 	if (ret) {
 		DRM_ERROR("framebuffer init failed %d\n", ret);
-- 
1.7.3.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] drm/i915: Handle framebuffer offsets[]

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Take fb->offset[0] into account when calculating the linear and tile x/y
offsets.

For non-tiled surfaces fb->offset[0] is simply added to the linear
byte offset.

For tiled surfaces treat fb->offsets[0] as a byte offset into the
linearized view of the surface. So we end up converting fb->offsets[0]
into additional x and y offsets.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9df15ee..f4338cb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1826,6 +1826,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	unsigned long Start, Offset;
 	u32 dspcntr;
 	u32 reg;
+	unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 
 	switch (plane) {
 	case 0:
@@ -1885,12 +1886,14 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	I915_WRITE(reg, dspcntr);
 
 	Start = obj->gtt_offset;
-	Offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
+	Offset = fb->offsets[0] + y * fb->pitches[0] + x * cpp;
 
 	DRM_DEBUG_KMS("Writing base %08lX %08lX %d %d %d\n",
 		      Start, Offset, x, y, fb->pitches[0]);
 	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
 	if (INTEL_INFO(dev)->gen >= 4) {
+		y += fb->offsets[0] / fb->pitches[0];
+		x += fb->offsets[0] % fb->pitches[0] / cpp;
 		I915_MODIFY_DISPBASE(DSPSURF(plane), Start);
 		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
 		I915_WRITE(DSPADDR(plane), Offset);
@@ -1913,6 +1916,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
 	unsigned long Start, Offset;
 	u32 dspcntr;
 	u32 reg;
+	unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 
 	switch (plane) {
 	case 0:
@@ -1970,7 +1974,10 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
 	I915_WRITE(reg, dspcntr);
 
 	Start = obj->gtt_offset;
-	Offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
+	Offset = fb->offsets[0] + y * fb->pitches[0] + x * cpp;
+
+	y += fb->offsets[0] / fb->pitches[0];
+	x += fb->offsets[0] % fb->pitches[0] / cpp;
 
 	DRM_DEBUG_KMS("Writing base %08lX %08lX %d %d %d\n",
 		      Start, Offset, x, y, fb->pitches[0]);
@@ -5993,7 +6000,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
 		goto err;
 
 	/* Offset into the new buffer for cases of shared fbs between CRTCs */
-	offset = crtc->y * fb->pitches[0] + crtc->x * fb->bits_per_pixel/8;
+	offset = fb->offsets[0] + crtc->y * fb->pitches[0] + crtc->x * fb->bits_per_pixel/8;
 
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
@@ -6039,7 +6046,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
 		goto err;
 
 	/* Offset into the new buffer for cases of shared fbs between CRTCs */
-	offset = crtc->y * fb->pitches[0] + crtc->x * fb->bits_per_pixel/8;
+	offset = fb->offsets[0] + crtc->y * fb->pitches[0] + crtc->x * fb->bits_per_pixel/8;
 
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
-- 
1.7.3.4

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

[PATCH 6/6] drm/i915: Reject page flips with changed format/offset/pitch

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

MI display flips can't handle some changes in the framebuffer
format or layout. Return an error in such cases.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f4338cb..72ac2f9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6217,6 +6217,19 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	unsigned long flags;
 	int ret;
 
+	/* Can't change pixel format via MI display flips. */
+	if (fb->pixel_format != crtc->fb->pixel_format)
+		return -EINVAL;
+
+	/*
+	 * TILEOFF/LINOFF registers can't be changed via MI display flips.
+	 * Note that pitch changes could also affect these register.
+	 */
+	if (INTEL_INFO(dev)->gen > 3 &&
+	    (fb->offsets[0] != crtc->fb->offsets[0] ||
+	     fb->pitches[0] != crtc->fb->pitches[0]))
+		return -EINVAL;
+
 	work = kzalloc(sizeof *work, GFP_KERNEL);
 	if (work == NULL)
 		return -ENOMEM;
-- 
1.7.3.4

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

Re: [PATCH 5/6] drm/i915: Handle framebuffer offsets[]

[Jesse Barnes]

On Thu, 24 May 2012 21:08:58 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Take fb->offset[0] into account when calculating the linear and tile x/y
> offsets.
> 
> For non-tiled surfaces fb->offset[0] is simply added to the linear
> byte offset.
> 
> For tiled surfaces treat fb->offsets[0] as a byte offset into the
> linearized view of the surface. So we end up converting fb->offsets[0]
> into additional x and y offsets.

Do you have code using a non-zero offsets[0]?  At least for current
code that would indicate some kind of problem... though hopefully we'll
be adding planar support back again sometime soon.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915: Handle framebuffer offsets[]

[Ville Syrjälä]

On Thu, May 24, 2012 at 11:31:32AM -0700, Jesse Barnes wrote:
> On Thu, 24 May 2012 21:08:58 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Take fb->offset[0] into account when calculating the linear and tile x/y
> > offsets.
> > 
> > For non-tiled surfaces fb->offset[0] is simply added to the linear
> > byte offset.
> > 
> > For tiled surfaces treat fb->offsets[0] as a byte offset into the
> > linearized view of the surface. So we end up converting fb->offsets[0]
> > into additional x and y offsets.
> 
> Do you have code using a non-zero offsets[0]?  At least for current
> code that would indicate some kind of problem... though hopefully we'll
> be adding planar support back again sometime soon.

I did have some test app that used offsets[] at some point, but tbh I
didn't excercise these changes with it.

I have a sort of semi working skeleton of a test app which I just modify
for various use cases as need arises. I really should try to clean it up
a bit and generalize it so that it wouldn't need constant code changes
to test different scenarios.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 5/6] drm/i915: Handle framebuffer offsets[]

[Daniel Vetter]

On Thu, May 24, 2012 at 09:49:15PM +0300, Ville Syrjälä wrote:
> On Thu, May 24, 2012 at 11:31:32AM -0700, Jesse Barnes wrote:
> > On Thu, 24 May 2012 21:08:58 +0300
> > ville.syrjala@linux.intel.com wrote:
> > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Take fb->offset[0] into account when calculating the linear and tile x/y
> > > offsets.
> > > 
> > > For non-tiled surfaces fb->offset[0] is simply added to the linear
> > > byte offset.
> > > 
> > > For tiled surfaces treat fb->offsets[0] as a byte offset into the
> > > linearized view of the surface. So we end up converting fb->offsets[0]
> > > into additional x and y offsets.
> > 
> > Do you have code using a non-zero offsets[0]?  At least for current
> > code that would indicate some kind of problem... though hopefully we'll
> > be adding planar support back again sometime soon.
> 
> I did have some test app that used offsets[] at some point, but tbh I
> didn't excercise these changes with it.
> 
> I have a sort of semi working skeleton of a test app which I just modify
> for various use cases as need arises. I really should try to clean it up
> a bit and generalize it so that it wouldn't need constant code changes
> to test different scenarios.

Yeah, I want these little test apps as testcases in i-g-t.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/6] drm/i915: Check framebuffer stride more thoroughly

[Daniel Vetter]

On Thu, May 24, 2012 at 09:08:55PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Make sure the the framebuffer stride is smaller than the maximum
> accepted by any plane.
> 
> Also when using a tiled memory make sure the object stride matches
> the framebuffer stride.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7cf639c..8fea475 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6643,6 +6643,17 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
>  	.create_handle = intel_user_framebuffer_create_handle,
>  };
>  
> +static unsigned int intel_max_fb_stride(const struct drm_device *dev)
> +{
> +	/* FIXME: BSpec for pre-Gen5 is a bit unclear on stride limits */
> +	if (INTEL_INFO(dev)->gen <= 3)
> +		return 8192;

8k pitch limit is gen2, gen3 can have a 4kx4k framebuffer @32bit.
-Daniel

> +	else if (INTEL_INFO(dev)->gen <= 4)
> +		return 16384;

Iirc gen4 can also do 32k, see the pixel-based limits in
intel_modset_init.

> +	else
> +		return 32768;
> +}
> +
>  int intel_framebuffer_init(struct drm_device *dev,
>  			   struct intel_framebuffer *intel_fb,
>  			   struct drm_mode_fb_cmd2 *mode_cmd,
> @@ -6656,6 +6667,13 @@ int intel_framebuffer_init(struct drm_device *dev,
>  	if (mode_cmd->pitches[0] & 63)
>  		return -EINVAL;
>  
> +	if (mode_cmd->pitches[0] > intel_max_fb_stride(dev))
> +		return -EINVAL;
> +
> +	if (obj->tiling_mode != I915_TILING_NONE &&
> +	    mode_cmd->pitches[0] != obj->stride)
> +		return -EINVAL;
> +
>  	/* Reject formats not supported by any plane early. */
>  	switch (mode_cmd->pixel_format) {
>  	case DRM_FORMAT_C8:
> -- 
> 1.7.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/6] drm/i915: Zero initialize mode_cmd

[Daniel Vetter]

On Thu, May 24, 2012 at 09:08:56PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Zero initialize the mode_cmd structure when creating the kernel
> framebuffer. Avoids having uninitialized data in offsets[0] for
> instance.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 5/6] drm/i915: Handle framebuffer offsets[]

[Daniel Vetter]

On Thu, May 24, 2012 at 09:01:24PM +0200, Daniel Vetter wrote:
> On Thu, May 24, 2012 at 09:49:15PM +0300, Ville Syrjälä wrote:
> > On Thu, May 24, 2012 at 11:31:32AM -0700, Jesse Barnes wrote:
> > > On Thu, 24 May 2012 21:08:58 +0300
> > > ville.syrjala@linux.intel.com wrote:
> > > 
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Take fb->offset[0] into account when calculating the linear and tile x/y
> > > > offsets.
> > > > 
> > > > For non-tiled surfaces fb->offset[0] is simply added to the linear
> > > > byte offset.
> > > > 
> > > > For tiled surfaces treat fb->offsets[0] as a byte offset into the
> > > > linearized view of the surface. So we end up converting fb->offsets[0]
> > > > into additional x and y offsets.
> > > 
> > > Do you have code using a non-zero offsets[0]?  At least for current
> > > code that would indicate some kind of problem... though hopefully we'll
> > > be adding planar support back again sometime soon.
> > 
> > I did have some test app that used offsets[] at some point, but tbh I
> > didn't excercise these changes with it.
> > 
> > I have a sort of semi working skeleton of a test app which I just modify
> > for various use cases as need arises. I really should try to clean it up
> > a bit and generalize it so that it wouldn't need constant code changes
> > to test different scenarios.
> 
> Yeah, I want these little test apps as testcases in i-g-t.

Ping about these testcases, ported to i-g-t ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Reject page flips with changed format/offset/pitch

[Daniel Vetter]

On Thu, May 24, 2012 at 09:08:59PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> MI display flips can't handle some changes in the framebuffer
> format or layout. Return an error in such cases.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Queued for -next, thanks for the patch. I've punted on the others, hoping
for a few i-g-t tests (and maybe someone else that could review them).
Safe for the uninitialized stack var patch and this one, because we need
this check to fix up gen4+ tileoffset limitations.

Yours, Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f4338cb..72ac2f9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6217,6 +6217,19 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	unsigned long flags;
>  	int ret;
>  
> +	/* Can't change pixel format via MI display flips. */
> +	if (fb->pixel_format != crtc->fb->pixel_format)
> +		return -EINVAL;
> +
> +	/*
> +	 * TILEOFF/LINOFF registers can't be changed via MI display flips.
> +	 * Note that pitch changes could also affect these register.
> +	 */
> +	if (INTEL_INFO(dev)->gen > 3 &&
> +	    (fb->offsets[0] != crtc->fb->offsets[0] ||
> +	     fb->pitches[0] != crtc->fb->pitches[0]))
> +		return -EINVAL;
> +
>  	work = kzalloc(sizeof *work, GFP_KERNEL);
>  	if (work == NULL)
>  		return -ENOMEM;
> -- 
> 1.7.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Check framebuffer stride more thoroughly

[Ville Syrjälä]

On Thu, Jul 05, 2012 at 01:27:47PM +0200, Daniel Vetter wrote:
> On Thu, May 24, 2012 at 09:08:55PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Make sure the the framebuffer stride is smaller than the maximum
> > accepted by any plane.
> > 
> > Also when using a tiled memory make sure the object stride matches
> > the framebuffer stride.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   18 ++++++++++++++++++
> >  1 files changed, 18 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7cf639c..8fea475 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6643,6 +6643,17 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
> >  	.create_handle = intel_user_framebuffer_create_handle,
> >  };
> >  
> > +static unsigned int intel_max_fb_stride(const struct drm_device *dev)
> > +{
> > +	/* FIXME: BSpec for pre-Gen5 is a bit unclear on stride limits */
> > +	if (INTEL_INFO(dev)->gen <= 3)
> > +		return 8192;
> 
> 8k pitch limit is gen2, gen3 can have a 4kx4k framebuffer @32bit.

OK. I was just looking at BSpec but there were gaps in the docs. For
example, for gen3, only the limit for the OVL (8k) and tiled DSP (8k)
were mentioned. Nothing about non-tiled DSP. OTOH I don't know if even
the documented limits were really correct.

> -Daniel
> 
> > +	else if (INTEL_INFO(dev)->gen <= 4)
> > +		return 16384;
> 
> Iirc gen4 can also do 32k, see the pixel-based limits in
> intel_modset_init.

OK, BSpec was equally unclear here. Only tiled limit (16k) was
mentioned.

Seeing as the limits are a bit unclear, I don't know if I should even
try to add these checks. Unfortunately I don't have any pre-gen6
hardware, so I can't coax the real limits out of the hardware
empirically.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 5/6] drm/i915: Handle framebuffer offsets[]

[Ville Syrjälä]

On Thu, Jul 05, 2012 at 01:29:36PM +0200, Daniel Vetter wrote:
> On Thu, May 24, 2012 at 09:01:24PM +0200, Daniel Vetter wrote:
> > On Thu, May 24, 2012 at 09:49:15PM +0300, Ville Syrjälä wrote:
> > > On Thu, May 24, 2012 at 11:31:32AM -0700, Jesse Barnes wrote:
> > > > On Thu, 24 May 2012 21:08:58 +0300
> > > > ville.syrjala@linux.intel.com wrote:
> > > > 
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > Take fb->offset[0] into account when calculating the linear and tile x/y
> > > > > offsets.
> > > > > 
> > > > > For non-tiled surfaces fb->offset[0] is simply added to the linear
> > > > > byte offset.
> > > > > 
> > > > > For tiled surfaces treat fb->offsets[0] as a byte offset into the
> > > > > linearized view of the surface. So we end up converting fb->offsets[0]
> > > > > into additional x and y offsets.
> > > > 
> > > > Do you have code using a non-zero offsets[0]?  At least for current
> > > > code that would indicate some kind of problem... though hopefully we'll
> > > > be adding planar support back again sometime soon.
> > > 
> > > I did have some test app that used offsets[] at some point, but tbh I
> > > didn't excercise these changes with it.
> > > 
> > > I have a sort of semi working skeleton of a test app which I just modify
> > > for various use cases as need arises. I really should try to clean it up
> > > a bit and generalize it so that it wouldn't need constant code changes
> > > to test different scenarios.
> > 
> > Yeah, I want these little test apps as testcases in i-g-t.
> 
> Ping about these testcases, ported to i-g-t ...

Sorry, haven't had time to look at those. I'm going on vacation after
tomorrow but I'll add a task to the list so I won't forget about this.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Reject page flips with changed format/offset/pitch

[Laurent Pinchart]

Hi Daniel,

On Thursday 05 July 2012 13:31:17 Daniel Vetter wrote:
> On Thu, May 24, 2012 at 09:08:59PM +0300, ville.syrjala@linux.intel.com 
wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > MI display flips can't handle some changes in the framebuffer
> > format or layout. Return an error in such cases.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Queued for -next, thanks for the patch. I've punted on the others, hoping
> for a few i-g-t tests (and maybe someone else that could review them).
> Safe for the uninitialized stack var patch and this one, because we need
> this check to fix up gen4+ tileoffset limitations.
> 
> Yours, Daniel
> 
> > ---
> > 
> >  drivers/gpu/drm/i915/intel_display.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c index f4338cb..72ac2f9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6217,6 +6217,19 @@ static int intel_crtc_page_flip(struct drm_crtc
> > *crtc,> 
> >  	unsigned long flags;
> >  	int ret;
> > 
> > +	/* Can't change pixel format via MI display flips. */
> > +	if (fb->pixel_format != crtc->fb->pixel_format)
> > +		return -EINVAL;

Is this still needed if we apply my "drm: Don't allow page flip to change 
pixel format" patch ?

> > +	/*
> > +	 * TILEOFF/LINOFF registers can't be changed via MI display flips.
> > +	 * Note that pitch changes could also affect these register.
> > +	 */
> > +	if (INTEL_INFO(dev)->gen > 3 &&
> > +	    (fb->offsets[0] != crtc->fb->offsets[0] ||
> > +	     fb->pitches[0] != crtc->fb->pitches[0]))
> > +		return -EINVAL;
> > +
> > 
> >  	work = kzalloc(sizeof *work, GFP_KERNEL);
> >  	if (work == NULL)
> >  	
> >  		return -ENOMEM;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/6] drm/i915: Reject page flips with changed format/offset/pitch

[Daniel Vetter]

On Thu, Jul 19, 2012 at 02:27:47PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Thursday 05 July 2012 13:31:17 Daniel Vetter wrote:
> > On Thu, May 24, 2012 at 09:08:59PM +0300, ville.syrjala@linux.intel.com 
> wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > MI display flips can't handle some changes in the framebuffer
> > > format or layout. Return an error in such cases.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Queued for -next, thanks for the patch. I've punted on the others, hoping
> > for a few i-g-t tests (and maybe someone else that could review them).
> > Safe for the uninitialized stack var patch and this one, because we need
> > this check to fix up gen4+ tileoffset limitations.
> > 
> > Yours, Daniel
> > 
> > > ---
> > > 
> > >  drivers/gpu/drm/i915/intel_display.c |   13 +++++++++++++
> > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c index f4338cb..72ac2f9 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6217,6 +6217,19 @@ static int intel_crtc_page_flip(struct drm_crtc
> > > *crtc,> 
> > >  	unsigned long flags;
> > >  	int ret;
> > > 
> > > +	/* Can't change pixel format via MI display flips. */
> > > +	if (fb->pixel_format != crtc->fb->pixel_format)
> > > +		return -EINVAL;
> 
> Is this still needed if we apply my "drm: Don't allow page flip to change 
> pixel format" patch ?

Actually, drm/i915 is on track to grow itself a complete new modeset
implementation which does not use the crtc helpers (at least as little as
possible). See

http://cgit.freedesktop.org/~danvet/drm/log/?h=modeset-rework

Cheers, Daniel

> 
> > > +	/*
> > > +	 * TILEOFF/LINOFF registers can't be changed via MI display flips.
> > > +	 * Note that pitch changes could also affect these register.
> > > +	 */
> > > +	if (INTEL_INFO(dev)->gen > 3 &&
> > > +	    (fb->offsets[0] != crtc->fb->offsets[0] ||
> > > +	     fb->pitches[0] != crtc->fb->pitches[0]))
> > > +		return -EINVAL;
> > > +
> > > 
> > >  	work = kzalloc(sizeof *work, GFP_KERNEL);
> > >  	if (work == NULL)
> > >  	
> > >  		return -ENOMEM;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48