[PATCH v5 08/16] drm/msm/dpu: drop msm_format from struct dpu_hw_fmt_layout

[Abhinav Kumar]

On 7/17/2024 3:09 PM, Dmitry Baryshkov wrote:
> On Wed, 17 Jul 2024 at 23:15, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote:
>>> The struct dpu_hw_fmt_layout defines hardware data layout (addresses,
>>> sizes and pitches. Drop format field from this structure as it's not a
>> Missing closing brace ")" here?
>>
>>> part of the data layout.
>>>
>>
>> Its a bit subjective IMO whether you consider format as part of hardware
>> data layout or not. Registers do have format bitfields too so I am
>> somewhat unsure if this change is really needed.
> 
> It's not a part of the data buffer layout (num_planes, sizes, pitches
> and offsets) - the items that are defined by struct dpu_hw_fmt_layout.
> As such there is no need to store it in the structure. When necessary
> we can always get it from the framebuffer itself.
> 

Alright,

Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>

>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>> ---
>>>    .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    | 31 +++++++---------------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c        | 23 ++++++++--------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h        |  2 --
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c          |  4 +--
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h          |  3 ++-
>>>    5 files changed, 25 insertions(+), 38 deletions(-)
>>>
>>
>> <Snip>
>>
>>> @@ -318,15 +318,10 @@ static void dpu_encoder_phys_wb_setup(
>>>    {
>>>        struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>>        struct drm_display_mode mode = phys_enc->cached_mode;
>>> -     struct drm_framebuffer *fb = NULL;
>>>        struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
>>> -     struct drm_writeback_job *wb_job;
>>>        const struct msm_format *format;
>>> -     const struct msm_format *dpu_fmt;
>>>
>>> -     wb_job = wb_enc->wb_job;
>>>        format = msm_framebuffer_format(wb_enc->wb_job->fb);
>>> -     dpu_fmt = mdp_get_format(&phys_enc->dpu_kms->base, format->pixel_format, wb_job->fb->modifier);
>>>
>>
>> This is interesting. I wonder why I just didnt use format directly that
>> time itself :)
>>
>> Maybe I was thinking that mdp_get_format() will also match the modifiers
>> and return the corresponding msm_format.
>>
>>>        DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n",
>>>                        hw_wb->idx - WB_0, mode.name,
>>> @@ -338,9 +333,9 @@ static void dpu_encoder_phys_wb_setup(
>>>
>>>        dpu_encoder_phys_wb_set_qos(phys_enc);
>>>
>>> -     dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
>>> +     dpu_encoder_phys_wb_setup_fb(phys_enc, format);
>>>
>>> -     dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, CDM_CDWN_OUTPUT_WB);
>>> +     dpu_encoder_helper_phys_setup_cdm(phys_enc, format, CDM_CDWN_OUTPUT_WB);
>>>
>>>        dpu_encoder_phys_wb_setup_ctl(phys_enc);
>>>    }
>>> @@ -584,14 +579,6 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct dpu_encoder_phys *phys_enc
>>>
>>>        format = msm_framebuffer_format(job->fb);
>>>
>>> -     wb_cfg->dest.format = mdp_get_format(&phys_enc->dpu_kms->base,
>>> -                                          format->pixel_format, job->fb->modifier);
>>> -     if (!wb_cfg->dest.format) {
>>> -             /* this error should be detected during atomic_check */
>>> -             DPU_ERROR("failed to get format %p4cc\n", &format->pixel_format);
>>> -             return;
>>> -     }
>>> -
>>>        ret = dpu_format_populate_layout(aspace, job->fb, &wb_cfg->dest);
>>>        if (ret) {
>>>                DPU_DEBUG("failed to populate layout %d\n", ret);
> 
> 
>