Re: [PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
Quoting Laurent Pinchart (2021-03-22 17:00:23) > Hi Stephen, > > On Mon, Nov 02, 2020 at 05:15:24PM -0800, Stephen Boyd wrote: > > Quoting Sam Ravnborg (2020-11-01 09:37:41) > > > 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 > > > > > > I can apply after Douglas have had a look at the patches he did not r-b > > > yet. > > > > > > 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. > > > > I started to look at this and got stuck at ti_sn_bridge_get_bpp(). Where > > can I get the details of the bpc for the downstream bridge or panel? Is > > there some function that can tell this bridge what the bpc is for the > > attached connector? > > I've posted a patch series to convert to DRM_BRIDGE_ATTACH_NO_CONNECTOR > yesterday (and have CC'ed you), but I've overlooked this particular > problem :-S ! > > You can't get the connector in the .enable() operation, but you can get > it in .atomic_enable(), with > drm_atomic_get_new_connector_for_encoder(). This being said, it's > probably not the right option. > > What matters here isn't the bpc for the connector, but the format > expected by the next bridge in the chain. drm_bridge_funcs has two > operations, .atomic_get_output_bus_fmts() and > .atomic_get_input_bus_fmts(), to negotiate that format along a chain of > bridges. The panel bridge driver (drivers/gpu/drm/bridge/panel.c) > doesn't implement those operations, and neither does > display-connector.c, so that may be what we should start with. Ok, makes sense. I'd gladly test things out if you come up with some solution here. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
Hi Stephen, On Mon, Nov 02, 2020 at 05:15:24PM -0800, Stephen Boyd wrote: > Quoting Sam Ravnborg (2020-11-01 09:37:41) > > 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 > > > > I can apply after Douglas have had a look at the patches he did not r-b > > yet. > > > > 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. > > I started to look at this and got stuck at ti_sn_bridge_get_bpp(). Where > can I get the details of the bpc for the downstream bridge or panel? Is > there some function that can tell this bridge what the bpc is for the > attached connector? I've posted a patch series to convert to DRM_BRIDGE_ATTACH_NO_CONNECTOR yesterday (and have CC'ed you), but I've overlooked this particular problem :-S You can't get the connector in the .enable() operation, but you can get it in .atomic_enable(), with drm_atomic_get_new_connector_for_encoder(). This being said, it's probably not the right option. What matters here isn't the bpc for the connector, but the format expected by the next bridge in the chain. drm_bridge_funcs has two operations, .atomic_get_output_bus_fmts() and .atomic_get_input_bus_fmts(), to negotiate that format along a chain of bridges. The panel bridge driver (drivers/gpu/drm/bridge/panel.c) doesn't implement those operations, and neither does display-connector.c, so that may be what we should start with. > I see that td_mode_valid() in > drivers/gpu/drm/bridge/tc358775.c stores away the bpc from the incoming > drm_display_info pointer but I'm not sure that is correct because can't > that be called for various and not necessarily the one we're using? You're right, .mode_valid() shouldn't do that. -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
Quoting Stephen Boyd (2020-11-02 17:15:24) > Quoting Sam Ravnborg (2020-11-01 09:37:41) > > 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 > > > > I can apply after Douglas have had a look at the patches he did not r-b > > yet. > > > > 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. > > > > I started to look at this and got stuck at ti_sn_bridge_get_bpp(). Where > can I get the details of the bpc for the downstream bridge or panel? Is > there some function that can tell this bridge what the bpc is for the > attached connector? I see that td_mode_valid() in > drivers/gpu/drm/bridge/tc358775.c stores away the bpc from the incoming > drm_display_info pointer but I'm not sure that is correct because can't > that be called for various and not necessarily the one we're using? Looking closer I see that tc358775 doesn't store away the incoming value, it derives a bpc from what comes in. I guess it isn't really a good example for this problem. I tried to hack around this and convert this driver to use bridge funcs with Vinod's msm patch underneath but now the encoder is invalid. I wonder if something is wrong with how the msm display driver drives the bridge and connector? That TODO in ti_sn_bridge_attach() makes me think something else needs fixing. I'll send another patch series tomorrow on top of this one that kicks off the conversion to bridge connector so we can discuss there. [4.577814] [drm:dpu_core_perf_crtc_update:397] [dpu error]crtc-49: failed to update bus bw vote [4.603697] panel-simple panel: error waiting for hpd GPIO: -6 [4.613106] dsi_calc_pclk: forcing mdss_dsi lanes to 1 [4.619070] dsi_link_clk_set_rate_6g: Failed to set rate pixel clk, -22 [4.625883] msm_dsi_host_power_on: failed to enable link clocks. ret=-22 [4.632784] dsi_mgr_bridge_pre_enable: power on host 0 failed, -22 [4.646235] Unable to handle kernel NULL pointer dereference at virtual address 0370 [4.655279] Mem abort info: [4.658249] ESR = 0x9606 [4.661456] EC = 0x25: DABT (current EL), IL = 32 bits [4.667047] SET = 0, FnV = 0 [4.670195] EA = 0, S1PTW = 0 [4.673430] Data abort info: [4.676401] ISV = 0, ISS = 0x0006 [4.680347] CM = 0, WnR = 0 [4.683413] user pgtable: 4k pages, 39-bit VAs, pgdp=000104f18000 [4.690033] [0370] pgd=000104f21003, p4d=000104f21003, pud=000104f21003, pmd= [4.700941] Internal error: Oops: 9606 [#1] PREEMPT SMP [4.706672] Modules linked in: cdc_ether usbnet r8152 joydev mii [4.712866] CPU: 6 PID: 376 Comm: frecon Not tainted 5.10.0-rc2+ #5 [4.719302] Hardware name: Google Lazor (rev1+) with LTE (DT) [4.725201] pstate: 6049 (nZCv daif +PAN -UAO -TCO BTYPE=--) [4.731390] pc : ti_sn_bridge_enable+0xa4/0x7ec [4.736047] lr : ti_sn_bridge_enable+0x54/0x7ec [4.740704] sp : ffc0109db960 [4.744119] x29: ffc0109db990 x28: ffd216acb000 [4.749586] x27: ffd216854000 x26: ffd216acb000 [4.755046] x25: ffd2164e89d0 x24: [4.760508] x23: x22: ff8281de5080 [4.765970] x21: ff8281de56f0 x20: ff8281de5670 [4.771431] x19: ff8281de5090 x18: 0484 [4.776893] x17
Re: [PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
Quoting Doug Anderson (2020-11-02 08:37:21) > Hi, > > On Sun, Nov 1, 2020 at 9:37 AM Sam Ravnborg 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 > > > > 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. I did test it but the test isn't thorough enough to cover the timeout case. I'll resend with v1 of this patch and pick up acks and include Sam on To line. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
Quoting Sam Ravnborg (2020-11-01 09:37:41) > 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 > > I can apply after Douglas have had a look at the patches he did not r-b > yet. > > 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. > I started to look at this and got stuck at ti_sn_bridge_get_bpp(). Where can I get the details of the bpc for the downstream bridge or panel? Is there some function that can tell this bridge what the bpc is for the attached connector? I see that td_mode_valid() in drivers/gpu/drm/bridge/tc358775.c stores away the bpc from the incoming drm_display_info pointer but I'm not sure that is correct because can't that be called for various and not necessarily the one we're using? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
Hi, Thanks Doug for adding me On 02-11-20, 08:37, Doug Anderson wrote: > > On Thu, Oct 29, 2020 at 06:17:34PM -0700, Stephen Boyd wrote: > > 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. Yes it is and doable and you find this at [1], would need a rebase though. > 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? It worked fine for me on Rb3 and db410c where we had HDMI connector. I don't have a panel device to test and Bjorn tried to help out with a bit of testing. This didn't work on the laptop, that is why I haven't posted it yet. This has conversion of msm driver and bridge drivers lt9611, adv7511 and ti-sn65dsi86. [1]: https://git.linaro.org/people/vinod.koul/kernel.git/log/?h=wip/msm_bridges_no_conn Thanks -- ~Vinod ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
Hi, On Sun, Nov 1, 2020 at 9:37 AM Sam Ravnborg 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 > > 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
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 I can apply after Douglas have had a look at the patches he did not r-b yet. 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. Sam > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 108 ++ > 1 file changed, 75 insertions(+), 33 deletions(-) > > Cc: Douglas Anderson > Cc: Laurent Pinchart > Cc: Jonas Karlman > Cc: Jernej Skrabec > Cc: Sean Paul > > base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec > -- > Sent by a computer, using git, on the internet > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
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 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 108 ++ 1 file changed, 75 insertions(+), 33 deletions(-) Cc: Douglas Anderson Cc: Laurent Pinchart Cc: Jonas Karlman Cc: Jernej Skrabec Cc: Sean Paul base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec -- Sent by a computer, using git, on the internet ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
