| Message ID | 20230321011821.635977-11-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,01/13] drm/atomic-helper: split not-scaling part of drm_atomic_helper_check_plane_state | expand |
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? > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > .../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; > }; > > /**
On 5/24/2023 6:47 PM, Dmitry Baryshkov wrote: > On Thu, 25 May 2023 at 02:16, Abhinav Kumar <quic_abhinavk@quicinc.com> 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 > 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@linaro.org> >>> --- >>> .../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; >>> }; >>> >>> /** > > >
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@quicinc.com> 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. 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. > > >> 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@linaro.org> >>>> --- >>>> .../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; >>>> }; >>>> >>>> /** >> >> >>
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@quicinc.com> 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. > 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@linaro.org> >>>>> --- >>>>> .../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; >>>>> }; >>>>> >>>>> /** >>> >>> >>> >
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@quicinc.com> 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@linaro.org> >>>>>> --- >>>>>> .../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; >>>>>> }; >>>>>> >>>>>> /** >>>> >>>> >>>> >>
On 6/6/2023 2:52 PM, Dmitry Baryshkov wrote: > 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@quicinc.com> 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. > No, the DPU revision checks are there to add P010 UBWC formats on top of the Vig formats. At the moment, the latest downstream code (code which is not on CLO hence I cannot share) has dropped all that and just checks if P010 UBWC is supported for the Vig SSPP and adds all those. So its still tied to the feature bit whether P010 UBWC is supported in the Vig SSPP and not at the chipset level. >> >>> 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@linaro.org> >>>>>>> --- >>>>>>> .../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; >>>>>>> }; >>>>>>> >>>>>>> /** >>>>> >>>>> >>>>> >>> >
On 07/06/2023 01:47, Abhinav Kumar wrote: > > > On 6/6/2023 2:52 PM, Dmitry Baryshkov wrote: >> 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@quicinc.com> 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. >> > > No, the DPU revision checks are there to add P010 UBWC formats on top of > the Vig formats. > > At the moment, the latest downstream code (code which is not on CLO > hence I cannot share) has dropped all that and just checks if P010 UBWC > is supported for the Vig SSPP and adds all those. > > So its still tied to the feature bit whether P010 UBWC is supported in > the Vig SSPP and not at the chipset level. So, what is the difference? This means that depending on some conditions either we can support P010 UBWC or we can not. So the list of all suppored formats is not static. > >>> >>>> 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. >>>>>>
On 6/6/2023 3:50 PM, Dmitry Baryshkov wrote: > On 07/06/2023 01:47, Abhinav Kumar wrote: >> >> >> On 6/6/2023 2:52 PM, Dmitry Baryshkov wrote: >>> 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@quicinc.com> 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. >>> >> >> No, the DPU revision checks are there to add P010 UBWC formats on top >> of the Vig formats. >> >> At the moment, the latest downstream code (code which is not on CLO >> hence I cannot share) has dropped all that and just checks if P010 >> UBWC is supported for the Vig SSPP and adds all those. >> >> So its still tied to the feature bit whether P010 UBWC is supported in >> the Vig SSPP and not at the chipset level. > > So, what is the difference? This means that depending on some conditions > either we can support P010 UBWC or we can not. So the list of all > suppored formats is not static. > The difference is SSPP level vs chipset level. This patch was made with chipset level in mind and had to expand the format list for each chipset. What I am suggesting is its a SSPP level decision. Please note its not just P010 UBWC but P010 UBWC FOR Vig SSPP. So expanding each chipset's format list is not needed when the decision can be made just on the basis of the SSPP's feature flag OR the presence of the csc blk. >> >>>> >>>>> 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. >>>>>>> >
On 07/06/2023 01:57, Abhinav Kumar wrote: > > > On 6/6/2023 3:50 PM, Dmitry Baryshkov wrote: >> On 07/06/2023 01:47, Abhinav Kumar wrote: >>> >>> >>> On 6/6/2023 2:52 PM, Dmitry Baryshkov wrote: >>>> 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@quicinc.com> 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. >>>> >>> >>> No, the DPU revision checks are there to add P010 UBWC formats on top >>> of the Vig formats. >>> >>> At the moment, the latest downstream code (code which is not on CLO >>> hence I cannot share) has dropped all that and just checks if P010 >>> UBWC is supported for the Vig SSPP and adds all those. >>> >>> So its still tied to the feature bit whether P010 UBWC is supported >>> in the Vig SSPP and not at the chipset level. >> >> So, what is the difference? This means that depending on some >> conditions either we can support P010 UBWC or we can not. So the list >> of all suppored formats is not static. >> > > The difference is SSPP level vs chipset level. This patch was made with > chipset level in mind and had to expand the format list for each chipset. > > What I am suggesting is its a SSPP level decision. Please note its not > just P010 UBWC but P010 UBWC FOR Vig SSPP. > > So expanding each chipset's format list is not needed when the decision > can be made just on the basis of the SSPP's feature flag OR the presence > of the csc blk. Maybe I misunderstand something. Is P010 UBWC format supported on VIG SSPPs on all generations? The code that you have pointed me suggests that is is not. > >>> >>>>> >>>>>> 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. >>>>>>>> >>
On 6/6/2023 3:59 PM, Dmitry Baryshkov wrote: > On 07/06/2023 01:57, Abhinav Kumar wrote: >> >> >> On 6/6/2023 3:50 PM, Dmitry Baryshkov wrote: >>> On 07/06/2023 01:47, Abhinav Kumar wrote: >>>> >>>> >>>> On 6/6/2023 2:52 PM, Dmitry Baryshkov wrote: >>>>> 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@quicinc.com> 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. >>>>> >>>> >>>> No, the DPU revision checks are there to add P010 UBWC formats on >>>> top of the Vig formats. >>>> >>>> At the moment, the latest downstream code (code which is not on CLO >>>> hence I cannot share) has dropped all that and just checks if P010 >>>> UBWC is supported for the Vig SSPP and adds all those. >>>> >>>> So its still tied to the feature bit whether P010 UBWC is supported >>>> in the Vig SSPP and not at the chipset level. >>> >>> So, what is the difference? This means that depending on some >>> conditions either we can support P010 UBWC or we can not. So the list >>> of all suppored formats is not static. >>> >> >> The difference is SSPP level vs chipset level. This patch was made >> with chipset level in mind and had to expand the format list for each >> chipset. >> >> What I am suggesting is its a SSPP level decision. Please note its not >> just P010 UBWC but P010 UBWC FOR Vig SSPP. >> >> So expanding each chipset's format list is not needed when the >> decision can be made just on the basis of the SSPP's feature flag OR >> the presence of the csc blk. > > Maybe I misunderstand something. Is P010 UBWC format supported on VIG > SSPPs on all generations? The code that you have pointed me suggests > that is is not. > No, your understanding is correct that P010 UBWC format is supported only on Vig SSPPs of certain generations. But my point is that, its still a SSPP level decision. So even if we add P010 UBWC formats later to the list of supported formats, what we will do is add that feature bit alone in the Vig SSPP's feature mask of that chipset and based on that all the P010 UBWC formats for the virtual planes. But if we follow your approach, we will add another array called plane_formats_p010_ubwc and add that to each chipset which will support it. The only difference in idea is that, based on the SSPP's feature set of that chipset we can still determine that Vs adding it to each chipset. >> >>>> >>>>>> >>>>>>> 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. >>>>>>>>> >>> >
On 07/06/2023 02:14, Abhinav Kumar wrote: > > > On 6/6/2023 3:59 PM, Dmitry Baryshkov wrote: >> On 07/06/2023 01:57, Abhinav Kumar wrote: >>> >>> >>> On 6/6/2023 3:50 PM, Dmitry Baryshkov wrote: >>>> On 07/06/2023 01:47, Abhinav Kumar wrote: >>>>> >>>>> >>>>> On 6/6/2023 2:52 PM, Dmitry Baryshkov wrote: >>>>>> 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@quicinc.com> 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. >>>>>> >>>>> >>>>> No, the DPU revision checks are there to add P010 UBWC formats on >>>>> top of the Vig formats. >>>>> >>>>> At the moment, the latest downstream code (code which is not on CLO >>>>> hence I cannot share) has dropped all that and just checks if P010 >>>>> UBWC is supported for the Vig SSPP and adds all those. >>>>> >>>>> So its still tied to the feature bit whether P010 UBWC is supported >>>>> in the Vig SSPP and not at the chipset level. >>>> >>>> So, what is the difference? This means that depending on some >>>> conditions either we can support P010 UBWC or we can not. So the >>>> list of all suppored formats is not static. >>>> >>> >>> The difference is SSPP level vs chipset level. This patch was made >>> with chipset level in mind and had to expand the format list for each >>> chipset. >>> >>> What I am suggesting is its a SSPP level decision. Please note its >>> not just P010 UBWC but P010 UBWC FOR Vig SSPP. >>> >>> So expanding each chipset's format list is not needed when the >>> decision can be made just on the basis of the SSPP's feature flag OR >>> the presence of the csc blk. >> >> Maybe I misunderstand something. Is P010 UBWC format supported on VIG >> SSPPs on all generations? The code that you have pointed me suggests >> that is is not. >> > > No, your understanding is correct that P010 UBWC format is supported > only on Vig SSPPs of certain generations. > > But my point is that, its still a SSPP level decision. > > So even if we add P010 UBWC formats later to the list of supported > formats, what we will do is add that feature bit alone in the Vig SSPP's > feature mask of that chipset and based on that all the P010 UBWC formats > for the virtual planes. > > But if we follow your approach, we will add another array called > plane_formats_p010_ubwc and add that to each chipset which will support it. sspp->sblk->formats_list is constant, so we will have to add another formats array anyway. > The only difference in idea is that, based on the SSPP's feature set of > that chipset we can still determine that Vs adding it to each chipset. > >>> >>>>> >>>>>>> >>>>>>>> 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. >>>>>>>>>> >>>> >>
On 6/6/2023 4:21 PM, Dmitry Baryshkov wrote: > On 07/06/2023 02:14, Abhinav Kumar wrote: >> >> >> On 6/6/2023 3:59 PM, Dmitry Baryshkov wrote: >>> On 07/06/2023 01:57, Abhinav Kumar wrote: >>>> >>>> >>>> On 6/6/2023 3:50 PM, Dmitry Baryshkov wrote: >>>>> On 07/06/2023 01:47, Abhinav Kumar wrote: >>>>>> >>>>>> >>>>>> On 6/6/2023 2:52 PM, Dmitry Baryshkov wrote: >>>>>>> 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@quicinc.com> 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. >>>>>>> >>>>>> >>>>>> No, the DPU revision checks are there to add P010 UBWC formats on >>>>>> top of the Vig formats. >>>>>> >>>>>> At the moment, the latest downstream code (code which is not on >>>>>> CLO hence I cannot share) has dropped all that and just checks if >>>>>> P010 UBWC is supported for the Vig SSPP and adds all those. >>>>>> >>>>>> So its still tied to the feature bit whether P010 UBWC is >>>>>> supported in the Vig SSPP and not at the chipset level. >>>>> >>>>> So, what is the difference? This means that depending on some >>>>> conditions either we can support P010 UBWC or we can not. So the >>>>> list of all suppored formats is not static. >>>>> >>>> >>>> The difference is SSPP level vs chipset level. This patch was made >>>> with chipset level in mind and had to expand the format list for >>>> each chipset. >>>> >>>> What I am suggesting is its a SSPP level decision. Please note its >>>> not just P010 UBWC but P010 UBWC FOR Vig SSPP. >>>> >>>> So expanding each chipset's format list is not needed when the >>>> decision can be made just on the basis of the SSPP's feature flag OR >>>> the presence of the csc blk. >>> >>> Maybe I misunderstand something. Is P010 UBWC format supported on VIG >>> SSPPs on all generations? The code that you have pointed me suggests >>> that is is not. >>> >> >> No, your understanding is correct that P010 UBWC format is supported >> only on Vig SSPPs of certain generations. >> >> But my point is that, its still a SSPP level decision. >> >> So even if we add P010 UBWC formats later to the list of supported >> formats, what we will do is add that feature bit alone in the Vig >> SSPP's feature mask of that chipset and based on that all the P010 >> UBWC formats for the virtual planes. >> >> But if we follow your approach, we will add another array called >> plane_formats_p010_ubwc and add that to each chipset which will >> support it. > > sspp->sblk->formats_list is constant, so we will have to add another > formats array anyway. > Not to each chipset. Yes you will have one new array for P010 UBWC formats which you will use to do the plane_init() if any of the Vig SSPPs have the feature bit set. As opposed to adding the format_list to each chipset like you are doing now. Its just a decision of whether you want to expand the catalog to have this for each chipset OR not when you can easily make that decision at run time using the SSPP's capabilities. BTW, all of this is just discussion for future. For this series, I think we both agree that you can just check if the chipset has any Vigs with CSC to populate YUV in the format list. >> The only difference in idea is that, based on the SSPP's feature set >> of that chipset we can still determine that Vs adding it to each chipset. >> >>>> >>>>>> >>>>>>>> >>>>>>>>> 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. >>>>>>>>>>> >>>>> >>> >
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; }; /**
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. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- .../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(+)