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

Abhinav Kumar
2023-12-08T16:50:36Z

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
>
>
>