Re: [RFC PATCH v2 10/13] drm/msm/dpu: add list of supported formats to the DPU caps

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 07/06/2023 00:47, Abhinav Kumar wrote:
> On 6/6/2023 2:29 PM, Dmitry Baryshkov wrote:
> > On 07/06/2023 00:14, Abhinav Kumar wrote:
> > > On 5/24/2023 6:47 PM, Dmitry Baryshkov wrote:
> > > > On Thu, 25 May 2023 at 02:16, Abhinav Kumar 
> > > > <quic_abhinavk@xxxxxxxxxxx> wrote:
> > > > > On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote:
> > > > > > As we are going to add virtual planes, add the list of supported 
> > > > > > formats
> > > > > > to the hw catalog entry. It will be used to setup universal 
> > > > > > planes, with
> > > > > > later selecting a pipe depending on whether the YUV format is used 
> > > > > > for
> > > > > > the framebuffer.
> > > > >
> > > > > If your usage of format_list is going to be internal to dpu_plane.c, I
> > > > > can think of another idea for this change.
> > > > >
> > > > > This essentially translates to if (num_vig >= 1)
> > > > >
> > > > > If we can just have a small helper to detect that from the catalog can
> > > > > we use that instead of adding formats to the dpu caps?
> > > >
> > > > I'd prefer to be explicit here. The list of supported formats might
> > > > vary between generations, might it not? Also we don't have a case of
> > > > the devices not having VIG planes. Even the qcm2290 (which doesn't
> > > > have CSC) lists YUV as supported.
> > >
> > > the list of formats is tied to the sspps the dpu generation has and 
> > > the capabilities of those sspps.
> > >
> > > qcm2290 is really an interesting case. It has one vig sspp but no csc 
> > > block for that vig sspp, hence it cannot support non-RGB formats.
> > >
> > > I have confirmed that downstream is incorrect to populate yuv formats 
> > > for qcm2290.
> > >
> > > I still think that having atleast one vig sspp with csc sub-blk is 
> > > the right condition to check if we want to decide if the dpu for that 
> > > chipset supports yuv formats. Extra csc-blk check to handle qcm2290.
> > >
> > > Having a small helper in dpu_plane.c is good enough for that because 
> > > with virtual planes, you only need to know "if such a plane exists 
> > > and not which plane" and a full catalog change isnt needed IMO
> >
> > This goes down to the question: is the list of YUV and non-YUV formats 
> > static or not? Do all DPU devices support the same set of YUV and 
> > non-YUV formats? If it is static, we might as well drop 
> > dpu_sspp_sub_blks::format_list.
>
> I would say yes based on the below reference:
>
> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/clo/main/msm/sde/sde_hw_catalog.c#L3858
>
> We always add the same set of YUV formats for all Vig SSPPs irrespective 
> of the chipsets.

Well, as your example pointed out, few lines below it starts adding 
formats to the list, so the format list is not static and depends on the 
generation.

> > Note to myself: consider 
> > dpu_mdss_cfg::{dma_formats,cursor_formats,vig_formats}. Either migrate 
> > dpu_sspp_sub_blks::format_list users to these fields or drop them.
>
> Yes, I agree. There is no need to have format list in the sub_blk as for 
> one type of SSPP, it supports the same format across DPU generations.
>
> > > > Note: I think at some point we should have a closer look at the list
> > > > of supported formats and crosscheck that we do not have either
> > > > unsupported formats being listed, or missing formats which are not
> > > > listed as supported.
> > > >
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > > ---
> > > > > >    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 26 
> > > > > > +++++++++++++++++++
> > > > > >    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |  4 +++
> > > > > >    2 files changed, 30 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
> > > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > index 212d546b6c5d..2d6944a9679a 100644
> > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > @@ -315,6 +315,8 @@ static const struct dpu_caps msm8998_dpu_caps = {
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > >        .max_hdeci_exp = MAX_HORZ_DECIMATION,
> > > > > >        .max_vdeci_exp = MAX_VERT_DECIMATION,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps qcm2290_dpu_caps = {
> > > > > > @@ -324,6 +326,8 @@ static const struct dpu_caps qcm2290_dpu_caps = {
> > > > > >        .has_idle_pc = true,
> > > > > >        .max_linewidth = 2160,
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sdm845_dpu_caps = {
> > > > > > @@ -339,6 +343,8 @@ static const struct dpu_caps sdm845_dpu_caps = {
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > >        .max_hdeci_exp = MAX_HORZ_DECIMATION,
> > > > > >        .max_vdeci_exp = MAX_VERT_DECIMATION,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sc7180_dpu_caps = {
> > > > > > @@ -350,6 +356,8 @@ static const struct dpu_caps sc7180_dpu_caps = {
> > > > > >        .has_idle_pc = true,
> > > > > >        .max_linewidth = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sm6115_dpu_caps = {
> > > > > > @@ -361,6 +369,8 @@ static const struct dpu_caps sm6115_dpu_caps = {
> > > > > >        .has_idle_pc = true,
> > > > > >        .max_linewidth = 2160,
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sm8150_dpu_caps = {
> > > > > > @@ -376,6 +386,8 @@ static const struct dpu_caps sm8150_dpu_caps = {
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > >        .max_hdeci_exp = MAX_HORZ_DECIMATION,
> > > > > >        .max_vdeci_exp = MAX_VERT_DECIMATION,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sc8180x_dpu_caps = {
> > > > > > @@ -391,6 +403,8 @@ static const struct dpu_caps sc8180x_dpu_caps = {
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > >        .max_hdeci_exp = MAX_HORZ_DECIMATION,
> > > > > >        .max_vdeci_exp = MAX_VERT_DECIMATION,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sc8280xp_dpu_caps = {
> > > > > > @@ -404,6 +418,8 @@ static const struct dpu_caps sc8280xp_dpu_caps 
> > > > > > = {
> > > > > >        .has_3d_merge = true,
> > > > > >        .max_linewidth = 5120,
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sm8250_dpu_caps = {
> > > > > > @@ -417,6 +433,8 @@ static const struct dpu_caps sm8250_dpu_caps = {
> > > > > >        .has_3d_merge = true,
> > > > > >        .max_linewidth = 900,
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sm8350_dpu_caps = {
> > > > > > @@ -430,6 +448,8 @@ static const struct dpu_caps sm8350_dpu_caps = {
> > > > > >        .has_3d_merge = true,
> > > > > >        .max_linewidth = 4096,
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sm8450_dpu_caps = {
> > > > > > @@ -443,6 +463,8 @@ static const struct dpu_caps sm8450_dpu_caps = {
> > > > > >        .has_3d_merge = true,
> > > > > >        .max_linewidth = 5120,
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sm8550_dpu_caps = {
> > > > > > @@ -456,6 +478,8 @@ static const struct dpu_caps sm8550_dpu_caps = {
> > > > > >        .has_3d_merge = true,
> > > > > >        .max_linewidth = 5120,
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_caps sc7280_dpu_caps = {
> > > > > > @@ -467,6 +491,8 @@ static const struct dpu_caps sc7280_dpu_caps = {
> > > > > >        .has_idle_pc = true,
> > > > > >        .max_linewidth = 2400,
> > > > > >        .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > > > > > +     .format_list = plane_formats_yuv,
> > > > > > +     .num_formats = ARRAY_SIZE(plane_formats_yuv),
> > > > > >    };
> > > > > >
> > > > > >    static const struct dpu_mdp_cfg msm8998_mdp[] = {
> > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h 
> > > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > > > > > index 89b372cdca92..4847aae78db2 100644
> > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > > > > > @@ -404,6 +404,8 @@ struct dpu_rotation_cfg {
> > > > > >     * @pixel_ram_size     size of latency hiding and de-tiling 
> > > > > > buffer in bytes
> > > > > >     * @max_hdeci_exp      max horizontal decimation supported (max 
> > > > > > is 2^value)
> > > > > >     * @max_vdeci_exp      max vertical decimation supported (max 
> > > > > > is 2^value)
> > > > > > + * @format_list: Pointer to list of supported formats
> > > > > > + * @num_formats: Number of supported formats
> > > > > >     */
> > > > > >    struct dpu_caps {
> > > > > >        u32 max_mixer_width;
> > > > > > @@ -419,6 +421,8 @@ struct dpu_caps {
> > > > > >        u32 pixel_ram_size;
> > > > > >        u32 max_hdeci_exp;
> > > > > >        u32 max_vdeci_exp;
> > > > > > +     const u32 *format_list;
> > > > > > +     u32 num_formats;
> > > > > >    };
> > > > > >
> > > > > >    /**

--
With best wishes
Dmitry