Re: [PATCH 07/16] drm/msm/dpu: add cdm blocks to RM

[Abhinav Kumar]

On 8/30/2023 4:48 PM, Dmitry Baryshkov wrote:
> On Thu, 31 Aug 2023 at 01:50, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > Add the RM APIs necessary to initialize and allocate CDM
> > blocks by the rest of the DPU pipeline.
>
> ... to be used by the rest?

Yes, thanks.


> > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 17 +++++++++++++++++
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h |  2 ++
> >   2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > index f9215643c71a..7b6444a3fcb1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > @@ -8,6 +8,7 @@
> >   #include "dpu_kms.h"
> >   #include "dpu_hw_lm.h"
> >   #include "dpu_hw_ctl.h"
> > +#include "dpu_hw_cdm.h"
> >   #include "dpu_hw_pingpong.h"
> >   #include "dpu_hw_sspp.h"
> >   #include "dpu_hw_intf.h"
> > @@ -90,6 +91,9 @@ int dpu_rm_destroy(struct dpu_rm *rm)
> >                  }
> >          }
> >
> > +       if (rm->cdm_blk)
> > +               dpu_hw_cdm_destroy(to_dpu_hw_cdm(rm->cdm_blk));
> > +
> >          for (i = 0; i < ARRAY_SIZE(rm->hw_wb); i++)
> >                  dpu_hw_wb_destroy(rm->hw_wb[i]);
> >
> > @@ -240,6 +244,19 @@ int dpu_rm_init(struct dpu_rm *rm,
> >                  rm->hw_sspp[sspp->id - SSPP_NONE] = hw;
> >          }
> >
> > +       if (cat->cdm) {
> > +               struct dpu_hw_cdm *hw;
> > +
> > +               hw = dpu_hw_cdm_init(cat->cdm, mmio);
> > +               /* CDM is optional so no need to bail out */
> > +               if (IS_ERR(hw)) {
> > +                       rc = PTR_ERR(hw);
> > +                       DPU_DEBUG("failed cdm object creation: err %d\n", rc);
>
> No. If it is a part of the catalog, we should fail here as we do in other cases.

I guess, the only reason for not failing here was other hw blocks are 
needed even for basic display to come up but cdm is only for YUV formats.

Thats the only reason to mark this a failure which is "OK" to ignore.

But I see your point that if someone is listing this in the catalog but 
still RM fails thats an error.

Hence, ack.

> > +               } else {
> > +                       rm->cdm_blk = &hw->base;
> > +               }
> > +       }
> > +
> >          return 0;
> >
> >   fail:
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > index 2b551566cbf4..29b221491926 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > @@ -22,6 +22,7 @@ struct dpu_global_state;
> >    * @hw_wb: array of wb hardware resources
> >    * @dspp_blks: array of dspp hardware resources
> >    * @hw_sspp: array of sspp hardware resources
> > + * @cdm_blk: cdm hardware resource
> >    */
> >   struct dpu_rm {
> >          struct dpu_hw_blk *pingpong_blks[PINGPONG_MAX - PINGPONG_0];
> > @@ -33,6 +34,7 @@ struct dpu_rm {
> >          struct dpu_hw_blk *merge_3d_blks[MERGE_3D_MAX - MERGE_3D_0];
> >          struct dpu_hw_blk *dsc_blks[DSC_MAX - DSC_0];
> >          struct dpu_hw_sspp *hw_sspp[SSPP_MAX - SSPP_NONE];
> > +       struct dpu_hw_blk *cdm_blk;
>
> struct dpu_hw_cdm *cdm (or cdm_blk), please.

Ack.

> >   };
> >
> >   /**
> > --
> > 2.40.1
>
>
> --
> With best wishes
> Dmitry