Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <8eea4a8e-0c70-3768-79f0-1a2bfe083ed7@quicinc.com>
Date:   Fri, 8 Dec 2023 09:53:00 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v2 15/16] drm/msm/dpu: introduce separate wb2_format
 arrays for rgb and yuv
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC:     <freedreno@lists.freedesktop.org>, Rob Clark <robdclark@gmail.com>,
        "Sean Paul" <sean@poorly.run>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        "David Airlie" <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, <quic_jesszhan@quicinc.com>,
        <quic_parellan@quicinc.com>, <linux-kernel@vger.kernel.org>,
        <dri-devel@lists.freedesktop.org>, <linux-arm-msm@vger.kernel.org>
References: <20231208050641.32582-1-quic_abhinavk@quicinc.com>
 <20231208050641.32582-16-quic_abhinavk@quicinc.com>
 <CAA8EJpqfCfETawp1up76S6gryO+Q4KxPB3ThwZCe7DCkp=GkBQ@mail.gmail.com>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAA8EJpqfCfETawp1up76S6gryO+Q4KxPB3ThwZCe7DCkp=GkBQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 12/8/2023 3:44 AM, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Lets rename the existing wb2_formats array wb2_formats_rgb to indicate
>> that it has only RGB formats and can be used on any chipset having a WB
>> block.
>>
>> Introduce a new wb2_formats_rgb_yuv array to the catalog to
>> indicate support for YUV formats to writeback in addition to RGB.
>>
>> Chipsets which have support for CDM block will use the newly added
>> wb2_formats_rgb_yuv array.
> 
> This means that the catalog can go out of sync, if one adds a CDM
> block but doesn't update wb_formats and vice versa.
> Can we deduce the format list from the WB code? Is the format list
> really static or does it change between platforms (please keep msm8996
> / msm8998 in mind).
> 

Yes this is a valid concern. catalog could potentially go out of sync.

I checked a few chipsets now and the WB formats didnt change among them.

I do need to check more chipsets but downstream does not maintain this 
in devicetree which means we can just move these arrays to WB code 
instead of maintaining them in the catalog.

We will still need to maintain two arrays. One to be used if CDM block 
has been added and the other if not.

I must confess one point though. I have not seen any chipset yet where 
WB block is present but CDM block is not.

So at this point, the only purpose of the two arrays will be till the 
point where CDM blk has been added to all the required chipsets in the 
catalog. Then we can drop the RGB only array and maintain the one which 
has all formats.

>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   .../msm/disp/dpu1/catalog/dpu_10_0_sm8650.h   |  4 +-
>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  4 +-
>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  4 +-
>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  4 +-
>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  4 +-
>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 37 ++++++++++++++++++-
>>   6 files changed, 46 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_10_0_sm8650.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_10_0_sm8650.h
>> index 04d2a73dd942..eb5dfff2ec4f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_10_0_sm8650.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_10_0_sm8650.h
>> @@ -341,8 +341,8 @@ static const struct dpu_wb_cfg sm8650_wb[] = {
>>                  .name = "wb_2", .id = WB_2,
>>                  .base = 0x65000, .len = 0x2c8,
>>                  .features = WB_SM8250_MASK,
>> -               .format_list = wb2_formats,
>> -               .num_formats = ARRAY_SIZE(wb2_formats),
>> +               .format_list = wb2_formats_rgb,
>> +               .num_formats = ARRAY_SIZE(wb2_formats_rgb),
>>                  .xin_id = 6,
>>                  .vbif_idx = VBIF_RT,
>>                  .maxlinewidth = 4096,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> index 58b0f50518c8..a57d50b1f028 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> @@ -336,8 +336,8 @@ static const struct dpu_wb_cfg sm8250_wb[] = {
>>                  .name = "wb_2", .id = WB_2,
>>                  .base = 0x65000, .len = 0x2c8,
>>                  .features = WB_SM8250_MASK,
>> -               .format_list = wb2_formats,
>> -               .num_formats = ARRAY_SIZE(wb2_formats),
>> +               .format_list = wb2_formats_rgb_yuv,
>> +               .num_formats = ARRAY_SIZE(wb2_formats_rgb_yuv),
>>                  .clk_ctrl = DPU_CLK_CTRL_WB2,
>>                  .xin_id = 6,
>>                  .vbif_idx = VBIF_RT,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> index bcfedfc8251a..7382ebb6e5b2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> @@ -157,8 +157,8 @@ static const struct dpu_wb_cfg sc7180_wb[] = {
>>                  .name = "wb_2", .id = WB_2,
>>                  .base = 0x65000, .len = 0x2c8,
>>                  .features = WB_SM8250_MASK,
>> -               .format_list = wb2_formats,
>> -               .num_formats = ARRAY_SIZE(wb2_formats),
>> +               .format_list = wb2_formats_rgb,
>> +               .num_formats = ARRAY_SIZE(wb2_formats_rgb),
>>                  .clk_ctrl = DPU_CLK_CTRL_WB2,
>>                  .xin_id = 6,
>>                  .vbif_idx = VBIF_RT,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> index 19c2b7454796..2f153e0b5c6a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> @@ -169,8 +169,8 @@ static const struct dpu_wb_cfg sc7280_wb[] = {
>>                  .name = "wb_2", .id = WB_2,
>>                  .base = 0x65000, .len = 0x2c8,
>>                  .features = WB_SM8250_MASK,
>> -               .format_list = wb2_formats,
>> -               .num_formats = ARRAY_SIZE(wb2_formats),
>> +               .format_list = wb2_formats_rgb_yuv,
>> +               .num_formats = ARRAY_SIZE(wb2_formats_rgb_yuv),
>>                  .clk_ctrl = DPU_CLK_CTRL_WB2,
>>                  .xin_id = 6,
>>                  .vbif_idx = VBIF_RT,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> index bf56265967c0..ad48defa154f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> @@ -315,8 +315,8 @@ static const struct dpu_wb_cfg sm8550_wb[] = {
>>                  .name = "wb_2", .id = WB_2,
>>                  .base = 0x65000, .len = 0x2c8,
>>                  .features = WB_SM8250_MASK,
>> -               .format_list = wb2_formats,
>> -               .num_formats = ARRAY_SIZE(wb2_formats),
>> +               .format_list = wb2_formats_rgb,
>> +               .num_formats = ARRAY_SIZE(wb2_formats_rgb),
>>                  .xin_id = 6,
>>                  .vbif_idx = VBIF_RT,
>>                  .maxlinewidth = 4096,
>> 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 1be3156cde05..c52cac7a2288 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -202,7 +202,7 @@ static const u32 rotation_v2_formats[] = {
>>          /* TODO add formats after validation */
>>   };
>>
>> -static const uint32_t wb2_formats[] = {
>> +static const uint32_t wb2_formats_rgb[] = {
>>          DRM_FORMAT_RGB565,
>>          DRM_FORMAT_BGR565,
>>          DRM_FORMAT_RGB888,
>> @@ -236,6 +236,41 @@ static const uint32_t wb2_formats[] = {
>>          DRM_FORMAT_XBGR4444,
>>   };
>>
>> +static const uint32_t wb2_formats_rgb_yuv[] = {
>> +       DRM_FORMAT_RGB565,
>> +       DRM_FORMAT_BGR565,
>> +       DRM_FORMAT_RGB888,
>> +       DRM_FORMAT_ARGB8888,
>> +       DRM_FORMAT_RGBA8888,
>> +       DRM_FORMAT_ABGR8888,
>> +       DRM_FORMAT_XRGB8888,
>> +       DRM_FORMAT_RGBX8888,
>> +       DRM_FORMAT_XBGR8888,
>> +       DRM_FORMAT_ARGB1555,
>> +       DRM_FORMAT_RGBA5551,
>> +       DRM_FORMAT_XRGB1555,
>> +       DRM_FORMAT_RGBX5551,
>> +       DRM_FORMAT_ARGB4444,
>> +       DRM_FORMAT_RGBA4444,
>> +       DRM_FORMAT_RGBX4444,
>> +       DRM_FORMAT_XRGB4444,
>> +       DRM_FORMAT_BGR565,
>> +       DRM_FORMAT_BGR888,
>> +       DRM_FORMAT_ABGR8888,
>> +       DRM_FORMAT_BGRA8888,
>> +       DRM_FORMAT_BGRX8888,
>> +       DRM_FORMAT_XBGR8888,
>> +       DRM_FORMAT_ABGR1555,
>> +       DRM_FORMAT_BGRA5551,
>> +       DRM_FORMAT_XBGR1555,
>> +       DRM_FORMAT_BGRX5551,
>> +       DRM_FORMAT_ABGR4444,
>> +       DRM_FORMAT_BGRA4444,
>> +       DRM_FORMAT_BGRX4444,
>> +       DRM_FORMAT_XBGR4444,
>> +       DRM_FORMAT_NV12,
>> +};
>> +
>>   /*************************************************************
>>    * SSPP sub blocks config
>>    *************************************************************/
>> --
>> 2.40.1
>>
> 
>