Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
Hi,
On Tue, Apr 6, 2021 at 9:52 AM Andrzej Hajda wrote:
>
> Hello again after easter,
Sorry, I was out last week and I'm just getting back to this now.
> 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.
I am personally working on "sc7180-trogdor" boards right now
(arch/arm64/boot/dts/qcom/sc7180-trogdor*.dts). However I believe that
this patch series also enables proper EDID reading on
"sdm850-lenovo-yoga-c630.dts". That board, while also a Qualcomm
board, has completely different heritage than the trogdor ones. It's a
laptop that ships with Windows and (as far as I know) was designed
mostly independently.
On the trogdor boards the bridge has some power rails and an enable
line hooked up to it and the bridge itself can work when these rails
are turned on. The panel is on a separate power rail and you can't
talk to the panel at all until it's powered on and then asserts HPD to
us to say it finished its boot sequence.
> W dniu 01.04.2021 o 16:57, Doug Anderson pisze:
> > Hi,
> >
> > On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda wrote:
> >>
> >> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> >>> Hi,
> >>>
> >>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda 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
> > ---
> > 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
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
Hi Andrzej,
On Tue, Apr 06, 2021 at 06:52:07PM +0200, Andrzej Hajda wrote:
> 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:
> > On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda wrote:
> >> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> >>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda 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
> > ---
> > 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.
Sure, but I think Doug was referring to powering up the device connected
to the SN65DSI86 output. When that device (from a DT and DRM bridge
point of view) is an external connector, it means that the hardware
device is an external HDMI/DP sink, and we have no way to control its
power. The SN65DSI86 itself of course needs to be powered.
> 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
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
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 wrote:
>>
>> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
>>> Hi,
>>>
>>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda 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
> ---
> 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
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
Hi,
On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda wrote:
>
>
> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> > Hi,
> >
> > On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda 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
> >>> ---
> >>> 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).
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
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> Hi,
>
> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda 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
>>> ---
>>> 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, 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...
>
> 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 :)
>
> 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.
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.
- ddc driver on i2c request should power up the panel - seems also correct,
Regards
Andrzej
>
> 2. It wasn't totally clear to me which state to use for telling if the
> bridge had already been pre-enabled so I could avoid double-calling
> it. I could dig more if need be but I spent a bit of time looking and
> was coming up empty. If you have advice I'd appreciate it, though.
>
> 3. It wasn't clear to me if I should be using the atomic version
> (drm_atomic_bridge_chain_pre_enable) if I put this in the core and how
> exactly to do this, though I am a self-admitted DRM noob. I can do
> more digging if need be. Again, advice is appreciated.
>
> 4. Since I got feedback that the EDID caching belongs in the driver,
> not in the core [1] then we might end up powering things up
> pointlessly since the core wouldn't know if the driver was going to
> return the cache
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
Hi,
On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda 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
> > ---
> > 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.
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).
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?
2. It wasn't totally clear to me which state to use for telling if the
bridge had already been pre-enabled so I could avoid double-calling
it. I could dig more if need be but I spent a bit of time looking and
was coming up empty. If you have advice I'd appreciate it, though.
3. It wasn't clear to me if I should be using the atomic version
(drm_atomic_bridge_chain_pre_enable) if I put this in the core and how
exactly to do this, though I am a self-admitted DRM noob. I can do
more digging if need be. Again, advice is appreciated.
4. Since I got feedback that the EDID caching belongs in the driver,
not in the core [1] then we might end up powering things up
pointlessly since the core wouldn't know if the driver was going to
return the cache or not.
Given that this patch isn't too much code and not too complicated (and
will be even less complicated if I move the EDID caching back into the
driver), maybe we can land it and if we see the pattern repeat a bunch
more times then think about moving it to the core?
[1] https://lore.kernel.org/dri-devel/ygmvo3pndcibm...@intel.com/
-Doug
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
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
> ---
> 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?
Regards
Andrzej
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 +--
> 1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c0398daaa4a6..673c9f1c2d8e 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -128,6 +128,7 @@
>* @dp_lanes: Count of dp_lanes we're using.
>* @ln_assign:Value to program to the LN_ASSIGN register.
>* @ln_polrs: Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
> + * @pre_enabled: If true then pre_enable() has run.
>*
>* @gchip:If we expose our GPIOs, this is used.
>* @gchip_output: A cache of whether we've set GPIOs to output. This
> @@ -155,6 +156,7 @@ struct ti_sn_bridge {
> int dp_lanes;
> u8 ln_assign;
> u8 ln_polrs;
> + boolpre_enabled;
>
> #if defined(CONFIG_OF_GPIO)
> struct gpio_chipgchip;
> @@ -268,11 +270,33 @@ static int ti_sn_bridge_connector_get_modes(struct
> drm_connector *connector)
> {
> struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> struct edid *edid;
> + bool was_enabled;
> int num = 0;
>
> - pm_runtime_get_sync(pdata->dev);
> + /*
> + * Try to get the EDID first without anything special. There are
> + * three things that could happen with this call.
> + * a) It might just return from its cache.
> + * b) It might try to initiate an AUX transfer which might work.
> + * c) It might try to initiate an AUX transfer which might fail because
> + *we're not powered up.
> + *
> + * If we get a failure we'll assume case c) and try again. NOTE: we
> + * don't want to power up every time because that's slow and we don't
> + * have visibility into whether the data has already been cached.
> + */
> edid = drm_get_edid(connector, >aux.ddc);
> - pm_runtime_put(pdata->dev);
> + if (!edid) {
> + was_enabled = pdata->pre_enabled;
> +
> + if (!was_enabled)
> + drm_bridge_chain_pre_enable(>bridge);
> +
> + edid = drm_get_edid(connector, >aux.ddc);
> +
> + if (!was_enabled)
> + drm_bridge_chain_post_disable(>bridge);
> + }
>
> if (edid) {
> if (drm_edid_is_valid(edid))
> @@ -852,12 +876,16 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge
> *bridge)
> HPD_DISABLE);
>
> drm_panel_prepare(pdata->panel);
> +
> + pdata->pre_enabled = true;
> }
>
> static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> {
> struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>
> + pdata->pre_enabled = false;
> +
> drm_panel_unprepare(pdata->panel);
>
> clk_disable_unprepare(pdata->refclk);
> @@ -891,6 +919,13 @@ static ssize_t
