| Message ID | 20210330025345.3980086-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | drm: Fix EDID reading on ti-sn65dsi86 | expand |
On Mon, Mar 29, 2021 at 07:53:40PM -0700, Douglas Anderson wrote: > Each time we call drm_get_edid() we: > 1. Go out to the bus and ask for the EDID. > 2. Cache the EDID. > > We can improve this to actually use the cached EDID so that if > drm_get_edid() is called multiple times then we don't need to go out > to the bus over and over again. > > In normal DP/HDMI cases reading the EDID over and over again isn't > _that_ expensive so, presumably, this wasn't that critical in the > past. However for eDP going out to the bus can be expensive. This is > because eDP panels might be powered off before the EDID was requested > so we need to do power sequencing in addition to the transfer. > > In theory we should be able to cache the EDID for all types of > displays. There is already code throwing the cache away when we detect > that a display was unplugged. However, it can be noted that it's > _extra_ safe to cache the EDID for eDP since eDP isn't a hot-pluggable > interface. If we get the EDID once then we've got the EDID and we > should never need to read it again. For now we'll only use the cache > for eDP both because it's more important and extra safe. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/drm_edid.c | 32 ++++++++++++++++++++++++++++---- > 1 file changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index c2bbe7bee7b6..fcbf468d73c9 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2049,15 +2049,39 @@ struct edid *drm_get_edid(struct drm_connector *connector, > struct i2c_adapter *adapter) > { > struct edid *edid; > + size_t old_edid_size; > + const struct edid *old_edid; > > if (connector->force == DRM_FORCE_OFF) > return NULL; > > - if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) > - return NULL; > + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP && > + connector->edid_blob_ptr) { > + /* > + * eDP devices are non-removable, or at least not something > + * that's expected to be hot-pluggable. We can freely use > + * the cached EDID. > + * > + * NOTE: technically we could probably even use the cached > + * EDID even for non-eDP because the cached EDID should be > + * cleared if we ever notice a display is not connected, but > + * we'll use an abundance of caution and only do it for eDP. > + * It's more important for eDP anyway because the EDID may not > + * always be readable, like when the panel is powered down. > + */ > + old_edid = (const struct edid *)connector->edid_blob_ptr->data; > + old_edid_size = ksize(old_edid); > + edid = kmalloc(old_edid_size, GFP_KERNEL); > + if (edid) > + memcpy(edid, old_edid, old_edid_size); > + } else { > + if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) > + return NULL; > + > + edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); > + drm_connector_update_edid_property(connector, edid); > + } This is a pretty low level function. Too low level for this caching IMO. So I think better just do it a bit higher up like other drivers. > > - edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); > - drm_connector_update_edid_property(connector, edid); > return edid; > } > EXPORT_SYMBOL(drm_get_edid); > -- > 2.31.0.291.g576ba9dcdaf-goog > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Douglas, W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > The drm_bridge_chain_pre_enable() is not the proper opposite of > drm_bridge_chain_post_disable(). It continues along the chain to > _before_ the starting bridge. Let's fix that. > > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/drm_bridge.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 64f0effb52ac..044acd07c153 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) > list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { > if (iter->funcs->pre_enable) > iter->funcs->pre_enable(iter); > + > + if (iter == bridge) > + break; Looking at the bridge chaining code always makes me sick :) but beside this the change looks correct, and follows drm_atomic_bridge_chain_pre_enable. Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Regards Andrzej > } > } > EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > Let's make the remove() function strictly the reverse of the probe() > function so it's easier to reason about. > > NOTES: > - The MIPI calls probably belong in detach() but will be moved in a > separate patch. The mipi is incorrectly handled already - mipi devices are searched after bridge registration - it should be reverse, there is comment in the driver that it is due to some dsi hosts, maybe it would be better to fix it there instead of conserve this bad design. > - The cached EDID freeing isn't actually part of probe but needs to be > in remove to avoid orphaning memory until better handling of the > EDID happens. > This patch was created by code inspection and should move us closer to > a proper remove. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 76f43af6735d..c006678c9921 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -1315,20 +1315,21 @@ static int ti_sn_bridge_remove(struct i2c_client *client) > if (!pdata) > return -EINVAL; > > - kfree(pdata->edid); > - ti_sn_debugfs_remove(pdata); > - > - of_node_put(pdata->host_node); > - > - pm_runtime_disable(pdata->dev); > - > if (pdata->dsi) { > mipi_dsi_detach(pdata->dsi); > mipi_dsi_device_unregister(pdata->dsi); > } > > + kfree(pdata->edid); > + > + ti_sn_debugfs_remove(pdata); > + > drm_bridge_remove(&pdata->bridge); > > + pm_runtime_disable(pdata->dev); > + > + of_node_put(pdata->host_node); > + Looks good. Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Regards Andrzej > return 0; > } >
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. 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/YGMvO3PNDCiBmqov@intel.com/ -Doug
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, 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 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/YGMvO3PNDCiBmqov@intel.com/ > > -Doug > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/v1/url?k=e133dd76-bea8e47a-e1325639-000babff3563-c6b07779450426d5&q=1&e=fd12ab0a-1858-4d09-a3b6-0ff1336fc2ba&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel >
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). 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. > - 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. -Doug
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 >
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 <a.hajda@samsung.com> wrote: > >> W dniu 31.03.2021 o 16:48, Doug Anderson pisze: > >>> 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. 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 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. Not if the panel exposes modes through EDID, in that case it's the responsibility of the device connected to the DDC/AUX port to read the EDID and provide modes. The panel driver won't be able to handle it on its own. > >> - 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 ??? If the SN65DSI86 has to call drm_panel_get_modes(), which will then call back into the SN65DSI86 driver to perform the EDID read, it seems to me that the panel driver shouldn't be involved at all. DRM bridges have "recently" gained new operations to retrieve EDID, and there's a helper (drm_bridge_connector) that creates a connector for a chain of bridges, delegating connector operations to the appropriate bridge in the chain. This seems a better way forward to me (but I'm biased, as I've authored that code :-)). > Of course this only my idea - to be discussed with others. -- Regards, Laurent Pinchart
The primary goal of this series is to try to properly fix EDID reading for eDP panels using the ti-sn65dsi86 bridge. Previously we had a patch that added EDID reading but it turned out not to work at bootup. This caused some extra churn at bootup as we tried (and failed) to read the EDID several times and also ended up forcing us to use the hardcoded mode at boot. With this patch series I believe EDID reading is reliable at boot now and we never use the hardcoded mode. This series is the logical successor to the 3-part series containing the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk") [1] though only one actual patch is the same between the two. This series starts out with some general / obvious fixes and moves on to some more specific and maybe controversial ones. I wouldn't object to some of the earlier ones landing if they look ready. This patch was developed against drm-misc-next on a sc7180-trogdor-lazor device. To get things booting for me, I had to use Stephen's patch [2] to keep from crashing but otherwise all the patches I needed were here. [1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/ [2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.mtv.corp.google.com/ Changes in v2: - Removed 2nd paragraph in commit message. Douglas Anderson (14): drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable() drm/bridge: ti-sn65dsi86: Simplify refclk handling drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment drm/bridge: ti-sn65dsi86: Reorder remove() drm/bridge: ti-sn65dsi86: Move MIPI detach() / unregister() to detach() drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable() drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function drm/bridge: ti-sn65dsi86: Remove extra call: drm_connector_update_edid_property() drm/edid: Use the cached EDID in drm_get_edid() if eDP drm/bridge: ti-sn65dsi86: Stop caching the EDID ourselves drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID drm/bridge: ti-sn65dsi86: Read the EDID only if refclk was provided drm/bridge: ti-sn65dsi86: Print an error if we fallback to panel modes drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare drivers/gpu/drm/bridge/ti-sn65dsi86.c | 125 ++++++++++++++++---------- drivers/gpu/drm/drm_bridge.c | 3 + drivers/gpu/drm/drm_edid.c | 32 ++++++- drivers/gpu/drm/panel/Kconfig | 1 + drivers/gpu/drm/panel/panel-simple.c | 93 ++++++++++++++----- 5 files changed, 184 insertions(+), 70 deletions(-)