Re: [PATCH 04/12] drm/msm: add arrays listing formats supported by MDP4/MDP5 hardware

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
> MDP4 and MDP5 drivers enumerate supported formats each time the plane is
> created. In preparation to merger of MDP DPU format databases, define
> precise formats list, so that changes to the database do not cause the
> driver to add unsupported format to the list.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 57 ++++++++++++++++++++--
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 36 +++++++++++---
>   drivers/gpu/drm/msm/disp/mdp_format.c      | 28 -----------
>   drivers/gpu/drm/msm/disp/mdp_kms.h         |  1 -
>   4 files changed, 80 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
> index b689b618da78..cebe20c82a54 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
> @@ -371,6 +371,47 @@ static const uint64_t supported_format_modifiers[] = {
>   	DRM_FORMAT_MOD_INVALID
>   };
>   
> +const uint32_t mdp4_rgb_formats[] = {
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_RGBA8888,
> +	DRM_FORMAT_BGRA8888,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_RGBX8888,
> +	DRM_FORMAT_BGRX8888,
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_BGR888,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_BGR565,
> +};
> +
> +const uint32_t mdp4_rgb_yuv_formats[] = {
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_RGBA8888,
> +	DRM_FORMAT_BGRA8888,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_RGBX8888,
> +	DRM_FORMAT_BGRX8888,
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_BGR888,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_BGR565,
> +
> +	DRM_FORMAT_NV12,
> +	DRM_FORMAT_NV21,
> +	DRM_FORMAT_NV16,
> +	DRM_FORMAT_NV61,
> +	DRM_FORMAT_VYUY,
> +	DRM_FORMAT_UYVY,
> +	DRM_FORMAT_YUYV,
> +	DRM_FORMAT_YVYU,
> +	DRM_FORMAT_YUV420,
> +	DRM_FORMAT_YVU420,
> +};
> +
>   /* initialize plane */
>   struct drm_plane *mdp4_plane_init(struct drm_device *dev,
>   		enum mdp4_pipe pipe_id, bool private_plane)
> @@ -379,6 +420,8 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
>   	struct mdp4_plane *mdp4_plane;
>   	int ret;
>   	enum drm_plane_type type;
> +	const uint32_t *formats;
> +	unsigned int nformats;
>   
>   	mdp4_plane = kzalloc(sizeof(*mdp4_plane), GFP_KERNEL);
>   	if (!mdp4_plane) {
> @@ -392,13 +435,17 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
>   	mdp4_plane->name = pipe_names[pipe_id];
>   	mdp4_plane->caps = mdp4_pipe_caps(pipe_id);
>   
> -	mdp4_plane->nformats = mdp_get_formats(mdp4_plane->formats,
> -			ARRAY_SIZE(mdp4_plane->formats),
> -			!pipe_supports_yuv(mdp4_plane->caps));
> -
>   	type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> +
> +	if (pipe_supports_yuv(mdp4_plane->caps)) {
> +		formats = mdp4_rgb_yuv_formats;
> +		nformats = ARRAY_SIZE(mdp4_rgb_yuv_formats);
> +	} else {
> +		formats = mdp4_rgb_formats;
> +		nformats = ARRAY_SIZE(mdp4_rgb_formats);
> +	}
>   	ret = drm_universal_plane_init(dev, plane, 0xff, &mdp4_plane_funcs,
> -				 mdp4_plane->formats, mdp4_plane->nformats,
> +				 formats, nformats,
>   				 supported_format_modifiers, type, NULL);
>   	if (ret)
>   		goto fail;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index 0d5ff03cb091..aa8342d93393 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -17,9 +17,6 @@
>   
>   struct mdp5_plane {
>   	struct drm_plane base;
> -
> -	uint32_t nformats;
> -	uint32_t formats[32];
>   };
>   #define to_mdp5_plane(x) container_of(x, struct mdp5_plane, base)
>   
> @@ -1007,6 +1004,32 @@ uint32_t mdp5_plane_get_flush(struct drm_plane *plane)
>   	return mask;
>   }
>   
> +const uint32_t mdp5_plane_formats[] = {
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_RGBA8888,
> +	DRM_FORMAT_BGRA8888,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_RGBX8888,
> +	DRM_FORMAT_BGRX8888,
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_BGR888,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_BGR565,
> +
> +	DRM_FORMAT_NV12,
> +	DRM_FORMAT_NV21,
> +	DRM_FORMAT_NV16,
> +	DRM_FORMAT_NV61,
> +	DRM_FORMAT_VYUY,
> +	DRM_FORMAT_UYVY,
> +	DRM_FORMAT_YUYV,
> +	DRM_FORMAT_YVYU,
> +	DRM_FORMAT_YUV420,
> +	DRM_FORMAT_YVU420,
> +};
> +
>   /* initialize plane */
>   struct drm_plane *mdp5_plane_init(struct drm_device *dev,
>   				  enum drm_plane_type type)
> @@ -1023,12 +1046,9 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
>   
>   	plane = &mdp5_plane->base;
>   
> -	mdp5_plane->nformats = mdp_get_formats(mdp5_plane->formats,
> -		ARRAY_SIZE(mdp5_plane->formats), false);
> -
>   	ret = drm_universal_plane_init(dev, plane, 0xff, &mdp5_plane_funcs,
> -			mdp5_plane->formats, mdp5_plane->nformats,
> -			NULL, type, NULL);
> +				       mdp5_plane_formats, ARRAY_SIZE(mdp5_plane_formats),
> +				       NULL, type, NULL);
>   	if (ret)
>   		goto fail;
>   

Did you accidentally drop checking for YUV format cap before adding the 
formats for the plane similar to

> +	if (pipe_supports_yuv(mdp4_plane->caps)) {
> +		formats = mdp4_rgb_yuv_formats;
> +		nformats = ARRAY_SIZE(mdp4_rgb_yuv_formats);
> +	} else {
> +		formats = mdp4_rgb_formats;
> +		nformats = ARRAY_SIZE(mdp4_rgb_formats);
> +	}


Also, from what I checked the format table is identical between mdp4 and 
mdp5. Even if we cannot unify mdp4/5 and dpu tables, we can atleast have 
mdp_4_5_rgb and mdp_4_5_rgb_yuv?