subject:"\[Intel\-gfx\] \[PATCH 11\/13\] drm\/i915\: clean up plane bpp confusion"

Re: [Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

2013-03-28 Thread Ville Syrjälä

On Wed, Mar 27, 2013 at 12:45:00AM +0100, Daniel Vetter wrote:
 - Update_plane should never see a wrong fb bpp value, BUG in the
   corresponding cases.

That's not true actually. For sprites the common drm code already
checks the format against the list provided by the driver, but for
primary planes there's no such check. The check in
intel_framebuffer_init() isn't enough since it'll also accept
formats that are supported by sprites but not by the primary planes.

 
 v2: Rebase on top of Ville's plane pixel layout changes.
 
 v3: Actually drop the old gen4 check for 10bpc planes, spotted
 by Ville Syrjälä.
 
 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 ---
  drivers/gpu/drm/i915/intel_display.c | 20 
  1 file changed, 8 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 51557ba..bbf31aa 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, 
 struct drm_framebuffer *fb,
   dspcntr |= DISPPLANE_RGBX101010;
   break;
   default:
 - DRM_ERROR(Unknown pixel format 0x%08x\n, fb-pixel_format);
 - return -EINVAL;
 + BUG();
   }
  
   if (INTEL_INFO(dev)-gen = 4) {
 @@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
   dspcntr |= DISPPLANE_RGBX101010;
   break;
   default:
 - DRM_ERROR(Unknown pixel format 0x%08x\n, fb-pixel_format);
 - return -EINVAL;
 + BUG();
   }
  
   if (obj-tiling_mode != I915_TILING_NONE)
 @@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
   bpp = 8*3;
   break;
   case 30:
 + if (INTEL_INFO(dev)-gen  4) {
 + DRM_DEBUG_KMS(10 bpc not supported on gen2/3\n);
 + return -EINVAL;
 + }
 +
   bpp = 10*3;
   break;
 - case 48:
 - bpp = 12*3;
 - break;
 + /* TODO: gen4+ supports 16 bpc floating point, too. */
   default:
   DRM_DEBUG_KMS(unsupported depth\n);
   return -EINVAL;
   }
  
 - if (fb-depth  24  !HAS_PCH_SPLIT(dev)) {
 - DRM_DEBUG_KMS(high depth not supported on gmch platforms\n);
 - return -EINVAL;
 - }
 -
   pipe_config-pipe_bpp = bpp;
  
   /* Clamp display bpp to EDID value */
 -- 
 1.7.11.7
 
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

2013-03-28 Thread Daniel Vetter

On Thu, Mar 28, 2013 at 01:26:28PM +0200, Ville Syrjälä wrote:
 On Wed, Mar 27, 2013 at 12:45:00AM +0100, Daniel Vetter wrote:
  - Update_plane should never see a wrong fb bpp value, BUG in the
corresponding cases.
 
 That's not true actually. For sprites the common drm code already
 checks the format against the list provided by the driver, but for
 primary planes there's no such check. The check in
 intel_framebuffer_init() isn't enough since it'll also accept
 formats that are supported by sprites but not by the primary planes.

With the new pipe_config step we check plane bpp in the new compute config
stage, so before we start touching the hw. Which means by the time we
reach the various *_update_plane functions, we shouldn't see an
unsupported pixel format any more.

There's the shortpath in the setcrtc ioctl implementation which goes
directly to set_base, but that one checks whether the bits_per_pixel of
the fb matches. I guess we should switch that one over to
fb-pixel_format, but besides that nit I think we really should be
covered, and the below default cases are indeed BUGs.

Or have I missed a place somewhere?

I'll follow up with the pixel_format patch.

Cheers, Daniel
 
  
  v2: Rebase on top of Ville's plane pixel layout changes.
  
  v3: Actually drop the old gen4 check for 10bpc planes, spotted
  by Ville Syrjälä.
  
  Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
  ---
   drivers/gpu/drm/i915/intel_display.c | 20 
   1 file changed, 8 insertions(+), 12 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/intel_display.c 
  b/drivers/gpu/drm/i915/intel_display.c
  index 51557ba..bbf31aa 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, 
  struct drm_framebuffer *fb,
  dspcntr |= DISPPLANE_RGBX101010;
  break;
  default:
  -   DRM_ERROR(Unknown pixel format 0x%08x\n, fb-pixel_format);
  -   return -EINVAL;
  +   BUG();
  }
   
  if (INTEL_INFO(dev)-gen = 4) {
  @@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc 
  *crtc,
  dspcntr |= DISPPLANE_RGBX101010;
  break;
  default:
  -   DRM_ERROR(Unknown pixel format 0x%08x\n, fb-pixel_format);
  -   return -EINVAL;
  +   BUG();
  }
   
  if (obj-tiling_mode != I915_TILING_NONE)
  @@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
  bpp = 8*3;
  break;
  case 30:
  +   if (INTEL_INFO(dev)-gen  4) {
  +   DRM_DEBUG_KMS(10 bpc not supported on gen2/3\n);
  +   return -EINVAL;
  +   }
  +
  bpp = 10*3;
  break;
  -   case 48:
  -   bpp = 12*3;
  -   break;
  +   /* TODO: gen4+ supports 16 bpc floating point, too. */
  default:
  DRM_DEBUG_KMS(unsupported depth\n);
  return -EINVAL;
  }
   
  -   if (fb-depth  24  !HAS_PCH_SPLIT(dev)) {
  -   DRM_DEBUG_KMS(high depth not supported on gmch platforms\n);
  -   return -EINVAL;
  -   }
  -
  pipe_config-pipe_bpp = bpp;
   
  /* Clamp display bpp to EDID value */
  -- 
  1.7.11.7
  
  ___
  Intel-gfx mailing list
  Intel-gfx@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/intel-gfx
 
 -- 
 Ville Syrjälä
 Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

2013-03-28 Thread Daniel Vetter

On Thu, Mar 28, 2013 at 01:59:59PM +0200, Ville Syrjälä wrote:
 On Thu, Mar 28, 2013 at 12:46:42PM +0100, Daniel Vetter wrote:
  On Thu, Mar 28, 2013 at 01:26:28PM +0200, Ville Syrjälä wrote:
   On Wed, Mar 27, 2013 at 12:45:00AM +0100, Daniel Vetter wrote:
- Update_plane should never see a wrong fb bpp value, BUG in the
  corresponding cases.
   
   That's not true actually. For sprites the common drm code already
   checks the format against the list provided by the driver, but for
   primary planes there's no such check. The check in
   intel_framebuffer_init() isn't enough since it'll also accept
   formats that are supported by sprites but not by the primary planes.
  
  With the new pipe_config step we check plane bpp in the new compute config
  stage, so before we start touching the hw. Which means by the time we
  reach the various *_update_plane functions, we shouldn't see an
  unsupported pixel format any more.
 
 Do you mean the fb-depth check in pipe_config_set_bpp()? That's not
 enough. It doesn't have any gen checks, so it could very well let some
 unsupported format through, even though the depth/bpp might match. Or
 did I miss some more thorough check somewhere?

Yeah, although on closer examination I think we get away since all the
missing checks are done by intel_framebuffer_init, and yuv doesn't have a
depth. Still checking pixel_format looks much better.

 I've been pondering if I should just propose removing depth/bpp from
 drm_framebuffer altogether to make it harder to write code that doesn't
 do proper format checks...

Yeah, I think we should aim for that, at least in our own driver codebase.

  There's the shortpath in the setcrtc ioctl implementation which goes
  directly to set_base, but that one checks whether the bits_per_pixel of
  the fb matches. I guess we should switch that one over to
  fb-pixel_format, but besides that nit I think we really should be
  covered, and the below default cases are indeed BUGs.
  
  Or have I missed a place somewhere?
  
  I'll follow up with the pixel_format patch.
 
 Perhaps some common func that you can call early in both set_base
 and full modeset paths?

Well, if the pixel_format check fails we'll do a full modeset, where any
inappropriate framebuffers will be caught in the (now hopefully solid)
checks in pipe_config_set_bpp.

Cheers, Daniel

 
  
  Cheers, Daniel
   

v2: Rebase on top of Ville's plane pixel layout changes.

v3: Actually drop the old gen4 check for 10bpc planes, spotted
by Ville Syrjälä.

Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
 drivers/gpu/drm/i915/intel_display.c | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 51557ba..bbf31aa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc 
*crtc, struct drm_framebuffer *fb,
dspcntr |= DISPPLANE_RGBX101010;
break;
default:
-   DRM_ERROR(Unknown pixel format 0x%08x\n, 
fb-pixel_format);
-   return -EINVAL;
+   BUG();
}
 
if (INTEL_INFO(dev)-gen = 4) {
@@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc 
*crtc,
dspcntr |= DISPPLANE_RGBX101010;
break;
default:
-   DRM_ERROR(Unknown pixel format 0x%08x\n, 
fb-pixel_format);
-   return -EINVAL;
+   BUG();
}
 
if (obj-tiling_mode != I915_TILING_NONE)
@@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
bpp = 8*3;
break;
case 30:
+   if (INTEL_INFO(dev)-gen  4) {
+   DRM_DEBUG_KMS(10 bpc not supported on 
gen2/3\n);
+   return -EINVAL;
+   }
+
bpp = 10*3;
break;
-   case 48:
-   bpp = 12*3;
-   break;
+   /* TODO: gen4+ supports 16 bpc floating point, too. */
default:
DRM_DEBUG_KMS(unsupported depth\n);
return -EINVAL;
}
 
-   if (fb-depth  24  !HAS_PCH_SPLIT(dev)) {
-   DRM_DEBUG_KMS(high depth not supported on gmch 
platforms\n);
-   return -EINVAL;
-   }
-
pipe_config-pipe_bpp = bpp;
 
/* Clamp display bpp to EDID value */
-- 
1.7.11.7

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

Re: [Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

2013-03-27 Thread Jesse Barnes

On Wed, 27 Mar 2013 00:45:00 +0100
Daniel Vetter daniel.vet...@ffwll.ch wrote:

 - There is no 16bpc linear color format in our hw. gen4+ has a 16 bpc
   float layout, but we don't really support it.
 - 10bpc is a gen4+ feature, fix up the support for it.
 - Update_plane should never see a wrong fb bpp value, BUG in the
   corresponding cases.
 
 v2: Rebase on top of Ville's plane pixel layout changes.
 
 v3: Actually drop the old gen4 check for 10bpc planes, spotted
 by Ville Syrjälä.
 
 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 ---
  drivers/gpu/drm/i915/intel_display.c | 20 
  1 file changed, 8 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 51557ba..bbf31aa 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, 
 struct drm_framebuffer *fb,
   dspcntr |= DISPPLANE_RGBX101010;
   break;
   default:
 - DRM_ERROR(Unknown pixel format 0x%08x\n, fb-pixel_format);
 - return -EINVAL;
 + BUG();
   }
  
   if (INTEL_INFO(dev)-gen = 4) {
 @@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
   dspcntr |= DISPPLANE_RGBX101010;
   break;
   default:
 - DRM_ERROR(Unknown pixel format 0x%08x\n, fb-pixel_format);
 - return -EINVAL;
 + BUG();
   }
  
   if (obj-tiling_mode != I915_TILING_NONE)
 @@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
   bpp = 8*3;
   break;
   case 30:
 + if (INTEL_INFO(dev)-gen  4) {
 + DRM_DEBUG_KMS(10 bpc not supported on gen2/3\n);
 + return -EINVAL;
 + }
 +
   bpp = 10*3;
   break;
 - case 48:
 - bpp = 12*3;
 - break;
 + /* TODO: gen4+ supports 16 bpc floating point, too. */
   default:
   DRM_DEBUG_KMS(unsupported depth\n);
   return -EINVAL;
   }
  
 - if (fb-depth  24  !HAS_PCH_SPLIT(dev)) {
 - DRM_DEBUG_KMS(high depth not supported on gmch platforms\n);
 - return -EINVAL;
 - }
 -
   pipe_config-pipe_bpp = bpp;
  
   /* Clamp display bpp to EDID value */

You don't want to squash this into 8/13?  It looks ok.

Sorry about the 48; it's 16:16:16:16 ignoring alpha, so you end up with
48bpp and my backwards calc for bpc ignored alpha again and ended up at
12. :)

Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org

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

[Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

2013-03-26 Thread Daniel Vetter

- There is no 16bpc linear color format in our hw. gen4+ has a 16 bpc
  float layout, but we don't really support it.
- 10bpc is a gen4+ feature, fix up the support for it.
- Update_plane should never see a wrong fb bpp value, BUG in the
  corresponding cases.

v2: Rebase on top of Ville's plane pixel layout changes.

v3: Actually drop the old gen4 check for 10bpc planes, spotted
by Ville Syrjälä.

Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
 drivers/gpu/drm/i915/intel_display.c | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 51557ba..bbf31aa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, 
struct drm_framebuffer *fb,
dspcntr |= DISPPLANE_RGBX101010;
break;
default:
-   DRM_ERROR(Unknown pixel format 0x%08x\n, fb-pixel_format);
-   return -EINVAL;
+   BUG();
}
 
if (INTEL_INFO(dev)-gen = 4) {
@@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
dspcntr |= DISPPLANE_RGBX101010;
break;
default:
-   DRM_ERROR(Unknown pixel format 0x%08x\n, fb-pixel_format);
-   return -EINVAL;
+   BUG();
}
 
if (obj-tiling_mode != I915_TILING_NONE)
@@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
bpp = 8*3;
break;
case 30:
+   if (INTEL_INFO(dev)-gen  4) {
+   DRM_DEBUG_KMS(10 bpc not supported on gen2/3\n);
+   return -EINVAL;
+   }
+
bpp = 10*3;
break;
-   case 48:
-   bpp = 12*3;
-   break;
+   /* TODO: gen4+ supports 16 bpc floating point, too. */
default:
DRM_DEBUG_KMS(unsupported depth\n);
return -EINVAL;
}
 
-   if (fb-depth  24  !HAS_PCH_SPLIT(dev)) {
-   DRM_DEBUG_KMS(high depth not supported on gmch platforms\n);
-   return -EINVAL;
-   }
-
pipe_config-pipe_bpp = bpp;
 
/* Clamp display bpp to EDID value */
-- 
1.7.11.7

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

Re: [Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

Re: [Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

Re: [Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

Re: [Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

[Intel-gfx] [PATCH 11/13] drm/i915: clean up plane bpp confusion

5 matches

Site Navigation

Mail list logo

Footer information