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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxx>



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux