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.
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; }
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:
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
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.
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;
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
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);
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)
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.
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.
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
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
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.
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;
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
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;
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jesse Barnes -
Laurent Pinchart -
Ville Syrjälä -
ville.syrjala＠linux.intel.com