On Tue, Feb 28, 2023 at 5:34 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> 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?
Let me answer here for the above 2 queries.
This is clearly a point 2 scenario from the documentation.
"
The upstream driver doesn’t use the component framework, but is a
MIPI-DSI host. 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.
"
So, the samsung-dsim follows the same rule, mipi_dsi_host_register()
in the probe and drm_bridge_attach() in mipi_dsi_host_ops.attach hook.
Above also mentioned "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" that means we need to find the
panel or bridge in mipi_dsi_host_ops.attach am I correct? not in the
probe.
lcdif => samsung-dsim => ti-sn65dsi (I2C) => panel this is the bridge
chain. ti-sn65dsi is registered the MIPI Device and attaches the
upstream driver via mipi_dsi_attach that indeed calls upstream driver
mipi_dsi_host_ops.attach so if we move finding panel or bridge from
mipi_dsi_host_ops.attach to the probe then dev->of_node cannot be
valid so it will not find the bridge driver.
>
> > 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?
May be mtk_dsi has some different approach, not sure.
>
> > 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.
I got it, Points 3, and 4 are followed by three points that are
related to Exynos DSI and the connected bridge is I2C-based. In this
case, the existing Exynos DSI has some faults. and It is working for
Exynos DSI since they didn't deal any I2C-based bridge driver, may be.
Jagan.
