On Thu, Mar 28, 2013 at 01:49:27PM +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.
Yeah I tried to go over the possible combinations of supported
formats and came to the same conclusion. The bpp=0 thing for YUV
saved us, and otherwise the sprite RGB formats always seem to be
a subset of the primary plane formats. At least it's now safe
against someone getting the urge to populate depth/bpp for YUV
formats.
> But it surely looks better!
>
> Cc: Ville Syrj?l? <ville.syrjala at linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++----------
> 1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8cbb65c..86130a2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7394,23 +7394,31 @@ 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_XRGB8888:
> + case DRM_FORMAT_ARGB8888:
> + case DRM_FORMAT_XBGR8888:
> + case DRM_FORMAT_ABGR8888:
If you want to be entirely pedantic XBGR8888/ABGR8888 should also have
the 'gen < 4' check.
> 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
--
Ville Syrj?l?
Intel OTC
