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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
