Re: [PATCH 6/7] drm: add RGB formats with separate alpha plane

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 19, 2015 at 06:06:00PM +0200, Philipp Zabel wrote:
> Some hardware can read the alpha components separately and then
> conditionally fetch color components only for non-zero alpha values.
> This patch adds fourcc definitions for two-plane RGB formats with an
> 8-bit alpha channel on a second plane.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/drm_crtc.c    | 35 +++++++++++++++++++++++++++++++++++
>  include/uapi/drm/drm_fourcc.h | 14 ++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 3007b44..2ac0d7c 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3245,6 +3245,14 @@ static int format_check(const struct drm_mode_fb_cmd2 *r)
>  	case DRM_FORMAT_YVU422:
>  	case DRM_FORMAT_YUV444:
>  	case DRM_FORMAT_YVU444:
> +	case DRM_FORMAT_XRGB8888_A8:
> +	case DRM_FORMAT_XBGR8888_A8:
> +	case DRM_FORMAT_RGBX8888_A8:
> +	case DRM_FORMAT_BGRX8888_A8:
> +	case DRM_FORMAT_RGB888_A8:
> +	case DRM_FORMAT_BGR888_A8:
> +	case DRM_FORMAT_RGB565_A8:
> +	case DRM_FORMAT_BGR565_A8:
>  		return 0;
>  	default:
>  		DRM_DEBUG_KMS("invalid pixel format %s\n",
> @@ -5198,11 +5206,15 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
>  		break;
>  	case DRM_FORMAT_RGB565:
>  	case DRM_FORMAT_BGR565:
> +	case DRM_FORMAT_RGB565_A8:
> +	case DRM_FORMAT_BGR565_A8:
>  		*depth = 16;
>  		*bpp = 16;
>  		break;
>  	case DRM_FORMAT_RGB888:
>  	case DRM_FORMAT_BGR888:
> +	case DRM_FORMAT_RGB888_A8:
> +	case DRM_FORMAT_BGR888_A8:
>  		*depth = 24;
>  		*bpp = 24;
>  		break;
> @@ -5210,6 +5222,10 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
>  	case DRM_FORMAT_XBGR8888:
>  	case DRM_FORMAT_RGBX8888:
>  	case DRM_FORMAT_BGRX8888:
> +	case DRM_FORMAT_XRGB8888_A8:
> +	case DRM_FORMAT_XBGR8888_A8:
> +	case DRM_FORMAT_RGBX8888_A8:
> +	case DRM_FORMAT_BGRX8888_A8:
>  		*depth = 24;
>  		*bpp = 32;
>  		break;

Please drop the above two hunks, these functions are only for backwards
compat with drivers from the addfb1 days. Modern drivers should only use
the format tags directly. Extending the plane_cpp function like you do
below is enough.

Maybe we should add a WARN_ON(num_planes(format) != 0) to the top of this
and a comment that this is for legacy stuff only.

lgtm otherwise.
-Daniel

> @@ -5268,6 +5284,14 @@ int drm_format_num_planes(uint32_t format)
>  	case DRM_FORMAT_NV61:
>  	case DRM_FORMAT_NV24:
>  	case DRM_FORMAT_NV42:
> +	case DRM_FORMAT_RGB565_A8:
> +	case DRM_FORMAT_BGR565_A8:
> +	case DRM_FORMAT_RGB888_A8:
> +	case DRM_FORMAT_BGR888_A8:
> +	case DRM_FORMAT_XRGB8888_A8:
> +	case DRM_FORMAT_XBGR8888_A8:
> +	case DRM_FORMAT_RGBX8888_A8:
> +	case DRM_FORMAT_BGRX8888_A8:
>  		return 2;
>  	default:
>  		return 1;
> @@ -5315,6 +5339,17 @@ int drm_format_plane_cpp(uint32_t format, int plane)
>  	case DRM_FORMAT_YUV444:
>  	case DRM_FORMAT_YVU444:
>  		return 1;
> +	case DRM_FORMAT_RGB565_A8:
> +	case DRM_FORMAT_BGR565_A8:
> +		return plane ? 1 : 2;
> +	case DRM_FORMAT_RGB888_A8:
> +	case DRM_FORMAT_BGR888_A8:
> +		return plane ? 1 : 3;
> +	case DRM_FORMAT_XRGB8888_A8:
> +	case DRM_FORMAT_XBGR8888_A8:
> +	case DRM_FORMAT_RGBX8888_A8:
> +	case DRM_FORMAT_BGRX8888_A8:
> +		return plane ? 1 : 4;
>  	default:
>  		drm_fb_get_bpp_depth(format, &depth, &bpp);
>  		return bpp >> 3;
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 0773582..48d6ec8 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -96,6 +96,20 @@
>  #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */
>  
>  /*
> + * 2 plane RGB + A
> + * index 0 = RGB plane
> + * index 1 = A plane
> + */
> +#define DRM_FORMAT_XRGB8888_A8	fourcc_code('X', 'R', 'A', '8')
> +#define DRM_FORMAT_XBGR8888_A8	fourcc_code('X', 'B', 'A', '8')
> +#define DRM_FORMAT_RGBX8888_A8	fourcc_code('R', 'X', 'A', '8')
> +#define DRM_FORMAT_BGRX8888_A8	fourcc_code('B', 'X', 'A', '8')
> +#define DRM_FORMAT_RGB888_A8	fourcc_code('R', '8', 'A', '8')
> +#define DRM_FORMAT_BGR888_A8	fourcc_code('B', '8', 'A', '8')
> +#define DRM_FORMAT_RGB565_A8	fourcc_code('R', '5', 'A', '8')
> +#define DRM_FORMAT_BGR565_A8	fourcc_code('B', '5', 'A', '8')
> +
> +/*
>   * 2 plane YCbCr
>   * index 0 = Y plane, [7:0] Y
>   * index 1 = Cr:Cb plane, [15:0] Cr:Cb little endian
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel