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>
