On Tue, Feb 28, 2023 at 8:26 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> On Tue, Feb 28, 2023 at 08:09:26PM +0530, Jagan Teki wrote:
> > 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.
>
> But samsung-dsim is used together with the component framework, so this
> doesn't work.
>
> Seriously, I've been telling you that it doesn't work. We spent an hour
> discussing this with Marek yesterday who also explained this to you.
> Stop trying to make that happen, it just doesn't work.
>
> Can we leave that solution behind and move forward?
I have given the logs of why it's not working. I did my best to
explain. Samsung-dsim is not component-based it is a non-component DSI
host bridge exclusively for imx8mm and Exynos DSI is component-based.
Samsung-dsim we have a platform calls to call Exynos which will
operate component binding. This means that imx8mm stuff cannot use the
component framework at all.
This is all I can explain. and adding panel or bridge finding code in
the probe simply not working on imx8mm as it is non-component based.
Thanks for your patience in answering queries.
Jagan.
