[Intel-gfx] [PATCH 2/2] drm/i915: fixup fb bpp computation in pipe_config_set_bpp

[Ville Syrjälä]

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