On 8/30/2023 4:48 PM, Dmitry Baryshkov wrote:
> On Thu, 31 Aug 2023 at 01:50, Abhinav Kumar <[email protected]> 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 <[email protected]>
>> ---
>> 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
>> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]> 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
> >> <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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