20.12.2021 19:55, Dmitry Osipenko пишет: > 20.12.2021 19:12, Dmitry Osipenko пишет: >> 20.12.2021 18:27, Thierry Reding пишет: >>> On Mon, Dec 20, 2021 at 05:45:41PM +0300, Dmitry Osipenko wrote: >>>> 20.12.2021 13:48, Thierry Reding пишет: >>>>> From: Thierry Reding <treding@xxxxxxxxxx> >>>>> >>>>> Hi, >>>>> >>>>> this is an alternative proposal to fix panel support on Venice 2 and >>>>> Nyan. Dmitry had proposed a different solution that involved reverting >>>>> the I2C/DDC registration order and would complicate things by breaking >>>>> the encapsulation of the driver by introducing a global (though locally >>>>> scoped) variable[0]. >>>>> >>>>> This set of patches avoids that by using the recently introduced DP AUX >>>>> bus infrastructure. The result is that the changes are actually less >>>>> intrusive and not a step back. Instead they nicely remove the circular >>>>> dependency that previously existed and caused these issues in the first >>>>> place. >>>>> >>>>> To be fair, this is not perfect either because it requires a device tree >>>>> change and hence isn't technically backwards-compatible. However, given >>>>> that the original device tree was badly broken in the first place, I >>>>> think we can make an exception, especially since it is not generally a >>>>> problem to update device trees on the affected devices. >>>>> >>>>> Secondly, this relies on infrastructure that was introduced in v5.15 and >>>>> therefore will be difficult to backport beyond that. However, since this >>>>> functionality has been broken since v5.13 and all of the kernel versions >>>>> between that and v5.15 are EOL anyway, there isn't much that we can do >>>>> to fix the interim versions anyway. >>>>> >>>>> Adding Doug and Laurent since they originally designed the AUX bus >>>>> patches in case they see anything in here that would be objectionable. >>>>> >>>>> Thierry >>>>> >>>>> [0]: https://lore.kernel.org/dri-devel/20211130230957.30213-1-digetx@xxxxxxxxx/ >>>>> >>>>> Thierry Reding (2): >>>>> drm/tegra: dpaux: Populate AUX bus >>>>> ARM: tegra: Move panels to AUX bus >>>>> >>>>> arch/arm/boot/dts/tegra124-nyan-big.dts | 15 +++++++++------ >>>>> arch/arm/boot/dts/tegra124-nyan-blaze.dts | 15 +++++++++------ >>>>> arch/arm/boot/dts/tegra124-venice2.dts | 14 +++++++------- >>>>> drivers/gpu/drm/tegra/Kconfig | 1 + >>>>> drivers/gpu/drm/tegra/dpaux.c | 7 +++++++ >>>>> 5 files changed, 33 insertions(+), 19 deletions(-) >>>>> >>>> >>>> It should "work" since you removed the ddc-i2c-bus phandle from the >>>> panel nodes, and thus, panel->ddc won't be used during panel-edp driver >>>> probe. But this looks like a hack rather than a fix. >>> >>> The AUX ->ddc will be used for panel->ddc if the ddc-i2c-bus property is >>> not specified. And that makes perfect sense because we'd basically just >>> be pointing back to the AUX node anyway. >>> >>>> I'm not sure why and how devm_of_dp_aux_populate_ep_devices() usage >>>> should be relevant here. The drm_dp_aux_register() still should to >>>> invoked before devm_of_dp_aux_populate_ep_devices(), otherwise >>>> panel->ddc adapter won't be registered. >>> >>> drm_dp_aux_register() is only needed to expose the device to userspace >>> and make the I2C adapter available to the rest of the system. But since >>> we already know the AUX and I2C adapter, we can use it directly without >>> doing a separate lookup. drm_dp_aux_init() should be enough to set the >>> adapter up to work for what we need. >>> >>> See also the kerneldoc for drm_dp_aux_register() where this is described >>> in a bit more detail. >> >> Alright, so you fixed it by removing the ddc-i2c-bus phandles and I2C >> DDC will work properly anyways with that on v5.16. >> >> But the aux-bus usage still is irrelevant for the fix. Let's not use it >> then. >> >>>> The panel->ddc isn't used by the new panel-edp driver unless panel is >>>> compatible with "edp-panel". Hence the generic_edp_panel_probe() should >>>> either fail or crash for a such "edp-panel" since panel->ddc isn't fully >>>> instantiated, AFAICS. >>> >>> I've tested this and it works fine on Venice 2. Since that was the >>> reference design for Nyan, I suspect that Nyan's will also work. >>> >>> It'd be great if Thomas or anyone else with access to a Nyan could >>> test this to verify that. >> >> There is no panel-edp driver in the v5.15. The EOL of v5.15 is Oct, >> 2023, hence we need to either use: >> >> Approach #1: >> >> 1. apply my variant of the fix >> 2. backport it to v5.15 >> 3. apply your variant without aux-bus, replacing my fix on 5.16+ > > Although, I see that it doesn't make much sense to say "your variant > without aux-bus". "Remove ddc-i2c-bus phandles from DTs" will be better. > >> Or >> >> Approach #2: >> >> 1. remove ddc-i2c-bus phandles in DTs >> 2. backport (?) it to v5.15 >> 3. apply your variant without aux-bus >> >> Or >> >> Approach #3: >> >> 1. ignore v5.15 and keep it screwed >> 2. apply your variant as is >> >> Which approach will you prefer? >> >> I'm not happy that older DTBs will be broken. Can we do better about it? >> >> Is it possible to patch DT in the code by removing the ddc-i2c-bus phandle? >> >> Or can we patch panel-simple on 5.15 and panel-edp on 5.16, making these >> drivers to skip ddc-i2c-bus on Tegra+eDP? The eDP driver fixup will be >> trivial, fixing older panel-simple will be more invasive. >> >> I think the best solution will be to use Approach #1 and in the end >> complete it with this panel-edp workaround below. This approach will >> have minimal code impact on 5.16+ kernels and will keep older DTBs >> working. Are you okay with this? >> >> diff --git a/drivers/gpu/drm/panel/panel-edp.c >> b/drivers/gpu/drm/panel/panel-edp.c >> index 176ef0c3cc1d..4e5b84324659 100644 >> --- a/drivers/gpu/drm/panel/panel-edp.c >> +++ b/drivers/gpu/drm/panel/panel-edp.c >> @@ -793,7 +793,11 @@ static int panel_edp_probe(struct device *dev, >> const struct panel_desc *desc, >> return err; >> } >> >> - ddc = of_parse_phandle(dev->of_node, "ddc-i2c-bus", 0); >> + if (of_machine_is_compatible("nvidia,tegra124")) >> + ddc = NULL; >> + else >> + ddc = of_parse_phandle(dev->of_node, "ddc-i2c-bus", 0); >> + >> if (ddc) { >> panel->ddc = of_find_i2c_adapter_by_node(ddc); >> of_node_put(ddc); >> > > Another alternative that may work is to check whether ddc-i2c-bus and > DPAUX use the same node. > > diff --git a/drivers/gpu/drm/panel/panel-edp.c > b/drivers/gpu/drm/panel/panel-edp.c > index 176ef0c3cc1d..c8cf5bc3d148 100644 > --- a/drivers/gpu/drm/panel/panel-edp.c > +++ b/drivers/gpu/drm/panel/panel-edp.c > @@ -794,7 +794,7 @@ static int panel_edp_probe(struct device *dev, const > struct panel_desc *desc, > } > > ddc = of_parse_phandle(dev->of_node, "ddc-i2c-bus", 0); > - if (ddc) { > + if (ddc && ddc != aux->dev->of_node) { > panel->ddc = of_find_i2c_adapter_by_node(ddc); > of_node_put(ddc); > I see now that the aux pointer should be populated only if aux-bus is used, so this won't work.
