diff mbox series

[5.14,147/334] drm/bridge: ti-sn65dsi86: Dont read EDID blob over DDC

Message ID 20210913131118.330293390@linuxfoundation.org
State New
Headers show
Series None | expand

Commit Message

Greg Kroah-Hartman Sept. 13, 2021, 1:13 p.m. UTC
From: Douglas Anderson <dianders@chromium.org>

[ Upstream commit a70e558c151043ce46a5e5999f4310e0b3551f57 ]

This is really just a revert of commit 58074b08c04a ("drm/bridge:
ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.

The old code failed to read the EDID properly in a very important
case: before the bridge's pre_enable() was called. The way things need
to work:
1. Read the EDID.
2. Based on the EDID, decide on video settings and pixel clock.
3. Enable the bridge w/ the desired settings.

The way things were working:
1. Try to read the EDID but fail; fall back to hardcoded values.
2. Based on hardcoded values, decide on video settings and pixel clock.
3. Enable the bridge w/ the desired settings.
4. Try again to read the EDID, it works now!
5. Realize that the hardcoded settings weren't quite right.
6. Disable / reenable the bridge w/ the right settings.

The reasons for the failures were twofold:
a) 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.
b) Even if we fixed things to ignore HPD, the EDID still wouldn't read
   if the panel wasn't on.

Instead of reverting the code, we could fix it to set the HPD bit and
also power on the panel. However, it also works nicely to just let the
panel code read the EDID. Now that we've split the driver up we can
expose the DDC AUX channel bus to the panel node. The panel can take
charge of reading the EDID.

NOTE: in order for things to work, anyone that needs to read the EDID
will need to instantiate their panel using the new DP AUX bus (AKA by
listing their panel under the "aux-bus" node of the bridge chip in the
device tree).

In the future if we want to use the bridge chip to provide a full
external DP port (which won't have a panel) then we will have to
conditinally add EDID reading back in.

Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20210611101711.v10.9.I9330684c25f65bb318eff57f0616500f83eac3cc@changeid
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ----------------------
 1 file changed, 22 deletions(-)

Comments

Doug Anderson Sept. 13, 2021, 1:57 p.m. UTC | #1
Hi,

On Mon, Sep 13, 2021 at 6:51 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> From: Douglas Anderson <dianders@chromium.org>
>
> [ Upstream commit a70e558c151043ce46a5e5999f4310e0b3551f57 ]
>
> This is really just a revert of commit 58074b08c04a ("drm/bridge:
> ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.
>
> The old code failed to read the EDID properly in a very important
> case: before the bridge's pre_enable() was called. The way things need
> to work:
> 1. Read the EDID.
> 2. Based on the EDID, decide on video settings and pixel clock.
> 3. Enable the bridge w/ the desired settings.
>
> The way things were working:
> 1. Try to read the EDID but fail; fall back to hardcoded values.
> 2. Based on hardcoded values, decide on video settings and pixel clock.
> 3. Enable the bridge w/ the desired settings.
> 4. Try again to read the EDID, it works now!
> 5. Realize that the hardcoded settings weren't quite right.
> 6. Disable / reenable the bridge w/ the right settings.
>
> The reasons for the failures were twofold:
> a) 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.
> b) Even if we fixed things to ignore HPD, the EDID still wouldn't read
>    if the panel wasn't on.
>
> Instead of reverting the code, we could fix it to set the HPD bit and
> also power on the panel. However, it also works nicely to just let the
> panel code read the EDID. Now that we've split the driver up we can
> expose the DDC AUX channel bus to the panel node. The panel can take
> charge of reading the EDID.
>
> NOTE: in order for things to work, anyone that needs to read the EDID
> will need to instantiate their panel using the new DP AUX bus (AKA by
> listing their panel under the "aux-bus" node of the bridge chip in the
> device tree).
>
> In the future if we want to use the bridge chip to provide a full
> external DP port (which won't have a panel) then we will have to
> conditinally add EDID reading back in.
>
> Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Link: https://patchwork.freedesktop.org/patch/msgid/20210611101711.v10.9.I9330684c25f65bb318eff57f0616500f83eac3cc@changeid
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ----------------------
>  1 file changed, 22 deletions(-)

I guess it's not a huge deal, but I did respond to Sasha and request
that this patch be dropped from the stable queue unless the whole big
pile of patches was being backported. See:

https://lore.kernel.org/lkml/CAD=FV=U2dGjeEzp+K1vnLTj8oPJ-GKBTTKz2XQ1OZ7QF_sTHuw@mail.gmail.com/

I said:

> I would suggest against backporting this one unless you're going to
> backport the whole pile of DP AUX bus patches, which probably doesn't
> make sense for stable. Even though the old EDID reading was broken for
> the first read, it still worked for later reads. ...and the first read
. didn't crash or anything--it just timed out.


-Doug
Doug Anderson Sept. 13, 2021, 4:31 p.m. UTC | #2
Hi,

On Mon, Sep 13, 2021 at 9:09 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Sep 13, 2021 at 06:57:20AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Sep 13, 2021 at 6:51 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > From: Douglas Anderson <dianders@chromium.org>
> > >
> > > [ Upstream commit a70e558c151043ce46a5e5999f4310e0b3551f57 ]
> > >
> > > This is really just a revert of commit 58074b08c04a ("drm/bridge:
> > > ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.
> > >
> > > The old code failed to read the EDID properly in a very important
> > > case: before the bridge's pre_enable() was called. The way things need
> > > to work:
> > > 1. Read the EDID.
> > > 2. Based on the EDID, decide on video settings and pixel clock.
> > > 3. Enable the bridge w/ the desired settings.
> > >
> > > The way things were working:
> > > 1. Try to read the EDID but fail; fall back to hardcoded values.
> > > 2. Based on hardcoded values, decide on video settings and pixel clock.
> > > 3. Enable the bridge w/ the desired settings.
> > > 4. Try again to read the EDID, it works now!
> > > 5. Realize that the hardcoded settings weren't quite right.
> > > 6. Disable / reenable the bridge w/ the right settings.
> > >
> > > The reasons for the failures were twofold:
> > > a) 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.
> > > b) Even if we fixed things to ignore HPD, the EDID still wouldn't read
> > >    if the panel wasn't on.
> > >
> > > Instead of reverting the code, we could fix it to set the HPD bit and
> > > also power on the panel. However, it also works nicely to just let the
> > > panel code read the EDID. Now that we've split the driver up we can
> > > expose the DDC AUX channel bus to the panel node. The panel can take
> > > charge of reading the EDID.
> > >
> > > NOTE: in order for things to work, anyone that needs to read the EDID
> > > will need to instantiate their panel using the new DP AUX bus (AKA by
> > > listing their panel under the "aux-bus" node of the bridge chip in the
> > > device tree).
> > >
> > > In the future if we want to use the bridge chip to provide a full
> > > external DP port (which won't have a panel) then we will have to
> > > conditinally add EDID reading back in.
> > >
> > > Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Link: https://patchwork.freedesktop.org/patch/msgid/20210611101711.v10.9.I9330684c25f65bb318eff57f0616500f83eac3cc@changeid
> > > Signed-off-by: Sasha Levin <sashal@kernel.org>
> > > ---
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ----------------------
> > >  1 file changed, 22 deletions(-)
> >
> > I guess it's not a huge deal, but I did respond to Sasha and request
> > that this patch be dropped from the stable queue unless the whole big
> > pile of patches was being backported. See:
> >
> > https://lore.kernel.org/lkml/CAD=FV=U2dGjeEzp+K1vnLTj8oPJ-GKBTTKz2XQ1OZ7QF_sTHuw@mail.gmail.com/
> >
> > I said:
> >
> > > I would suggest against backporting this one unless you're going to
> > > backport the whole pile of DP AUX bus patches, which probably doesn't
> > > make sense for stable. Even though the old EDID reading was broken for
> > > the first read, it still worked for later reads. ...and the first read
> > . didn't crash or anything--it just timed out.
>
> I see a "bunch" of patches for this driver in this -rc, did Sasha not
> get them all?  If not, I can drop this one, but maybe it was needed for
> the follow-on patches?

It's been a long journey trying to make this bridge work better. I
think the easiest way to say it is that if you don't have the parent
of ${SUBJECT} patch, AKA:

e0bbcc6233f7 drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus

...then you don't have DP AUX bus support and you shouldn't take
${SUBJECT} patch. If you have that patch and it compiles / builds then
it means that you have all the proper dependencies. However, there are
_a lot_ of dependencies and I wouldn't suggest picking them all to
stable unless it's critical for someone.

-Doug
Greg Kroah-Hartman Sept. 14, 2021, 4:27 p.m. UTC | #3
On Mon, Sep 13, 2021 at 09:31:03AM -0700, Doug Anderson wrote:
> Hi,

> 

> On Mon, Sep 13, 2021 at 9:09 AM Greg Kroah-Hartman

> <gregkh@linuxfoundation.org> wrote:

> >

> > On Mon, Sep 13, 2021 at 06:57:20AM -0700, Doug Anderson wrote:

> > > Hi,

> > >

> > > On Mon, Sep 13, 2021 at 6:51 AM Greg Kroah-Hartman

> > > <gregkh@linuxfoundation.org> wrote:

> > > >

> > > > From: Douglas Anderson <dianders@chromium.org>

> > > >

> > > > [ Upstream commit a70e558c151043ce46a5e5999f4310e0b3551f57 ]

> > > >

> > > > This is really just a revert of commit 58074b08c04a ("drm/bridge:

> > > > ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.

> > > >

> > > > The old code failed to read the EDID properly in a very important

> > > > case: before the bridge's pre_enable() was called. The way things need

> > > > to work:

> > > > 1. Read the EDID.

> > > > 2. Based on the EDID, decide on video settings and pixel clock.

> > > > 3. Enable the bridge w/ the desired settings.

> > > >

> > > > The way things were working:

> > > > 1. Try to read the EDID but fail; fall back to hardcoded values.

> > > > 2. Based on hardcoded values, decide on video settings and pixel clock.

> > > > 3. Enable the bridge w/ the desired settings.

> > > > 4. Try again to read the EDID, it works now!

> > > > 5. Realize that the hardcoded settings weren't quite right.

> > > > 6. Disable / reenable the bridge w/ the right settings.

> > > >

> > > > The reasons for the failures were twofold:

> > > > a) 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.

> > > > b) Even if we fixed things to ignore HPD, the EDID still wouldn't read

> > > >    if the panel wasn't on.

> > > >

> > > > Instead of reverting the code, we could fix it to set the HPD bit and

> > > > also power on the panel. However, it also works nicely to just let the

> > > > panel code read the EDID. Now that we've split the driver up we can

> > > > expose the DDC AUX channel bus to the panel node. The panel can take

> > > > charge of reading the EDID.

> > > >

> > > > NOTE: in order for things to work, anyone that needs to read the EDID

> > > > will need to instantiate their panel using the new DP AUX bus (AKA by

> > > > listing their panel under the "aux-bus" node of the bridge chip in the

> > > > device tree).

> > > >

> > > > In the future if we want to use the bridge chip to provide a full

> > > > external DP port (which won't have a panel) then we will have to

> > > > conditinally add EDID reading back in.

> > > >

> > > > Suggested-by: Andrzej Hajda <a.hajda@samsung.com>

> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>

> > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> > > > Link: https://patchwork.freedesktop.org/patch/msgid/20210611101711.v10.9.I9330684c25f65bb318eff57f0616500f83eac3cc@changeid

> > > > Signed-off-by: Sasha Levin <sashal@kernel.org>

> > > > ---

> > > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ----------------------

> > > >  1 file changed, 22 deletions(-)

> > >

> > > I guess it's not a huge deal, but I did respond to Sasha and request

> > > that this patch be dropped from the stable queue unless the whole big

> > > pile of patches was being backported. See:

> > >

> > > https://lore.kernel.org/lkml/CAD=FV=U2dGjeEzp+K1vnLTj8oPJ-GKBTTKz2XQ1OZ7QF_sTHuw@mail.gmail.com/

> > >

> > > I said:

> > >

> > > > I would suggest against backporting this one unless you're going to

> > > > backport the whole pile of DP AUX bus patches, which probably doesn't

> > > > make sense for stable. Even though the old EDID reading was broken for

> > > > the first read, it still worked for later reads. ...and the first read

> > > . didn't crash or anything--it just timed out.

> >

> > I see a "bunch" of patches for this driver in this -rc, did Sasha not

> > get them all?  If not, I can drop this one, but maybe it was needed for

> > the follow-on patches?

> 

> It's been a long journey trying to make this bridge work better. I

> think the easiest way to say it is that if you don't have the parent

> of ${SUBJECT} patch, AKA:

> 

> e0bbcc6233f7 drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus

> 

> ...then you don't have DP AUX bus support and you shouldn't take

> ${SUBJECT} patch. If you have that patch and it compiles / builds then

> it means that you have all the proper dependencies. However, there are

> _a lot_ of dependencies and I wouldn't suggest picking them all to

> stable unless it's critical for someone.


I tried to drop this one, and it turned out to be a depandancy for
another patch for this driver.  And that was another dependancy.  So
I've now dropped all of these from the queue.

Here are the commits I dropped.  If you think any should be added back,
please let us know:

05a7f4a8dff1 ("devlink: Break parameter notification sequence to be before/after unload/load driver")
e183bf31cf0d ("drm/bridge: ti-sn65dsi86: Add some 100 us delays")
c7782443a889 ("drm/bridge: ti-sn65dsi86: Avoid creating multiple connectors")
a70e558c1510 ("drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC")
acb06210b096 ("drm/bridge: ti-sn65dsi86: Fix power off sequence")
4c1b3d94bf63 ("drm/bridge: ti-sn65dsi86: Improve probe errors with dev_err_probe()")
4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")

thanks,

greg k-h
Sasha Levin Sept. 14, 2021, 5:02 p.m. UTC | #4
On Tue, Sep 14, 2021 at 06:27:08PM +0200, Greg Kroah-Hartman wrote:
>On Mon, Sep 13, 2021 at 09:31:03AM -0700, Doug Anderson wrote:

>> Hi,

>>

>> On Mon, Sep 13, 2021 at 9:09 AM Greg Kroah-Hartman

>> <gregkh@linuxfoundation.org> wrote:

>> >

>> > On Mon, Sep 13, 2021 at 06:57:20AM -0700, Doug Anderson wrote:

>> > > Hi,

>> > >

>> > > On Mon, Sep 13, 2021 at 6:51 AM Greg Kroah-Hartman

>> > > <gregkh@linuxfoundation.org> wrote:

>> > > >

>> > > > From: Douglas Anderson <dianders@chromium.org>

>> > > >

>> > > > [ Upstream commit a70e558c151043ce46a5e5999f4310e0b3551f57 ]

>> > > >

>> > > > This is really just a revert of commit 58074b08c04a ("drm/bridge:

>> > > > ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.

>> > > >

>> > > > The old code failed to read the EDID properly in a very important

>> > > > case: before the bridge's pre_enable() was called. The way things need

>> > > > to work:

>> > > > 1. Read the EDID.

>> > > > 2. Based on the EDID, decide on video settings and pixel clock.

>> > > > 3. Enable the bridge w/ the desired settings.

>> > > >

>> > > > The way things were working:

>> > > > 1. Try to read the EDID but fail; fall back to hardcoded values.

>> > > > 2. Based on hardcoded values, decide on video settings and pixel clock.

>> > > > 3. Enable the bridge w/ the desired settings.

>> > > > 4. Try again to read the EDID, it works now!

>> > > > 5. Realize that the hardcoded settings weren't quite right.

>> > > > 6. Disable / reenable the bridge w/ the right settings.

>> > > >

>> > > > The reasons for the failures were twofold:

>> > > > a) 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.

>> > > > b) Even if we fixed things to ignore HPD, the EDID still wouldn't read

>> > > >    if the panel wasn't on.

>> > > >

>> > > > Instead of reverting the code, we could fix it to set the HPD bit and

>> > > > also power on the panel. However, it also works nicely to just let the

>> > > > panel code read the EDID. Now that we've split the driver up we can

>> > > > expose the DDC AUX channel bus to the panel node. The panel can take

>> > > > charge of reading the EDID.

>> > > >

>> > > > NOTE: in order for things to work, anyone that needs to read the EDID

>> > > > will need to instantiate their panel using the new DP AUX bus (AKA by

>> > > > listing their panel under the "aux-bus" node of the bridge chip in the

>> > > > device tree).

>> > > >

>> > > > In the future if we want to use the bridge chip to provide a full

>> > > > external DP port (which won't have a panel) then we will have to

>> > > > conditinally add EDID reading back in.

>> > > >

>> > > > Suggested-by: Andrzej Hajda <a.hajda@samsung.com>

>> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>

>> > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

>> > > > Link: https://patchwork.freedesktop.org/patch/msgid/20210611101711.v10.9.I9330684c25f65bb318eff57f0616500f83eac3cc@changeid

>> > > > Signed-off-by: Sasha Levin <sashal@kernel.org>

>> > > > ---

>> > > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ----------------------

>> > > >  1 file changed, 22 deletions(-)

>> > >

>> > > I guess it's not a huge deal, but I did respond to Sasha and request

>> > > that this patch be dropped from the stable queue unless the whole big

>> > > pile of patches was being backported. See:

>> > >

>> > > https://lore.kernel.org/lkml/CAD=FV=U2dGjeEzp+K1vnLTj8oPJ-GKBTTKz2XQ1OZ7QF_sTHuw@mail.gmail.com/

>> > >

>> > > I said:

>> > >

>> > > > I would suggest against backporting this one unless you're going to

>> > > > backport the whole pile of DP AUX bus patches, which probably doesn't

>> > > > make sense for stable. Even though the old EDID reading was broken for

>> > > > the first read, it still worked for later reads. ...and the first read

>> > > . didn't crash or anything--it just timed out.

>> >

>> > I see a "bunch" of patches for this driver in this -rc, did Sasha not

>> > get them all?  If not, I can drop this one, but maybe it was needed for

>> > the follow-on patches?

>>

>> It's been a long journey trying to make this bridge work better. I

>> think the easiest way to say it is that if you don't have the parent

>> of ${SUBJECT} patch, AKA:

>>

>> e0bbcc6233f7 drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus

>>

>> ...then you don't have DP AUX bus support and you shouldn't take

>> ${SUBJECT} patch. If you have that patch and it compiles / builds then

>> it means that you have all the proper dependencies. However, there are

>> _a lot_ of dependencies and I wouldn't suggest picking them all to

>> stable unless it's critical for someone.

>

>I tried to drop this one, and it turned out to be a depandancy for

>another patch for this driver.  And that was another dependancy.  So

>I've now dropped all of these from the queue.

>

>Here are the commits I dropped.  If you think any should be added back,

>please let us know:

>

>05a7f4a8dff1 ("devlink: Break parameter notification sequence to be before/after unload/load driver")

>e183bf31cf0d ("drm/bridge: ti-sn65dsi86: Add some 100 us delays")

>c7782443a889 ("drm/bridge: ti-sn65dsi86: Avoid creating multiple connectors")

>a70e558c1510 ("drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC")

>acb06210b096 ("drm/bridge: ti-sn65dsi86: Fix power off sequence")

>4c1b3d94bf63 ("drm/bridge: ti-sn65dsi86: Improve probe errors with dev_err_probe()")

>4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")


Thanks! And yes, I dropped the patch based on the request but it snuck
in as a dependency for a few patches with a Fixes tag.

-- 
Thanks,
Sasha
Doug Anderson Sept. 14, 2021, 5:24 p.m. UTC | #5
Hi,

On Tue, Sep 14, 2021 at 9:27 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>

> On Mon, Sep 13, 2021 at 09:31:03AM -0700, Doug Anderson wrote:

> > Hi,

> >

> > On Mon, Sep 13, 2021 at 9:09 AM Greg Kroah-Hartman

> > <gregkh@linuxfoundation.org> wrote:

> > >

> > > On Mon, Sep 13, 2021 at 06:57:20AM -0700, Doug Anderson wrote:

> > > > Hi,

> > > >

> > > > On Mon, Sep 13, 2021 at 6:51 AM Greg Kroah-Hartman

> > > > <gregkh@linuxfoundation.org> wrote:

> > > > >

> > > > > From: Douglas Anderson <dianders@chromium.org>

> > > > >

> > > > > [ Upstream commit a70e558c151043ce46a5e5999f4310e0b3551f57 ]

> > > > >

> > > > > This is really just a revert of commit 58074b08c04a ("drm/bridge:

> > > > > ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.

> > > > >

> > > > > The old code failed to read the EDID properly in a very important

> > > > > case: before the bridge's pre_enable() was called. The way things need

> > > > > to work:

> > > > > 1. Read the EDID.

> > > > > 2. Based on the EDID, decide on video settings and pixel clock.

> > > > > 3. Enable the bridge w/ the desired settings.

> > > > >

> > > > > The way things were working:

> > > > > 1. Try to read the EDID but fail; fall back to hardcoded values.

> > > > > 2. Based on hardcoded values, decide on video settings and pixel clock.

> > > > > 3. Enable the bridge w/ the desired settings.

> > > > > 4. Try again to read the EDID, it works now!

> > > > > 5. Realize that the hardcoded settings weren't quite right.

> > > > > 6. Disable / reenable the bridge w/ the right settings.

> > > > >

> > > > > The reasons for the failures were twofold:

> > > > > a) 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.

> > > > > b) Even if we fixed things to ignore HPD, the EDID still wouldn't read

> > > > >    if the panel wasn't on.

> > > > >

> > > > > Instead of reverting the code, we could fix it to set the HPD bit and

> > > > > also power on the panel. However, it also works nicely to just let the

> > > > > panel code read the EDID. Now that we've split the driver up we can

> > > > > expose the DDC AUX channel bus to the panel node. The panel can take

> > > > > charge of reading the EDID.

> > > > >

> > > > > NOTE: in order for things to work, anyone that needs to read the EDID

> > > > > will need to instantiate their panel using the new DP AUX bus (AKA by

> > > > > listing their panel under the "aux-bus" node of the bridge chip in the

> > > > > device tree).

> > > > >

> > > > > In the future if we want to use the bridge chip to provide a full

> > > > > external DP port (which won't have a panel) then we will have to

> > > > > conditinally add EDID reading back in.

> > > > >

> > > > > Suggested-by: Andrzej Hajda <a.hajda@samsung.com>

> > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>

> > > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> > > > > Link: https://patchwork.freedesktop.org/patch/msgid/20210611101711.v10.9.I9330684c25f65bb318eff57f0616500f83eac3cc@changeid

> > > > > Signed-off-by: Sasha Levin <sashal@kernel.org>

> > > > > ---

> > > > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ----------------------

> > > > >  1 file changed, 22 deletions(-)

> > > >

> > > > I guess it's not a huge deal, but I did respond to Sasha and request

> > > > that this patch be dropped from the stable queue unless the whole big

> > > > pile of patches was being backported. See:

> > > >

> > > > https://lore.kernel.org/lkml/CAD=FV=U2dGjeEzp+K1vnLTj8oPJ-GKBTTKz2XQ1OZ7QF_sTHuw@mail.gmail.com/

> > > >

> > > > I said:

> > > >

> > > > > I would suggest against backporting this one unless you're going to

> > > > > backport the whole pile of DP AUX bus patches, which probably doesn't

> > > > > make sense for stable. Even though the old EDID reading was broken for

> > > > > the first read, it still worked for later reads. ...and the first read

> > > > . didn't crash or anything--it just timed out.

> > >

> > > I see a "bunch" of patches for this driver in this -rc, did Sasha not

> > > get them all?  If not, I can drop this one, but maybe it was needed for

> > > the follow-on patches?

> >

> > It's been a long journey trying to make this bridge work better. I

> > think the easiest way to say it is that if you don't have the parent

> > of ${SUBJECT} patch, AKA:

> >

> > e0bbcc6233f7 drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus

> >

> > ...then you don't have DP AUX bus support and you shouldn't take

> > ${SUBJECT} patch. If you have that patch and it compiles / builds then

> > it means that you have all the proper dependencies. However, there are

> > _a lot_ of dependencies and I wouldn't suggest picking them all to

> > stable unless it's critical for someone.

>

> I tried to drop this one, and it turned out to be a depandancy for

> another patch for this driver.  And that was another dependancy.  So

> I've now dropped all of these from the queue.


Ugh. :(


> Here are the commits I dropped.  If you think any should be added back,

> please let us know:

>

> 05a7f4a8dff1 ("devlink: Break parameter notification sequence to be before/after unload/load driver")


I don't understand what the "devlink" patch had to do with
ti-sn65dsi86. I'll assume you didn't intend to have it in your list.


> e183bf31cf0d ("drm/bridge: ti-sn65dsi86: Add some 100 us delays")

> c7782443a889 ("drm/bridge: ti-sn65dsi86: Avoid creating multiple connectors")

> a70e558c1510 ("drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC")

> acb06210b096 ("drm/bridge: ti-sn65dsi86: Fix power off sequence")

> 4c1b3d94bf63 ("drm/bridge: ti-sn65dsi86: Improve probe errors with dev_err_probe()")

> 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")


I'd say it's OK to drop these. If someone demonstrates a need for them
on stable channel then I can help with backporting, but otherwise I
can't see that it's worth it. Basically:

1. I think ${SUBJECT} patch without full DP AUX support for people
could cause a real regression if anyone is using this bridge chip on
5.14 stable.

2. I think picking back full DP AUX support is too heavy for a
stable-channel backport without a demonstrated need.

3. Of the above patches, only one fixes a problem that was observed on
real hardware ("Fix power off sequence"). That was only seen on a
panel that _requires_ the full DP AUX support in order to work. Other
ones are fixes based on code inspection, cleanups, or fixes for other
patches in the series there.

-Doug
Greg Kroah-Hartman Sept. 14, 2021, 5:34 p.m. UTC | #6
On Tue, Sep 14, 2021 at 10:24:35AM -0700, Doug Anderson wrote:
> Hi,

> 

> On Tue, Sep 14, 2021 at 9:27 AM Greg Kroah-Hartman

> <gregkh@linuxfoundation.org> wrote:

> >

> > On Mon, Sep 13, 2021 at 09:31:03AM -0700, Doug Anderson wrote:

> > > Hi,

> > >

> > > On Mon, Sep 13, 2021 at 9:09 AM Greg Kroah-Hartman

> > > <gregkh@linuxfoundation.org> wrote:

> > > >

> > > > On Mon, Sep 13, 2021 at 06:57:20AM -0700, Doug Anderson wrote:

> > > > > Hi,

> > > > >

> > > > > On Mon, Sep 13, 2021 at 6:51 AM Greg Kroah-Hartman

> > > > > <gregkh@linuxfoundation.org> wrote:

> > > > > >

> > > > > > From: Douglas Anderson <dianders@chromium.org>

> > > > > >

> > > > > > [ Upstream commit a70e558c151043ce46a5e5999f4310e0b3551f57 ]

> > > > > >

> > > > > > This is really just a revert of commit 58074b08c04a ("drm/bridge:

> > > > > > ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.

> > > > > >

> > > > > > The old code failed to read the EDID properly in a very important

> > > > > > case: before the bridge's pre_enable() was called. The way things need

> > > > > > to work:

> > > > > > 1. Read the EDID.

> > > > > > 2. Based on the EDID, decide on video settings and pixel clock.

> > > > > > 3. Enable the bridge w/ the desired settings.

> > > > > >

> > > > > > The way things were working:

> > > > > > 1. Try to read the EDID but fail; fall back to hardcoded values.

> > > > > > 2. Based on hardcoded values, decide on video settings and pixel clock.

> > > > > > 3. Enable the bridge w/ the desired settings.

> > > > > > 4. Try again to read the EDID, it works now!

> > > > > > 5. Realize that the hardcoded settings weren't quite right.

> > > > > > 6. Disable / reenable the bridge w/ the right settings.

> > > > > >

> > > > > > The reasons for the failures were twofold:

> > > > > > a) 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.

> > > > > > b) Even if we fixed things to ignore HPD, the EDID still wouldn't read

> > > > > >    if the panel wasn't on.

> > > > > >

> > > > > > Instead of reverting the code, we could fix it to set the HPD bit and

> > > > > > also power on the panel. However, it also works nicely to just let the

> > > > > > panel code read the EDID. Now that we've split the driver up we can

> > > > > > expose the DDC AUX channel bus to the panel node. The panel can take

> > > > > > charge of reading the EDID.

> > > > > >

> > > > > > NOTE: in order for things to work, anyone that needs to read the EDID

> > > > > > will need to instantiate their panel using the new DP AUX bus (AKA by

> > > > > > listing their panel under the "aux-bus" node of the bridge chip in the

> > > > > > device tree).

> > > > > >

> > > > > > In the future if we want to use the bridge chip to provide a full

> > > > > > external DP port (which won't have a panel) then we will have to

> > > > > > conditinally add EDID reading back in.

> > > > > >

> > > > > > Suggested-by: Andrzej Hajda <a.hajda@samsung.com>

> > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>

> > > > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> > > > > > Link: https://patchwork.freedesktop.org/patch/msgid/20210611101711.v10.9.I9330684c25f65bb318eff57f0616500f83eac3cc@changeid

> > > > > > Signed-off-by: Sasha Levin <sashal@kernel.org>

> > > > > > ---

> > > > > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ----------------------

> > > > > >  1 file changed, 22 deletions(-)

> > > > >

> > > > > I guess it's not a huge deal, but I did respond to Sasha and request

> > > > > that this patch be dropped from the stable queue unless the whole big

> > > > > pile of patches was being backported. See:

> > > > >

> > > > > https://lore.kernel.org/lkml/CAD=FV=U2dGjeEzp+K1vnLTj8oPJ-GKBTTKz2XQ1OZ7QF_sTHuw@mail.gmail.com/

> > > > >

> > > > > I said:

> > > > >

> > > > > > I would suggest against backporting this one unless you're going to

> > > > > > backport the whole pile of DP AUX bus patches, which probably doesn't

> > > > > > make sense for stable. Even though the old EDID reading was broken for

> > > > > > the first read, it still worked for later reads. ...and the first read

> > > > > . didn't crash or anything--it just timed out.

> > > >

> > > > I see a "bunch" of patches for this driver in this -rc, did Sasha not

> > > > get them all?  If not, I can drop this one, but maybe it was needed for

> > > > the follow-on patches?

> > >

> > > It's been a long journey trying to make this bridge work better. I

> > > think the easiest way to say it is that if you don't have the parent

> > > of ${SUBJECT} patch, AKA:

> > >

> > > e0bbcc6233f7 drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus

> > >

> > > ...then you don't have DP AUX bus support and you shouldn't take

> > > ${SUBJECT} patch. If you have that patch and it compiles / builds then

> > > it means that you have all the proper dependencies. However, there are

> > > _a lot_ of dependencies and I wouldn't suggest picking them all to

> > > stable unless it's critical for someone.

> >

> > I tried to drop this one, and it turned out to be a depandancy for

> > another patch for this driver.  And that was another dependancy.  So

> > I've now dropped all of these from the queue.

> 

> Ugh. :(

> 

> 

> > Here are the commits I dropped.  If you think any should be added back,

> > please let us know:

> >

> > 05a7f4a8dff1 ("devlink: Break parameter notification sequence to be before/after unload/load driver")

> 

> I don't understand what the "devlink" patch had to do with

> ti-sn65dsi86. I'll assume you didn't intend to have it in your list.


Odd, good point, I think I messed up in removing too many patches, let
me go add that back...

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 45a2969afb2b..aef850296756 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -124,7 +124,6 @@ 
  * @connector:    Our connector.
  * @host_node:    Remote DSI node.
  * @dsi:          Our MIPI DSI source.
- * @edid:         Detected EDID of eDP panel.
  * @refclk:       Our reference clock.
  * @panel:        Our panel.
  * @enable_gpio:  The GPIO we toggle to enable the bridge.
@@ -154,7 +153,6 @@  struct ti_sn65dsi86 {
 	struct drm_dp_aux		aux;
 	struct drm_bridge		bridge;
 	struct drm_connector		connector;
-	struct edid			*edid;
 	struct device_node		*host_node;
 	struct mipi_dsi_device		*dsi;
 	struct clk			*refclk;
@@ -403,24 +401,6 @@  connector_to_ti_sn65dsi86(struct drm_connector *connector)
 static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 {
 	struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector);
-	struct edid *edid = pdata->edid;
-	int num, ret;
-
-	if (!edid) {
-		pm_runtime_get_sync(pdata->dev);
-		edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
-		pm_runtime_put_autosuspend(pdata->dev);
-	}
-
-	if (edid && drm_edid_is_valid(edid)) {
-		ret = drm_connector_update_edid_property(connector, edid);
-		if (!ret) {
-			num = drm_add_edid_modes(connector, edid);
-			if (num)
-				return num;
-		}
-	}
-
 	return drm_panel_get_modes(pdata->panel, connector);
 }
 
@@ -1358,8 +1338,6 @@  static void ti_sn_bridge_remove(struct auxiliary_device *adev)
 		mipi_dsi_device_unregister(pdata->dsi);
 	}
 
-	kfree(pdata->edid);
-
 	drm_bridge_remove(&pdata->bridge);
 
 	of_node_put(pdata->host_node);