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. 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 problem to have it in host.attach for both cases? some DSI
host bridges still do the same (mtk_dsi) 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. 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.
Thanks,
Jagan.
