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

[Dmitry Baryshkov]

On Fri, 8 Dec 2023 at 18:34, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 12/8/2023 3:54 AM, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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.

>
> >>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> >> ---
> >>   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



-- 
With best wishes
Dmitry