| Message ID | 1364485088-12684-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 28, 2013 at 04:38:08PM +0100, Daniel Vetter wrote: > Ville pointed out that my assumption that no unsupported pixel format > can get past the pipe config computation stage to the platform > update_plane callbacks is wrong. The reason is that this function > still checks the old fb->depth value instead of the new pixel_format. > > While checking with all the other places that use this I've noticed > that intel_framebuffer_init already has all the platform checks we > need, so replace those checks with a WARN_ON. > > Since fb->depth isn't set for YUV pixel formats and since we already > can't create an fb with an rgb layout not support on the running > platform I /think/ this patch doesn't fix any bug. > > But it surely looks better! > > v2: BGR formats are also only gen4+, so add the corresponding WARN_ON, > too (Ville). > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++---------- > 1 file changed, 21 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3f3a3dc..5e8b91f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7394,23 +7394,34 @@ pipe_config_set_bpp(struct drm_crtc *crtc, > struct drm_connector *connector; > int bpp; > > - switch (fb->depth) { > - case 8: > + switch (fb->pixel_format) { > + case DRM_FORMAT_C8: > bpp = 8*3; /* since we go through a colormap */ > break; > - case 15: > - case 16: > + case DRM_FORMAT_XRGB1555: > + case DRM_FORMAT_ARGB1555: > + /* checked in intel_framebuffer_init already */ > + if (WARN_ON(INTEL_INFO(dev)->gen > 3)) > + return -EINVAL; > + case DRM_FORMAT_RGB565: > bpp = 6*3; /* min is 18bpp */ > break; > - case 24: > + case DRM_FORMAT_XBGR8888: > + case DRM_FORMAT_ABGR8888: > + /* checked in intel_framebuffer_init already */ > + if (WARN_ON(INTEL_INFO(dev)->gen < 4)) > + return -EINVAL; > + case DRM_FORMAT_XRGB8888: > + case DRM_FORMAT_ARGB8888: > bpp = 8*3; > break; > - case 30: > - if (INTEL_INFO(dev)->gen < 4) { > - DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n"); > + case DRM_FORMAT_XRGB2101010: > + case DRM_FORMAT_ARGB2101010: > + case DRM_FORMAT_XBGR2101010: > + case DRM_FORMAT_ABGR2101010: > + /* checked in intel_framebuffer_init already */ > + if (WARN_ON(INTEL_INFO(dev)->gen < 4)) > return -EINVAL; > - } > - > bpp = 10*3; > break; > /* TODO: gen4+ supports 16 bpc floating point, too. */ > -- > 1.7.10.4
On Thu, Mar 28, 2013 at 05:45:06PM +0200, Ville Syrjälä wrote: > On Thu, Mar 28, 2013 at 04:38:08PM +0100, Daniel Vetter wrote: > > Ville pointed out that my assumption that no unsupported pixel format > > can get past the pipe config computation stage to the platform > > update_plane callbacks is wrong. The reason is that this function > > still checks the old fb->depth value instead of the new pixel_format. > > > > While checking with all the other places that use this I've noticed > > that intel_framebuffer_init already has all the platform checks we > > need, so replace those checks with a WARN_ON. > > > > Since fb->depth isn't set for YUV pixel formats and since we already > > can't create an fb with an rgb layout not support on the running > > platform I /think/ this patch doesn't fix any bug. > > > > But it surely looks better! > > > > v2: BGR formats are also only gen4+, so add the corresponding WARN_ON, > > too (Ville). > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Both patches merged to dinq, thanks for the review. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3f3a3dc..5e8b91f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7394,23 +7394,34 @@ pipe_config_set_bpp(struct drm_crtc *crtc, struct drm_connector *connector; int bpp; - switch (fb->depth) { - case 8: + switch (fb->pixel_format) { + case DRM_FORMAT_C8: bpp = 8*3; /* since we go through a colormap */ break; - case 15: - case 16: + case DRM_FORMAT_XRGB1555: + case DRM_FORMAT_ARGB1555: + /* checked in intel_framebuffer_init already */ + if (WARN_ON(INTEL_INFO(dev)->gen > 3)) + return -EINVAL; + case DRM_FORMAT_RGB565: bpp = 6*3; /* min is 18bpp */ break; - case 24: + case DRM_FORMAT_XBGR8888: + case DRM_FORMAT_ABGR8888: + /* checked in intel_framebuffer_init already */ + if (WARN_ON(INTEL_INFO(dev)->gen < 4)) + return -EINVAL; + case DRM_FORMAT_XRGB8888: + case DRM_FORMAT_ARGB8888: bpp = 8*3; break; - case 30: - if (INTEL_INFO(dev)->gen < 4) { - DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n"); + case DRM_FORMAT_XRGB2101010: + case DRM_FORMAT_ARGB2101010: + case DRM_FORMAT_XBGR2101010: + case DRM_FORMAT_ABGR2101010: + /* checked in intel_framebuffer_init already */ + if (WARN_ON(INTEL_INFO(dev)->gen < 4)) return -EINVAL; - } - bpp = 10*3; break; /* TODO: gen4+ supports 16 bpc floating point, too. */
Ville pointed out that my assumption that no unsupported pixel format can get past the pipe config computation stage to the platform update_plane callbacks is wrong. The reason is that this function still checks the old fb->depth value instead of the new pixel_format. While checking with all the other places that use this I've noticed that intel_framebuffer_init already has all the platform checks we need, so replace those checks with a WARN_ON. Since fb->depth isn't set for YUV pixel formats and since we already can't create an fb with an rgb layout not support on the running platform I /think/ this patch doesn't fix any bug. But it surely looks better! v2: BGR formats are also only gen4+, so add the corresponding WARN_ON, too (Ville). Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)