Hi,
On Sun, Nov 1, 2020 at 9:37 AM Sam Ravnborg sam@ravnborg.org wrote:
Hi Stephen.
On Thu, Oct 29, 2020 at 06:17:34PM -0700, Stephen Boyd wrote:
This patch series cleans up the DDC code a little bit so that it is more efficient time wise and supports grabbing the EDID of the eDP panel over the aux channel. I timed this on a board I have on my desk and it takes about 20ms to grab the EDID out of the panel and make sure it is valid.
The first two patches seem less controversial so I stuck them at the beginning. The third patch does the EDID reading and caches it so we don't have to keep grabbing it over and over again. And finally the last patch updates the reply field so that short reads and nacks over the channel are reflected properly instead of treating them as some sort of error that can't be discerned.
Stephen Boyd (4): drm/bridge: ti-sn65dsi86: Combine register accesses in ti_sn_aux_transfer() drm/bridge: ti-sn65dsi86: Make polling a busy loop drm/bridge: ti-sn65dsi86: Read EDID blob over DDC drm/bridge: ti-sn65dsi86: Update reply on aux failures
Series looks good. You can add my a-b on the full series. Acked-by: Sam Ravnborg sam@ravnborg.org
I can apply after Douglas have had a look at the patches he did not r-b yet.
They look fine to me now assuming that Stepehn has tested patch #1 enough that we're confident that the slight change in ordering isn't going to mess anything up. Laurent also had comments about caching the EDID on patch #3. If he feels strongly about getting rid of that, it'll need another spin and we'll just have to suck up the small boot time penalty until we can find a solution in the core.
Any chance we can convince you to prepare this bridge driver for use in a chained bridge setup where the connector is created by the display driver and uses drm_bridge_funcs?
First step wuld be to introduce the use of a panel_bridge. Then add get_edid to drm_bridge_funcs and maybe more helpers.
Then natural final step would be to move connector creation to the display driver - see how other uses drm_bridge_connector_init() to do so
- it is relatively simple.
Should be doable - and reach out if you need some help.
At some point I think Vinod tried to prepare a patch for this and I tried it, but it didn't just work. I spent an hour or so poking at it and I couldn't quite figure out why and I couldn't find enough other examples to compare against to see what was wrong... That was a few months ago, though. Maybe things are in a better shape now?
-Doug