* [PATCH] drm/bridge: Move mhl.h into driver directory @ 2020-04-15 17:38 Daniel Vetter 2020-04-15 17:48 ` Laurent Pinchart 0 siblings, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2020-04-15 17:38 UTC (permalink / raw) To: DRI Development Cc: Kate Stewart, Jernej Skrabec, Neil Armstrong, Daniel Vetter, Jonas Karlman, Alexey Brodkin, Gustavo A. R. Silva, Andrzej Hajda, Laurent Pinchart, Greg Kroah-Hartman, Daniel Vetter, Thomas Gleixner, Sam Ravnborg, Allison Randal include/drm/bridge is a bit a mistake, drivers are supposed to find their bridges using one of the standard of_* functions the drm_bridge core provides. dw-hdmi and analogix-dp are the only, historically grown exception that we haven't managed to get rid of yet. Make sure that at least no new ones grow by moving hardware header files into the correct driver directory. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Alexey Brodkin <abrodkin@synopsys.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Neil Armstrong <narmstrong@baylibre.com> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Jernej Skrabec <jernej.skrabec@siol.net> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Kate Stewart <kstewart@linuxfoundation.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Allison Randal <allison@lohutok.net> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com> --- {include => drivers/gpu}/drm/bridge/mhl.h | 0 drivers/gpu/drm/bridge/sii9234.c | 3 ++- drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) rename {include => drivers/gpu}/drm/bridge/mhl.h (100%) diff --git a/include/drm/bridge/mhl.h b/drivers/gpu/drm/bridge/mhl.h similarity index 100% rename from include/drm/bridge/mhl.h rename to drivers/gpu/drm/bridge/mhl.h diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c index b1258f0ed205..4c862c3af038 100644 --- a/drivers/gpu/drm/bridge/sii9234.c +++ b/drivers/gpu/drm/bridge/sii9234.c @@ -12,7 +12,6 @@ * Shankar Bandal <shankar.b@samsung.com> * Dharam Kumar <dharam.kr@samsung.com> */ -#include <drm/bridge/mhl.h> #include <drm/drm_bridge.h> #include <drm/drm_crtc.h> #include <drm/drm_edid.h> @@ -29,6 +28,8 @@ #include <linux/regulator/consumer.h> #include <linux/slab.h> +#include "mhl.h" + #define CBUS_DEVCAP_OFFSET 0x80 #define SII9234_MHL_VERSION 0x11 diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 92acd336aa89..017dbb67404e 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -8,7 +8,6 @@ #include <asm/unaligned.h> -#include <drm/bridge/mhl.h> #include <drm/drm_bridge.h> #include <drm/drm_crtc.h> #include <drm/drm_edid.h> @@ -31,6 +30,7 @@ #include <media/rc-core.h> +#include "mhl.h" #include "sil-sii8620.h" #define SII8620_BURST_BUF_LEN 288 -- 2.25.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bridge: Move mhl.h into driver directory 2020-04-15 17:38 [PATCH] drm/bridge: Move mhl.h into driver directory Daniel Vetter @ 2020-04-15 17:48 ` Laurent Pinchart 2020-04-15 18:06 ` Daniel Vetter 0 siblings, 1 reply; 5+ messages in thread From: Laurent Pinchart @ 2020-04-15 17:48 UTC (permalink / raw) To: Daniel Vetter Cc: Kate Stewart, Jernej Skrabec, Neil Armstrong, Greg Kroah-Hartman, Jonas Karlman, Alexey Brodkin, DRI Development, Gustavo A. R. Silva, Andrzej Hajda, Daniel Vetter, Thomas Gleixner, Sam Ravnborg, Allison Randal Hi Daniel, Thank you for the patch. On Wed, Apr 15, 2020 at 07:38:33PM +0200, Daniel Vetter wrote: > include/drm/bridge is a bit a mistake, drivers are supposed to find > their bridges using one of the standard of_* functions the drm_bridge > core provides. I'm confused, I don't really see how that's related to mhl.h. The header defines constants and structures related to the MHL (Mobile High-Definition Link) protocol, which is an industry standard. If you want to move it out of include/drm/bridge/ to eventually remove that directory, I think it should be renamted to include/drm/drm_mhl.h. > dw-hdmi and analogix-dp are the only, historically > grown exception that we haven't managed to get rid of yet. The reason why we have shared headers for those is because they're IP cores integrated with different glue layers in different SoCs. There's one driver for the IP core itself, and SoC-specific glue drivers that need to provide the IP core drivers with data and callbacks, defined in shared headers. Granted, there's also data in those headers that are only internal to the IP core drivers, and that should be moved out, but for the interface header, include/drm/bridge/ doesn't seem to be a bad location to me. > Make sure that at least no new ones grow by moving hardware header > files into the correct driver directory. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Jernej Skrabec <jernej.skrabec@siol.net> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Kate Stewart <kstewart@linuxfoundation.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Allison Randal <allison@lohutok.net> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com> > --- > {include => drivers/gpu}/drm/bridge/mhl.h | 0 > drivers/gpu/drm/bridge/sii9234.c | 3 ++- > drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- > 3 files changed, 3 insertions(+), 2 deletions(-) > rename {include => drivers/gpu}/drm/bridge/mhl.h (100%) > > diff --git a/include/drm/bridge/mhl.h b/drivers/gpu/drm/bridge/mhl.h > similarity index 100% > rename from include/drm/bridge/mhl.h > rename to drivers/gpu/drm/bridge/mhl.h > diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c > index b1258f0ed205..4c862c3af038 100644 > --- a/drivers/gpu/drm/bridge/sii9234.c > +++ b/drivers/gpu/drm/bridge/sii9234.c > @@ -12,7 +12,6 @@ > * Shankar Bandal <shankar.b@samsung.com> > * Dharam Kumar <dharam.kr@samsung.com> > */ > -#include <drm/bridge/mhl.h> > #include <drm/drm_bridge.h> > #include <drm/drm_crtc.h> > #include <drm/drm_edid.h> > @@ -29,6 +28,8 @@ > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > > +#include "mhl.h" > + > #define CBUS_DEVCAP_OFFSET 0x80 > > #define SII9234_MHL_VERSION 0x11 > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c > index 92acd336aa89..017dbb67404e 100644 > --- a/drivers/gpu/drm/bridge/sil-sii8620.c > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > @@ -8,7 +8,6 @@ > > #include <asm/unaligned.h> > > -#include <drm/bridge/mhl.h> > #include <drm/drm_bridge.h> > #include <drm/drm_crtc.h> > #include <drm/drm_edid.h> > @@ -31,6 +30,7 @@ > > #include <media/rc-core.h> > > +#include "mhl.h" > #include "sil-sii8620.h" > > #define SII8620_BURST_BUF_LEN 288 -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bridge: Move mhl.h into driver directory 2020-04-15 17:48 ` Laurent Pinchart @ 2020-04-15 18:06 ` Daniel Vetter 2020-04-15 18:12 ` Laurent Pinchart 0 siblings, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2020-04-15 18:06 UTC (permalink / raw) To: Laurent Pinchart Cc: Kate Stewart, Jernej Skrabec, Neil Armstrong, Daniel Vetter, Jonas Karlman, Alexey Brodkin, DRI Development, Gustavo A. R. Silva, Andrzej Hajda, Greg Kroah-Hartman, Daniel Vetter, Thomas Gleixner, Sam Ravnborg, Allison Randal On Wed, Apr 15, 2020 at 08:48:06PM +0300, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Wed, Apr 15, 2020 at 07:38:33PM +0200, Daniel Vetter wrote: > > include/drm/bridge is a bit a mistake, drivers are supposed to find > > their bridges using one of the standard of_* functions the drm_bridge > > core provides. > > I'm confused, I don't really see how that's related to mhl.h. The header > defines constants and structures related to the MHL (Mobile > High-Definition Link) protocol, which is an industry standard. If you > want to move it out of include/drm/bridge/ to eventually remove that > directory, I think it should be renamted to include/drm/drm_mhl.h. It looked misplaced at least ... I guess moving this out of drm/bridge makes more sense. > > dw-hdmi and analogix-dp are the only, historically > > grown exception that we haven't managed to get rid of yet. > > The reason why we have shared headers for those is because they're IP > cores integrated with different glue layers in different SoCs. There's > one driver for the IP core itself, and SoC-specific glue drivers that > need to provide the IP core drivers with data and callbacks, defined in > shared headers. Granted, there's also data in those headers that are > only internal to the IP core drivers, and that should be moved out, but > for the interface header, include/drm/bridge/ doesn't seem to be a bad > location to me. The thing that irks me on them is that they kinda implement bridges, but they don't load like bridges. That's the part I think should get changed, or we need to finally figure out what exactly isn't good with the current drm_bridge handling and get that fixed (the relevant patches seem forever stuck in limbo, hence why I'm kicking). If that's not possible because these things just dont fit as drm_bridge, then maybe they shouldn't be a bridge, but something else. But looking at both dw-hdmi and analogix-dp these things look a lot like midlayers that get fed huge structures. Instead of more bare-bones toolboxes to build a set of similar drm_bridge drivers, which drivers then bind into using dt. So all a bit fishy imo. I guess step 1 at least would be to throw the connector and encoder code out of all these drivers, that would be at least a first step. Next one maybe push the per-variant bind code into drm/bridge and out of drivers, and use more standard of_ functions to find the bridges and tie them into the drm_device. Then 3rd round, some refactoring to demidlayer these libraries and make them real toolboxes. -Daniel > > > Make sure that at least no new ones grow by moving hardware header > > files into the correct driver directory. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Neil Armstrong <narmstrong@baylibre.com> > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > Cc: Jonas Karlman <jonas@kwiboo.se> > > Cc: Jernej Skrabec <jernej.skrabec@siol.net> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Kate Stewart <kstewart@linuxfoundation.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Allison Randal <allison@lohutok.net> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com> > > --- > > {include => drivers/gpu}/drm/bridge/mhl.h | 0 > > drivers/gpu/drm/bridge/sii9234.c | 3 ++- > > drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- > > 3 files changed, 3 insertions(+), 2 deletions(-) > > rename {include => drivers/gpu}/drm/bridge/mhl.h (100%) > > > > diff --git a/include/drm/bridge/mhl.h b/drivers/gpu/drm/bridge/mhl.h > > similarity index 100% > > rename from include/drm/bridge/mhl.h > > rename to drivers/gpu/drm/bridge/mhl.h > > diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c > > index b1258f0ed205..4c862c3af038 100644 > > --- a/drivers/gpu/drm/bridge/sii9234.c > > +++ b/drivers/gpu/drm/bridge/sii9234.c > > @@ -12,7 +12,6 @@ > > * Shankar Bandal <shankar.b@samsung.com> > > * Dharam Kumar <dharam.kr@samsung.com> > > */ > > -#include <drm/bridge/mhl.h> > > #include <drm/drm_bridge.h> > > #include <drm/drm_crtc.h> > > #include <drm/drm_edid.h> > > @@ -29,6 +28,8 @@ > > #include <linux/regulator/consumer.h> > > #include <linux/slab.h> > > > > +#include "mhl.h" > > + > > #define CBUS_DEVCAP_OFFSET 0x80 > > > > #define SII9234_MHL_VERSION 0x11 > > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c > > index 92acd336aa89..017dbb67404e 100644 > > --- a/drivers/gpu/drm/bridge/sil-sii8620.c > > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > > @@ -8,7 +8,6 @@ > > > > #include <asm/unaligned.h> > > > > -#include <drm/bridge/mhl.h> > > #include <drm/drm_bridge.h> > > #include <drm/drm_crtc.h> > > #include <drm/drm_edid.h> > > @@ -31,6 +30,7 @@ > > > > #include <media/rc-core.h> > > > > +#include "mhl.h" > > #include "sil-sii8620.h" > > > > #define SII8620_BURST_BUF_LEN 288 > > -- > Regards, > > Laurent Pinchart -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bridge: Move mhl.h into driver directory 2020-04-15 18:06 ` Daniel Vetter @ 2020-04-15 18:12 ` Laurent Pinchart 2020-04-15 18:19 ` Daniel Vetter 0 siblings, 1 reply; 5+ messages in thread From: Laurent Pinchart @ 2020-04-15 18:12 UTC (permalink / raw) To: Daniel Vetter Cc: Kate Stewart, Jernej Skrabec, Neil Armstrong, Daniel Vetter, Jonas Karlman, Alexey Brodkin, DRI Development, Gustavo A. R. Silva, Andrzej Hajda, Greg Kroah-Hartman, Daniel Vetter, Thomas Gleixner, Sam Ravnborg, Allison Randal Hi Daniel, On Wed, Apr 15, 2020 at 08:06:20PM +0200, Daniel Vetter wrote: > On Wed, Apr 15, 2020 at 08:48:06PM +0300, Laurent Pinchart wrote: > > On Wed, Apr 15, 2020 at 07:38:33PM +0200, Daniel Vetter wrote: > > > include/drm/bridge is a bit a mistake, drivers are supposed to find > > > their bridges using one of the standard of_* functions the drm_bridge > > > core provides. > > > > I'm confused, I don't really see how that's related to mhl.h. The header > > defines constants and structures related to the MHL (Mobile > > High-Definition Link) protocol, which is an industry standard. If you > > want to move it out of include/drm/bridge/ to eventually remove that > > directory, I think it should be renamted to include/drm/drm_mhl.h. > > It looked misplaced at least ... I guess moving this out of drm/bridge > makes more sense. > > > > dw-hdmi and analogix-dp are the only, historically > > > grown exception that we haven't managed to get rid of yet. > > > > The reason why we have shared headers for those is because they're IP > > cores integrated with different glue layers in different SoCs. There's > > one driver for the IP core itself, and SoC-specific glue drivers that > > need to provide the IP core drivers with data and callbacks, defined in > > shared headers. Granted, there's also data in those headers that are > > only internal to the IP core drivers, and that should be moved out, but > > for the interface header, include/drm/bridge/ doesn't seem to be a bad > > location to me. > > The thing that irks me on them is that they kinda implement bridges, but > they don't load like bridges. That's the part I think should get changed, > or we need to finally figure out what exactly isn't good with the current > drm_bridge handling and get that fixed (the relevant patches seem forever > stuck in limbo, hence why I'm kicking). dw-hdmi certainly loads like a bridge when used with R-Car DU :-) Are you referring to the component-based probe mode for that driver ? > If that's not possible because these things just dont fit as drm_bridge, > then maybe they shouldn't be a bridge, but something else. But looking at > both dw-hdmi and analogix-dp these things look a lot like midlayers that > get fed huge structures. Instead of more bare-bones toolboxes to build a > set of similar drm_bridge drivers, which drivers then bind into using dt. > > So all a bit fishy imo. > > I guess step 1 at least would be to throw the connector and encoder code > out of all these drivers, that would be at least a first step. Oh yes!! DRM_BRIDGE_ATTACH_NO_CONNECTOR for everybody :-) It's a step-by-step process though: 1. Convert bridge drivers to support both modes (adding support for DRM_BRIDGE_ATTACH_NO_CONNECTOR). 2. Convert display drivers to make use of DRM_BRIDGE_ATTACH_NO_CONNECTOR (with the DRM bridge connector helper, or custom code if really needed). 3. Remove support for the !DRM_BRIDGE_ATTACH_NO_CONNECTOR mode in bridge drivers. 4. Drop the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag itself. Sam and I are working on the first step (I'll convert the dw-hdmi driver soon), and we're passing the message around that new bridge drivers must support DRM_BRIDGE_ATTACH_NO_CONNECTOR and new display controller drivers must use it. > Next one maybe push the per-variant bind code into drm/bridge and out of > drivers, and use more standard of_ functions to find the bridges and tie > them into the drm_device. > > Then 3rd round, some refactoring to demidlayer these libraries and make > them real toolboxes. > > > > Make sure that at least no new ones grow by moving hardware header > > > files into the correct driver directory. > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Alexey Brodkin <abrodkin@synopsys.com> > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > > Cc: Neil Armstrong <narmstrong@baylibre.com> > > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > > Cc: Jonas Karlman <jonas@kwiboo.se> > > > Cc: Jernej Skrabec <jernej.skrabec@siol.net> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Kate Stewart <kstewart@linuxfoundation.org> > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > Cc: Allison Randal <allison@lohutok.net> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com> > > > --- > > > {include => drivers/gpu}/drm/bridge/mhl.h | 0 > > > drivers/gpu/drm/bridge/sii9234.c | 3 ++- > > > drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- > > > 3 files changed, 3 insertions(+), 2 deletions(-) > > > rename {include => drivers/gpu}/drm/bridge/mhl.h (100%) > > > > > > diff --git a/include/drm/bridge/mhl.h b/drivers/gpu/drm/bridge/mhl.h > > > similarity index 100% > > > rename from include/drm/bridge/mhl.h > > > rename to drivers/gpu/drm/bridge/mhl.h > > > diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c > > > index b1258f0ed205..4c862c3af038 100644 > > > --- a/drivers/gpu/drm/bridge/sii9234.c > > > +++ b/drivers/gpu/drm/bridge/sii9234.c > > > @@ -12,7 +12,6 @@ > > > * Shankar Bandal <shankar.b@samsung.com> > > > * Dharam Kumar <dharam.kr@samsung.com> > > > */ > > > -#include <drm/bridge/mhl.h> > > > #include <drm/drm_bridge.h> > > > #include <drm/drm_crtc.h> > > > #include <drm/drm_edid.h> > > > @@ -29,6 +28,8 @@ > > > #include <linux/regulator/consumer.h> > > > #include <linux/slab.h> > > > > > > +#include "mhl.h" > > > + > > > #define CBUS_DEVCAP_OFFSET 0x80 > > > > > > #define SII9234_MHL_VERSION 0x11 > > > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c > > > index 92acd336aa89..017dbb67404e 100644 > > > --- a/drivers/gpu/drm/bridge/sil-sii8620.c > > > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > > > @@ -8,7 +8,6 @@ > > > > > > #include <asm/unaligned.h> > > > > > > -#include <drm/bridge/mhl.h> > > > #include <drm/drm_bridge.h> > > > #include <drm/drm_crtc.h> > > > #include <drm/drm_edid.h> > > > @@ -31,6 +30,7 @@ > > > > > > #include <media/rc-core.h> > > > > > > +#include "mhl.h" > > > #include "sil-sii8620.h" > > > > > > #define SII8620_BURST_BUF_LEN 288 -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bridge: Move mhl.h into driver directory 2020-04-15 18:12 ` Laurent Pinchart @ 2020-04-15 18:19 ` Daniel Vetter 0 siblings, 0 replies; 5+ messages in thread From: Daniel Vetter @ 2020-04-15 18:19 UTC (permalink / raw) To: Laurent Pinchart Cc: Kate Stewart, Jernej Skrabec, Neil Armstrong, Greg Kroah-Hartman, Jonas Karlman, Alexey Brodkin, DRI Development, Gustavo A. R. Silva, Andrzej Hajda, Daniel Vetter, Thomas Gleixner, Sam Ravnborg, Allison Randal On Wed, Apr 15, 2020 at 8:12 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > On Wed, Apr 15, 2020 at 08:06:20PM +0200, Daniel Vetter wrote: > > On Wed, Apr 15, 2020 at 08:48:06PM +0300, Laurent Pinchart wrote: > > > On Wed, Apr 15, 2020 at 07:38:33PM +0200, Daniel Vetter wrote: > > > > include/drm/bridge is a bit a mistake, drivers are supposed to find > > > > their bridges using one of the standard of_* functions the drm_bridge > > > > core provides. > > > > > > I'm confused, I don't really see how that's related to mhl.h. The header > > > defines constants and structures related to the MHL (Mobile > > > High-Definition Link) protocol, which is an industry standard. If you > > > want to move it out of include/drm/bridge/ to eventually remove that > > > directory, I think it should be renamted to include/drm/drm_mhl.h. > > > > It looked misplaced at least ... I guess moving this out of drm/bridge > > makes more sense. > > > > > > dw-hdmi and analogix-dp are the only, historically > > > > grown exception that we haven't managed to get rid of yet. > > > > > > The reason why we have shared headers for those is because they're IP > > > cores integrated with different glue layers in different SoCs. There's > > > one driver for the IP core itself, and SoC-specific glue drivers that > > > need to provide the IP core drivers with data and callbacks, defined in > > > shared headers. Granted, there's also data in those headers that are > > > only internal to the IP core drivers, and that should be moved out, but > > > for the interface header, include/drm/bridge/ doesn't seem to be a bad > > > location to me. > > > > The thing that irks me on them is that they kinda implement bridges, but > > they don't load like bridges. That's the part I think should get changed, > > or we need to finally figure out what exactly isn't good with the current > > drm_bridge handling and get that fixed (the relevant patches seem forever > > stuck in limbo, hence why I'm kicking). > > dw-hdmi certainly loads like a bridge when used with R-Car DU :-) Are > you referring to the component-based probe mode for that driver ? Yeah I guess component.c hand-rolled loading vs. just calling of_drm_find_bridge and then binding that to the encoder. Component.c is meant for driver-specific building blocks, imo for anything that's as standardized as drm_bridge at least aims to be it's totally the wrong thing. The other issue is that imo there's no abstraction between the part that binds something like dw-hdmi on the drm_device side, and the side that implements its on the drm_bridge side. The drm_bridge interface feels very fake in that regard, and that's why I think we should: - move the binding point slightly out, so that the variant-specific binding stuff is behind the abstraction - convert the dw-hdmi library to a helper library, with the variant-specific binding drivers in the driver seat. If you look through the code dw_hdmi_probe is full of switch statements and if ladders and that's all to special case specific variants. That's screaming midlayer mistake :-) > > If that's not possible because these things just dont fit as drm_bridge, > > then maybe they shouldn't be a bridge, but something else. But looking at > > both dw-hdmi and analogix-dp these things look a lot like midlayers that > > get fed huge structures. Instead of more bare-bones toolboxes to build a > > set of similar drm_bridge drivers, which drivers then bind into using dt. > > > > So all a bit fishy imo. > > > > I guess step 1 at least would be to throw the connector and encoder code > > out of all these drivers, that would be at least a first step. > > Oh yes!! DRM_BRIDGE_ATTACH_NO_CONNECTOR for everybody :-) It's a > step-by-step process though: > > 1. Convert bridge drivers to support both modes (adding support for > DRM_BRIDGE_ATTACH_NO_CONNECTOR). > 2. Convert display drivers to make use of DRM_BRIDGE_ATTACH_NO_CONNECTOR > (with the DRM bridge connector helper, or custom code if really > needed). > 3. Remove support for the !DRM_BRIDGE_ATTACH_NO_CONNECTOR mode in bridge > drivers. > 4. Drop the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag itself. > > Sam and I are working on the first step (I'll convert the dw-hdmi driver > soon), and we're passing the message around that new bridge drivers > must support DRM_BRIDGE_ATTACH_NO_CONNECTOR and new display controller > drivers must use it. Yup, I think that's at least one problem I'm seeing here with these. But there's a pile more I think. -Daniel > > > Next one maybe push the per-variant bind code into drm/bridge and out of > > drivers, and use more standard of_ functions to find the bridges and tie > > them into the drm_device. > > > > Then 3rd round, some refactoring to demidlayer these libraries and make > > them real toolboxes. > > > > > > Make sure that at least no new ones grow by moving hardware header > > > > files into the correct driver directory. > > > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > Cc: Alexey Brodkin <abrodkin@synopsys.com> > > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > > > Cc: Neil Armstrong <narmstrong@baylibre.com> > > > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > > > Cc: Jonas Karlman <jonas@kwiboo.se> > > > > Cc: Jernej Skrabec <jernej.skrabec@siol.net> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Cc: Kate Stewart <kstewart@linuxfoundation.org> > > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > > Cc: Allison Randal <allison@lohutok.net> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com> > > > > --- > > > > {include => drivers/gpu}/drm/bridge/mhl.h | 0 > > > > drivers/gpu/drm/bridge/sii9234.c | 3 ++- > > > > drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- > > > > 3 files changed, 3 insertions(+), 2 deletions(-) > > > > rename {include => drivers/gpu}/drm/bridge/mhl.h (100%) > > > > > > > > diff --git a/include/drm/bridge/mhl.h b/drivers/gpu/drm/bridge/mhl.h > > > > similarity index 100% > > > > rename from include/drm/bridge/mhl.h > > > > rename to drivers/gpu/drm/bridge/mhl.h > > > > diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c > > > > index b1258f0ed205..4c862c3af038 100644 > > > > --- a/drivers/gpu/drm/bridge/sii9234.c > > > > +++ b/drivers/gpu/drm/bridge/sii9234.c > > > > @@ -12,7 +12,6 @@ > > > > * Shankar Bandal <shankar.b@samsung.com> > > > > * Dharam Kumar <dharam.kr@samsung.com> > > > > */ > > > > -#include <drm/bridge/mhl.h> > > > > #include <drm/drm_bridge.h> > > > > #include <drm/drm_crtc.h> > > > > #include <drm/drm_edid.h> > > > > @@ -29,6 +28,8 @@ > > > > #include <linux/regulator/consumer.h> > > > > #include <linux/slab.h> > > > > > > > > +#include "mhl.h" > > > > + > > > > #define CBUS_DEVCAP_OFFSET 0x80 > > > > > > > > #define SII9234_MHL_VERSION 0x11 > > > > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c > > > > index 92acd336aa89..017dbb67404e 100644 > > > > --- a/drivers/gpu/drm/bridge/sil-sii8620.c > > > > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > > > > @@ -8,7 +8,6 @@ > > > > > > > > #include <asm/unaligned.h> > > > > > > > > -#include <drm/bridge/mhl.h> > > > > #include <drm/drm_bridge.h> > > > > #include <drm/drm_crtc.h> > > > > #include <drm/drm_edid.h> > > > > @@ -31,6 +30,7 @@ > > > > > > > > #include <media/rc-core.h> > > > > > > > > +#include "mhl.h" > > > > #include "sil-sii8620.h" > > > > > > > > #define SII8620_BURST_BUF_LEN 288 > > -- > Regards, > > Laurent Pinchart -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-04-15 18:19 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-15 17:38 [PATCH] drm/bridge: Move mhl.h into driver directory Daniel Vetter 2020-04-15 17:48 ` Laurent Pinchart 2020-04-15 18:06 ` Daniel Vetter 2020-04-15 18:12 ` Laurent Pinchart 2020-04-15 18:19 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).