On Tue, Feb 28, 2023 at 02:08:53AM +0530, Jagan Teki wrote:
> On Tue, Feb 28, 2023 at 1:40 AM Marek Vasut <marex@xxxxxxx> wrote:
> >
> > On 2/27/23 20:49, Jagan Teki wrote:
> > > On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut <marex@xxxxxxx> wrote:
> > >>
> > >> On 2/27/23 20:34, Jagan Teki wrote:
> > >>> On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@xxxxxxx> wrote:
> > >>>>
> > >>>> On 2/27/23 20:15, Jagan Teki wrote:
> > >>>>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@xxxxxxx> wrote:
> > >>>>>>
> > >>>>>> On 2/27/23 20:01, Jagan Teki wrote:
> > >>>>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@xxxxxxx> wrote:
> > >>>>>>>>
> > >>>>>>>> On 2/27/23 12:39, Jagan Teki wrote:
> > >>>>>>>>> drm_of_dsi_find_panel_or_bridge is capable of looking up the
> > >>>>>>>>> downstream DSI bridge and panel and trying to add a panel bridge
> > >>>>>>>>> if the panel is found.
> > >>>>>>>>>
> > >>>>>>>>> Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
> > >>>>>>>>> followed with drmm_panel_bridge_add.
> > >>>>>>>>>
> > >>>>>>>>> Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> > >>>>>>>>> ---
> > >>>>>>>>> Changes for v13, v12, v11:
> > >>>>>>>>> - none
> > >>>>>>>>> Changes for v10:
> > >>>>>>>>> - new patch
> > >>>>>>>>>
> > >>>>>>>>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 +++++++++++++------------
> > >>>>>>>>> 1 file changed, 13 insertions(+), 12 deletions(-)
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > >>>>>>>>> index df15501b1075..12a6dd987e8f 100644
> > >>>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > >>>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > >>>>>>>>> @@ -25,6 +25,7 @@
> > >>>>>>>>> #include <drm/drm_atomic_helper.h>
> > >>>>>>>>> #include <drm/drm_bridge.h>
> > >>>>>>>>> #include <drm/drm_mipi_dsi.h>
> > >>>>>>>>> +#include <drm/drm_of.h>
> > >>>>>>>>> #include <drm/drm_panel.h>
> > >>>>>>>>> #include <drm/drm_print.h>
> > >>>>>>>>> #include <drm/drm_probe_helper.h>
> > >>>>>>>>> @@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
> > >>>>>>>>> struct device *dev = dsi->dev;
> > >>>>>>>>> struct drm_encoder *encoder = &dsi->encoder;
> > >>>>>>>>> struct drm_device *drm = encoder->dev;
> > >>>>>>>>> + struct drm_bridge *bridge;
> > >>>>>>>>> struct drm_panel *panel;
> > >>>>>>>>> int ret;
> > >>>>>>>>>
> > >>>>>>>>> - panel = of_drm_find_panel(device->dev.of_node);
> > >>>>>>>>> - if (!IS_ERR(panel)) {
> > >>>>>>>>> - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > >>>>>>>>> - } else {
> > >>>>>>>>> - dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
> > >>>>>>>>> - if (!dsi->out_bridge)
> > >>>>>>>>> - dsi->out_bridge = ERR_PTR(-EINVAL);
> > >>>>>>>>> - }
> > >>>>>>>>
> > >>>>>>>> As far as I understand this from my conversation with Maxime (please put
> > >>>>>>>> him on CC of V15), the change here should instead perform the panel look
> > >>>>>>>> up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in
> > >>>>>>>> the component_ops .bind callback . Here is an example of correct
> > >>>>>>>> implementation:
> > >>>>>>>>
> > >>>>>>>> https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805
> > >>>>>>>
> > >>>>>>> But, we don't have component_ops.bind for imx8m case, so where do we
> > >>>>>>> want to place the panel hook?
> > >>>>>>>
> > >>>>>>> Exynos drm drivers are component-based but, imx8mm is not.
> > >>>>>>
> > >>>>>> In 14/18 patch, the same should be added to generic_dsim_register_host()
> > >>>>>> , which is called from the driver .probe() callback, but that is OK.
> > >>>>>>
> > >>>>>> That's ^ is the iMX part.
> > >>>>>
> > >>>>> Ohh. You mean, we need to find the panel hook separately in two places like
> > >>>>> - bind for exynos
> > >>>>> - probe for imx8mm
> > >>>>
> > >>>> Yes
> > >>>>
> > >>>>> If so? how can I find the drm_device pointer in the probe?
> > >>>>
> > >>>> What for ? The panel lookup uses OF graph . Where do you need the
> > >>>> drm_device in it ?
> > >>>
> > >>> May I can summarize the whole setback here so that everybody is on the
> > >>> same page and find the proper solution?
> > >>>
> > >>> The key blocker is accessing the DRM pointer in order to use the
> > >>> DRM-managed action helper.
> > >>>
> > >>> 1. If we find the panel hook in Exynos component_ops.bind then we can
> > >>> use the DRM pointer directly as VC4 does.
> > >>> 2. if we find the panel hook in Samsung drm_bridge_funcs.attach (for
> > >>> imx8mm) then we can use the DRM pointer directly via bridge->dev.
> > >>>
> > >>> If we make 2) has common across like finding the panel hook in
> > >>> drm_bridge_funcs.attach the Exynos drm pipeline cannot find the
> > >>> panels.
> > >>>
> > >>> The common solution for both exynos and imx8m is host.attach but if we
> > >>> do so we cannot get find the DRM pointer.
> > >>
> > >> There isn't going to be common solution, you would really have to do the
> > >> look up in component_ops .bind callback for exynos , and
> > >> generic_dsim_register_host() for i.MX .
> > >>
> > >>> If we go ahead with no need for DRM-managed helper at the moment, then
> > >>> find the panel hook in host.attach and then drop 2/18.
> > >>
> > >> The panel lookup must happen in .bind/.probe for exynos/imx respectively
> > >> , that's really all that is required here. Then you can drop 1,2,3/18
> > >> and get this series applied (I hope) .
> > >
> > > I'm not quite sure, if the panel hook in .bind work for exynos or not?
> >
> > Marek should be able to test exynos for you one more time I hope.
> >
> > > the host. attach has KMS hotplug code after the panel hook and
> > > bridge_attach as before. I believe that is a working scenario for
> > > Exynos to be the panel hook in the host. attach.
> >
> > As far as I understand it, the look up has to be in .bind callback (and
> > generic_dsim_register_host() for imx). Can you try if/how that works ?
> >
> > >> Can you implement just this one change ?
> > >>
> > >> There is no need to use drmm_* helper for now, that can be improved
> > >> later if possible.
> > >
> > > If this is the case then 1/18 will need otherwise
> >
> > Ah yes, 1/18 will be needed indeed. It should just be called from .bind
> > callback or generic_dsim_register_host() (i.e. probe callback).
>
> panel hook at the probe call would be wrong.
Why?
> the downstream bridge will call mipi_dsi_attach for finding the
> connected upstream, so it indeed calls host.attach. If we move the
> panel hook at the probe, then probing will defer.
>
> [ 12.046862] platform 32e10000.dsi: deferred probe pending
> [ 12.052309] platform 32e00000.lcdif: deferred probe pending
What is the dependency chain there, and why doesn't it probe?
> What is the problem to have it in host.attach for both cases?
You've put a link to the documentation that explains what the problem is
in your mail.
> some DSI host bridges still do the same (mtk_dsi)
Then some are broken. How is that an argument?
> and this is what is mentioned in documentation point 2 and 3 here,
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges
>
> "
> The upstream driver doesn’t use the component framework, but is a
> MIPI-DSI host.
Exynos uses the component framework. So that point doesn't apply to it.
> The bridge device uses the MIPI-DCS commands to be
> controlled. In this case, the bridge device is a child of the display
> device and when it will probe it’s assured that the display device
> (and MIPI-DSI host) is present. The upstream driver will be assured
> that the bridge driver is connected between the
> mipi_dsi_host_ops.attach and mipi_dsi_host_ops.detach operations.
> Therefore, it must run mipi_dsi_host_register() in its probe function,
> and then run drm_bridge_attach() in its mipi_dsi_host_ops.attach hook.
>
> The upstream driver uses the component framework and is a MIPI-DSI
> host. The bridge device uses the MIPI-DCS commands to be controlled.
> This is the same situation than above, and can run
> mipi_dsi_host_register() in either its probe or bind hooks.
> "
>
> Point 2 for Exynos and 3 for imx8m flow, for both the cases
> drm_bridge_attach in host_ops.attach hook so the panel hook must be in
> same place.
And you forgot the fourth point:
> The upstream driver uses the component framework and is a MIPI-DSI
> host. The bridge device uses a separate bus (such as I2C) to be
> controlled. In this case, there’s no correlation between the probe of
> the bridge and upstream drivers, so care must be taken to avoid an
> endless EPROBE_DEFER loop, with each driver waiting for the other to
> probe.
Which is what the whole discussion is about.
Maxime
