Re: [PATCH] drm/vc4: Enable the DSI module and link before other enables.

[Andrzej Hajda]

On 05.06.2018 20:49, Eric Anholt wrote:
> Andrzej Hajda <a.hajda@xxxxxxxxxxx> writes:
>
>> On 04.06.2018 21:44, Eric Anholt wrote:
>>> We want the DSI PHY to be enabled and the DSI module clocked before a
>>> panel or bridge's prepare() function, since most DSI panel drivers
>>> with a prepare() hook are doing DCS transactions inside of them.
>>>
>>> Signed-off-by: Eric Anholt <eric@xxxxxxxxxx>
>>> Cc: Kevin Quigley <kevin@xxxxxxxxxxxxxx>
>>> Cc: James Hughes <james.hughes@xxxxxxxxxxxxxxx>
>>> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
>>> ---
>>>
>>> I'm not sure it makes sense to enable CRTCs before encoders on vc4 at
>>> all.  Why start scanning pixels before the encoder is ready to hear
>>> about VSTART?  However, this patch will hopefully unblock people
>>> trying to attach other panels to rpi
>> But this patch is about enabling encoder before panel/bridge prepare. Or
>> am I missing something.
>> Anyway I would be more precise here, MIPI-DSI bus is used for two things:
>> - control bus - accessing DSI device (configuration/detection,...),
>> - video data bus - sending video stream.
>>
>> I suspect in prepare/pre_enable phase of DSI panel/bridge only control
>> bus should be functional, video stream transfer does not make sense.
>> So the best solution (I guess) would be to split DSI-encoder enable
>> sequence into control bus enable and video transfer enable parts and
>> call the former before 1st transfer request from device and the latter
>> in encoder enable callback. Of course there will be a problem then with
>> disabling sequence, ie stopping video stream should be performed in
>> encoder's disable, but when we should disable control bus? If we do it
>> in encoder's disable callback we could not perform transfers in
>> post_disable cb of the bridge - in most cases (maybe all currently
>> present in kernel) it will work, but it does not look ideal.
>> All this recalls me discussion that hardwiring bridge callbacks into drm
>> core is problematic and maybe it would be better to call downstream
>> callbacks explicitly from upstream element - the callback order should
>> depend on the local bus protocol, and should not be fixed in drm core.
> It does seem like for DSI encoders they generally would need to be able
> to control when the call down to the bridge happens.  However, from my
> experience with panels, drivers are bad about calling both prepare and
> enable, if their particular panel only cares about one of them.  So, how
> about:
>
> - encoders can call drm_bridge_disable_midlayer_calls() (any naming
>   suggestions?) to flag this bridge as not wanting the calls from the
>   atomic helpers.
>
> - atomic helpers WARN if bridge midlayer_calls flag was set and
>   drm_bridge_pre_enable/enable/disable/post_disable failed to be called
>   by the encoder
>
> - drm_bridge_detach() clears disable_midlayer_calls flag for the next
>   encoder


I like the idea, I guess the flag should be placed in "struct
drm_bridge". Since I plan to propose flag to avoid device links in
panels/bridges maybe it would be good to set flags directly, or by
helper similar to irq_set_status_flags, instead of creating separate
helper for each flag.
I am not sure about warns from atomic helpers, maybe it would be enough
to track and verify state transitions of bridges similarly to the ones
proposed (and abandoned?) by Sean [1].

And little off-topic: all these duplication of
ideas/code/functionalities added to/from panels from/to bridges and this
crazy code:
ÂÂÂ sink = drm_of_find_panel_or_bridge...
ÂÂÂ if (sink is panel)
ÂÂÂ ÂÂÂ do_something
ÂÂÂ else if (sink is bridge)
ÂÂÂ ÂÂÂ do_something_similar_but_with_different_functions
laying in multiple encoders/bridges, provokes me to raise again a
question, wouldn't be better to merge drm_bridge and drm_panel into one
drm_sink object.

[1]: https://marc.info/?l=dri-devel&m=150827480716580

Regards
Andrzej