From: Andrzej Hajda <a.hajda@samsung.com>
To: Doug Anderson <dianders@chromium.org>
Cc: Rob Clark <robdclark@chromium.org>,
Jernej Skrabec <jernej.skrabec@siol.net>,
Jonas Karlman <jonas@kwiboo.se>, David Airlie <airlied@linux.ie>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Neil Armstrong <narmstrong@baylibre.com>,
LKML <linux-kernel@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
Stephen Boyd <swboyd@chromium.org>,
Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>,
Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
Robert Foss <robert.foss@linaro.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Sam Ravnborg <sam@ravnborg.org>,
Steev Klimaszewski <steev@kali.org>
Subject: Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
Date: Tue, 6 Apr 2021 18:52:07 +0200 [thread overview]
Message-ID: <7bc4ce04-4110-8b8a-067b-824296b52480@samsung.com> (raw)
In-Reply-To: <CAD=FV=WS8=hi07tA=t_5xOfPkb8TqY63A712uhJg4H8pUPCRJw@mail.gmail.com>
Hello again after easter,
I have looked little bit more at sn65* driver and its application to
have better background.
I miss only info what panel do you have, how it is enabled/power controlled.
W dniu 01.04.2021 o 16:57, Doug Anderson pisze:
> Hi,
>
> On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>
>> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
>>> Hi,
>>>
>>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
>>>>> eDP panels won't provide their EDID unless they're powered on. Let's
>>>>> chain a power-on before we read the EDID. This roughly matches what
>>>>> was done in 'parade-ps8640.c'.
>>>>>
>>>>> NOTE: The old code attempted to call pm_runtime_get_sync() before
>>>>> reading the EDID. While that was enough to power the bridge chip on,
>>>>> it wasn't enough to talk to the panel for two reasons:
>>>>> 1. Since we never ran the bridge chip's pre-enable then we never set
>>>>> the bit to ignore HPD. This meant the bridge chip didn't even _try_
>>>>> to go out on the bus and communicate with the panel.
>>>>> 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read
>>>>> if the panel wasn't on.
>>>>>
>>>>> One thing that's a bit odd here is taking advantage of the EDID that
>>>>> the core might have cached for us. See the patch ("drm/edid: Use the
>>>>> cached EDID in drm_get_edid() if eDP"). We manage to get at the cache
>>>>> by:
>>>>> - Instantly failing aux transfers if we're not powered.
>>>>> - If the first read of the EDID fails we try again after powering.
>>>>>
>>>>> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>> ---
>>>>> Depending on what people think of the other patches in this series,
>>>>> some of this could change.
>>>>> - If everyone loves the "runtime PM" in the panel driver then we
>>>>> could, in theory, put the pre-enable chaining straight in the "aux
>>>>> transfer" function.
>>>>> - If everyone hates the EDID cache moving to the core then we can
>>>>> avoid some of the awkward flow of things and keep the EDID cache in
>>>>> the sn65dsi86 driver.
>>>> I wonder if this shouldn't be solved in the core - ie caller of
>>>> get_modes callback should be responsible for powering up the pipeline,
>>>> otherwise we need to repeat this stuff in every bridge/panel driver.
>>>>
>>>> Any thoughts?
>>> Yeah, I did look at this a little bit. Presumably it would only make
>>> sense to do it for eDP connections since:
>>>
>>> a) The concept of reading an EDID doesn't make sense for things like MIPI.
>> I guess you mean MIPI DSI
> Yes, sorry! I'll try to be more clear.
>
>
>> and yes I agree, more generally it usually(!)
>> doesn't make sense for any setup with fixed display panel.
>>
>> On the other hand there are DSI/HDMI or DSI/DP adapters which usually
>> have EDID reading logic.
>>
>> And the concept makes sense for most connectors accepting external
>> displays: HDMI, DP, MHL, VGA...
> So, actually, IMO the concept doesn't make sense for anything with an
> external connector. Here's the logic for a handful of connectors:
>
> 1. MIPI DSI: there is no EDID so this doesn't make sense.
>
> 2. An external connector (HDMI, DP, etc): the display that's plugged
> in is externally powered so doesn't need us to power it up to read the
> EDID. By definition, when the HPD signal is asserted then it's OK to
> read the EDID and we don't even know if a display is plugged in until
> HPD is asserted. Thus no special power sequencing is needed to read
> the EDID. (Yes, we need to make sure that the eDP controller itself
> is powered, but that doesn't seem like it's the core's business).
Not true IMO, even if external device is powered on, you must enable
EDID-reader logic.
I guess it is not uncommon to have different power states for EDID
reading and bridge/panel pre-enablement (especially in embedded world).
In fact there are setups where EDID-reader is totally different device
than the bridge itself, and these devices should be
powered/enabled/operational only for time of EDID reading.
>
> 3. eDP: this is where it matters. This is because:
>
> 3a) eDP displays aren't powered all the time. If you just boot up or
> you blank your screen, likely the display has no power at all.
>
> 3b) Because the display has no power, the "HPD" signal doesn't assert.
> In fact, for eDP the "HPD" signal really should mean "display ready"
> or "display finished powering up".
>
> 3c) Even though we never get a HPD signal, we still simply assume that
> a display is present because this is an "embedded" device.
>
> So eDP is unique (as far as I know) in that it's a type of display
> that has an EDID but that we will report "a display is here" before
> we've powered up the display and before we can read the EDID.
>
>>> b) For something with an external connector (DP and HDMI) you don't
>>> even know they're inserted unless the EDID is ready to read (these
>>> devices are, essentially, always powered).
>> Usually there are two elements which are not the same:
>>
>> 1. HotPlug signal/wire.
>>
>> 2. EDID reading logic.
>>
>> The logic responsible for reading EDID needs to be enabled only for time
>> required for EDID reading :) So it's power state often must be
>> controlled explicitly by the bridge driver. So even if in many cases
>> pre_enable powers on the logic for EDID reading it does not make it the
>> rule, so I must step back from my claim that it is up to caller :)
> OK, I'll plan to keep it in the bridge chip driver now.
>
>
>>> So I started off trying to do this in the core for eDP, but then it
>>> wasn't completely clear how to write this code in a way that was super
>>> generic. Specifically:
>>>
>>> 1. I don't think it's a 100% guarantee that everything is powered on
>>> in pre-enable and powered off in post-disable. In this bridge chip
>>> it's true, but maybe not every eDP driver? Would you want me to just
>>> assume this, or add a flag?
>> Ok, pre_enable should power on the chip, but for performing
>> initialization of video transport layer. Assumption it will power on
>> EDID logic is incorrect, so my claim seems wrong, but also this patch
>> looks incorrect :)
>>
>> In general only device containing EDID logic knows how to power it up.
> I still believe my patch is correct. Specifically I don't need to make
> any assumptions about display elements upstream of me (sources of the
> bridge chip). I only need to make assumptions about the pre-enable of
> the bridge driver itself and anything downstream of it.
>
> At the moment downstream of this particular bridge chip is always a
> panel device. Even further, all known downstream devices are
> "simple-panel". That is known to power up the panel enough to read the
> EDID in the "prepare" stage.
>
> Sure, someone _could_ add another bridge downstream in some design,
> but it would be up to that person to either fix that downstream driver
> to power itself in pre-enable or to add some type of quirk disabling
> the EDID reading.
>
>
>> Since I do not know your particular case I can propose few possible ways
>> to investigate:
>>
>> - call bridge.next->get_modes - you leave responsibility for powering up
>> to the downstream device.
> The "next" bridge is the panel, so I don't think this works.
Then drm_panel_get_modes will work then.
>
>
>> - ddc driver on i2c request should power up the panel - seems also correct,
> Right, so I could put the
> "drm_bridge_chain_pre_enable(&pdata->bridge)" into the
> ti_sn_aux_transfer() function. I talked about that a little bit "after
> the cut" in my post where I said:
>
>> - If everyone loves the "runtime PM" in the panel driver then we
>> could, in theory, put the pre-enable chaining straight in the "aux
>> transfer" function.
> The reason for the dependence on "runtime PM" in the panel driver is
> that we are doing DDC over AUX and it breaks the EDID reading into
> lots of chunks so if we did the powering up and powering down there it
> would be crazy slow without the delayed poweroff.
OK, it resembles to me DSI-controlled panel - to query/configure panel
panel driver asks DSI-host to transfer some bytes to the panel and/or
back via DSI-bus.
In case of eDP panels we could do similar thing to read edid - we call
drm_panel_get_modes - it calls drm_panel_funcs.get_modes callback and it
decides (based on DT) if it should fill modes according to hardcoded
info into the driver or to ask the physical panel via DP-controller -
this way all the players (the panel, AUX/DDC device) will know what to
power-up.
I guess there is missing pieces - there is no DP bus :), I am not sure
if there is straight way to access panel's aux/ddc from the panel
driver, maybe somehow via drm_connector ???
Of course this only my idea - to be discussed with others.
Regards
Andrzej
>
> -Doug
>
next prev parent reply other threads:[~2021-04-06 16:52 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-30 2:53 [PATCH v2 00/14] drm: Fix EDID reading on ti-sn65dsi86 Douglas Anderson
2021-03-30 2:53 ` [PATCH v2 01/14] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable() Douglas Anderson
2021-03-31 9:05 ` Andrzej Hajda
2021-03-30 2:53 ` [PATCH v2 02/14] drm/bridge: ti-sn65dsi86: Simplify refclk handling Douglas Anderson
2021-03-31 9:09 ` Andrzej Hajda
2021-03-30 2:53 ` [PATCH v2 03/14] drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment Douglas Anderson
2021-03-31 9:10 ` Andrzej Hajda
2021-03-30 2:53 ` [PATCH v2 04/14] drm/bridge: ti-sn65dsi86: Reorder remove() Douglas Anderson
2021-03-31 9:50 ` Andrzej Hajda
2021-03-30 2:53 ` [PATCH v2 05/14] drm/bridge: ti-sn65dsi86: Move MIPI detach() / unregister() to detach() Douglas Anderson
2021-03-31 9:53 ` Andrzej Hajda
2021-03-31 16:43 ` Doug Anderson
2021-03-30 2:53 ` [PATCH v2 06/14] drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable() Douglas Anderson
2021-03-31 9:54 ` Andrzej Hajda
2021-03-30 2:53 ` [PATCH v2 07/14] drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function Douglas Anderson
2021-03-31 9:55 ` Andrzej Hajda
2021-03-30 2:53 ` [PATCH v2 08/14] drm/bridge: ti-sn65dsi86: Remove extra call: drm_connector_update_edid_property() Douglas Anderson
2021-03-31 9:58 ` Andrzej Hajda
2021-03-30 2:53 ` [PATCH v2 09/14] drm/edid: Use the cached EDID in drm_get_edid() if eDP Douglas Anderson
2021-03-30 14:01 ` Ville Syrjälä
2021-03-30 21:31 ` Doug Anderson
2021-03-30 2:53 ` [PATCH v2 10/14] drm/bridge: ti-sn65dsi86: Stop caching the EDID ourselves Douglas Anderson
2021-03-31 10:12 ` Andrzej Hajda
2021-03-31 14:32 ` Doug Anderson
2021-03-30 2:53 ` [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID Douglas Anderson
2021-03-31 11:08 ` Andrzej Hajda
2021-03-31 14:48 ` Doug Anderson
2021-04-01 11:12 ` Andrzej Hajda
2021-04-01 14:57 ` Doug Anderson
2021-04-06 16:52 ` Andrzej Hajda [this message]
2021-04-15 0:47 ` Laurent Pinchart
2021-04-15 1:18 ` Doug Anderson
2021-03-30 2:53 ` [PATCH v2 12/14] drm/bridge: ti-sn65dsi86: Read the EDID only if refclk was provided Douglas Anderson
2021-03-30 2:53 ` [PATCH v2 13/14] drm/bridge: ti-sn65dsi86: Print an error if we fallback to panel modes Douglas Anderson
2021-03-30 2:53 ` [PATCH v2 14/14] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare Douglas Anderson
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=7bc4ce04-4110-8b8a-067b-824296b52480@samsung.com \
--to=a.hajda@samsung.com \
--cc=Laurent.pinchart@ideasonboard.com \
--cc=airlied@linux.ie \
--cc=bjorn.andersson@linaro.org \
--cc=dianders@chromium.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=jernej.skrabec@siol.net \
--cc=jonas@kwiboo.se \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=narmstrong@baylibre.com \
--cc=robdclark@chromium.org \
--cc=robert.foss@linaro.org \
--cc=sam@ravnborg.org \
--cc=stanislav.lisovskiy@intel.com \
--cc=steev@kali.org \
--cc=swboyd@chromium.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).