| Message ID | 20200417175238.27154-3-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support DRM bridges on NVIDIA Tegra | expand |
Hi Dmitry, Thank you for the patch. On Fri, Apr 17, 2020 at 08:52:37PM +0300, Dmitry Osipenko wrote: > Newer Tegra device-trees will specify a video output graph which involves > a bridge. This patch adds initial support for the DRM bridges to the > Tegra's DRM output. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/gpu/drm/tegra/drm.h | 2 ++ > drivers/gpu/drm/tegra/output.c | 13 ++++++++++++- > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h > index 804869799305..cccd368b6752 100644 > --- a/drivers/gpu/drm/tegra/drm.h > +++ b/drivers/gpu/drm/tegra/drm.h > @@ -12,6 +12,7 @@ > #include <linux/of_gpio.h> > > #include <drm/drm_atomic.h> > +#include <drm/drm_bridge.h> You could add a forward declaration of struct drm_bridge instead, that can lower the compilation time a little bit. > #include <drm/drm_edid.h> > #include <drm/drm_encoder.h> > #include <drm/drm_fb_helper.h> > @@ -116,6 +117,7 @@ struct tegra_output { > struct device_node *of_node; > struct device *dev; > > + struct drm_bridge *bridge; > struct drm_panel *panel; > struct i2c_adapter *ddc; > const struct edid *edid; > diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c > index a6a711d54e88..ec0cd4a1ced1 100644 > --- a/drivers/gpu/drm/tegra/output.c > +++ b/drivers/gpu/drm/tegra/output.c > @@ -5,6 +5,7 @@ > */ > > #include <drm/drm_atomic_helper.h> > +#include <drm/drm_of.h> > #include <drm/drm_panel.h> > #include <drm/drm_simple_kms_helper.h> > > @@ -92,13 +93,23 @@ static irqreturn_t hpd_irq(int irq, void *data) > > int tegra_output_probe(struct tegra_output *output) > { > - struct device_node *ddc, *panel; > + struct device_node *ddc, *panel, *port; > unsigned long flags; > int err, size; > > if (!output->of_node) > output->of_node = output->dev->of_node; > > + port = of_get_child_by_name(output->of_node, "port"); Do you need to check for the presence of a port node first ? Can you just check the return value of drm_of_find_panel_or_bridge(), and fall back to "nvidia,panel" if it returns -ENODEV ? > + if (port) { > + err = drm_of_find_panel_or_bridge(output->of_node, 0, 0, NULL, > + &output->bridge); > + of_node_put(port); > + > + if (err) > + return err; > + } > + > panel = of_parse_phandle(output->of_node, "nvidia,panel", 0); > if (panel) { > output->panel = of_drm_find_panel(panel);
Hello Laurent, 17.04.2020 22:30, Laurent Pinchart пишет: ... >> #include <drm/drm_atomic.h> >> +#include <drm/drm_bridge.h> > > You could add a forward declaration of struct drm_bridge instead, that > can lower the compilation time a little bit. This include is not only for the struct, but also for the drm_bridge_attach(). It looks to me that it should be nicer to keep the include here. ... >> + port = of_get_child_by_name(output->of_node, "port"); > > Do you need to check for the presence of a port node first ? Can you > just check the return value of drm_of_find_panel_or_bridge(), and fall > back to "nvidia,panel" if it returns -ENODEV ? Without the check, the drm_of_find_panel_or_bridge() prints a very noisy error message about missing port node for every output that doesn't have a graph specified in a device-tree (HDMI, DSI and etc). https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621
Hi Dmitry, On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote: > 17.04.2020 22:30, Laurent Pinchart пишет: > ... > >> #include <drm/drm_atomic.h> > >> +#include <drm/drm_bridge.h> > > > > You could add a forward declaration of struct drm_bridge instead, that > > can lower the compilation time a little bit. > > This include is not only for the struct, but also for the > drm_bridge_attach(). It looks to me that it should be nicer to keep the > include here. drm_bridge_attach() is called from .c files. In the .h file you can use a forward declaration. It's entirely up to you, but as a general rule, I personally try to use forward structure declarations in .h files as much as possible. > ... > >> + port = of_get_child_by_name(output->of_node, "port"); > > > > Do you need to check for the presence of a port node first ? Can you > > just check the return value of drm_of_find_panel_or_bridge(), and fall > > back to "nvidia,panel" if it returns -ENODEV ? > > Without the check, the drm_of_find_panel_or_bridge() prints a very noisy > error message about missing port node for every output that doesn't have > a graph specified in a device-tree (HDMI, DSI and etc). > > https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621 Ah yes indeed. That's not very nice.
17.04.2020 23:31, Laurent Pinchart пишет: > Hi Dmitry, > > On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote: >> 17.04.2020 22:30, Laurent Pinchart пишет: >> ... >>>> #include <drm/drm_atomic.h> >>>> +#include <drm/drm_bridge.h> >>> >>> You could add a forward declaration of struct drm_bridge instead, that >>> can lower the compilation time a little bit. >> >> This include is not only for the struct, but also for the >> drm_bridge_attach(). It looks to me that it should be nicer to keep the >> include here. > > drm_bridge_attach() is called from .c files. In the .h file you can use > a forward declaration. It's entirely up to you, but as a general rule, I > personally try to use forward structure declarations in .h files as much > as possible. The current Tegra DRM code is a bit inconsistent in regards to having forward declarations, it doesn't have them more than have. I'll add a forward declaration if there will be need to make a v5, ok? >> ... >>>> + port = of_get_child_by_name(output->of_node, "port"); >>> >>> Do you need to check for the presence of a port node first ? Can you >>> just check the return value of drm_of_find_panel_or_bridge(), and fall >>> back to "nvidia,panel" if it returns -ENODEV ? >> >> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy >> error message about missing port node for every output that doesn't have >> a graph specified in a device-tree (HDMI, DSI and etc). >> >> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621 > > Ah yes indeed. That's not very nice. > Please let me know if you'll have a better idea about how this could be handled.
Hi Dmitry, On Fri, Apr 17, 2020 at 11:52:11PM +0300, Dmitry Osipenko wrote: > 17.04.2020 23:31, Laurent Pinchart пишет: > > On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote: > >> 17.04.2020 22:30, Laurent Pinchart пишет: > >> ... > >>>> #include <drm/drm_atomic.h> > >>>> +#include <drm/drm_bridge.h> > >>> > >>> You could add a forward declaration of struct drm_bridge instead, that > >>> can lower the compilation time a little bit. > >> > >> This include is not only for the struct, but also for the > >> drm_bridge_attach(). It looks to me that it should be nicer to keep the > >> include here. > > > > drm_bridge_attach() is called from .c files. In the .h file you can use > > a forward declaration. It's entirely up to you, but as a general rule, I > > personally try to use forward structure declarations in .h files as much > > as possible. > > The current Tegra DRM code is a bit inconsistent in regards to having > forward declarations, it doesn't have them more than have. > > I'll add a forward declaration if there will be need to make a v5, ok? It's up to you, you don't have to use a forward declaration if you don't want to, I was just pointing out what I think is a best practice rule :-) > >> ... > >>>> + port = of_get_child_by_name(output->of_node, "port"); > >>> > >>> Do you need to check for the presence of a port node first ? Can you > >>> just check the return value of drm_of_find_panel_or_bridge(), and fall > >>> back to "nvidia,panel" if it returns -ENODEV ? > >> > >> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy > >> error message about missing port node for every output that doesn't have > >> a graph specified in a device-tree (HDMI, DSI and etc). > >> > >> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621 > > > > Ah yes indeed. That's not very nice. > > Please let me know if you'll have a better idea about how this could be > handled. It should be good enough as-is I think. You may however want to support both "port" and "ports", as even when there's a single port node, it could be put inside a ports node.
17.04.2020 23:58, Laurent Pinchart пишет: > Hi Dmitry, > > On Fri, Apr 17, 2020 at 11:52:11PM +0300, Dmitry Osipenko wrote: >> 17.04.2020 23:31, Laurent Pinchart пишет: >>> On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote: >>>> 17.04.2020 22:30, Laurent Pinchart пишет: >>>> ... >>>>>> #include <drm/drm_atomic.h> >>>>>> +#include <drm/drm_bridge.h> >>>>> >>>>> You could add a forward declaration of struct drm_bridge instead, that >>>>> can lower the compilation time a little bit. >>>> >>>> This include is not only for the struct, but also for the >>>> drm_bridge_attach(). It looks to me that it should be nicer to keep the >>>> include here. >>> >>> drm_bridge_attach() is called from .c files. In the .h file you can use >>> a forward declaration. It's entirely up to you, but as a general rule, I >>> personally try to use forward structure declarations in .h files as much >>> as possible. >> >> The current Tegra DRM code is a bit inconsistent in regards to having >> forward declarations, it doesn't have them more than have. >> >> I'll add a forward declaration if there will be need to make a v5, ok? > > It's up to you, you don't have to use a forward declaration if you don't > want to, I was just pointing out what I think is a best practice rule > :-) Alright, then I'll leave the include as-is in this patch since it should be better to keep the code consistent even if it's a bit less optimal than it could be, IMO. We may return to cleaning up of driver includes later on. >>>> ... >>>>>> + port = of_get_child_by_name(output->of_node, "port"); >>>>> >>>>> Do you need to check for the presence of a port node first ? Can you >>>>> just check the return value of drm_of_find_panel_or_bridge(), and fall >>>>> back to "nvidia,panel" if it returns -ENODEV ? >>>> >>>> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy >>>> error message about missing port node for every output that doesn't have >>>> a graph specified in a device-tree (HDMI, DSI and etc). >>>> >>>> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621 >>> >>> Ah yes indeed. That's not very nice. >> >> Please let me know if you'll have a better idea about how this could be >> handled. > > It should be good enough as-is I think. You may however want to support > both "port" and "ports", as even when there's a single port node, it > could be put inside a ports node. > I'll make a v5 that will have additional patches for making drm_of_find_panel_or_bridge() to better handle that case. While at it, I'll also add a patch that will wrap RGB panel into bridge. Thank you very much for the reviews!
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index 804869799305..cccd368b6752 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -12,6 +12,7 @@ #include <linux/of_gpio.h> #include <drm/drm_atomic.h> +#include <drm/drm_bridge.h> #include <drm/drm_edid.h> #include <drm/drm_encoder.h> #include <drm/drm_fb_helper.h> @@ -116,6 +117,7 @@ struct tegra_output { struct device_node *of_node; struct device *dev; + struct drm_bridge *bridge; struct drm_panel *panel; struct i2c_adapter *ddc; const struct edid *edid; diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c index a6a711d54e88..ec0cd4a1ced1 100644 --- a/drivers/gpu/drm/tegra/output.c +++ b/drivers/gpu/drm/tegra/output.c @@ -5,6 +5,7 @@ */ #include <drm/drm_atomic_helper.h> +#include <drm/drm_of.h> #include <drm/drm_panel.h> #include <drm/drm_simple_kms_helper.h> @@ -92,13 +93,23 @@ static irqreturn_t hpd_irq(int irq, void *data) int tegra_output_probe(struct tegra_output *output) { - struct device_node *ddc, *panel; + struct device_node *ddc, *panel, *port; unsigned long flags; int err, size; if (!output->of_node) output->of_node = output->dev->of_node; + port = of_get_child_by_name(output->of_node, "port"); + if (port) { + err = drm_of_find_panel_or_bridge(output->of_node, 0, 0, NULL, + &output->bridge); + of_node_put(port); + + if (err) + return err; + } + panel = of_parse_phandle(output->of_node, "nvidia,panel", 0); if (panel) { output->panel = of_drm_find_panel(panel);
Newer Tegra device-trees will specify a video output graph which involves a bridge. This patch adds initial support for the DRM bridges to the Tegra's DRM output. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/gpu/drm/tegra/drm.h | 2 ++ drivers/gpu/drm/tegra/output.c | 13 ++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-)