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

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 02/12/2023 03:36, Abhinav Kumar wrote:
> On 7/7/2023 6:03 PM, Dmitry Baryshkov wrote:
> > MDP4 and MDP5 drivers enumerate supported formats each time the plane is
> > created. As the list of supported image formats is constant, create
> > corresponding data arrays to be used by MDP4 and MDP5 drivers.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/disp/mdp_format.c | 49 +++++++++++++++++++++++++--
> >   drivers/gpu/drm/msm/disp/mdp_kms.h    |  5 +++
> >   2 files changed, 52 insertions(+), 2 deletions(-)
>
> After going through the patch series, as commented in patch 17 I am 
> totally fine with migrating to drmm-managed APIs but I dont like to 
> maintain 3 format arrays.
>
> Can we keep the existing format mechanism to avoid having two more arrays?

For DPU we have exactly the same idea: single formats data array 
describing and per-usecase arrays, like plane RGB arrays, plane YUV+RGB 
array, WB arrays.

Anyway. formats was one of the topics where we had a lot of duplication 
between mdp4/mdp5 and dpu anyway. I think I'm going to back up the 
patches 1, 10, 16, 17 (plane patches depend on the format arrays), push 
the rest of the patches to msm-next-lumag and send a series reworking 
all the formats handling.


> > diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c 
> > b/drivers/gpu/drm/msm/disp/mdp_format.c
> > index 025595336f26..ba9abe8b3acc 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp_format.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp_format.c
> > @@ -81,8 +81,8 @@ static struct csc_cfg csc_convert[CSC_MAX] = {
> >   #define BPC0A 0
> >   /*
> > - * Note: Keep RGB formats 1st, followed by YUV formats to avoid breaking
> > - * mdp_get_rgb_formats()'s implementation.
> > + * Note: Keep mdp_rgb_formats and mdp_rgb_yuv_formats in sync when 
> > adding
> > + * entries to this array.
> >    */
> >   static const struct mdp_format formats[] = {
> >       /*  name      a  r  g  b   e0 e1 e2 e3  alpha   tight  cpp cnt 
> > ... */
> > @@ -138,6 +138,51 @@ static const struct mdp_format formats[] = {
> >               MDP_PLANE_PLANAR, CHROMA_420, true),
> >   };
> > +const uint32_t mdp_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,
> > +};
> > +
> > +size_t mdp_rgb_num_formats = ARRAY_SIZE(mdp_rgb_formats);
> > +
> > +const uint32_t mdp_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,
> > +};
> > +
> > +size_t mdp_rgb_yuv_num_formats = ARRAY_SIZE(mdp_rgb_yuv_formats);
> > +
> >   /*
> >    * Note:
> >    * @rgb_only must be set to true, when requesting
> > diff --git a/drivers/gpu/drm/msm/disp/mdp_kms.h 
> > b/drivers/gpu/drm/msm/disp/mdp_kms.h
> > index b0286d5d5130..11402a859574 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp_kms.h
> > +++ b/drivers/gpu/drm/msm/disp/mdp_kms.h
> > @@ -94,6 +94,11 @@ struct mdp_format {
> >   uint32_t mdp_get_formats(uint32_t *formats, uint32_t max_formats, 
> > bool rgb_only);
> >   const struct msm_format *mdp_get_format(struct msm_kms *kms, 
> > uint32_t format, uint64_t modifier);
> > +extern const uint32_t mdp_rgb_formats[];
> > +extern size_t mdp_rgb_num_formats;
> > +extern const uint32_t mdp_rgb_yuv_formats[];
> > +extern size_t mdp_rgb_yuv_num_formats;
> > +
> >   /* MDP capabilities */
> >   #define MDP_CAP_SMP        BIT(0)    /* Shared Memory 
> > Pool                 */
> >   #define MDP_CAP_DSC        BIT(1)    /* VESA Display Stream 
> > Compression    */

--
With best wishes
Dmitry