| Message ID | 20230830224910.8091-8-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [01/16] drm/msm/dpu: fix writeback programming for YUV cases | expand |
On Thu, 31 Aug 2023 at 01:50, Abhinav Kumar <quic_abhinavk@quicinc.com> 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? > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > 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. > + } 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. > }; > > /** > -- > 2.40.1 > -- With best wishes Dmitry
On 8/30/2023 4:48 PM, Dmitry Baryshkov wrote: > On Thu, 31 Aug 2023 at 01:50, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com> >> --- >> 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
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@quicinc.com> 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@quicinc.com> >>> --- >>> 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. >> >>> }; >>> >>> /** >>> -- >>> 2.40.1 >>> >> >> >> -- >> With best wishes >> Dmitry
On Wed, 6 Dec 2023 at 23:02, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com> 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@quicinc.com> > >>> --- > >>> 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
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); + } 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; }; /**
Add the RM APIs necessary to initialize and allocate CDM blocks by the rest of the DPU pipeline. Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- 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(+)