Re: [PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 5/1/2023 2:27 PM, Dmitry Baryshkov wrote:
> On 02/05/2023 00:22, Abhinav Kumar wrote:
>>
>>
>> On 5/1/2023 1:45 PM, Dmitry Baryshkov wrote:
>>> On 01/05/2023 22:58, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>>>> There is no reason to split the dpu_encoder interface into separate
>>>>> _init() and _setup() phases. Merge them into a single function.
>>>>>
>>>>
>>>> I think the reason for having this split was to pass a valid encoder 
>>>> to the interface_modeset_init() and then do the rest of encoder 
>>>> initialization after modeset_init().
>>>>
>>>> Looking at the current code, one issue i am seeing is that you will 
>>>> now initialize the dpu_encoder's msm_display_info along with 
>>>> dpu_encoder_init().
>>>>
>>>> Most of it is fine but in the case of bonded_dsi(), I see an issue.
>>>>
>>>> The info.num_of_h_tiles++ happens after the modeset_init() of the 
>>>> second dsi but now it has been moved earlier.
>>>>
>>>> If for some reason, msm_dsi_modeset_init() fails for the second DSI, 
>>>> num_of_h_tiles will still be 2 now.
>>>
>>> If msm_dsi_modeset_init() fails, the function will err out and fail 
>>> dpu_kms initialization. So it's not important, what is the value of 
>>> num_h_tiles in this case.
>>>
>>
>> But I still feel the msm_display_info should be saved in the dpu 
>> encoder after the modeset_init() and not before. That way if some 
>> display interface specific init is done in the modeset_init(), we save 
>> the info after that.
> 
> Up to now we have been using 'poll' model, e.g. we specifically asked 
> for the DSC info from the DSI host rather than making msm_dsi set it. So 
> far I don't see a good reason why this should be changed.
> 

Ok got it, so my concern came from the fact that we individually poll 
each feature today but lets say the number of features keeps growing we 
will have to combine them all into xxx_xxx_get_disp_info() which fills 
up all the fields of the display_info in one go.

But yes, as long as we do that before calling dpu_encoder_init() it 
should be fine.

Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>