| Message ID | 1364341502-1184-12-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 27 Mar 2013 00:45:00 +0100 Daniel Vetter <daniel.vetter@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.vetter@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 <jbarnes@virtuousgeek.org>
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.vetter@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
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.vetter@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
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? 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... > 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? > > 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.vetter@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
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.vetter@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 > > -- > Ville Syrjälä > Intel OTC
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 */
- 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.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)