diff mbox series

[v13,04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper

Message ID 20230227113925.875425-5-jagan@amarulasolutions.com (mailing list archive)
State New
Headers show
Series drm: Add Samsung MIPI DSIM bridge | expand

Commit Message

Jagan Teki Feb. 27, 2023, 11:39 a.m. UTC
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@amarulasolutions.com>
---
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(-)

Comments

Marek Vasut Feb. 27, 2023, 6:55 p.m. UTC | #1
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@amarulasolutions.com>
> ---
> 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

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.

With that implemented, you shouldn't need patches 1,2,3/18 anymore , so 
they can be dropped . And with this fixed, I think the driver should be 
good to be picked.

Please correct me if I'm wrong here.
Jagan Teki Feb. 27, 2023, 7:01 p.m. UTC | #2
On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> > ---
> > 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.

Jagan.
Marek Vasut Feb. 27, 2023, 7:08 p.m. UTC | #3
On 2/27/23 20:01, Jagan Teki wrote:
> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
>>> ---
>>> 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.
Jagan Teki Feb. 27, 2023, 7:15 p.m. UTC | #4
On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/27/23 20:01, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> >>> ---
> >>> 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

If so? how can I find the drm_device pointer in the probe?

Jagan.
Marek Vasut Feb. 27, 2023, 7:24 p.m. UTC | #5
On 2/27/23 20:15, Jagan Teki wrote:
> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/27/23 20:01, Jagan Teki wrote:
>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
>>>>> ---
>>>>> 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 ?
Jagan Teki Feb. 27, 2023, 7:34 p.m. UTC | #6
On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/27/23 20:15, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 2/27/23 20:01, Jagan Teki wrote:
> >>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> >>>>> ---
> >>>>> 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.

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.

Thanks,
Jagan.
Marek Vasut Feb. 27, 2023, 7:41 p.m. UTC | #7
On 2/27/23 20:34, Jagan Teki wrote:
> On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/27/23 20:15, Jagan Teki wrote:
>>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 2/27/23 20:01, Jagan Teki wrote:
>>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
>>>>>>> ---
>>>>>>> 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) .

Can you implement just this one change ?

There is no need to use drmm_* helper for now, that can be improved 
later if possible.
Jagan Teki Feb. 27, 2023, 7:49 p.m. UTC | #8
On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/27/23 20:34, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 2/27/23 20:15, Jagan Teki wrote:
> >>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 2/27/23 20:01, Jagan Teki wrote:
> >>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> >>>>>>> ---
> >>>>>>> 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?
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.

>
> 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 we can drop 1,2,3/18
by doing the panel hook we did in v7
https://patchwork.kernel.org/project/dri-devel/patch/20221005151309.7278-3-jagan@amarulasolutions.com/

Jagan
Jagan Teki Feb. 27, 2023, 7:52 p.m. UTC | #9
On Tue, Feb 28, 2023 at 1:19 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut <marex@denx.de> wrote:
> >
> > On 2/27/23 20:34, Jagan Teki wrote:
> > > On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
> > >>
> > >> On 2/27/23 20:15, Jagan Teki wrote:
> > >>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
> > >>>>
> > >>>> On 2/27/23 20:01, Jagan Teki wrote:
> > >>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> > >>>>>>> ---
> > >>>>>>> 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?
> 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.
>
> >
> > 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 we can drop 1,2,3/18
> by doing the panel hook we did in v7
> https://patchwork.kernel.org/project/dri-devel/patch/20221005151309.7278-3-jagan@amarulasolutions.com/

This makes panel hook common a cross bridge by handling in the host.attach

Jagan.
Marek Vasut Feb. 27, 2023, 8:10 p.m. UTC | #10
On 2/27/23 20:49, Jagan Teki wrote:
> On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/27/23 20:34, Jagan Teki wrote:
>>> On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 2/27/23 20:15, Jagan Teki wrote:
>>>>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 2/27/23 20:01, Jagan Teki wrote:
>>>>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
>>>>>>>>> ---
>>>>>>>>> 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).

>  we can drop 1,2,3/18
> by doing the panel hook we did in v7
> https://patchwork.kernel.org/project/dri-devel/patch/20221005151309.7278-3-jagan@amarulasolutions.com/

[...]
Jagan Teki Feb. 27, 2023, 8:38 p.m. UTC | #11
On Tue, Feb 28, 2023 at 1:40 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/27/23 20:49, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 2/27/23 20:34, Jagan Teki wrote:
> >>> On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 2/27/23 20:15, Jagan Teki wrote:
> >>>>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 2/27/23 20:01, Jagan Teki wrote:
> >>>>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> >>>>>>>>> ---
> >>>>>>>>> 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.
Maxime Ripard Feb. 28, 2023, 12:04 p.m. UTC | #12
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@denx.de> wrote:
> >
> > On 2/27/23 20:49, Jagan Teki wrote:
> > > On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut <marex@denx.de> wrote:
> > >>
> > >> On 2/27/23 20:34, Jagan Teki wrote:
> > >>> On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
> > >>>>
> > >>>> On 2/27/23 20:15, Jagan Teki wrote:
> > >>>>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
> > >>>>>>
> > >>>>>> On 2/27/23 20:01, Jagan Teki wrote:
> > >>>>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> > >>>>>>>>> ---
> > >>>>>>>>> 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
Maxime Ripard Feb. 28, 2023, 12:10 p.m. UTC | #13
On Mon, Feb 27, 2023 at 08:41:22PM +0100, Marek Vasut wrote:
> > 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) .
> 
> Can you implement just this one change ?
> 
> There is no need to use drmm_* helper for now, that can be improved later if
> possible.

Yeah... The drmm helper isn't needed per se, but not using it will
create a use-after-free pattern that is very easy to miss.

I'd really prefer not to add a new helper that favors an unsafe pattern,
but the driver seems to have a whole bunch of them anyway so it's not
really a big deal.

Which also raises another question: if it's code that is only really
relevant in the context of that driver, why are you creating a helper
for it in the first place?

It would be much easier to just have that code in your driver: there's
no need to consider all cases, document it properly, you can have as
many workarounds as possible, etc.

Maxime
Jagan Teki Feb. 28, 2023, 12:34 p.m. UTC | #14
On Tue, Feb 28, 2023 at 5:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Mon, Feb 27, 2023 at 08:41:22PM +0100, Marek Vasut wrote:
> > > 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) .
> >
> > Can you implement just this one change ?
> >
> > There is no need to use drmm_* helper for now, that can be improved later if
> > possible.
>
> Yeah... The drmm helper isn't needed per se, but not using it will
> create a use-after-free pattern that is very easy to miss.
>
> I'd really prefer not to add a new helper that favors an unsafe pattern,
> but the driver seems to have a whole bunch of them anyway so it's not
> really a big deal.
>
> Which also raises another question: if it's code that is only really
> relevant in the context of that driver, why are you creating a helper
> for it in the first place?

I can answer this question as I did add these helpers.

1. DSI-specific helper added since it is a good candidate for
common/helper code, based on the comments in V9 by Marek. V
https://patchwork.kernel.org/project/dri-devel/patch/20221209152343.180139-8-jagan@amarulasolutions.com/

So, I have added this to the common drm_of code in V10.
https://patchwork.kernel.org/project/dri-devel/patch/20221214125907.376148-2-jagan@amarulasolutions.com/

2. DRM-managed discussion was commented on in V11 by you, so from
where all discussion moved.
https://patchwork.kernel.org/project/dri-devel/patch/20230123151212.269082-3-jagan@amarulasolutions.com/

1) helper wouldn't be an unsafe helper as it can reuse many DSI
drivers but 2) helper might be an unsafe helper at the moment.

Jagan.
Maxime Ripard Feb. 28, 2023, 2:35 p.m. UTC | #15
On Tue, Feb 28, 2023 at 06:04:39PM +0530, Jagan Teki wrote:
> On Tue, Feb 28, 2023 at 5:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Mon, Feb 27, 2023 at 08:41:22PM +0100, Marek Vasut wrote:
> > > > 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) .
> > >
> > > Can you implement just this one change ?
> > >
> > > There is no need to use drmm_* helper for now, that can be improved later if
> > > possible.
> >
> > Yeah... The drmm helper isn't needed per se, but not using it will
> > create a use-after-free pattern that is very easy to miss.
> >
> > I'd really prefer not to add a new helper that favors an unsafe pattern,
> > but the driver seems to have a whole bunch of them anyway so it's not
> > really a big deal.
> >
> > Which also raises another question: if it's code that is only really
> > relevant in the context of that driver, why are you creating a helper
> > for it in the first place?
> 
> I can answer this question as I did add these helpers.
> 
> 1. DSI-specific helper added since it is a good candidate for
> common/helper code, based on the comments in V9 by Marek. V
> https://patchwork.kernel.org/project/dri-devel/patch/20221209152343.180139-8-jagan@amarulasolutions.com/
> 
> So, I have added this to the common drm_of code in V10.
> https://patchwork.kernel.org/project/dri-devel/patch/20221214125907.376148-2-jagan@amarulasolutions.com/
> 
> 2. DRM-managed discussion was commented on in V11 by you, so from
> where all discussion moved.
> https://patchwork.kernel.org/project/dri-devel/patch/20230123151212.269082-3-jagan@amarulasolutions.com/
> 
> 1) helper wouldn't be an unsafe helper as it can reuse many DSI
> drivers but 2) helper might be an unsafe helper at the moment.

You're wrong. The first one is unsafe, for the same reason than the devm
one you did is unsafe: the assumption everyone will get (and the one you
implemented in your driver) is that the bridge reference will be put
back at remove time.

The DRM/KMS structures however are free'd only when the last user closes
the file descriptor of the KMS device, so your driver functions will get
called after remove has been called.

If you are using the panel or bridge in any of your KMS hooks
implementations, this is now a use-after-free.

The drmm variant is safe though, because drmm actions run not when the
device is removed but when the DRM device is last closed.

Maxime
Jagan Teki Feb. 28, 2023, 2:39 p.m. UTC | #16
On Tue, Feb 28, 2023 at 5:34 PM Maxime Ripard <maxime@cerno.tech> 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@denx.de> wrote:
> > >
> > > On 2/27/23 20:49, Jagan Teki wrote:
> > > > On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut <marex@denx.de> wrote:
> > > >>
> > > >> On 2/27/23 20:34, Jagan Teki wrote:
> > > >>> On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
> > > >>>>
> > > >>>> On 2/27/23 20:15, Jagan Teki wrote:
> > > >>>>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
> > > >>>>>>
> > > >>>>>> On 2/27/23 20:01, Jagan Teki wrote:
> > > >>>>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> > > >>>>>>>>> ---
> > > >>>>>>>>> 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.
Jagan Teki Feb. 28, 2023, 2:50 p.m. UTC | #17
On Tue, Feb 28, 2023 at 8:05 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Tue, Feb 28, 2023 at 06:04:39PM +0530, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 5:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Mon, Feb 27, 2023 at 08:41:22PM +0100, Marek Vasut wrote:
> > > > > 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) .
> > > >
> > > > Can you implement just this one change ?
> > > >
> > > > There is no need to use drmm_* helper for now, that can be improved later if
> > > > possible.
> > >
> > > Yeah... The drmm helper isn't needed per se, but not using it will
> > > create a use-after-free pattern that is very easy to miss.
> > >
> > > I'd really prefer not to add a new helper that favors an unsafe pattern,
> > > but the driver seems to have a whole bunch of them anyway so it's not
> > > really a big deal.
> > >
> > > Which also raises another question: if it's code that is only really
> > > relevant in the context of that driver, why are you creating a helper
> > > for it in the first place?
> >
> > I can answer this question as I did add these helpers.
> >
> > 1. DSI-specific helper added since it is a good candidate for
> > common/helper code, based on the comments in V9 by Marek. V
> > https://patchwork.kernel.org/project/dri-devel/patch/20221209152343.180139-8-jagan@amarulasolutions.com/
> >
> > So, I have added this to the common drm_of code in V10.
> > https://patchwork.kernel.org/project/dri-devel/patch/20221214125907.376148-2-jagan@amarulasolutions.com/
> >
> > 2. DRM-managed discussion was commented on in V11 by you, so from
> > where all discussion moved.
> > https://patchwork.kernel.org/project/dri-devel/patch/20230123151212.269082-3-jagan@amarulasolutions.com/
> >
> > 1) helper wouldn't be an unsafe helper as it can reuse many DSI
> > drivers but 2) helper might be an unsafe helper at the moment.
>
> You're wrong. The first one is unsafe, for the same reason than the devm
> one you did is unsafe: the assumption everyone will get (and the one you
> implemented in your driver) is that the bridge reference will be put
> back at remove time.
>
> The DRM/KMS structures however are free'd only when the last user closes
> the file descriptor of the KMS device, so your driver functions will get
> called after remove has been called.
>
> If you are using the panel or bridge in any of your KMS hooks
> implementations, this is now a use-after-free.
>
> The drmm variant is safe though, because drmm actions run not when the
> device is removed but when the DRM device is last closed.

Okay. So, even if we manually use drm_of_dsi_find_panel_or_bridge in
mipi_dsi_host_ops.attach and drm_panel_bridge_remove() in
mipi_dsi_host_ops.detach KMS doesn't aware of it and is still unsafe.
- am I correct?

Jagan.
Maxime Ripard Feb. 28, 2023, 2:56 p.m. UTC | #18
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@cerno.tech> 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@denx.de> wrote:
> > > >
> > > > On 2/27/23 20:49, Jagan Teki wrote:
> > > > > On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut <marex@denx.de> wrote:
> > > > >>
> > > > >> On 2/27/23 20:34, Jagan Teki wrote:
> > > > >>> On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
> > > > >>>>
> > > > >>>> On 2/27/23 20:15, Jagan Teki wrote:
> > > > >>>>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
> > > > >>>>>>
> > > > >>>>>> On 2/27/23 20:01, Jagan Teki wrote:
> > > > >>>>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> > > > >>>>>>>>> ---
> > > > >>>>>>>>> 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?

> 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.

No, you're not correct. Just like the documentation, Marek and I have
repeatedly told you.

> lcdif => samsung-ti => dsim-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.

Why not, dev->of_node is very much valid at probe time.

Maxime
Jagan Teki Feb. 28, 2023, 3:09 p.m. UTC | #19
On Tue, Feb 28, 2023 at 8:26 PM Maxime Ripard <maxime@cerno.tech> 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@cerno.tech> 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@denx.de> wrote:
> > > > >
> > > > > On 2/27/23 20:49, Jagan Teki wrote:
> > > > > > On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut <marex@denx.de> wrote:
> > > > > >>
> > > > > >> On 2/27/23 20:34, Jagan Teki wrote:
> > > > > >>> On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut <marex@denx.de> wrote:
> > > > > >>>>
> > > > > >>>> On 2/27/23 20:15, Jagan Teki wrote:
> > > > > >>>>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut <marex@denx.de> wrote:
> > > > > >>>>>>
> > > > > >>>>>> On 2/27/23 20:01, Jagan Teki wrote:
> > > > > >>>>>>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut <marex@denx.de> 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@amarulasolutions.com>
> > > > > >>>>>>>>> ---
> > > > > >>>>>>>>> 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.
diff mbox series

Patch

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);
-	}
+	ret = drm_of_dsi_find_panel_or_bridge(dev->of_node, 1, 0,
+					      &panel, &bridge);
+	if (ret)
+		return ret;
 
-	if (IS_ERR(dsi->out_bridge)) {
-		ret = PTR_ERR(dsi->out_bridge);
+	if (panel)
+		bridge = drmm_panel_bridge_add(NULL, panel);
+
+	if (IS_ERR(bridge)) {
+		ret = PTR_ERR(bridge);
 		DRM_DEV_ERROR(dev, "failed to find the bridge: %d\n", ret);
 		return ret;
 	}
 
+	dsi->out_bridge = bridge;
+
 	DRM_DEV_INFO(dev, "Attached %s device\n", device->name);
 
 	drm_bridge_add(&dsi->bridge);
@@ -1531,8 +1534,6 @@  static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 	struct exynos_dsi *dsi = host_to_dsi(host);
 	struct drm_device *drm = dsi->encoder.dev;
 
-	dsi->out_bridge = NULL;
-
 	if (drm->mode_config.poll_enabled)
 		drm_kms_helper_hotplug_event(drm);