[PATCH v4 0/8] Add tda998x (HDMI) support to atmel-hlcdc

[PATCH v4 0/8] Add tda998x (HDMI) support to atmel-hlcdc

[Peter Rosin]

Hi!

I naively thought that since there was support for both nxp,tda19988 (in
the tda998x driver) and the atmel-hlcdc, things would be a smooth ride.
But it wasn't, so I started looking around and realized I had to fix
things.

In v1 and v2 I fixed things by making the atmel-hlcdc driver a master
component, but now I fix things by making the tda998x driver a bridge
instead. This was after a suggestion from Boris Brezillon the
atmel-hlcdc maintainer), so there was some risk of bias ... but after
comparing what was needed, I too find the bridge approach better.

In addition to the above, our PCB interface between the SAMA5D3 and the
HDMI encoder is only using 16 bits, and this has to be described
somewhere, or the atmel-hlcdc driver have no chance of selecting the
correct output mode. Since I have similar problems with a ds90c185 lvds
encoder I added patches to override the atmel-hlcdc output format via
DT properties compatible with the media video-interface binding and
things start to play together.

Since this series superseeds the bridge series [1], I have included the
leftover bindings patch for the ti,ds90c185 here.

Anyway, this series solves some real issues for my HW.

And as a last bit I remove the master component code from the tilcdc
driver. It should no longer be needed, but I have only compile-tested
the patch and there could easily be some gotcha. I marked it RFC, and
it should get a Tested-by from someone with a tilcdc/tda998x combo
before it is applied.

Cheers,
Peter

Changes since v3   https://lkml.org/lkml/2018/4/19/736
- moved the meat of the drm_of_media_bus_fmt function from patch 3/7
  to a driver-local atmel_hlcdc_of_bus_fmt function, and squashed with
  patch 4/7 (this is now patch 3/8).
- patch 3/8 now parses and stores the DT bus format together with the
  encoder in a reintroduced struct atmel_hlcdc_rgb_output instead of
  allocating a separate encoder-indexed array for the bus formats.
- patch 5/8 is new and splits tda988x_encoder_dpms into a pair of
  functions to enable/disable separately. This fits better with both
  the current code and for the followup drm_bridge patch (7/8).
- adjust patch 6/8 and 7/8 to the above simplification.
- added patch 8/8 that removes component master code from tilcdc.

Changes since v2   https://lkml.org/lkml/2018/4/17/385
- patch 2/7 fixed spelling and added an example
- patch 4/7 parse the DT up front and store the result indexed by encoder
- old patch 5/6 and 6/6 dropped
- patch 5-7/7 are new and makes the tda998x driver a drm_bridge

Changes since v1   https://lkml.org/lkml/2018/4/9/294
- added reviewed-by from Rob to patch 1/6
- patch 2/6 changed so that the bus format override is in the endpoint
  DT node, and follows the binding of media video-interfaces.
- patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above
  media video-interface binding (partially).
- patch 4/6 now makes use of the above helper (and also fixes problems
  with the 3/5 patch from v1 when no override was specified).
- do not mention unrelated connector display_info details in the cover
  letter and commit messages.

[1]
"Bridge" series v2   https://lkml.org/lkml/2018/3/26/610
"Bridge" series v1   https://lkml.org/lkml/2018/3/17/221

Peter Rosin (8):
  dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
  dt-bindings: display: atmel: optional video-interface of endpoints
  drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes
  drm/i2c: tda998x: find the drm_device via the drm_connector
  drm/i2c: tda998x: split tda998x_encoder_dpms into enable/disable
  drm/i2c: tda998x: split encoder and component functions from the work
  drm/i2c: tda998x: register as a drm bridge
  drm/tilcdc: decomponentize now that tda998x is a bridge

 .../devicetree/bindings/display/atmel/hlcdc-dc.txt |  26 ++
 .../bindings/display/bridge/lvds-transmitter.txt   |   8 +-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     |  70 ++++--
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h       |   1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  67 +++++-
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 268 ++++++++++++++++-----
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c               |  11 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                |  67 +-----
 drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   1 -
 drivers/gpu/drm/tilcdc/tilcdc_external.c           |  51 ----
 drivers/gpu/drm/tilcdc/tilcdc_external.h           |   4 +-
 11 files changed, 357 insertions(+), 217 deletions(-)

-- 
2.11.0

[PATCH v4 1/8] dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185

[Peter Rosin]

Start list of actual chips compatible with "lvds-encoder".

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/bridge/lvds-transmitter.txt       | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index fd39ad34c383..50220190c203 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -22,7 +22,13 @@ among others.
 
 Required properties:
 
-- compatible: Must be "lvds-encoder"
+- compatible: Must be one or more of the following
+  - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
+  - "lvds-encoder" for a generic LVDS encoder device
+
+  When compatible with the generic version, nodes must list the
+  device-specific version corresponding to the device first
+  followed by the generic version.
 
 Required nodes:
 
-- 
2.11.0

[PATCH v4 2/8] dt-bindings: display: atmel: optional video-interface of endpoints

[Peter Rosin]

With bus-type/bus-width properties in the endpoint nodes, the video-
interface of the connection can be specified for cases where the
heuristic fails to select the correct output mode. This can happen
e.g. if not all RGB pins are routed on the PCB; the driver has no
way of knowing this, and needs to be told explicitly.

This is critical for the devices that have the "conflicting output
formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
RGB bits move around depending on the selected output mode. For
devices that do not have the "conflicting output formats" issue
(SAMA5D2, SAMA5D4), this is completely irrelevant.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
index 82f2acb3d374..9de434a8f523 100644
--- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
+++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
@@ -15,6 +15,14 @@ Required children nodes:
  to external devices using the OF graph reprensentation (see ../graph.txt).
  At least one port node is required.
 
+Optional properties in grandchild nodes:
+ Any endpoint grandchild node may specify a desired video interface
+ according to ../../media/video-interfaces.txt, specifically
+ - bus-type: must be <0>.
+ - bus-width: recognized values are <12>, <16>, <18> and <24>, and
+   override any output mode selection heuristic, forcing "rgb444",
+   "rgb565", "rgb666" and "rgb888" respectively.
+
 Example:
 
 	hlcdc: hlcdc@f0030000 {
@@ -50,3 +58,21 @@ Example:
 			#pwm-cells = <3>;
 		};
 	};
+
+
+Example 2: With a video interface override to force rgb565; as above
+but with these changes/additions:
+
+	&hlcdc {
+		hlcdc-display-controller {
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
+
+			port@0 {
+				hlcdc_panel_output: endpoint@0 {
+					bus-type = <0>;
+					bus-width = <16>;
+				};
+			};
+		};
+	};
-- 
2.11.0

Re: [PATCH v4 2/8] dt-bindings: display: atmel: optional video-interface of endpoints

[Boris Brezillon]

On Mon, 23 Apr 2018 09:22:55 +0200
Peter Rosin <peda@axentia.se> wrote:

> With bus-type/bus-width properties in the endpoint nodes, the video-
> interface of the connection can be specified for cases where the
> heuristic fails to select the correct output mode. This can happen
> e.g. if not all RGB pins are routed on the PCB; the driver has no
> way of knowing this, and needs to be told explicitly.
> 
> This is critical for the devices that have the "conflicting output
> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> RGB bits move around depending on the selected output mode. For
> devices that do not have the "conflicting output formats" issue
> (SAMA5D2, SAMA5D4), this is completely irrelevant.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> index 82f2acb3d374..9de434a8f523 100644
> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> @@ -15,6 +15,14 @@ Required children nodes:
>   to external devices using the OF graph reprensentation (see ../graph.txt).
>   At least one port node is required.
>  
> +Optional properties in grandchild nodes:
> + Any endpoint grandchild node may specify a desired video interface
> + according to ../../media/video-interfaces.txt, specifically
> + - bus-type: must be <0>.
> + - bus-width: recognized values are <12>, <16>, <18> and <24>, and
> +   override any output mode selection heuristic, forcing "rgb444",
> +   "rgb565", "rgb666" and "rgb888" respectively.
> +
>  Example:
>  
>  	hlcdc: hlcdc@f0030000 {
> @@ -50,3 +58,21 @@ Example:
>  			#pwm-cells = <3>;
>  		};
>  	};
> +
> +
> +Example 2: With a video interface override to force rgb565; as above
> +but with these changes/additions:
> +
> +	&hlcdc {
> +		hlcdc-display-controller {
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
> +
> +			port@0 {
> +				hlcdc_panel_output: endpoint@0 {
> +					bus-type = <0>;
> +					bus-width = <16>;
> +				};
> +			};
> +		};
> +	};

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/8] dt-bindings: display: atmel: optional video-interface of endpoints

[Rob Herring]

On Mon, Apr 23, 2018 at 09:22:55AM +0200, Peter Rosin wrote:
> With bus-type/bus-width properties in the endpoint nodes, the video-
> interface of the connection can be specified for cases where the
> heuristic fails to select the correct output mode. This can happen
> e.g. if not all RGB pins are routed on the PCB; the driver has no
> way of knowing this, and needs to be told explicitly.
> 
> This is critical for the devices that have the "conflicting output
> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> RGB bits move around depending on the selected output mode. For
> devices that do not have the "conflicting output formats" issue
> (SAMA5D2, SAMA5D4), this is completely irrelevant.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v4 3/8] drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes

[Peter Rosin]

This beats the heuristic that the connector is involved in what format
should be output for cases where this fails.

E.g. if there is a bridge that changes format between the encoder and the
connector, or if some of the RGB pins between the lcd controller and the
encoder are not routed on the PCB.

This is critical for the devices that have the "conflicting output
formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
RGB bits move around depending on the selected output mode. For
devices that do not have the "conflicting output formats" issue
(SAMA5D2, SAMA5D4), this is completely irrelevant.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c   | 70 +++++++++++++++++-------
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h     |  1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 67 ++++++++++++++++++++---
 3 files changed, 111 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index d73281095fac..c38a479ada98 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -226,6 +226,55 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc *c,
 #define ATMEL_HLCDC_RGB888_OUTPUT	BIT(3)
 #define ATMEL_HLCDC_OUTPUT_MODE_MASK	GENMASK(3, 0)
 
+static int atmel_hlcdc_connector_output_mode(struct drm_connector_state *state)
+{
+	struct drm_connector *connector = state->connector;
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_encoder *encoder;
+	unsigned int supported_fmts = 0;
+	int j;
+
+	encoder = state->best_encoder;
+	if (!encoder)
+		encoder = connector->encoder;
+
+	switch (atmel_hlcdc_encoder_get_bus_fmt(encoder)) {
+	case 0:
+		break;
+	case MEDIA_BUS_FMT_RGB444_1X12:
+		return ATMEL_HLCDC_RGB444_OUTPUT;
+	case MEDIA_BUS_FMT_RGB565_1X16:
+		return ATMEL_HLCDC_RGB565_OUTPUT;
+	case MEDIA_BUS_FMT_RGB666_1X18:
+		return ATMEL_HLCDC_RGB666_OUTPUT;
+	case MEDIA_BUS_FMT_RGB888_1X24:
+		return ATMEL_HLCDC_RGB888_OUTPUT;
+	default:
+		return -EINVAL;
+	}
+
+	for (j = 0; j < info->num_bus_formats; j++) {
+		switch (info->bus_formats[j]) {
+		case MEDIA_BUS_FMT_RGB444_1X12:
+			supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
+			break;
+		case MEDIA_BUS_FMT_RGB565_1X16:
+			supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
+			break;
+		case MEDIA_BUS_FMT_RGB666_1X18:
+			supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
+			break;
+		case MEDIA_BUS_FMT_RGB888_1X24:
+			supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
+			break;
+		default:
+			break;
+		}
+	}
+
+	return supported_fmts;
+}
+
 static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
 {
 	unsigned int output_fmts = ATMEL_HLCDC_OUTPUT_MODE_MASK;
@@ -238,31 +287,12 @@ static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
 	crtc = drm_crtc_to_atmel_hlcdc_crtc(state->crtc);
 
 	for_each_new_connector_in_state(state->state, connector, cstate, i) {
-		struct drm_display_info *info = &connector->display_info;
 		unsigned int supported_fmts = 0;
-		int j;
 
 		if (!cstate->crtc)
 			continue;
 
-		for (j = 0; j < info->num_bus_formats; j++) {
-			switch (info->bus_formats[j]) {
-			case MEDIA_BUS_FMT_RGB444_1X12:
-				supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
-				break;
-			case MEDIA_BUS_FMT_RGB565_1X16:
-				supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
-				break;
-			case MEDIA_BUS_FMT_RGB666_1X18:
-				supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
-				break;
-			case MEDIA_BUS_FMT_RGB888_1X24:
-				supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
-				break;
-			default:
-				break;
-			}
-		}
+		supported_fmts = atmel_hlcdc_connector_output_mode(cstate);
 
 		if (crtc->dc->desc->conflicting_output_formats)
 			output_fmts &= supported_fmts;
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
index ab32d5b268d2..77bd2d0ae508 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
@@ -454,5 +454,6 @@ void atmel_hlcdc_crtc_irq(struct drm_crtc *c);
 int atmel_hlcdc_crtc_create(struct drm_device *dev);
 
 int atmel_hlcdc_create_outputs(struct drm_device *dev);
+int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder);
 
 #endif /* DRM_ATMEL_HLCDC_H */
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
index 8db51fb131db..db4d6aaaef93 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
@@ -27,33 +27,86 @@
 
 #include "atmel_hlcdc_dc.h"
 
+struct atmel_hlcdc_rgb_output {
+	struct drm_encoder encoder;
+	int bus_fmt;
+};
+
 static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
+static struct atmel_hlcdc_rgb_output *
+atmel_hlcdc_encoder_to_rgb_output(struct drm_encoder *encoder)
+{
+	return container_of(encoder, struct atmel_hlcdc_rgb_output, encoder);
+}
+
+int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder)
+{
+	struct atmel_hlcdc_rgb_output *output;
+
+	output = atmel_hlcdc_encoder_to_rgb_output(encoder);
+
+	return output->bus_fmt;
+}
+
+static int atmel_hlcdc_of_bus_fmt(struct device_node *ep)
+{
+	u32 bus_width = 0;
+
+	of_property_read_u32(ep, "bus-width", &bus_width);
+
+	switch (bus_width) {
+	case 0:
+		return 0;
+	case 12:
+		return MEDIA_BUS_FMT_RGB444_1X12;
+	case 16:
+		return MEDIA_BUS_FMT_RGB565_1X16;
+	case 18:
+		return MEDIA_BUS_FMT_RGB666_1X18;
+	case 24:
+		return MEDIA_BUS_FMT_RGB888_1X24;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
 {
-	struct drm_encoder *encoder;
+	struct atmel_hlcdc_rgb_output *output;
+	struct device_node *ep;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 	int ret;
 
+	ep = of_graph_get_endpoint_by_regs(dev->dev->of_node, 0, endpoint);
+	if (!ep)
+		return -ENODEV;
+
 	ret = drm_of_find_panel_or_bridge(dev->dev->of_node, 0, endpoint,
 					  &panel, &bridge);
 	if (ret)
 		return ret;
 
-	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
-	if (!encoder)
+	output = devm_kzalloc(dev->dev, sizeof(*output), GFP_KERNEL);
+	if (!output)
 		return -EINVAL;
 
-	ret = drm_encoder_init(dev, encoder,
+	output->bus_fmt = atmel_hlcdc_of_bus_fmt(ep);
+	if (output->bus_fmt < 0) {
+		dev_err(dev->dev, "invalid bus width\n");
+		return -EINVAL;
+	}
+
+	ret = drm_encoder_init(dev, &output->encoder,
 			       &atmel_hlcdc_panel_encoder_funcs,
 			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret)
 		return ret;
 
-	encoder->possible_crtcs = 0x1;
+	output->encoder.possible_crtcs = 0x1;
 
 	if (panel) {
 		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
@@ -62,7 +115,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
 	}
 
 	if (bridge) {
-		ret = drm_bridge_attach(encoder, bridge, NULL);
+		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
 		if (!ret)
 			return 0;
 
@@ -70,7 +123,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
 			drm_panel_bridge_remove(bridge);
 	}
 
-	drm_encoder_cleanup(encoder);
+	drm_encoder_cleanup(&output->encoder);
 
 	return ret;
 }
-- 
2.11.0

Re: [PATCH v4 3/8] drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes

[Boris Brezillon]

On Mon, 23 Apr 2018 09:22:56 +0200
Peter Rosin <peda@axentia.se> wrote:

> This beats the heuristic that the connector is involved in what format
> should be output for cases where this fails.
> 
> E.g. if there is a bridge that changes format between the encoder and the
> connector, or if some of the RGB pins between the lcd controller and the
> encoder are not routed on the PCB.
> 
> This is critical for the devices that have the "conflicting output
> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> RGB bits move around depending on the selected output mode. For
> devices that do not have the "conflicting output formats" issue
> (SAMA5D2, SAMA5D4), this is completely irrelevant.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c   | 70 +++++++++++++++++-------
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h     |  1 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 67 ++++++++++++++++++++---
>  3 files changed, 111 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index d73281095fac..c38a479ada98 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -226,6 +226,55 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc *c,
>  #define ATMEL_HLCDC_RGB888_OUTPUT	BIT(3)
>  #define ATMEL_HLCDC_OUTPUT_MODE_MASK	GENMASK(3, 0)
>  
> +static int atmel_hlcdc_connector_output_mode(struct drm_connector_state *state)
> +{
> +	struct drm_connector *connector = state->connector;
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_encoder *encoder;
> +	unsigned int supported_fmts = 0;
> +	int j;
> +
> +	encoder = state->best_encoder;
> +	if (!encoder)
> +		encoder = connector->encoder;
> +
> +	switch (atmel_hlcdc_encoder_get_bus_fmt(encoder)) {
> +	case 0:
> +		break;
> +	case MEDIA_BUS_FMT_RGB444_1X12:
> +		return ATMEL_HLCDC_RGB444_OUTPUT;
> +	case MEDIA_BUS_FMT_RGB565_1X16:
> +		return ATMEL_HLCDC_RGB565_OUTPUT;
> +	case MEDIA_BUS_FMT_RGB666_1X18:
> +		return ATMEL_HLCDC_RGB666_OUTPUT;
> +	case MEDIA_BUS_FMT_RGB888_1X24:
> +		return ATMEL_HLCDC_RGB888_OUTPUT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	for (j = 0; j < info->num_bus_formats; j++) {
> +		switch (info->bus_formats[j]) {
> +		case MEDIA_BUS_FMT_RGB444_1X12:
> +			supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
> +			break;
> +		case MEDIA_BUS_FMT_RGB565_1X16:
> +			supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
> +			break;
> +		case MEDIA_BUS_FMT_RGB666_1X18:
> +			supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
> +			break;
> +		case MEDIA_BUS_FMT_RGB888_1X24:
> +			supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return supported_fmts;
> +}
> +
>  static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
>  {
>  	unsigned int output_fmts = ATMEL_HLCDC_OUTPUT_MODE_MASK;
> @@ -238,31 +287,12 @@ static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
>  	crtc = drm_crtc_to_atmel_hlcdc_crtc(state->crtc);
>  
>  	for_each_new_connector_in_state(state->state, connector, cstate, i) {
> -		struct drm_display_info *info = &connector->display_info;
>  		unsigned int supported_fmts = 0;
> -		int j;
>  
>  		if (!cstate->crtc)
>  			continue;
>  
> -		for (j = 0; j < info->num_bus_formats; j++) {
> -			switch (info->bus_formats[j]) {
> -			case MEDIA_BUS_FMT_RGB444_1X12:
> -				supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
> -				break;
> -			case MEDIA_BUS_FMT_RGB565_1X16:
> -				supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
> -				break;
> -			case MEDIA_BUS_FMT_RGB666_1X18:
> -				supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
> -				break;
> -			case MEDIA_BUS_FMT_RGB888_1X24:
> -				supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
> -				break;
> -			default:
> -				break;
> -			}
> -		}
> +		supported_fmts = atmel_hlcdc_connector_output_mode(cstate);
>  
>  		if (crtc->dc->desc->conflicting_output_formats)
>  			output_fmts &= supported_fmts;
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> index ab32d5b268d2..77bd2d0ae508 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> @@ -454,5 +454,6 @@ void atmel_hlcdc_crtc_irq(struct drm_crtc *c);
>  int atmel_hlcdc_crtc_create(struct drm_device *dev);
>  
>  int atmel_hlcdc_create_outputs(struct drm_device *dev);
> +int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder);
>  
>  #endif /* DRM_ATMEL_HLCDC_H */
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> index 8db51fb131db..db4d6aaaef93 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -27,33 +27,86 @@
>  
>  #include "atmel_hlcdc_dc.h"
>  
> +struct atmel_hlcdc_rgb_output {
> +	struct drm_encoder encoder;
> +	int bus_fmt;
> +};
> +
>  static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
>  	.destroy = drm_encoder_cleanup,
>  };
>  
> +static struct atmel_hlcdc_rgb_output *
> +atmel_hlcdc_encoder_to_rgb_output(struct drm_encoder *encoder)
> +{
> +	return container_of(encoder, struct atmel_hlcdc_rgb_output, encoder);
> +}
> +
> +int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder)
> +{
> +	struct atmel_hlcdc_rgb_output *output;
> +
> +	output = atmel_hlcdc_encoder_to_rgb_output(encoder);
> +
> +	return output->bus_fmt;
> +}
> +
> +static int atmel_hlcdc_of_bus_fmt(struct device_node *ep)
> +{
> +	u32 bus_width = 0;
> +
> +	of_property_read_u32(ep, "bus-width", &bus_width);
> +
> +	switch (bus_width) {
> +	case 0:
> +		return 0;
> +	case 12:
> +		return MEDIA_BUS_FMT_RGB444_1X12;
> +	case 16:
> +		return MEDIA_BUS_FMT_RGB565_1X16;
> +	case 18:
> +		return MEDIA_BUS_FMT_RGB666_1X18;
> +	case 24:
> +		return MEDIA_BUS_FMT_RGB888_1X24;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
>  {
> -	struct drm_encoder *encoder;
> +	struct atmel_hlcdc_rgb_output *output;
> +	struct device_node *ep;
>  	struct drm_panel *panel;
>  	struct drm_bridge *bridge;
>  	int ret;
>  
> +	ep = of_graph_get_endpoint_by_regs(dev->dev->of_node, 0, endpoint);
> +	if (!ep)
> +		return -ENODEV;
> +
>  	ret = drm_of_find_panel_or_bridge(dev->dev->of_node, 0, endpoint,
>  					  &panel, &bridge);
>  	if (ret)
>  		return ret;
>  
> -	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
> -	if (!encoder)
> +	output = devm_kzalloc(dev->dev, sizeof(*output), GFP_KERNEL);
> +	if (!output)
>  		return -EINVAL;
>  
> -	ret = drm_encoder_init(dev, encoder,
> +	output->bus_fmt = atmel_hlcdc_of_bus_fmt(ep);
> +	if (output->bus_fmt < 0) {
> +		dev_err(dev->dev, "invalid bus width\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = drm_encoder_init(dev, &output->encoder,
>  			       &atmel_hlcdc_panel_encoder_funcs,
>  			       DRM_MODE_ENCODER_NONE, NULL);
>  	if (ret)
>  		return ret;
>  
> -	encoder->possible_crtcs = 0x1;
> +	output->encoder.possible_crtcs = 0x1;
>  
>  	if (panel) {
>  		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
> @@ -62,7 +115,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
>  	}
>  
>  	if (bridge) {
> -		ret = drm_bridge_attach(encoder, bridge, NULL);
> +		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
>  		if (!ret)
>  			return 0;
>  
> @@ -70,7 +123,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
>  			drm_panel_bridge_remove(bridge);
>  	}
>  
> -	drm_encoder_cleanup(encoder);
> +	drm_encoder_cleanup(&output->encoder);
>  
>  	return ret;
>  }

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 4/8] drm/i2c: tda998x: find the drm_device via the drm_connector

[Peter Rosin]

This prepares for being a drm_bridge which will not register the
encoder. That makes the connector the better choice.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index cd3f0873bbdd..8f6e013f2b87 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -630,7 +630,7 @@ static void tda998x_detect_work(struct work_struct *work)
 {
 	struct tda998x_priv *priv =
 		container_of(work, struct tda998x_priv, detect_work);
-	struct drm_device *dev = priv->encoder.dev;
+	struct drm_device *dev = priv->connector.dev;
 
 	if (dev)
 		drm_kms_helper_hotplug_event(dev);
-- 
2.11.0

[PATCH v4 5/8] drm/i2c: tda998x: split tda998x_encoder_dpms into enable/disable

[Peter Rosin]

This fits better with the drm_bridge callbacks for when this
driver becomes a drm_bridge.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 64 ++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 8f6e013f2b87..f9028d567ec2 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1163,36 +1163,42 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 
 /* DRM encoder functions */
 
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+static void tda998x_enable(struct tda998x_priv *priv)
 {
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
-	bool on;
+	if (priv->is_on)
+		return;
 
-	/* we only care about on or off: */
-	on = mode == DRM_MODE_DPMS_ON;
+	/* enable video ports, audio will be enabled later */
+	reg_write(priv, REG_ENA_VP_0, 0xff);
+	reg_write(priv, REG_ENA_VP_1, 0xff);
+	reg_write(priv, REG_ENA_VP_2, 0xff);
+	/* set muxing after enabling ports: */
+	reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
+	reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
+	reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
 
-	if (on == priv->is_on)
-		return;
+	priv->is_on = true;
+}
 
-	if (on) {
-		/* enable video ports, audio will be enabled later */
-		reg_write(priv, REG_ENA_VP_0, 0xff);
-		reg_write(priv, REG_ENA_VP_1, 0xff);
-		reg_write(priv, REG_ENA_VP_2, 0xff);
-		/* set muxing after enabling ports: */
-		reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
-		reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
-		reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
-
-		priv->is_on = true;
-	} else {
-		/* disable video ports */
-		reg_write(priv, REG_ENA_VP_0, 0x00);
-		reg_write(priv, REG_ENA_VP_1, 0x00);
-		reg_write(priv, REG_ENA_VP_2, 0x00);
+static void tda998x_disable(struct tda998x_priv *priv)
+{
+	/* disable video ports */
+	reg_write(priv, REG_ENA_VP_0, 0x00);
+	reg_write(priv, REG_ENA_VP_1, 0x00);
+	reg_write(priv, REG_ENA_VP_2, 0x00);
 
-		priv->is_on = false;
-	}
+	priv->is_on = false;
+}
+
+static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+{
+	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+
+	/* we only care about on or off: */
+	if (mode == DRM_MODE_DPMS_ON)
+		tda998x_enable(priv);
+	else
+		tda998x_disable(priv);
 }
 
 static void
@@ -1607,12 +1613,16 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 
 static void tda998x_encoder_prepare(struct drm_encoder *encoder)
 {
-	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
+	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+
+	tda998x_disable(priv);
 }
 
 static void tda998x_encoder_commit(struct drm_encoder *encoder)
 {
-	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
+	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+
+	tda998x_enable(priv);
 }
 
 static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {
-- 
2.11.0

[PATCH v4 6/8] drm/i2c: tda998x: split encoder and component functions from the work

[Peter Rosin]

This enables reuse of the machinery for the case where a drm_bridge
needs to do the same work via different interfaces.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index f9028d567ec2..53fd0be076b4 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1202,11 +1202,10 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 }
 
 static void
-tda998x_encoder_mode_set(struct drm_encoder *encoder,
-			 struct drm_display_mode *mode,
-			 struct drm_display_mode *adjusted_mode)
+tda998x_mode_set(struct tda998x_priv *priv,
+		 struct drm_display_mode *mode,
+		 struct drm_display_mode *adjusted_mode)
 {
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
 	u16 ref_pix, ref_line, n_pix, n_line;
 	u16 hs_pix_s, hs_pix_e;
 	u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1413,6 +1412,16 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	mutex_unlock(&priv->audio_mutex);
 }
 
+static void
+tda998x_encoder_mode_set(struct drm_encoder *encoder,
+			 struct drm_display_mode *mode,
+			 struct drm_display_mode *adjusted_mode)
+{
+	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+
+	tda998x_mode_set(priv, mode, adjusted_mode);
+}
+
 static void tda998x_destroy(struct tda998x_priv *priv)
 {
 	/* disable all IRQs and free the IRQ handler */
@@ -1663,11 +1672,10 @@ static void tda998x_set_config(struct tda998x_priv *priv,
 	priv->audio_params = p->audio_params;
 }
 
-static int tda998x_bind(struct device *dev, struct device *master, void *data)
+static int tda998x_init(struct device *dev, struct drm_device *drm)
 {
 	struct tda998x_encoder_params *params = dev->platform_data;
 	struct i2c_client *client = to_i2c_client(dev);
-	struct drm_device *drm = data;
 	struct tda998x_priv *priv;
 	u32 crtcs = 0;
 	int ret;
@@ -1715,8 +1723,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	return ret;
 }
 
-static void tda998x_unbind(struct device *dev, struct device *master,
-			   void *data)
+static void tda998x_fini(struct device *dev)
 {
 	struct tda998x_priv *priv = dev_get_drvdata(dev);
 
@@ -1725,6 +1732,19 @@ static void tda998x_unbind(struct device *dev, struct device *master,
 	tda998x_destroy(priv);
 }
 
+static int tda998x_bind(struct device *dev, struct device *master, void *data)
+{
+	struct drm_device *drm = data;
+
+	return tda998x_init(dev, drm);
+}
+
+static void tda998x_unbind(struct device *dev, struct device *master,
+			   void *data)
+{
+	tda998x_fini(dev);
+}
+
 static const struct component_ops tda998x_ops = {
 	.bind = tda998x_bind,
 	.unbind = tda998x_unbind,
-- 
2.11.0

[PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

This makes this driver work with all(?) drivers that are not
componentized and instead expect to connect to a panel/bridge. That
said, the only one tested is atmel-hlcdc.

This hooks the relevant work function previously called by the encoder
and the component also to the bridge, since the encoder goes away when
connecting to the bridge interface of the driver and the equivalent of
bind/unbind of the component is handled by bridge attach/detach.

The lifetime requirements of a bridge and a component are slightly
different, which is the reason for struct tda998x_bridge.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 170 ++++++++++++++++++++++++++++++++------
 1 file changed, 143 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 53fd0be076b4..b8cb6237a38b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -36,6 +36,14 @@ struct tda998x_audio_port {
 	u8 config;		/* AP value */
 };
 
+struct tda998x_priv;
+
+struct tda998x_bridge {
+	struct tda998x_priv *priv;
+	struct device *dev;
+	struct drm_bridge bridge;
+};
+
 struct tda998x_priv {
 	struct i2c_client *cec;
 	struct i2c_client *hdmi;
@@ -63,6 +71,8 @@ struct tda998x_priv {
 	wait_queue_head_t edid_delay_waitq;
 	bool edid_delay_active;
 
+	struct tda998x_bridge *bridge;
+	bool local_encoder;
 	struct drm_encoder encoder;
 	struct drm_connector connector;
 
@@ -75,6 +85,9 @@ struct tda998x_priv {
 #define enc_to_tda998x_priv(x) \
 	container_of(x, struct tda998x_priv, encoder)
 
+#define bridge_to_tda998x_bridge(x) \
+	container_of(x, struct tda998x_bridge, bridge)
+
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
  * write a given register, we need to make sure CURPAGE register is set
@@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 				   struct hdmi_codec_daifmt *daifmt,
 				   struct hdmi_codec_params *params)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 	int i, ret;
 	struct tda998x_audio_params audio = {
 		.sample_width = params->sample_width,
@@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 
 static void tda998x_audio_shutdown(struct device *dev, void *data)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 
 	mutex_lock(&priv->audio_mutex);
 
@@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev, void *data)
 
 int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 
 	mutex_lock(&priv->audio_mutex);
 
@@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
 static int tda998x_audio_get_eld(struct device *dev, void *data,
 				 uint8_t *buf, size_t len)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 
 	mutex_lock(&priv->audio_mutex);
 	memcpy(buf, priv->connector.eld,
@@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector *connector)
 {
 	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
 
-	return &priv->encoder;
+	if (priv->local_encoder)
+		return &priv->encoder;
+	else
+		return priv->bridge->bridge.encoder;
 }
 
 static
@@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 				  struct drm_device *drm)
 {
 	struct drm_connector *connector = &priv->connector;
+	struct drm_encoder *encoder;
 	int ret;
 
 	connector->interlace_allowed = 1;
@@ -1156,7 +1177,8 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 	if (ret)
 		return ret;
 
-	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
+	encoder = tda998x_connector_best_encoder(&priv->connector);
+	drm_mode_connector_attach_encoder(&priv->connector, encoder);
 
 	return 0;
 }
@@ -1672,8 +1694,10 @@ static void tda998x_set_config(struct tda998x_priv *priv,
 	priv->audio_params = p->audio_params;
 }
 
-static int tda998x_init(struct device *dev, struct drm_device *drm)
+static int tda998x_init(struct device *dev, struct drm_device *drm,
+			bool local_encoder)
 {
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
 	struct tda998x_encoder_params *params = dev->platform_data;
 	struct i2c_client *client = to_i2c_client(dev);
 	struct tda998x_priv *priv;
@@ -1684,18 +1708,22 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
 	if (!priv)
 		return -ENOMEM;
 
-	dev_set_drvdata(dev, priv);
+	bridge->priv = priv;
+	priv->bridge = bridge;
+	priv->local_encoder = local_encoder;
 
-	if (dev->of_node)
-		crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
+	if (local_encoder) {
+		if (dev->of_node)
+			crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
 
-	/* If no CRTCs were found, fall back to our old behaviour */
-	if (crtcs == 0) {
-		dev_warn(dev, "Falling back to first CRTC\n");
-		crtcs = 1 << 0;
-	}
+		/* If no CRTCs were found, fall back to our old behaviour */
+		if (crtcs == 0) {
+			dev_warn(dev, "Falling back to first CRTC\n");
+			crtcs = 1 << 0;
+		}
 
-	priv->encoder.possible_crtcs = crtcs;
+		priv->encoder.possible_crtcs = crtcs;
+	}
 
 	ret = tda998x_create(client, priv);
 	if (ret)
@@ -1704,11 +1732,15 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
 	if (!dev->of_node && params)
 		tda998x_set_config(priv, params);
 
-	drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs);
-	ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs,
-			       DRM_MODE_ENCODER_TMDS, NULL);
-	if (ret)
-		goto err_encoder;
+	if (local_encoder) {
+		drm_encoder_helper_add(&priv->encoder,
+				       &tda998x_encoder_helper_funcs);
+		ret = drm_encoder_init(drm, &priv->encoder,
+				       &tda998x_encoder_funcs,
+				       DRM_MODE_ENCODER_TMDS, NULL);
+		if (ret)
+			goto err_encoder;
+	}
 
 	ret = tda998x_connector_init(priv, drm);
 	if (ret)
@@ -1717,7 +1749,8 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
 	return 0;
 
 err_connector:
-	drm_encoder_cleanup(&priv->encoder);
+	if (local_encoder)
+		drm_encoder_cleanup(&priv->encoder);
 err_encoder:
 	tda998x_destroy(priv);
 	return ret;
@@ -1725,10 +1758,12 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
 
 static void tda998x_fini(struct device *dev)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 
 	drm_connector_cleanup(&priv->connector);
-	drm_encoder_cleanup(&priv->encoder);
+	if (priv->local_encoder)
+		drm_encoder_cleanup(&priv->encoder);
 	tda998x_destroy(priv);
 }
 
@@ -1736,7 +1771,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
 
-	return tda998x_init(dev, drm);
+	return tda998x_init(dev, drm, true);
 }
 
 static void tda998x_unbind(struct device *dev, struct device *master,
@@ -1750,19 +1785,100 @@ static const struct component_ops tda998x_ops = {
 	.unbind = tda998x_unbind,
 };
 
+/* DRM bridge functions */
+
+static int tda998x_bridge_attach(struct drm_bridge *dbridge)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct device *dev = bridge->dev;
+	struct drm_device *drm = bridge->bridge.dev;
+
+	return tda998x_init(dev, drm, false);
+}
+
+static void tda998x_bridge_detach(struct drm_bridge *dbridge)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct device *dev = bridge->dev;
+
+	tda998x_fini(dev);
+}
+
+static void tda998x_bridge_enable(struct drm_bridge *dbridge)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct tda998x_priv *priv = bridge->priv;
+
+	tda998x_enable(priv);
+}
+
+static void tda998x_bridge_disable(struct drm_bridge *dbridge)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct tda998x_priv *priv = bridge->priv;
+
+	tda998x_disable(priv);
+}
+
+static void tda998x_bridge_mode_set(struct drm_bridge *dbridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adjusted_mode)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct tda998x_priv *priv = bridge->priv;
+
+	tda998x_mode_set(priv, mode, adjusted_mode);
+}
+
+static const struct drm_bridge_funcs tda998x_bridge_funcs = {
+	.attach = tda998x_bridge_attach,
+	.detach = tda998x_bridge_detach,
+	.enable = tda998x_bridge_enable,
+	.disable = tda998x_bridge_disable,
+	.mode_set = tda998x_bridge_mode_set,
+};
+
 static int
 tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
+	struct device *dev = &client->dev;
+	struct tda998x_bridge *bridge;
+	int ret;
+
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		dev_warn(&client->dev, "adapter does not support I2C\n");
 		return -EIO;
 	}
-	return component_add(&client->dev, &tda998x_ops);
+
+	bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
+	if (!bridge)
+		return -ENOMEM;
+
+	bridge->dev = dev;
+	dev_set_drvdata(dev, bridge);
+
+	bridge->bridge.funcs = &tda998x_bridge_funcs;
+#ifdef CONFIG_OF
+	bridge->bridge.of_node = dev->of_node;
+#endif
+	drm_bridge_add(&bridge->bridge);
+
+	ret = component_add(dev, &tda998x_ops);
+
+	if (ret)
+		drm_bridge_remove(&bridge->bridge);
+
+	return ret;
 }
 
 static int tda998x_remove(struct i2c_client *client)
 {
-	component_del(&client->dev, &tda998x_ops);
+	struct device *dev = &client->dev;
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+
+	drm_bridge_remove(&bridge->bridge);
+	component_del(dev, &tda998x_ops);
+
 	return 0;
 }
 
-- 
2.11.0

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>  static int tda998x_remove(struct i2c_client *client)
>  {
> -	component_del(&client->dev, &tda998x_ops);
> +	struct device *dev = &client->dev;
> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> +	drm_bridge_remove(&bridge->bridge);
> +	component_del(dev, &tda998x_ops);
> +

I'd like to ask a rather fundamental question about DRM bridge support,
because I suspect that there's a major fsckup here.

The above is the function that deals with the TDA998x device being
unbound from the driver.  With the component API, this results in the
DRM device correctly being torn down, because one of the hardware
devices has gone.

With DRM bridge, the bridge is merely removed from the list of
bridges:

void drm_bridge_remove(struct drm_bridge *bridge)
{
        mutex_lock(&bridge_lock);
        list_del_init(&bridge->list);
        mutex_unlock(&bridge_lock);
}
EXPORT_SYMBOL(drm_bridge_remove);

and the memory backing the "struct tda998x_bridge" (which contains
the struct drm_bridge) will be freed by the devm subsystem.

However, there is no notification into the rest of the DRM subsystem
that the device has gone away.  Worse, the memory that is still in
use by DRM has now been freed, so further use of the DRM device
results in a use-after-free bug.

This is really not good, and to me looks like a fundamental problem
with the DRM bridge code.  I see nothing in the DRM bridge code that
deals with the lifetime of a "DRM bridge" or indeed the lifetime of
the actual device itself.

So, from what I can see, there seems to be a fundamental lifetime
issue with the design of the DRM bridge code.  This needs to be
fixed.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>  static int tda998x_remove(struct i2c_client *client)
>>  {
>> -	component_del(&client->dev, &tda998x_ops);
>> +	struct device *dev = &client->dev;
>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>> +
>> +	drm_bridge_remove(&bridge->bridge);
>> +	component_del(dev, &tda998x_ops);
>> +
> 
> I'd like to ask a rather fundamental question about DRM bridge support,
> because I suspect that there's a major fsckup here.
> 
> The above is the function that deals with the TDA998x device being
> unbound from the driver.  With the component API, this results in the
> DRM device correctly being torn down, because one of the hardware
> devices has gone.
> 
> With DRM bridge, the bridge is merely removed from the list of
> bridges:
> 
> void drm_bridge_remove(struct drm_bridge *bridge)
> {
>         mutex_lock(&bridge_lock);
>         list_del_init(&bridge->list);
>         mutex_unlock(&bridge_lock);
> }
> EXPORT_SYMBOL(drm_bridge_remove);
> 
> and the memory backing the "struct tda998x_bridge" (which contains
> the struct drm_bridge) will be freed by the devm subsystem.
> 
> However, there is no notification into the rest of the DRM subsystem
> that the device has gone away.  Worse, the memory that is still in
> use by DRM has now been freed, so further use of the DRM device
> results in a use-after-free bug.
> 
> This is really not good, and to me looks like a fundamental problem
> with the DRM bridge code.  I see nothing in the DRM bridge code that
> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> the actual device itself.
> 
> So, from what I can see, there seems to be a fundamental lifetime
> issue with the design of the DRM bridge code.  This needs to be
> fixed.

Oh crap. A gigantic can of worms...

Would a patch (completely untested btw) along this line of thinking make
any difference whatsoever?

Yeah, I know, all other drm_bridges also need to fill in .owner, and
the .of_node member could probably be ditched from struct drm_device
etc, but this was just a quick sketch...

Cheers,
Peter

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..3577e50cc6c0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -138,6 +138,10 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	else
 		encoder->bridge = bridge;
 
+	if (!device_link_add(encoder->dev->dev, bridge->owner,
+			     DL_FLAG_AUTOREMOVE))
+		return -EINVAL;
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_bridge_attach);
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b8cb6237a38b..29eba4e9a39d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1857,6 +1857,7 @@ tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	bridge->dev = dev;
 	dev_set_drvdata(dev, bridge);
 
+	bridge->bridge.owner = dev;
 	bridge->bridge.funcs = &tda998x_bridge_funcs;
 #ifdef CONFIG_OF
 	bridge->bridge.of_node = dev->of_node;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 682d01ba920c..f0f8b2a85c28 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -224,6 +224,7 @@ struct drm_bridge_funcs {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @owner: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -233,6 +234,7 @@ struct drm_bridge_funcs {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+	struct device *owner;
 	struct drm_device *dev;
 	struct drm_encoder *encoder;
 	struct drm_bridge *next;

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> > On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >>  static int tda998x_remove(struct i2c_client *client)
> >>  {
> >> -	component_del(&client->dev, &tda998x_ops);
> >> +	struct device *dev = &client->dev;
> >> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> >> +
> >> +	drm_bridge_remove(&bridge->bridge);
> >> +	component_del(dev, &tda998x_ops);
> >> +
> > 
> > I'd like to ask a rather fundamental question about DRM bridge support,
> > because I suspect that there's a major fsckup here.
> > 
> > The above is the function that deals with the TDA998x device being
> > unbound from the driver.  With the component API, this results in the
> > DRM device correctly being torn down, because one of the hardware
> > devices has gone.
> > 
> > With DRM bridge, the bridge is merely removed from the list of
> > bridges:
> > 
> > void drm_bridge_remove(struct drm_bridge *bridge)
> > {
> >         mutex_lock(&bridge_lock);
> >         list_del_init(&bridge->list);
> >         mutex_unlock(&bridge_lock);
> > }
> > EXPORT_SYMBOL(drm_bridge_remove);
> > 
> > and the memory backing the "struct tda998x_bridge" (which contains
> > the struct drm_bridge) will be freed by the devm subsystem.
> > 
> > However, there is no notification into the rest of the DRM subsystem
> > that the device has gone away.  Worse, the memory that is still in
> > use by DRM has now been freed, so further use of the DRM device
> > results in a use-after-free bug.
> > 
> > This is really not good, and to me looks like a fundamental problem
> > with the DRM bridge code.  I see nothing in the DRM bridge code that
> > deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> > the actual device itself.
> > 
> > So, from what I can see, there seems to be a fundamental lifetime
> > issue with the design of the DRM bridge code.  This needs to be
> > fixed.
> 
> Oh crap. A gigantic can of worms...

Yes, it's especially annoying for me, having put the effort in to
the component helper to cover all these cases.

> Would a patch (completely untested btw) along this line of thinking make
> any difference whatsoever?

It looks interesting - from what I can see of the device links code,
it would have the effect of unbinding the DRM device just before
TDA998x is unbound, so that's an improvement.

However, from what I can see, the link vanishes at that point (as
DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
in nothing further happening - the link will be recreated, but there
appears to be nothing that triggers the "consumer" to rebind at that
point.  Maybe I've missed something?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-24 10:08, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>>  static int tda998x_remove(struct i2c_client *client)
>>>>  {
>>>> -	component_del(&client->dev, &tda998x_ops);
>>>> +	struct device *dev = &client->dev;
>>>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>>> +
>>>> +	drm_bridge_remove(&bridge->bridge);
>>>> +	component_del(dev, &tda998x_ops);
>>>> +
>>>
>>> I'd like to ask a rather fundamental question about DRM bridge support,
>>> because I suspect that there's a major fsckup here.
>>>
>>> The above is the function that deals with the TDA998x device being
>>> unbound from the driver.  With the component API, this results in the
>>> DRM device correctly being torn down, because one of the hardware
>>> devices has gone.
>>>
>>> With DRM bridge, the bridge is merely removed from the list of
>>> bridges:
>>>
>>> void drm_bridge_remove(struct drm_bridge *bridge)
>>> {
>>>         mutex_lock(&bridge_lock);
>>>         list_del_init(&bridge->list);
>>>         mutex_unlock(&bridge_lock);
>>> }
>>> EXPORT_SYMBOL(drm_bridge_remove);
>>>
>>> and the memory backing the "struct tda998x_bridge" (which contains
>>> the struct drm_bridge) will be freed by the devm subsystem.
>>>
>>> However, there is no notification into the rest of the DRM subsystem
>>> that the device has gone away.  Worse, the memory that is still in
>>> use by DRM has now been freed, so further use of the DRM device
>>> results in a use-after-free bug.
>>>
>>> This is really not good, and to me looks like a fundamental problem
>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>>> the actual device itself.
>>>
>>> So, from what I can see, there seems to be a fundamental lifetime
>>> issue with the design of the DRM bridge code.  This needs to be
>>> fixed.
>>
>> Oh crap. A gigantic can of worms...
> 
> Yes, it's especially annoying for me, having put the effort in to
> the component helper to cover all these cases.
> 
>> Would a patch (completely untested btw) along this line of thinking make
>> any difference whatsoever?
> 
> It looks interesting - from what I can see of the device links code,
> it would have the effect of unbinding the DRM device just before
> TDA998x is unbound, so that's an improvement.
> 
> However, from what I can see, the link vanishes at that point (as
> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
> in nothing further happening - the link will be recreated, but there
> appears to be nothing that triggers the "consumer" to rebind at that
> point.  Maybe I've missed something?

Right, auto-remove is a no-go. So, improving on the previous...

(I think drm_panel might suffer from this issue too?)

Cheers,
Peter

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..b1365cfee445 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (bridge->dev)
 		return -EBUSY;
 
+	bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0);
+	if (!bridge->link)
+		return -EINVAL;
+
 	bridge->dev = encoder->dev;
 	bridge->encoder = encoder;
 
 	if (bridge->funcs->attach) {
 		ret = bridge->funcs->attach(bridge);
 		if (ret < 0) {
+			device_link_del(bridge->link);
 			bridge->dev = NULL;
 			bridge->encoder = NULL;
 			return ret;
@@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
+	device_link_del(bridge->link);
+
 	bridge->dev = NULL;
 }
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b8cb6237a38b..29eba4e9a39d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1857,6 +1857,7 @@ tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	bridge->dev = dev;
 	dev_set_drvdata(dev, bridge);
 
+	bridge->bridge.owner = dev;
 	bridge->bridge.funcs = &tda998x_bridge_funcs;
 #ifdef CONFIG_OF
 	bridge->bridge.of_node = dev->of_node;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 682d01ba920c..b8f33aba3216 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -224,6 +224,8 @@ struct drm_bridge_funcs {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @owner: device that owns the bridge
+ * @link: drm consumer <-> bridge supplier
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -233,6 +235,8 @@ struct drm_bridge_funcs {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+	struct device *owner;
+	struct device_link *link;
 	struct drm_device *dev;
 	struct drm_encoder *encoder;
 	struct drm_bridge *next;

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-24 12:14, Peter Rosin wrote:
> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>>>  static int tda998x_remove(struct i2c_client *client)
>>>>>  {
>>>>> -	component_del(&client->dev, &tda998x_ops);
>>>>> +	struct device *dev = &client->dev;
>>>>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>>>> +
>>>>> +	drm_bridge_remove(&bridge->bridge);
>>>>> +	component_del(dev, &tda998x_ops);
>>>>> +
>>>>
>>>> I'd like to ask a rather fundamental question about DRM bridge support,
>>>> because I suspect that there's a major fsckup here.
>>>>
>>>> The above is the function that deals with the TDA998x device being
>>>> unbound from the driver.  With the component API, this results in the
>>>> DRM device correctly being torn down, because one of the hardware
>>>> devices has gone.
>>>>
>>>> With DRM bridge, the bridge is merely removed from the list of
>>>> bridges:
>>>>
>>>> void drm_bridge_remove(struct drm_bridge *bridge)
>>>> {
>>>>         mutex_lock(&bridge_lock);
>>>>         list_del_init(&bridge->list);
>>>>         mutex_unlock(&bridge_lock);
>>>> }
>>>> EXPORT_SYMBOL(drm_bridge_remove);
>>>>
>>>> and the memory backing the "struct tda998x_bridge" (which contains
>>>> the struct drm_bridge) will be freed by the devm subsystem.
>>>>
>>>> However, there is no notification into the rest of the DRM subsystem
>>>> that the device has gone away.  Worse, the memory that is still in
>>>> use by DRM has now been freed, so further use of the DRM device
>>>> results in a use-after-free bug.
>>>>
>>>> This is really not good, and to me looks like a fundamental problem
>>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>>>> the actual device itself.
>>>>
>>>> So, from what I can see, there seems to be a fundamental lifetime
>>>> issue with the design of the DRM bridge code.  This needs to be
>>>> fixed.
>>>
>>> Oh crap. A gigantic can of worms...
>>
>> Yes, it's especially annoying for me, having put the effort in to
>> the component helper to cover all these cases.
>>
>>> Would a patch (completely untested btw) along this line of thinking make
>>> any difference whatsoever?
>>
>> It looks interesting - from what I can see of the device links code,
>> it would have the effect of unbinding the DRM device just before
>> TDA998x is unbound, so that's an improvement.
>>
>> However, from what I can see, the link vanishes at that point (as
>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>> in nothing further happening - the link will be recreated, but there
>> appears to be nothing that triggers the "consumer" to rebind at that
>> point.  Maybe I've missed something?
> 
> Right, auto-remove is a no-go. So, improving on the previous...

Heh, I didn't address the rebind triggering part at all, and while I'm by
no means responsible or have any deep knowledge, I thought this was true:

- driver .remove for the device owning the drm_bridge is what typically
  calls drm_bridge_remove()
- driver .remove is called as part of the device being unbound from the
  driver by the bus (i2c in this case)
- by registering a link to the consumer, this unbinding will trigger the
  removal of this main drm consumer device as part of the unbinding
- so everything aboput the drm device will be torn down, and everything
  will thus have to be reprobed to get things back

But I could easily have misunderstood just about everything in the above...

And maybe it's really inconvenient to have to trigger a reprobe of the
whole drm cluster? Maybe all drm driver parts should be components?

I have no idea.

Cheers,
Peter

PS. compile-tested the below and drm_bridge.c needs to
#include <drm/drm_device.h>

> (I think drm_panel might suffer from this issue too?)
> 
> Cheers,
> Peter
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..b1365cfee445 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (bridge->dev)
>  		return -EBUSY;
>  
> +	bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0);
> +	if (!bridge->link)
> +		return -EINVAL;
> +
>  	bridge->dev = encoder->dev;
>  	bridge->encoder = encoder;
>  
>  	if (bridge->funcs->attach) {
>  		ret = bridge->funcs->attach(bridge);
>  		if (ret < 0) {
> +			device_link_del(bridge->link);
>  			bridge->dev = NULL;
>  			bridge->encoder = NULL;
>  			return ret;
> @@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	device_link_del(bridge->link);
> +
>  	bridge->dev = NULL;
>  }
>  
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index b8cb6237a38b..29eba4e9a39d 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1857,6 +1857,7 @@ tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	bridge->dev = dev;
>  	dev_set_drvdata(dev, bridge);
>  
> +	bridge->bridge.owner = dev;
>  	bridge->bridge.funcs = &tda998x_bridge_funcs;
>  #ifdef CONFIG_OF
>  	bridge->bridge.of_node = dev->of_node;
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 682d01ba920c..b8f33aba3216 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -224,6 +224,8 @@ struct drm_bridge_funcs {
>  
>  /**
>   * struct drm_bridge - central DRM bridge control structure
> + * @owner: device that owns the bridge
> + * @link: drm consumer <-> bridge supplier
>   * @dev: DRM device this bridge belongs to
>   * @encoder: encoder to which this bridge is connected
>   * @next: the next bridge in the encoder chain
> @@ -233,6 +235,8 @@ struct drm_bridge_funcs {
>   * @driver_private: pointer to the bridge driver's internal context
>   */
>  struct drm_bridge {
> +	struct device *owner;
> +	struct device_link *link;
>  	struct drm_device *dev;
>  	struct drm_encoder *encoder;
>  	struct drm_bridge *next;
> 
> 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Jyri Sarha]

On 24/04/18 13:14, Peter Rosin wrote:
> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>>>  static int tda998x_remove(struct i2c_client *client)
>>>>>  {
>>>>> -	component_del(&client->dev, &tda998x_ops);
>>>>> +	struct device *dev = &client->dev;
>>>>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>>>> +
>>>>> +	drm_bridge_remove(&bridge->bridge);
>>>>> +	component_del(dev, &tda998x_ops);
>>>>> +
>>>>
>>>> I'd like to ask a rather fundamental question about DRM bridge support,
>>>> because I suspect that there's a major fsckup here.
>>>>
>>>> The above is the function that deals with the TDA998x device being
>>>> unbound from the driver.  With the component API, this results in the
>>>> DRM device correctly being torn down, because one of the hardware
>>>> devices has gone.
>>>>
>>>> With DRM bridge, the bridge is merely removed from the list of
>>>> bridges:
>>>>
>>>> void drm_bridge_remove(struct drm_bridge *bridge)
>>>> {
>>>>         mutex_lock(&bridge_lock);
>>>>         list_del_init(&bridge->list);
>>>>         mutex_unlock(&bridge_lock);
>>>> }
>>>> EXPORT_SYMBOL(drm_bridge_remove);
>>>>
>>>> and the memory backing the "struct tda998x_bridge" (which contains
>>>> the struct drm_bridge) will be freed by the devm subsystem.
>>>>
>>>> However, there is no notification into the rest of the DRM subsystem
>>>> that the device has gone away.  Worse, the memory that is still in
>>>> use by DRM has now been freed, so further use of the DRM device
>>>> results in a use-after-free bug.
>>>>
>>>> This is really not good, and to me looks like a fundamental problem
>>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>>>> the actual device itself.
>>>>
>>>> So, from what I can see, there seems to be a fundamental lifetime
>>>> issue with the design of the DRM bridge code.  This needs to be
>>>> fixed.
>>>
>>> Oh crap. A gigantic can of worms...
>>
>> Yes, it's especially annoying for me, having put the effort in to
>> the component helper to cover all these cases.
>>
>>> Would a patch (completely untested btw) along this line of thinking make
>>> any difference whatsoever?
>>
>> It looks interesting - from what I can see of the device links code,
>> it would have the effect of unbinding the DRM device just before
>> TDA998x is unbound, so that's an improvement.
>>
>> However, from what I can see, the link vanishes at that point (as
>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>> in nothing further happening - the link will be recreated, but there
>> appears to be nothing that triggers the "consumer" to rebind at that
>> point.  Maybe I've missed something?
> 
> Right, auto-remove is a no-go. So, improving on the previous...
> 
> (I think drm_panel might suffer from this issue too?)

Yes it does and I took a shot at trying to fix it at the end of the
previous merge window, but gave up as I run out of time. I re-spun the
work now after reading this thread. I add you and Russell to cc.

As far as I understand the re-binding problem can be solved with this patch:

https://lists.freedesktop.org/archives/dri-devel/2018-February/166907.html

The device_links are such a new concept that there is at least hope this
change won't have too unwanted side effects.

> > Cheers,
> Peter
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..b1365cfee445 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (bridge->dev)
>  		return -EBUSY;
>  
> +	bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0);
> +	if (!bridge->link)
> +		return -EINVAL;
> +

At least this piece code should prepare for the bridge owner and the
master drm device being the same, I which case the link should not be
needed. This happens at least when a panel is attached using
drm_panel_bridge_add().

Best reagards,
Jyri

>  	bridge->dev = encoder->dev;
>  	bridge->encoder = encoder;
>  
>  	if (bridge->funcs->attach) {
>  		ret = bridge->funcs->attach(bridge);
>  		if (ret < 0) {
> +			device_link_del(bridge->link);
>  			bridge->dev = NULL;
>  			bridge->encoder = NULL;
>  			return ret;
> @@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	device_link_del(bridge->link);
> +
>  	bridge->dev = NULL;
>  }
>  
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index b8cb6237a38b..29eba4e9a39d 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1857,6 +1857,7 @@ tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	bridge->dev = dev;
>  	dev_set_drvdata(dev, bridge);
>  
> +	bridge->bridge.owner = dev;
>  	bridge->bridge.funcs = &tda998x_bridge_funcs;
>  #ifdef CONFIG_OF
>  	bridge->bridge.of_node = dev->of_node;
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 682d01ba920c..b8f33aba3216 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -224,6 +224,8 @@ struct drm_bridge_funcs {
>  
>  /**
>   * struct drm_bridge - central DRM bridge control structure
> + * @owner: device that owns the bridge
> + * @link: drm consumer <-> bridge supplier
>   * @dev: DRM device this bridge belongs to
>   * @encoder: encoder to which this bridge is connected
>   * @next: the next bridge in the encoder chain
> @@ -233,6 +235,8 @@ struct drm_bridge_funcs {
>   * @driver_private: pointer to the bridge driver's internal context
>   */
>  struct drm_bridge {
> +	struct device *owner;
> +	struct device_link *link;
>  	struct drm_device *dev;
>  	struct drm_encoder *encoder;
>  	struct drm_bridge *next;
> 
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
> On 24/04/18 13:14, Peter Rosin wrote:
> > On 2018-04-24 10:08, Russell King - ARM Linux wrote:
> >> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> >>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> >>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >>>>>  static int tda998x_remove(struct i2c_client *client)
> >>>>>  {
> >>>>> -	component_del(&client->dev, &tda998x_ops);
> >>>>> +	struct device *dev = &client->dev;
> >>>>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> >>>>> +
> >>>>> +	drm_bridge_remove(&bridge->bridge);
> >>>>> +	component_del(dev, &tda998x_ops);
> >>>>> +
> >>>>
> >>>> I'd like to ask a rather fundamental question about DRM bridge support,
> >>>> because I suspect that there's a major fsckup here.
> >>>>
> >>>> The above is the function that deals with the TDA998x device being
> >>>> unbound from the driver.  With the component API, this results in the
> >>>> DRM device correctly being torn down, because one of the hardware
> >>>> devices has gone.
> >>>>
> >>>> With DRM bridge, the bridge is merely removed from the list of
> >>>> bridges:
> >>>>
> >>>> void drm_bridge_remove(struct drm_bridge *bridge)
> >>>> {
> >>>>         mutex_lock(&bridge_lock);
> >>>>         list_del_init(&bridge->list);
> >>>>         mutex_unlock(&bridge_lock);
> >>>> }
> >>>> EXPORT_SYMBOL(drm_bridge_remove);
> >>>>
> >>>> and the memory backing the "struct tda998x_bridge" (which contains
> >>>> the struct drm_bridge) will be freed by the devm subsystem.
> >>>>
> >>>> However, there is no notification into the rest of the DRM subsystem
> >>>> that the device has gone away.  Worse, the memory that is still in
> >>>> use by DRM has now been freed, so further use of the DRM device
> >>>> results in a use-after-free bug.
> >>>>
> >>>> This is really not good, and to me looks like a fundamental problem
> >>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
> >>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> >>>> the actual device itself.
> >>>>
> >>>> So, from what I can see, there seems to be a fundamental lifetime
> >>>> issue with the design of the DRM bridge code.  This needs to be
> >>>> fixed.
> >>>
> >>> Oh crap. A gigantic can of worms...
> >>
> >> Yes, it's especially annoying for me, having put the effort in to
> >> the component helper to cover all these cases.
> >>
> >>> Would a patch (completely untested btw) along this line of thinking make
> >>> any difference whatsoever?
> >>
> >> It looks interesting - from what I can see of the device links code,
> >> it would have the effect of unbinding the DRM device just before
> >> TDA998x is unbound, so that's an improvement.
> >>
> >> However, from what I can see, the link vanishes at that point (as
> >> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
> >> in nothing further happening - the link will be recreated, but there
> >> appears to be nothing that triggers the "consumer" to rebind at that
> >> point.  Maybe I've missed something?
> > 
> > Right, auto-remove is a no-go. So, improving on the previous...
> > 
> > (I think drm_panel might suffer from this issue too?)
> 
> Yes it does and I took a shot at trying to fix it at the end of the
> previous merge window, but gave up as I run out of time. I re-spun the
> work now after reading this thread. I add you and Russell to cc.

Right, and these exact problems are what the component helper is
there to sort out, in a subsystem independent way.

What is the problem with the component helper that people seem to
be soo loathed to use it, instead preferring to come up with sub-
standard and broken alternatives?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Jyri Sarha]

On 24/04/18 20:06, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
>> On 24/04/18 13:14, Peter Rosin wrote:
>>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>>>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>>>>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>>>>>  static int tda998x_remove(struct i2c_client *client)
>>>>>>>  {
>>>>>>> -	component_del(&client->dev, &tda998x_ops);
>>>>>>> +	struct device *dev = &client->dev;
>>>>>>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>>>>>> +
>>>>>>> +	drm_bridge_remove(&bridge->bridge);
>>>>>>> +	component_del(dev, &tda998x_ops);
>>>>>>> +
>>>>>>
>>>>>> I'd like to ask a rather fundamental question about DRM bridge support,
>>>>>> because I suspect that there's a major fsckup here.
>>>>>>
>>>>>> The above is the function that deals with the TDA998x device being
>>>>>> unbound from the driver.  With the component API, this results in the
>>>>>> DRM device correctly being torn down, because one of the hardware
>>>>>> devices has gone.
>>>>>>
>>>>>> With DRM bridge, the bridge is merely removed from the list of
>>>>>> bridges:
>>>>>>
>>>>>> void drm_bridge_remove(struct drm_bridge *bridge)
>>>>>> {
>>>>>>         mutex_lock(&bridge_lock);
>>>>>>         list_del_init(&bridge->list);
>>>>>>         mutex_unlock(&bridge_lock);
>>>>>> }
>>>>>> EXPORT_SYMBOL(drm_bridge_remove);
>>>>>>
>>>>>> and the memory backing the "struct tda998x_bridge" (which contains
>>>>>> the struct drm_bridge) will be freed by the devm subsystem.
>>>>>>
>>>>>> However, there is no notification into the rest of the DRM subsystem
>>>>>> that the device has gone away.  Worse, the memory that is still in
>>>>>> use by DRM has now been freed, so further use of the DRM device
>>>>>> results in a use-after-free bug.
>>>>>>
>>>>>> This is really not good, and to me looks like a fundamental problem
>>>>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>>>>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>>>>>> the actual device itself.
>>>>>>
>>>>>> So, from what I can see, there seems to be a fundamental lifetime
>>>>>> issue with the design of the DRM bridge code.  This needs to be
>>>>>> fixed.
>>>>>
>>>>> Oh crap. A gigantic can of worms...
>>>>
>>>> Yes, it's especially annoying for me, having put the effort in to
>>>> the component helper to cover all these cases.
>>>>
>>>>> Would a patch (completely untested btw) along this line of thinking make
>>>>> any difference whatsoever?
>>>>
>>>> It looks interesting - from what I can see of the device links code,
>>>> it would have the effect of unbinding the DRM device just before
>>>> TDA998x is unbound, so that's an improvement.
>>>>
>>>> However, from what I can see, the link vanishes at that point (as
>>>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>>>> in nothing further happening - the link will be recreated, but there
>>>> appears to be nothing that triggers the "consumer" to rebind at that
>>>> point.  Maybe I've missed something?
>>>
>>> Right, auto-remove is a no-go. So, improving on the previous...
>>>
>>> (I think drm_panel might suffer from this issue too?)
>>
>> Yes it does and I took a shot at trying to fix it at the end of the
>> previous merge window, but gave up as I run out of time. I re-spun the
>> work now after reading this thread. I add you and Russell to cc.
> 
> Right, and these exact problems are what the component helper is
> there to sort out, in a subsystem independent way.
> 
> What is the problem with the component helper that people seem to
> be soo loathed to use it, instead preferring to come up with sub-
> standard and broken alternatives?
> 

Nothing but time. Embedding component helpers seamlessly into drm
framework does not sound like a couple of days job. Right now I simply
do not have time to take on a challenge like that. If someone does it I
am all for it.

However, I would not call device links substandard. They are in the
device core after all.

Best regards,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 09:25:44PM +0300, Jyri Sarha wrote:
> On 24/04/18 20:06, Russell King - ARM Linux wrote:
> > On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
> >> On 24/04/18 13:14, Peter Rosin wrote:
> >>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
> >>>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> >>>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> >>>>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >>>>>>>  static int tda998x_remove(struct i2c_client *client)
> >>>>>>>  {
> >>>>>>> -	component_del(&client->dev, &tda998x_ops);
> >>>>>>> +	struct device *dev = &client->dev;
> >>>>>>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> >>>>>>> +
> >>>>>>> +	drm_bridge_remove(&bridge->bridge);
> >>>>>>> +	component_del(dev, &tda998x_ops);
> >>>>>>> +
> >>>>>>
> >>>>>> I'd like to ask a rather fundamental question about DRM bridge support,
> >>>>>> because I suspect that there's a major fsckup here.
> >>>>>>
> >>>>>> The above is the function that deals with the TDA998x device being
> >>>>>> unbound from the driver.  With the component API, this results in the
> >>>>>> DRM device correctly being torn down, because one of the hardware
> >>>>>> devices has gone.
> >>>>>>
> >>>>>> With DRM bridge, the bridge is merely removed from the list of
> >>>>>> bridges:
> >>>>>>
> >>>>>> void drm_bridge_remove(struct drm_bridge *bridge)
> >>>>>> {
> >>>>>>         mutex_lock(&bridge_lock);
> >>>>>>         list_del_init(&bridge->list);
> >>>>>>         mutex_unlock(&bridge_lock);
> >>>>>> }
> >>>>>> EXPORT_SYMBOL(drm_bridge_remove);
> >>>>>>
> >>>>>> and the memory backing the "struct tda998x_bridge" (which contains
> >>>>>> the struct drm_bridge) will be freed by the devm subsystem.
> >>>>>>
> >>>>>> However, there is no notification into the rest of the DRM subsystem
> >>>>>> that the device has gone away.  Worse, the memory that is still in
> >>>>>> use by DRM has now been freed, so further use of the DRM device
> >>>>>> results in a use-after-free bug.
> >>>>>>
> >>>>>> This is really not good, and to me looks like a fundamental problem
> >>>>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
> >>>>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> >>>>>> the actual device itself.
> >>>>>>
> >>>>>> So, from what I can see, there seems to be a fundamental lifetime
> >>>>>> issue with the design of the DRM bridge code.  This needs to be
> >>>>>> fixed.
> >>>>>
> >>>>> Oh crap. A gigantic can of worms...
> >>>>
> >>>> Yes, it's especially annoying for me, having put the effort in to
> >>>> the component helper to cover all these cases.
> >>>>
> >>>>> Would a patch (completely untested btw) along this line of thinking make
> >>>>> any difference whatsoever?
> >>>>
> >>>> It looks interesting - from what I can see of the device links code,
> >>>> it would have the effect of unbinding the DRM device just before
> >>>> TDA998x is unbound, so that's an improvement.
> >>>>
> >>>> However, from what I can see, the link vanishes at that point (as
> >>>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
> >>>> in nothing further happening - the link will be recreated, but there
> >>>> appears to be nothing that triggers the "consumer" to rebind at that
> >>>> point.  Maybe I've missed something?
> >>>
> >>> Right, auto-remove is a no-go. So, improving on the previous...
> >>>
> >>> (I think drm_panel might suffer from this issue too?)
> >>
> >> Yes it does and I took a shot at trying to fix it at the end of the
> >> previous merge window, but gave up as I run out of time. I re-spun the
> >> work now after reading this thread. I add you and Russell to cc.
> > 
> > Right, and these exact problems are what the component helper is
> > there to sort out, in a subsystem independent way.
> > 
> > What is the problem with the component helper that people seem to
> > be soo loathed to use it, instead preferring to come up with sub-
> > standard and broken alternatives?
> > 
> 
> Nothing but time. Embedding component helpers seamlessly into drm
> framework does not sound like a couple of days job. Right now I simply
> do not have time to take on a challenge like that. If someone does it I
> am all for it.
> 
> However, I would not call device links substandard. They are in the
> device core after all.

Umm, no, I was not talking about the device links, but the tendency to
have subsystem or component specific solutions to this problem.

We're now heading down the path of trying to retrofit the functionality
that one expects and is provided by the component helpers into DRM
bridge and DRM panel by using device links, which appears to only
partially resolve the problem.

On the point of device links, I've been wondering whether the component
helpers could take advantage of the device links, but at the moment
that would cause a regression if there's no facility to re-probe the
"consumer" when a "supplier" returns.

As you bring that up, I would say that they _are_ a substandard way to
solve this problem _if_, as I suspect, they would cause a regression
to the component helper if the component helper were to use them as a
means to control the probing of the "master" device.  This is not a
matter of which part of the kernel they are "in", but the functionality
that they can offer vs what would be expected.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Jyri Sarha]

On 25/04/18 02:25, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 09:25:44PM +0300, Jyri Sarha wrote:
>> On 24/04/18 20:06, Russell King - ARM Linux wrote:
>>> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
>>>> On 24/04/18 13:14, Peter Rosin wrote:
>>>>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>>>>>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>>>>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>>>>>>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>>>>>>>  static int tda998x_remove(struct i2c_client *client)
>>>>>>>>>  {
>>>>>>>>> -	component_del(&client->dev, &tda998x_ops);
>>>>>>>>> +	struct device *dev = &client->dev;
>>>>>>>>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>>>>>>>> +
>>>>>>>>> +	drm_bridge_remove(&bridge->bridge);
>>>>>>>>> +	component_del(dev, &tda998x_ops);
>>>>>>>>> +
>>>>>>>>
>>>>>>>> I'd like to ask a rather fundamental question about DRM bridge support,
>>>>>>>> because I suspect that there's a major fsckup here.
>>>>>>>>
>>>>>>>> The above is the function that deals with the TDA998x device being
>>>>>>>> unbound from the driver.  With the component API, this results in the
>>>>>>>> DRM device correctly being torn down, because one of the hardware
>>>>>>>> devices has gone.
>>>>>>>>
>>>>>>>> With DRM bridge, the bridge is merely removed from the list of
>>>>>>>> bridges:
>>>>>>>>
>>>>>>>> void drm_bridge_remove(struct drm_bridge *bridge)
>>>>>>>> {
>>>>>>>>         mutex_lock(&bridge_lock);
>>>>>>>>         list_del_init(&bridge->list);
>>>>>>>>         mutex_unlock(&bridge_lock);
>>>>>>>> }
>>>>>>>> EXPORT_SYMBOL(drm_bridge_remove);
>>>>>>>>
>>>>>>>> and the memory backing the "struct tda998x_bridge" (which contains
>>>>>>>> the struct drm_bridge) will be freed by the devm subsystem.
>>>>>>>>
>>>>>>>> However, there is no notification into the rest of the DRM subsystem
>>>>>>>> that the device has gone away.  Worse, the memory that is still in
>>>>>>>> use by DRM has now been freed, so further use of the DRM device
>>>>>>>> results in a use-after-free bug.
>>>>>>>>
>>>>>>>> This is really not good, and to me looks like a fundamental problem
>>>>>>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>>>>>>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>>>>>>>> the actual device itself.
>>>>>>>>
>>>>>>>> So, from what I can see, there seems to be a fundamental lifetime
>>>>>>>> issue with the design of the DRM bridge code.  This needs to be
>>>>>>>> fixed.
>>>>>>>
>>>>>>> Oh crap. A gigantic can of worms...
>>>>>>
>>>>>> Yes, it's especially annoying for me, having put the effort in to
>>>>>> the component helper to cover all these cases.
>>>>>>
>>>>>>> Would a patch (completely untested btw) along this line of thinking make
>>>>>>> any difference whatsoever?
>>>>>>
>>>>>> It looks interesting - from what I can see of the device links code,
>>>>>> it would have the effect of unbinding the DRM device just before
>>>>>> TDA998x is unbound, so that's an improvement.
>>>>>>
>>>>>> However, from what I can see, the link vanishes at that point (as
>>>>>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>>>>>> in nothing further happening - the link will be recreated, but there
>>>>>> appears to be nothing that triggers the "consumer" to rebind at that
>>>>>> point.  Maybe I've missed something?
>>>>>
>>>>> Right, auto-remove is a no-go. So, improving on the previous...
>>>>>
>>>>> (I think drm_panel might suffer from this issue too?)
>>>>
>>>> Yes it does and I took a shot at trying to fix it at the end of the
>>>> previous merge window, but gave up as I run out of time. I re-spun the
>>>> work now after reading this thread. I add you and Russell to cc.
>>>
>>> Right, and these exact problems are what the component helper is
>>> there to sort out, in a subsystem independent way.
>>>
>>> What is the problem with the component helper that people seem to
>>> be soo loathed to use it, instead preferring to come up with sub-
>>> standard and broken alternatives?
>>>
>>
>> Nothing but time. Embedding component helpers seamlessly into drm
>> framework does not sound like a couple of days job. Right now I simply
>> do not have time to take on a challenge like that. If someone does it I
>> am all for it.
>>
>> However, I would not call device links substandard. They are in the
>> device core after all.
> 
> Umm, no, I was not talking about the device links, but the tendency to
> have subsystem or component specific solutions to this problem.
> 

Oh yes. But in this case the substandard solution is already there and
it is already widely used, despite it being severely broken. I am merely
trying to fix the existing substandard solution.

I admit that a full integration with component helpers would probably be
more elegant solution to the problem, but the amount of work is just too
much. The change would impact the way all the master drm drivers pull
them selves together. The drivers that already use the component helpers
for some internal stuff will add their own challenge. Separate component
matching implementations are needed for device-tree and ACPI (are ther
more flavors?) etc. I just do not see this happening any time soon (am
happy to be wrong about this).

> We're now heading down the path of trying to retrofit the functionality
> that one expects and is provided by the component helpers into DRM
> bridge and DRM panel by using device links, which appears to only
> partially resolve the problem.
> 
> On the point of device links, I've been wondering whether the component
> helpers could take advantage of the device links, but at the moment
> that would cause a regression if there's no facility to re-probe the
> "consumer" when a "supplier" returns.
> 

I sent a patch[1] in February that solves the re-probing problem at
least in my environment.

> As you bring that up, I would say that they _are_ a substandard way to
> solve this problem _if_, as I suspect, they would cause a regression
> to the component helper if the component helper were to use them as a
> means to control the probing of the "master" device.  This is not a
> matter of which part of the kernel they are "in", but the functionality
> that they can offer vs what would be expected.
> 

The component helpers certainly have many features in that are not
(yet?) in device core's device links, but the amount of work needed to
fix the most pressing problem of attached drm panels or bridges is
couple of magnitudes less with device links when compared to full blown
component helper usage.

Best regards,
Jyri

[1]
https://lists.freedesktop.org/archives/dri-devel/2018-February/166907.html

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Wed, Apr 25, 2018 at 11:01:15PM +0300, Jyri Sarha wrote:
> On 25/04/18 02:25, Russell King - ARM Linux wrote:
> > On Tue, Apr 24, 2018 at 09:25:44PM +0300, Jyri Sarha wrote:
> >> On 24/04/18 20:06, Russell King - ARM Linux wrote:
> >>> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
> >>>> On 24/04/18 13:14, Peter Rosin wrote:
> >>>>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
> >>>>>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> >>>>>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> >>>>>>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >>>>>>>>>  static int tda998x_remove(struct i2c_client *client)
> >>>>>>>>>  {
> >>>>>>>>> -	component_del(&client->dev, &tda998x_ops);
> >>>>>>>>> +	struct device *dev = &client->dev;
> >>>>>>>>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> >>>>>>>>> +
> >>>>>>>>> +	drm_bridge_remove(&bridge->bridge);
> >>>>>>>>> +	component_del(dev, &tda998x_ops);
> >>>>>>>>> +
> >>>>>>>>
> >>>>>>>> I'd like to ask a rather fundamental question about DRM bridge support,
> >>>>>>>> because I suspect that there's a major fsckup here.
> >>>>>>>>
> >>>>>>>> The above is the function that deals with the TDA998x device being
> >>>>>>>> unbound from the driver.  With the component API, this results in the
> >>>>>>>> DRM device correctly being torn down, because one of the hardware
> >>>>>>>> devices has gone.
> >>>>>>>>
> >>>>>>>> With DRM bridge, the bridge is merely removed from the list of
> >>>>>>>> bridges:
> >>>>>>>>
> >>>>>>>> void drm_bridge_remove(struct drm_bridge *bridge)
> >>>>>>>> {
> >>>>>>>>         mutex_lock(&bridge_lock);
> >>>>>>>>         list_del_init(&bridge->list);
> >>>>>>>>         mutex_unlock(&bridge_lock);
> >>>>>>>> }
> >>>>>>>> EXPORT_SYMBOL(drm_bridge_remove);
> >>>>>>>>
> >>>>>>>> and the memory backing the "struct tda998x_bridge" (which contains
> >>>>>>>> the struct drm_bridge) will be freed by the devm subsystem.
> >>>>>>>>
> >>>>>>>> However, there is no notification into the rest of the DRM subsystem
> >>>>>>>> that the device has gone away.  Worse, the memory that is still in
> >>>>>>>> use by DRM has now been freed, so further use of the DRM device
> >>>>>>>> results in a use-after-free bug.
> >>>>>>>>
> >>>>>>>> This is really not good, and to me looks like a fundamental problem
> >>>>>>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
> >>>>>>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> >>>>>>>> the actual device itself.
> >>>>>>>>
> >>>>>>>> So, from what I can see, there seems to be a fundamental lifetime
> >>>>>>>> issue with the design of the DRM bridge code.  This needs to be
> >>>>>>>> fixed.
> >>>>>>>
> >>>>>>> Oh crap. A gigantic can of worms...
> >>>>>>
> >>>>>> Yes, it's especially annoying for me, having put the effort in to
> >>>>>> the component helper to cover all these cases.
> >>>>>>
> >>>>>>> Would a patch (completely untested btw) along this line of thinking make
> >>>>>>> any difference whatsoever?
> >>>>>>
> >>>>>> It looks interesting - from what I can see of the device links code,
> >>>>>> it would have the effect of unbinding the DRM device just before
> >>>>>> TDA998x is unbound, so that's an improvement.
> >>>>>>
> >>>>>> However, from what I can see, the link vanishes at that point (as
> >>>>>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
> >>>>>> in nothing further happening - the link will be recreated, but there
> >>>>>> appears to be nothing that triggers the "consumer" to rebind at that
> >>>>>> point.  Maybe I've missed something?
> >>>>>
> >>>>> Right, auto-remove is a no-go. So, improving on the previous...
> >>>>>
> >>>>> (I think drm_panel might suffer from this issue too?)
> >>>>
> >>>> Yes it does and I took a shot at trying to fix it at the end of the
> >>>> previous merge window, but gave up as I run out of time. I re-spun the
> >>>> work now after reading this thread. I add you and Russell to cc.
> >>>
> >>> Right, and these exact problems are what the component helper is
> >>> there to sort out, in a subsystem independent way.
> >>>
> >>> What is the problem with the component helper that people seem to
> >>> be soo loathed to use it, instead preferring to come up with sub-
> >>> standard and broken alternatives?
> >>>
> >>
> >> Nothing but time. Embedding component helpers seamlessly into drm
> >> framework does not sound like a couple of days job. Right now I simply
> >> do not have time to take on a challenge like that. If someone does it I
> >> am all for it.
> >>
> >> However, I would not call device links substandard. They are in the
> >> device core after all.
> > 
> > Umm, no, I was not talking about the device links, but the tendency to
> > have subsystem or component specific solutions to this problem.
> > 
> 
> Oh yes. But in this case the substandard solution is already there and
> it is already widely used, despite it being severely broken. I am merely
> trying to fix the existing substandard solution.
> 
> I admit that a full integration with component helpers would probably be
> more elegant solution to the problem, but the amount of work is just too
> much. The change would impact the way all the master drm drivers pull
> them selves together. The drivers that already use the component helpers
> for some internal stuff will add their own challenge. Separate component
> matching implementations are needed for device-tree and ACPI (are ther
> more flavors?) etc. I just do not see this happening any time soon (am
> happy to be wrong about this).

The issue is actually worse than that:

- drivers that are already componentised can't use bridges
- drivers that use bridges can't use componentised stuff

because bridges don't register themselves with the component helper,
and the helpers in drm_of.c assume that all graph nodes will be
components.

The whole thing about whether stuff is componentised or bridge based
is really getting out of hand, and the push is towards bridge based
stuff even though that is technically inferior when it comes to being
able to develop and test (which involves being able to remove and
re-insert modules.)

Consequently more and more code is being written for bridges, and
the component helper ignored, and the problems with bridges are
being ignored.  This is not healthy.

The problem is only going to get worse.  Someone needs to bite the
bullet and fix bridges before the problem gets any more out of hand.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Fri, Jul 06, 2018 at 11:03:46AM +0100, Russell King - ARM Linux wrote:
> On Wed, Apr 25, 2018 at 11:01:15PM +0300, Jyri Sarha wrote:
> > Oh yes. But in this case the substandard solution is already there and
> > it is already widely used, despite it being severely broken. I am merely
> > trying to fix the existing substandard solution.
> > 
> > I admit that a full integration with component helpers would probably be
> > more elegant solution to the problem, but the amount of work is just too
> > much. The change would impact the way all the master drm drivers pull
> > them selves together. The drivers that already use the component helpers
> > for some internal stuff will add their own challenge. Separate component
> > matching implementations are needed for device-tree and ACPI (are ther
> > more flavors?) etc. I just do not see this happening any time soon (am
> > happy to be wrong about this).
> 
> The issue is actually worse than that:
> 
> - drivers that are already componentised can't use bridges
> - drivers that use bridges can't use componentised stuff
> 
> because bridges don't register themselves with the component helper,
> and the helpers in drm_of.c assume that all graph nodes will be
> components.
> 
> The whole thing about whether stuff is componentised or bridge based
> is really getting out of hand, and the push is towards bridge based
> stuff even though that is technically inferior when it comes to being
> able to develop and test (which involves being able to remove and
> re-insert modules.)
> 
> Consequently more and more code is being written for bridges, and
> the component helper ignored, and the problems with bridges are
> being ignored.  This is not healthy.
> 
> The problem is only going to get worse.  Someone needs to bite the
> bullet and fix bridges before the problem gets any more out of hand.

This patch (which is actually two patches locally) allows the component
helper to know what's going on inside the bridge code wrt bridge
availability, and takes the appropriate action at the correct time.
No need for device links or similar, or incompatibilities between
bridges and components.  The only requirement is that bridges set the
"device" member of struct drm_bridge to opt-in to this.

Tested with Armada converted to support bridges, TDA998x as a
componentised bridge, and dumb-vga-dac as a non-componentised bridge:

root@cubox:~# less /sys/kernel/debug/device_component/display-subsystem
master name                                            status
-------------------------------------------------------------
display-subsystem                                       bound

device name                                            status
-------------------------------------------------------------
port                                               registered
port                                               registered
hdmi-encoder                                       registered
vga-bridge                                         registered
root@cubox:~# dmesg |grep bound
[    1.921798] armada-drm display-subsystem: bound f1820000.lcd-controller (ops
armada_lcd_ops)
[    1.931014] armada-drm display-subsystem: bound f1810000.lcd-controller (ops
armada_lcd_ops)
[    2.069231] armada-drm display-subsystem: bound 1-0070 (ops tda998x_ops)
[    2.076059] armada-drm display-subsystem: bound vga-bridge (ops dummy_ops)

Without this, the same DT fails because "vga-bridge" is never added
to the component helpers.

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 8946dfee4768..b14b3a3655ea 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -602,4 +602,32 @@ void component_del(struct device *dev, const struct component_ops *ops)
 }
 EXPORT_SYMBOL_GPL(component_del);
 
+static int component_dummy_bind(struct device *comp, struct device *master,
+				void *master_data)
+{
+	return 0;
+}
+
+static void component_dummy_unbind(struct device *comp, struct device *master,
+				   void *master_data)
+{
+}
+
+static const struct component_ops dummy_ops = {
+	.bind = component_dummy_bind,
+	.unbind = component_dummy_unbind,
+};
+
+int component_mark_available(struct device *dev)
+{
+	return component_add(dev, &dummy_ops);
+}
+EXPORT_SYMBOL_GPL(component_mark_available);
+
+void component_mark_unavailable(struct device *dev)
+{
+	component_del(dev, &dummy_ops);
+}
+EXPORT_SYMBOL_GPL(component_mark_unavailable);
+
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..ce3ccd327916 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -21,6 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include <linux/component.h>
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -73,6 +74,9 @@ void drm_bridge_add(struct drm_bridge *bridge)
 	mutex_lock(&bridge_lock);
 	list_add_tail(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);
+
+	if (bridge->device)
+		WARN_ON(component_mark_available(bridge->device));
 }
 EXPORT_SYMBOL(drm_bridge_add);
 
@@ -83,6 +87,9 @@ EXPORT_SYMBOL(drm_bridge_add);
  */
 void drm_bridge_remove(struct drm_bridge *bridge)
 {
+	if (bridge->device)
+		component_mark_unavailable(bridge->device);
+
 	mutex_lock(&bridge_lock);
 	list_del_init(&bridge->list);
 	mutex_unlock(&bridge_lock);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..e863da14d4d9 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -268,6 +268,7 @@ struct drm_bridge {
 	struct drm_device *dev;
 	struct drm_encoder *encoder;
 	struct drm_bridge *next;
+	struct device *device;
 #ifdef CONFIG_OF
 	struct device_node *of_node;
 #endif
diff --git a/include/linux/component.h b/include/linux/component.h
index e71fbbbc74e2..a1c824485f54 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -16,6 +16,10 @@ struct component_ops {
 int component_add(struct device *, const struct component_ops *);
 void component_del(struct device *, const struct component_ops *);
 
+/* For subsystems where drivers do not call component_add()/component_del() */
+int component_mark_available(struct device *dev);
+void component_mark_unavailable(struct device *dev);
+
 int component_bind_all(struct device *master, void *master_data);
 void component_unbind_all(struct device *master, void *master_data);
 


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

Ping - any comments from anyone on this idea?

On Fri, Jul 06, 2018 at 01:43:05PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 06, 2018 at 11:03:46AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Apr 25, 2018 at 11:01:15PM +0300, Jyri Sarha wrote:
> > > Oh yes. But in this case the substandard solution is already there and
> > > it is already widely used, despite it being severely broken. I am merely
> > > trying to fix the existing substandard solution.
> > > 
> > > I admit that a full integration with component helpers would probably be
> > > more elegant solution to the problem, but the amount of work is just too
> > > much. The change would impact the way all the master drm drivers pull
> > > them selves together. The drivers that already use the component helpers
> > > for some internal stuff will add their own challenge. Separate component
> > > matching implementations are needed for device-tree and ACPI (are ther
> > > more flavors?) etc. I just do not see this happening any time soon (am
> > > happy to be wrong about this).
> > 
> > The issue is actually worse than that:
> > 
> > - drivers that are already componentised can't use bridges
> > - drivers that use bridges can't use componentised stuff
> > 
> > because bridges don't register themselves with the component helper,
> > and the helpers in drm_of.c assume that all graph nodes will be
> > components.
> > 
> > The whole thing about whether stuff is componentised or bridge based
> > is really getting out of hand, and the push is towards bridge based
> > stuff even though that is technically inferior when it comes to being
> > able to develop and test (which involves being able to remove and
> > re-insert modules.)
> > 
> > Consequently more and more code is being written for bridges, and
> > the component helper ignored, and the problems with bridges are
> > being ignored.  This is not healthy.
> > 
> > The problem is only going to get worse.  Someone needs to bite the
> > bullet and fix bridges before the problem gets any more out of hand.
> 
> This patch (which is actually two patches locally) allows the component
> helper to know what's going on inside the bridge code wrt bridge
> availability, and takes the appropriate action at the correct time.
> No need for device links or similar, or incompatibilities between
> bridges and components.  The only requirement is that bridges set the
> "device" member of struct drm_bridge to opt-in to this.
> 
> Tested with Armada converted to support bridges, TDA998x as a
> componentised bridge, and dumb-vga-dac as a non-componentised bridge:
> 
> root@cubox:~# less /sys/kernel/debug/device_component/display-subsystem
> master name                                            status
> -------------------------------------------------------------
> display-subsystem                                       bound
> 
> device name                                            status
> -------------------------------------------------------------
> port                                               registered
> port                                               registered
> hdmi-encoder                                       registered
> vga-bridge                                         registered
> root@cubox:~# dmesg |grep bound
> [    1.921798] armada-drm display-subsystem: bound f1820000.lcd-controller (ops
> armada_lcd_ops)
> [    1.931014] armada-drm display-subsystem: bound f1810000.lcd-controller (ops
> armada_lcd_ops)
> [    2.069231] armada-drm display-subsystem: bound 1-0070 (ops tda998x_ops)
> [    2.076059] armada-drm display-subsystem: bound vga-bridge (ops dummy_ops)
> 
> Without this, the same DT fails because "vga-bridge" is never added
> to the component helpers.
> 
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 8946dfee4768..b14b3a3655ea 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -602,4 +602,32 @@ void component_del(struct device *dev, const struct component_ops *ops)
>  }
>  EXPORT_SYMBOL_GPL(component_del);
>  
> +static int component_dummy_bind(struct device *comp, struct device *master,
> +				void *master_data)
> +{
> +	return 0;
> +}
> +
> +static void component_dummy_unbind(struct device *comp, struct device *master,
> +				   void *master_data)
> +{
> +}
> +
> +static const struct component_ops dummy_ops = {
> +	.bind = component_dummy_bind,
> +	.unbind = component_dummy_unbind,
> +};
> +
> +int component_mark_available(struct device *dev)
> +{
> +	return component_add(dev, &dummy_ops);
> +}
> +EXPORT_SYMBOL_GPL(component_mark_available);
> +
> +void component_mark_unavailable(struct device *dev)
> +{
> +	component_del(dev, &dummy_ops);
> +}
> +EXPORT_SYMBOL_GPL(component_mark_unavailable);
> +
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..ce3ccd327916 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -21,6 +21,7 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <linux/component.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -73,6 +74,9 @@ void drm_bridge_add(struct drm_bridge *bridge)
>  	mutex_lock(&bridge_lock);
>  	list_add_tail(&bridge->list, &bridge_list);
>  	mutex_unlock(&bridge_lock);
> +
> +	if (bridge->device)
> +		WARN_ON(component_mark_available(bridge->device));
>  }
>  EXPORT_SYMBOL(drm_bridge_add);
>  
> @@ -83,6 +87,9 @@ EXPORT_SYMBOL(drm_bridge_add);
>   */
>  void drm_bridge_remove(struct drm_bridge *bridge)
>  {
> +	if (bridge->device)
> +		component_mark_unavailable(bridge->device);
> +
>  	mutex_lock(&bridge_lock);
>  	list_del_init(&bridge->list);
>  	mutex_unlock(&bridge_lock);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec46979..e863da14d4d9 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -268,6 +268,7 @@ struct drm_bridge {
>  	struct drm_device *dev;
>  	struct drm_encoder *encoder;
>  	struct drm_bridge *next;
> +	struct device *device;
>  #ifdef CONFIG_OF
>  	struct device_node *of_node;
>  #endif
> diff --git a/include/linux/component.h b/include/linux/component.h
> index e71fbbbc74e2..a1c824485f54 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -16,6 +16,10 @@ struct component_ops {
>  int component_add(struct device *, const struct component_ops *);
>  void component_del(struct device *, const struct component_ops *);
>  
> +/* For subsystems where drivers do not call component_add()/component_del() */
> +int component_mark_available(struct device *dev);
> +void component_mark_unavailable(struct device *dev);
> +
>  int component_bind_all(struct device *master, void *master_data);
>  void component_unbind_all(struct device *master, void *master_data);
>  
> 
> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
> According to speedtest.net: 13Mbps down 490kbps up

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-07-06 14:43, Russell King - ARM Linux wrote:
> On Fri, Jul 06, 2018 at 11:03:46AM +0100, Russell King - ARM Linux wrote:
>> On Wed, Apr 25, 2018 at 11:01:15PM +0300, Jyri Sarha wrote:
>>> Oh yes. But in this case the substandard solution is already there and
>>> it is already widely used, despite it being severely broken. I am merely
>>> trying to fix the existing substandard solution.
>>>
>>> I admit that a full integration with component helpers would probably be
>>> more elegant solution to the problem, but the amount of work is just too
>>> much. The change would impact the way all the master drm drivers pull
>>> them selves together. The drivers that already use the component helpers
>>> for some internal stuff will add their own challenge. Separate component
>>> matching implementations are needed for device-tree and ACPI (are ther
>>> more flavors?) etc. I just do not see this happening any time soon (am
>>> happy to be wrong about this).
>>
>> The issue is actually worse than that:
>>
>> - drivers that are already componentised can't use bridges
>> - drivers that use bridges can't use componentised stuff
>>
>> because bridges don't register themselves with the component helper,
>> and the helpers in drm_of.c assume that all graph nodes will be
>> components.
>>
>> The whole thing about whether stuff is componentised or bridge based
>> is really getting out of hand, and the push is towards bridge based
>> stuff even though that is technically inferior when it comes to being
>> able to develop and test (which involves being able to remove and
>> re-insert modules.)
>>
>> Consequently more and more code is being written for bridges, and
>> the component helper ignored, and the problems with bridges are
>> being ignored.  This is not healthy.
>>
>> The problem is only going to get worse.  Someone needs to bite the
>> bullet and fix bridges before the problem gets any more out of hand.
> 
> This patch (which is actually two patches locally) allows the component
> helper to know what's going on inside the bridge code wrt bridge
> availability, and takes the appropriate action at the correct time.
> No need for device links or similar, or incompatibilities between
> bridges and components.  The only requirement is that bridges set the
> "device" member of struct drm_bridge to opt-in to this.
> 
> Tested with Armada converted to support bridges, TDA998x as a
> componentised bridge, and dumb-vga-dac as a non-componentised bridge:
> 
> root@cubox:~# less /sys/kernel/debug/device_component/display-subsystem
> master name                                            status
> -------------------------------------------------------------
> display-subsystem                                       bound
> 
> device name                                            status
> -------------------------------------------------------------
> port                                               registered
> port                                               registered
> hdmi-encoder                                       registered
> vga-bridge                                         registered
> root@cubox:~# dmesg |grep bound
> [    1.921798] armada-drm display-subsystem: bound f1820000.lcd-controller (ops
> armada_lcd_ops)
> [    1.931014] armada-drm display-subsystem: bound f1810000.lcd-controller (ops
> armada_lcd_ops)
> [    2.069231] armada-drm display-subsystem: bound 1-0070 (ops tda998x_ops)
> [    2.076059] armada-drm display-subsystem: bound vga-bridge (ops dummy_ops)
> 
> Without this, the same DT fails because "vga-bridge" is never added
> to the component helpers.

What did you need to do to convert Armada to support bridges? How much
work is it to convert drivers that support bridges so that they
support components? Maybe that's not needed? What happens with tda998x?
I mean, it already calls component_add, and with this there's an
indirect call in drm_bridge_add which it also calls. I guess I'm asking
if a component may call component_add several times without things
sliding sideways?

> 
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 8946dfee4768..b14b3a3655ea 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -602,4 +602,32 @@ void component_del(struct device *dev, const struct component_ops *ops)
>  }
>  EXPORT_SYMBOL_GPL(component_del);
>  
> +static int component_dummy_bind(struct device *comp, struct device *master,
> +				void *master_data)
> +{
> +	return 0;
> +}
> +
> +static void component_dummy_unbind(struct device *comp, struct device *master,
> +				   void *master_data)
> +{
> +}
> +
> +static const struct component_ops dummy_ops = {
> +	.bind = component_dummy_bind,
> +	.unbind = component_dummy_unbind,
> +};
> +
> +int component_mark_available(struct device *dev)
> +{
> +	return component_add(dev, &dummy_ops);
> +}
> +EXPORT_SYMBOL_GPL(component_mark_available);
> +
> +void component_mark_unavailable(struct device *dev)
> +{
> +	component_del(dev, &dummy_ops);
> +}
> +EXPORT_SYMBOL_GPL(component_mark_unavailable);
> +

Is this really needed in component.c? I'd say that these dummy
bridge_component_bind/unbind can be added directly in drm_bridge.c
and that the new call to component_mark_available in drm_bridge
could simply be component_add(bridge->device, &bridge_component_ops)
(etc)

>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..ce3ccd327916 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -21,6 +21,7 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <linux/component.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -73,6 +74,9 @@ void drm_bridge_add(struct drm_bridge *bridge)
>  	mutex_lock(&bridge_lock);
>  	list_add_tail(&bridge->list, &bridge_list);
>  	mutex_unlock(&bridge_lock);
> +
> +	if (bridge->device)
> +		WARN_ON(component_mark_available(bridge->device));
>  }
>  EXPORT_SYMBOL(drm_bridge_add);
>  
> @@ -83,6 +87,9 @@ EXPORT_SYMBOL(drm_bridge_add);
>   */
>  void drm_bridge_remove(struct drm_bridge *bridge)
>  {
> +	if (bridge->device)
> +		component_mark_unavailable(bridge->device);
> +
>  	mutex_lock(&bridge_lock);
>  	list_del_init(&bridge->list);
>  	mutex_unlock(&bridge_lock);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec46979..e863da14d4d9 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -268,6 +268,7 @@ struct drm_bridge {
>  	struct drm_device *dev;
>  	struct drm_encoder *encoder;
>  	struct drm_bridge *next;
> +	struct device *device;

In patch [1] i add struct device *odev (for owner device) and the series
then proceeds to convert all bridges to add a link to its owner device
and to then remove the (below) of_node member.

[1] https://lkml.org/lkml/2018/5/16/382

Would it be bad if all bridges opted in to this? In other words, could
my "odev" and your "device" be shared?

Cheers,
Peter

>  #ifdef CONFIG_OF
>  	struct device_node *of_node;
>  #endif
> diff --git a/include/linux/component.h b/include/linux/component.h
> index e71fbbbc74e2..a1c824485f54 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -16,6 +16,10 @@ struct component_ops {
>  int component_add(struct device *, const struct component_ops *);
>  void component_del(struct device *, const struct component_ops *);
>  
> +/* For subsystems where drivers do not call component_add()/component_del() */
> +int component_mark_available(struct device *dev);
> +void component_mark_unavailable(struct device *dev);
> +
>  int component_bind_all(struct device *master, void *master_data);
>  void component_unbind_all(struct device *master, void *master_data);
>  
> 
> 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Aug 28, 2018 at 07:49:28PM +0200, Peter Rosin wrote:
> On 2018-07-06 14:43, Russell King - ARM Linux wrote:
> > On Fri, Jul 06, 2018 at 11:03:46AM +0100, Russell King - ARM Linux wrote:
> >> On Wed, Apr 25, 2018 at 11:01:15PM +0300, Jyri Sarha wrote:
> >>> Oh yes. But in this case the substandard solution is already there and
> >>> it is already widely used, despite it being severely broken. I am merely
> >>> trying to fix the existing substandard solution.
> >>>
> >>> I admit that a full integration with component helpers would probably be
> >>> more elegant solution to the problem, but the amount of work is just too
> >>> much. The change would impact the way all the master drm drivers pull
> >>> them selves together. The drivers that already use the component helpers
> >>> for some internal stuff will add their own challenge. Separate component
> >>> matching implementations are needed for device-tree and ACPI (are ther
> >>> more flavors?) etc. I just do not see this happening any time soon (am
> >>> happy to be wrong about this).
> >>
> >> The issue is actually worse than that:
> >>
> >> - drivers that are already componentised can't use bridges
> >> - drivers that use bridges can't use componentised stuff
> >>
> >> because bridges don't register themselves with the component helper,
> >> and the helpers in drm_of.c assume that all graph nodes will be
> >> components.
> >>
> >> The whole thing about whether stuff is componentised or bridge based
> >> is really getting out of hand, and the push is towards bridge based
> >> stuff even though that is technically inferior when it comes to being
> >> able to develop and test (which involves being able to remove and
> >> re-insert modules.)
> >>
> >> Consequently more and more code is being written for bridges, and
> >> the component helper ignored, and the problems with bridges are
> >> being ignored.  This is not healthy.
> >>
> >> The problem is only going to get worse.  Someone needs to bite the
> >> bullet and fix bridges before the problem gets any more out of hand.
> > 
> > This patch (which is actually two patches locally) allows the component
> > helper to know what's going on inside the bridge code wrt bridge
> > availability, and takes the appropriate action at the correct time.
> > No need for device links or similar, or incompatibilities between
> > bridges and components.  The only requirement is that bridges set the
> > "device" member of struct drm_bridge to opt-in to this.
> > 
> > Tested with Armada converted to support bridges, TDA998x as a
> > componentised bridge, and dumb-vga-dac as a non-componentised bridge:
> > 
> > root@cubox:~# less /sys/kernel/debug/device_component/display-subsystem
> > master name                                            status
> > -------------------------------------------------------------
> > display-subsystem                                       bound
> > 
> > device name                                            status
> > -------------------------------------------------------------
> > port                                               registered
> > port                                               registered
> > hdmi-encoder                                       registered
> > vga-bridge                                         registered
> > root@cubox:~# dmesg |grep bound
> > [    1.921798] armada-drm display-subsystem: bound f1820000.lcd-controller (ops
> > armada_lcd_ops)
> > [    1.931014] armada-drm display-subsystem: bound f1810000.lcd-controller (ops
> > armada_lcd_ops)
> > [    2.069231] armada-drm display-subsystem: bound 1-0070 (ops tda998x_ops)
> > [    2.076059] armada-drm display-subsystem: bound vga-bridge (ops dummy_ops)
> > 
> > Without this, the same DT fails because "vga-bridge" is never added
> > to the component helpers.
> 
> What did you need to do to convert Armada to support bridges? How much
> work is it to convert drivers that support bridges so that they
> support components? Maybe that's not needed? What happens with tda998x?
> I mean, it already calls component_add, and with this there's an
> indirect call in drm_bridge_add which it also calls. I guess I'm asking
> if a component may call component_add several times without things
> sliding sideways?

The difference with tda998x is that with the code below (as it stood
in an earlier revision of the bridge code, when we had a separate
bridge->of_node member), bridge->device is not set for the tda998x,
which avoids the duplicated component_add() - which would be illegal
(and cause problems.)

However, I also hacked tda998x to make tda998x_bind() a no-op - without
such a hack, the DRM driver needs to know whether the bridge is tda998x
or not, so it knows whether it needs to create the encoder.

I don't think there's any simple, non-hacky solution to this problem.

> 
> > 
> > diff --git a/drivers/base/component.c b/drivers/base/component.c
> > index 8946dfee4768..b14b3a3655ea 100644
> > --- a/drivers/base/component.c
> > +++ b/drivers/base/component.c
> > @@ -602,4 +602,32 @@ void component_del(struct device *dev, const struct component_ops *ops)
> >  }
> >  EXPORT_SYMBOL_GPL(component_del);
> >  
> > +static int component_dummy_bind(struct device *comp, struct device *master,
> > +				void *master_data)
> > +{
> > +	return 0;
> > +}
> > +
> > +static void component_dummy_unbind(struct device *comp, struct device *master,
> > +				   void *master_data)
> > +{
> > +}
> > +
> > +static const struct component_ops dummy_ops = {
> > +	.bind = component_dummy_bind,
> > +	.unbind = component_dummy_unbind,
> > +};
> > +
> > +int component_mark_available(struct device *dev)
> > +{
> > +	return component_add(dev, &dummy_ops);
> > +}
> > +EXPORT_SYMBOL_GPL(component_mark_available);
> > +
> > +void component_mark_unavailable(struct device *dev)
> > +{
> > +	component_del(dev, &dummy_ops);
> > +}
> > +EXPORT_SYMBOL_GPL(component_mark_unavailable);
> > +
> 
> Is this really needed in component.c? I'd say that these dummy
> bridge_component_bind/unbind can be added directly in drm_bridge.c
> and that the new call to component_mark_available in drm_bridge
> could simply be component_add(bridge->device, &bridge_component_ops)
> (etc)

What if other subsystems want this functionality?  IMHO, it belongs
in the component layer, not in other subsystems where it could end
up being duplicated.

> >  MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 1638bfe9627c..ce3ccd327916 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -21,6 +21,7 @@
> >   * DEALINGS IN THE SOFTWARE.
> >   */
> >  
> > +#include <linux/component.h>
> >  #include <linux/err.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > @@ -73,6 +74,9 @@ void drm_bridge_add(struct drm_bridge *bridge)
> >  	mutex_lock(&bridge_lock);
> >  	list_add_tail(&bridge->list, &bridge_list);
> >  	mutex_unlock(&bridge_lock);
> > +
> > +	if (bridge->device)
> > +		WARN_ON(component_mark_available(bridge->device));
> >  }
> >  EXPORT_SYMBOL(drm_bridge_add);
> >  
> > @@ -83,6 +87,9 @@ EXPORT_SYMBOL(drm_bridge_add);
> >   */
> >  void drm_bridge_remove(struct drm_bridge *bridge)
> >  {
> > +	if (bridge->device)
> > +		component_mark_unavailable(bridge->device);
> > +
> >  	mutex_lock(&bridge_lock);
> >  	list_del_init(&bridge->list);
> >  	mutex_unlock(&bridge_lock);
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 3270fec46979..e863da14d4d9 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -268,6 +268,7 @@ struct drm_bridge {
> >  	struct drm_device *dev;
> >  	struct drm_encoder *encoder;
> >  	struct drm_bridge *next;
> > +	struct device *device;
> 
> In patch [1] i add struct device *odev (for owner device) and the series
> then proceeds to convert all bridges to add a link to its owner device
> and to then remove the (below) of_node member.
> 
> [1] https://lkml.org/lkml/2018/5/16/382
> 
> Would it be bad if all bridges opted in to this? In other words, could
> my "odev" and your "device" be shared?

No (see my explanation above about duplicate registrations not being
permitted.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-24 19:06, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
>> On 24/04/18 13:14, Peter Rosin wrote:
>>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>>>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>>>>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>>>>>  static int tda998x_remove(struct i2c_client *client)
>>>>>>>  {
>>>>>>> -	component_del(&client->dev, &tda998x_ops);
>>>>>>> +	struct device *dev = &client->dev;
>>>>>>> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>>>>>> +
>>>>>>> +	drm_bridge_remove(&bridge->bridge);
>>>>>>> +	component_del(dev, &tda998x_ops);
>>>>>>> +
>>>>>>
>>>>>> I'd like to ask a rather fundamental question about DRM bridge support,
>>>>>> because I suspect that there's a major fsckup here.
>>>>>>
>>>>>> The above is the function that deals with the TDA998x device being
>>>>>> unbound from the driver.  With the component API, this results in the
>>>>>> DRM device correctly being torn down, because one of the hardware
>>>>>> devices has gone.
>>>>>>
>>>>>> With DRM bridge, the bridge is merely removed from the list of
>>>>>> bridges:
>>>>>>
>>>>>> void drm_bridge_remove(struct drm_bridge *bridge)
>>>>>> {
>>>>>>         mutex_lock(&bridge_lock);
>>>>>>         list_del_init(&bridge->list);
>>>>>>         mutex_unlock(&bridge_lock);
>>>>>> }
>>>>>> EXPORT_SYMBOL(drm_bridge_remove);
>>>>>>
>>>>>> and the memory backing the "struct tda998x_bridge" (which contains
>>>>>> the struct drm_bridge) will be freed by the devm subsystem.
>>>>>>
>>>>>> However, there is no notification into the rest of the DRM subsystem
>>>>>> that the device has gone away.  Worse, the memory that is still in
>>>>>> use by DRM has now been freed, so further use of the DRM device
>>>>>> results in a use-after-free bug.
>>>>>>
>>>>>> This is really not good, and to me looks like a fundamental problem
>>>>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>>>>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>>>>>> the actual device itself.
>>>>>>
>>>>>> So, from what I can see, there seems to be a fundamental lifetime
>>>>>> issue with the design of the DRM bridge code.  This needs to be
>>>>>> fixed.
>>>>>
>>>>> Oh crap. A gigantic can of worms...
>>>>
>>>> Yes, it's especially annoying for me, having put the effort in to
>>>> the component helper to cover all these cases.
>>>>
>>>>> Would a patch (completely untested btw) along this line of thinking make
>>>>> any difference whatsoever?
>>>>
>>>> It looks interesting - from what I can see of the device links code,
>>>> it would have the effect of unbinding the DRM device just before
>>>> TDA998x is unbound, so that's an improvement.
>>>>
>>>> However, from what I can see, the link vanishes at that point (as
>>>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>>>> in nothing further happening - the link will be recreated, but there
>>>> appears to be nothing that triggers the "consumer" to rebind at that
>>>> point.  Maybe I've missed something?
>>>
>>> Right, auto-remove is a no-go. So, improving on the previous...
>>>
>>> (I think drm_panel might suffer from this issue too?)
>>
>> Yes it does and I took a shot at trying to fix it at the end of the
>> previous merge window, but gave up as I run out of time. I re-spun the
>> work now after reading this thread. I add you and Russell to cc.
> 
> Right, and these exact problems are what the component helper is
> there to sort out, in a subsystem independent way.
> 
> What is the problem with the component helper that people seem to
> be soo loathed to use it, instead preferring to come up with sub-
> standard and broken alternatives?

I think the answer to that is rather obvious. If you design with these
components from the get-go, I see no problem with them, but it simply
seems way easier to retrofit device-links. Just take a look at my
untested patch and patch v3 2/2 from Jyri [1] for panels (that presumably
fix the big issue, namely leaving wild pointers). They either don't touch
neither suppliers nor consumers or are totally trivial (assigning a new
.owner field in suppliers). Compare that with adding a couple of dozen
boilerplate lines with hook functions etc to each and every drm_device,
drm_panel and drm_bridge.

Couple that with the fact that apparently the problem of unbinding
and leaving wild pointers hasn't been all that prevalent, implying that
the problem of rebinding can't be all that critical either.

But what do I know?

Cheers,
Peter

[1] I can't seem to find it in archives, so I'm including it here for
reference. It's small enough.

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 29d2c74..7474045 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -24,6 +24,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 
+#include <drm/drm_device.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_panel.h>
 
@@ -101,6 +102,13 @@ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
 	if (panel->connector)
 		return -EBUSY;
 
+	panel->link = device_link_add(connector->dev->dev, panel->dev, 0);
+	if (!panel->link) {
+		dev_err(panel->dev, "failed to link panel to %s\n",
+			dev_name(connector->dev->dev));
+		return -EINVAL;
+	}
+
 	panel->connector = connector;
 	panel->drm = connector->dev;
 
@@ -123,6 +131,8 @@ EXPORT_SYMBOL(drm_panel_attach);
  */
 int drm_panel_detach(struct drm_panel *panel)
 {
+	device_link_del(panel->link);
+
 	panel->connector = NULL;
 	panel->drm = NULL;
 
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 14ac240..26a1b5f 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -89,6 +89,7 @@ struct drm_panel {
 	struct drm_device *drm;
 	struct drm_connector *connector;
 	struct device *dev;
+	struct device_link *link;
 
 	const struct drm_panel_funcs *funcs;
 

[RFC PATCH v4 8/8] drm/tilcdc: decomponentize now that tda998x is a bridge

[Peter Rosin]

The driver only registered as a component master when connected
to a tda998x device, but that driver is now not only a component
but also a drm_bridge. Since drm_bridges are also handled, it
serves no purpose to register as a component master for the
special case of tda998x (which is no longer special).

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c     | 11 ------
 drivers/gpu/drm/tilcdc/tilcdc_drv.c      | 67 +++-----------------------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h      |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_external.c | 51 ------------------------
 drivers/gpu/drm/tilcdc/tilcdc_external.h |  4 +-
 5 files changed, 6 insertions(+), 128 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 8bf6bb93dc79..b410939a3458 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -1030,17 +1030,6 @@ int tilcdc_crtc_create(struct drm_device *dev)
 		goto fail;
 
 	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
-
-	if (priv->is_componentized) {
-		crtc->port = of_graph_get_port_by_id(dev->dev->of_node, 0);
-		if (!crtc->port) { /* This should never happen */
-			dev_err(dev->dev, "Port node not found in %pOF\n",
-				dev->dev->of_node);
-			ret = -EINVAL;
-			goto fail;
-		}
-	}
-
 	priv->crtc = crtc;
 	return 0;
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 1afde61f1247..d9d2e86c9430 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -17,7 +17,6 @@
 
 /* LCDC DRM driver, based on da8xx-fb */
 
-#include <linux/component.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/suspend.h>
 #include <drm/drm_atomic.h>
@@ -246,9 +245,6 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 	platform_set_drvdata(pdev, ddev);
 	drm_mode_config_init(ddev);
 
-	priv->is_componentized =
-		tilcdc_get_external_components(dev, NULL) > 0;
-
 	priv->wq = alloc_ordered_workqueue("tilcdc", 0);
 	if (!priv->wq) {
 		ret = -ENOMEM;
@@ -363,19 +359,9 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 	}
 	modeset_init(ddev);
 
-	if (priv->is_componentized) {
-		ret = component_bind_all(dev, ddev);
-		if (ret < 0)
-			goto init_failed;
-
-		ret = tilcdc_add_component_encoder(ddev);
-		if (ret < 0)
-			goto init_failed;
-	} else {
-		ret = tilcdc_attach_external_device(ddev);
-		if (ret)
-			goto init_failed;
-	}
+	ret = tilcdc_attach_external_device(ddev);
+	if (ret)
+		goto init_failed;
 
 	if (!priv->external_connector &&
 	    ((priv->num_encoders == 0) || (priv->num_connectors == 0))) {
@@ -586,63 +572,20 @@ static const struct dev_pm_ops tilcdc_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(tilcdc_pm_suspend, tilcdc_pm_resume)
 };
 
-/*
- * Platform driver:
- */
-static int tilcdc_bind(struct device *dev)
-{
-	return tilcdc_init(&tilcdc_driver, dev);
-}
-
-static void tilcdc_unbind(struct device *dev)
-{
-	struct drm_device *ddev = dev_get_drvdata(dev);
-
-	/* Check if a subcomponent has already triggered the unloading. */
-	if (!ddev->dev_private)
-		return;
-
-	tilcdc_fini(dev_get_drvdata(dev));
-}
-
-static const struct component_master_ops tilcdc_comp_ops = {
-	.bind = tilcdc_bind,
-	.unbind = tilcdc_unbind,
-};
-
 static int tilcdc_pdev_probe(struct platform_device *pdev)
 {
-	struct component_match *match = NULL;
-	int ret;
-
 	/* bail out early if no DT data: */
 	if (!pdev->dev.of_node) {
 		dev_err(&pdev->dev, "device-tree data is missing\n");
 		return -ENXIO;
 	}
 
-	ret = tilcdc_get_external_components(&pdev->dev, &match);
-	if (ret < 0)
-		return ret;
-	else if (ret == 0)
-		return tilcdc_init(&tilcdc_driver, &pdev->dev);
-	else
-		return component_master_add_with_match(&pdev->dev,
-						       &tilcdc_comp_ops,
-						       match);
+	return tilcdc_init(&tilcdc_driver, &pdev->dev);
 }
 
 static int tilcdc_pdev_remove(struct platform_device *pdev)
 {
-	int ret;
-
-	ret = tilcdc_get_external_components(&pdev->dev, NULL);
-	if (ret < 0)
-		return ret;
-	else if (ret == 0)
-		tilcdc_fini(platform_get_drvdata(pdev));
-	else
-		component_master_del(&pdev->dev, &tilcdc_comp_ops);
+	tilcdc_fini(platform_get_drvdata(pdev));
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index ead512216669..c47a2b6e2a3a 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -92,7 +92,6 @@ struct tilcdc_drm_private {
 	const struct drm_connector_helper_funcs *connector_funcs;
 
 	bool is_registered;
-	bool is_componentized;
 };
 
 /* Sub-module for display.  Since we don't know at compile time what panels
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
index 711c7b3289d3..1471f5f8cb5c 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
@@ -8,7 +8,6 @@
  *
  */
 
-#include <linux/component.h>
 #include <linux/of_graph.h>
 #include <drm/drm_of.h>
 
@@ -116,33 +115,6 @@ struct drm_connector *tilcdc_encoder_find_connector(struct drm_device *ddev,
 	return NULL;
 }
 
-int tilcdc_add_component_encoder(struct drm_device *ddev)
-{
-	struct tilcdc_drm_private *priv = ddev->dev_private;
-	struct drm_connector *connector;
-	struct drm_encoder *encoder;
-
-	list_for_each_entry(encoder, &ddev->mode_config.encoder_list, head)
-		if (encoder->possible_crtcs & (1 << priv->crtc->index))
-			break;
-
-	if (!encoder) {
-		dev_err(ddev->dev, "%s: No suitable encoder found\n", __func__);
-		return -ENODEV;
-	}
-
-	connector = tilcdc_encoder_find_connector(ddev, encoder);
-
-	if (!connector)
-		return -ENODEV;
-
-	/* Only tda998x is supported at the moment. */
-	tilcdc_crtc_set_simulate_vesa_sync(priv->crtc, true);
-	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_tda998x);
-
-	return tilcdc_add_external_connector(ddev, connector);
-}
-
 void tilcdc_remove_external_device(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;
@@ -221,26 +193,3 @@ int tilcdc_attach_external_device(struct drm_device *ddev)
 
 	return ret;
 }
-
-static int dev_match_of(struct device *dev, void *data)
-{
-	return dev->of_node == data;
-}
-
-int tilcdc_get_external_components(struct device *dev,
-				   struct component_match **match)
-{
-	struct device_node *node;
-
-	node = of_graph_get_remote_node(dev->of_node, 0, 0);
-
-	if (!of_device_is_compatible(node, "nxp,tda998x")) {
-		of_node_put(node);
-		return 0;
-	}
-
-	if (match)
-		drm_of_component_match_add(dev, match, dev_match_of, node);
-	of_node_put(node);
-	return 1;
-}
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.h b/drivers/gpu/drm/tilcdc/tilcdc_external.h
index 763d18f006c7..1df237e227c9 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.h
@@ -18,9 +18,7 @@
 #ifndef __TILCDC_EXTERNAL_H__
 #define __TILCDC_EXTERNAL_H__
 
-int tilcdc_add_component_encoder(struct drm_device *dev);
 void tilcdc_remove_external_device(struct drm_device *dev);
-int tilcdc_get_external_components(struct device *dev,
-				   struct component_match **match);
 int tilcdc_attach_external_device(struct drm_device *ddev);
+
 #endif /* __TILCDC_SLAVE_H__ */
-- 
2.11.0