Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <27074b58-25ed-dbcc-1048-dbd9ba3135c9@quicinc.com>
Date:   Wed, 6 Dec 2023 13:02:01 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 07/16] drm/msm/dpu: add cdm blocks to RM
Content-Language: en-US
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC:     <freedreno@lists.freedesktop.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
        <quic_khsieh@quicinc.com>, <quic_parellan@quicinc.com>,
        <quic_jesszhan@quicinc.com>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        Sean Paul <sean@poorly.run>
References: <20230830224910.8091-1-quic_abhinavk@quicinc.com>
 <20230830224910.8091-8-quic_abhinavk@quicinc.com>
 <CAA8EJpoRcdHtyp3mym5HB2A=O6V4qUNTpnMkvm+OiSt7nHuXJw@mail.gmail.com>
 <5bcbb092-1d29-f795-3be4-5ab1c708cba0@quicinc.com>
In-Reply-To: <5bcbb092-1d29-f795-3be4-5ab1c708cba0@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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