Re: [PATCH] drm/bridge: Move mhl.h into driver directory

[Daniel Vetter <daniel@ffwll.ch>]

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