Re: [PATCH v2 14/16] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 12/8/2023 8:38 AM, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 18:34, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 12/8/2023 3:54 AM, Dmitry Baryshkov wrote:
>>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> Reserve CDM blocks for writeback if the format of the output fb
>>>> is YUV. At the moment, the reservation is done only for writeback
>>>> but can easily be extended by relaxing the checks once other
>>>> interfaces are ready to output YUV.
>>>>
>>>> changes in v2:
>>>>           - use needs_cdm from topology struct
>>>>           - drop fb related checks from atomic_mode_set()
>>>
>>> It looks like this should be squashed with the patch 11. The 'unbind
>>> CDM' doesn't really make sense without this patch. We need to allocate
>>> it first,  before touching it.
>>>
>>
>> The way I was thinking was that patch just completes the
>> dpu_encoder_phys_cleanup() and yes it was intentionally kept ahead
>> because that will not kick in till hw_cdm is assigned.
>>
>> Then, this patch only handles reserving/assignment of hw_cdm when needed.
>>
>> That was the motivation behind this split.
> 
> It leaves a leaf code that is not used at all. There is no need to
> cleanup anything if it was not allocated. Please remove the split and
> squash it with allocation.
> 

Ack. No concerns with squashing them.

>>
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++++++
>>>>    1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 862912727925..a576e3e62429 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -16,6 +16,7 @@
>>>>    #include <drm/drm_crtc.h>
>>>>    #include <drm/drm_file.h>
>>>>    #include <drm/drm_probe_helper.h>
>>>> +#include <drm/drm_framebuffer.h>
>>>>
>>>>    #include "msm_drv.h"
>>>>    #include "dpu_kms.h"
>>>> @@ -583,6 +584,7 @@ static int dpu_encoder_virt_atomic_check(
>>>>           struct drm_display_mode *adj_mode;
>>>>           struct msm_display_topology topology;
>>>>           struct dpu_global_state *global_state;
>>>> +       struct drm_framebuffer *fb;
>>>>           struct drm_dsc_config *dsc;
>>>>           int i = 0;
>>>>           int ret = 0;
>>>> @@ -623,6 +625,22 @@ static int dpu_encoder_virt_atomic_check(
>>>>
>>>>           topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
>>>>
>>>> +       /*
>>>> +        * Use CDM only for writeback at the moment as other interfaces cannot handle it.
>>>> +        * if writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
>>>> +        * earlier.
>>>> +        */
>>>> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
>>>> +               fb = conn_state->writeback_job->fb;
>>>> +
>>>> +               if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb))))
>>>> +                       topology.needs_cdm = true;
>>>> +               if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
>>>> +                       crtc_state->mode_changed = true;
>>>> +               else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
>>>> +                       crtc_state->mode_changed = true;
>>>> +       }
>>>> +
>>>>           /*
>>>>            * Release and Allocate resources on every modeset
>>>>            * Dont allocate when active is false.
>>>> @@ -1063,6 +1081,15 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>>
>>>>           dpu_enc->dsc_mask = dsc_mask;
>>>>
>>>> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
>>>> +               struct dpu_hw_blk *hw_cdm = NULL;
>>>> +
>>>> +               dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
>>>> +                                             drm_enc->base.id, DPU_HW_BLK_CDM,
>>>> +                                             &hw_cdm, 1);
>>>> +               dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
>>>> +       }
>>>> +
>>>>           cstate = to_dpu_crtc_state(crtc_state);
>>>>
>>>>           for (i = 0; i < num_lm; i++) {
>>>> --
>>>> 2.40.1
>>>>
>>>
>>>
>>> --
>>> With best wishes
>>>
>>> Dmitry
> 
> 
>