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:
- Read the EDID.
- Based on the EDID, decide on video settings and pixel clock.
- Enable the bridge w/ the desired settings.
The way things were working:
- Try to read the EDID but fail; fall back to hardcoded values.
- Based on hardcoded values, decide on video settings and pixel clock.
- Enable the bridge w/ the desired settings.
- Try again to read the EDID, it works now!
- Realize that the hardcoded settings weren't quite right.
- 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.I9330684c... 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_s...
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