On Wed, 6 Dec 2023 at 23:02, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 11/30/2023 3:47 PM, Abhinav Kumar wrote:
> >
> >
> > 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.
> >
>
> I was going through this more. I think its better we leave this as a
> dpu_hw_blk because if you see the other blks in struct dpu_rm, all the
> blocks which are allocated dynamically / can change dynamically are of
> dpu_hw_blk type. That way the dpu_rm_get_assigned_resources() remains
> generic. Hence I would prefer to leave it this way.
Ack
>
> >>
> >>> };
> >>>
> >>> /**
> >>> --
> >>> 2.40.1
> >>>
> >>
> >>
> >> --
> >> With best wishes
> >> Dmitry
--
With best wishes
Dmitry
