[Freedreno] [PATCH 1/3] drm/msm/dpu: move intf and wb assignment to dpu_encoder_setup_display()

[Abhinav Kumar]

On 6/15/2022 11:34 AM, Dmitry Baryshkov wrote:
> On 15/06/2022 20:11, Abhinav Kumar wrote:
>>
>>
>> On 6/15/2022 10:04 AM, Dmitry Baryshkov wrote:
>>> On 15/06/2022 19:40, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/15/2022 5:36 AM, Dmitry Baryshkov wrote:
>>>>> On 14/06/2022 22:32, Abhinav Kumar wrote:
>>>>>> intf and wb resources are not dependent on the rm global
>>>>>> state so need not be allocated during 
>>>>>> dpu_encoder_virt_atomic_mode_set().
>>>>>>
>>>>>> Move the allocation of intf and wb resources to 
>>>>>> dpu_encoder_setup_display()
>>>>>> so that we can utilize the hw caps even during atomic_check() phase.
>>>>>>
>>>>>> Since dpu_encoder_setup_display() already has protection against
>>>>>> setting invalid intf_idx and wb_idx, these checks can now
>>>>>> be dropped as well.
>>>>>>
>>>>>> Fixes: e02a559a720f ("make changes to dpu_encoder to support 
>>>>>> virtual encoder")
>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25 
>>>>>> +++++++------------------
>>>>>>   1 file changed, 7 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> index 3a462e327e0e..e991d4ba8a40 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> @@ -1048,24 +1048,6 @@ static void 
>>>>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>>>>           phys->hw_pp = dpu_enc->hw_pp[i];
>>>>>>           phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>>>>>> -        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>>>>>> -            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>>>>>> phys->intf_idx);
>>>>>> -
>>>>>> -        if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
>>>>>> -            phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
>>>>>> -
>>>>>> -        if (!phys->hw_intf && !phys->hw_wb) {
>>>>>> -            DPU_ERROR_ENC(dpu_enc,
>>>>>> -                      "no intf or wb block assigned at idx: 
>>>>>> %d\n", i);
>>>>>> -            return;
>>>>>> -        }
>>>>>> -
>>>>>> -        if (phys->hw_intf && phys->hw_wb) {
>>>>>> -            DPU_ERROR_ENC(dpu_enc,
>>>>>> -                    "invalid phys both intf and wb block at idx: 
>>>>>> %d\n", i);
>>>>>> -            return;
>>>>>> -        }
>>>>>
>>>>> Please retain these checks in dpu_encoder_setup_display().
>>>>> It checks that we really have got the intf or wb. For example one 
>>>>> might have specified the INTF that leads to INTF_NONE interface. Or 
>>>>> non-existing/not supported WB.
>>>>
>>>> Right, so the reason I omitted that was dpu_encoder_setup_display() 
>>>> already has these checks:
>>>>
>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2273 
>>>>
>>>>
>>>> Please check lines 2273-2284.
>>>>
>>>> Only if all those checks succeeded we call 
>>>> dpu_encoder_virt_add_phys_encs which increments num_phys_encs.
>>>
>>> As I wrote, it checks indices from phys_params, but not the acquired 
>>> hardware instances.
>>
>> Right but today, both the get_intf() and get_wb() just return the 
>> intf/wb corresponding to the index. So as long as the index is valid 
>> how will checking hw_wb or hw_intf be different?
>>
>> static inline struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, 
>> enum dpu_intf intf_idx)
>> {
>>      return rm->hw_intf[intf_idx - INTF_0];
>> }
>>
>> /**
>>   * dpu_rm_get_wb - Return a struct dpu_hw_wb instance given it's index.
>>   * @rm: DPU Resource Manager handle
>>   * @wb_idx: WB index
>>   */
>> static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum 
>> dpu_wb wb_idx)
>> {
>>      return rm->hw_wb[wb_idx - WB_0];
>> }
> 
> WB_0 is valid, but dpu_rm_get_wb(WB_0) will return NULL.
> INTF_0 is valid, but dpu_rm_get_intf(INTF_0) on qcm2290 will return NULL.
> 
> Etc.

Ah okay got it, let me add them back in v2.

> 
>>>
>>>>
>>>> Thats why I dropped those.
>>>>
>>>> Let me know if you have more questions.
>>>>
>>>>>
>>>>>> -
>>>>>>           phys->cached_mode = crtc_state->adjusted_mode;
>>>>>>           if (phys->ops.atomic_mode_set)
>>>>>>               phys->ops.atomic_mode_set(phys, crtc_state, 
>>>>>> conn_state);
>>>>>> @@ -2293,7 +2275,14 @@ static int dpu_encoder_setup_display(struct 
>>>>>> dpu_encoder_virt *dpu_enc,
>>>>>>           struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>>>>>           atomic_set(&phys->vsync_cnt, 0);
>>>>>>           atomic_set(&phys->underrun_cnt, 0);
>>>>>> +
>>>>>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>>>>>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>>>>>> phys->intf_idx);
>>>>>> +
>>>>>> +        if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
>>>>>> +            phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
>>>>>>       }
>>>>>> +
>>>>>>       mutex_unlock(&dpu_enc->enc_lock);
>>>>>>       return ret;
>>>>>
>>>>>
>>>
>>>
> 
>