LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrzej Hajda <a.hajda@samsung.com>
To: Eric Anholt <eric@anholt.net>, dri-devel@lists.freedesktop.org
Cc: Kevin Quigley <kevin@kquigley.co.uk>,
	Boris Brezillon <boris.brezillon@bootlin.com>,
	linux-kernel@vger.kernel.org,
	James Hughes <james.hughes@raspberrypi.org>,
	Archit Taneja <architt@codeaurora.org>
Subject: Re: [PATCH] drm/vc4: Enable the DSI module and link before other enables.
Date: Wed, 6 Jun 2018 10:28:02 +0200	[thread overview]
Message-ID: <20180606082805eucas1p29c806a82abbd8bdb9879c83211976fb9~1hNHMfcIE0348503485eucas1p2R@eucas1p2.samsung.com> (raw)
In-Reply-To: <87lgbtt6em.fsf@anholt.net>

On 05.06.2018 20:49, Eric Anholt wrote:
> Andrzej Hajda <a.hajda@samsung.com> 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@anholt.net>
>>> Cc: Kevin Quigley <kevin@kquigley.co.uk>
>>> Cc: James Hughes <james.hughes@raspberrypi.org>
>>> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
>>> ---
>>>
>>> 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

  reply	other threads:[~2018-06-06  8:28 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20180604194448epcas1p29627759e2d8c0b1388c6700a7da77a45@epcas1p2.samsung.com>
2018-06-04 19:44 ` Eric Anholt
2018-06-05  8:25   ` Andrzej Hajda
2018-06-05 18:49     ` Eric Anholt
2018-06-06  8:28       ` Andrzej Hajda [this message]
2018-06-07 17:22     ` Kevin Quigley
2018-06-21 23:19       ` Eric Anholt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='20180606082805eucas1p29c806a82abbd8bdb9879c83211976fb9~1hNHMfcIE0348503485eucas1p2R@eucas1p2.samsung.com' \
    --to=a.hajda@samsung.com \
    --cc=architt@codeaurora.org \
    --cc=boris.brezillon@bootlin.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=eric@anholt.net \
    --cc=james.hughes@raspberrypi.org \
    --cc=kevin@kquigley.co.uk \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH] drm/vc4: Enable the DSI module and link before other enables.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).