On 07.07.2019 20:07, Laurent Pinchart wrote:

The drm_display_info structure contains many fields related to HDMI sinks, but none that identifies if a sink compliant with CEA-861 (EDID) shall be treated as an HDMI sink or a DVI sink. Add such a flag, and populate it according to section 8.3.3 ("DVI/HDMI Device Discrimination") of the HDMI v1.3 specification.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

It looks like it can replace drm_detect_hdmi_monitor usage in most cases.

Reviewed-by: Andrzej Hajda a.hajda@samsung.com

-- Regards Andrzej

---

drivers/gpu/drm/drm_edid.c | 3 +++ include/drm/drm_connector.h | 5 +++++ 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 82a4ceed3fcf..d2e7a5334c3f 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4559,6 +4559,8 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db) struct drm_display_info *info = &connector->display_info; u8 len = cea_db_payload_len(db);

• info->is_hdmi = true;
• if (len >= 6) info->dvi_dual = db[6] & 1; if (len >= 7)

@@ -4627,6 +4629,7 @@ drm_reset_display_info(struct drm_connector *connector) info->cea_rev = 0; info->max_tmds_clock = 0; info->dvi_dual = false;

• info->is_hdmi = false; info->has_hdmi_infoframe = false; info->rgb_quant_range_selectable = false; memset(&info->hdmi, 0, sizeof(info->hdmi));

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index ca745d9feaf5..e80ca0d149e5 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -426,6 +426,11 @@ struct drm_display_info { */ bool dvi_dual;

• /**

• * @is_hdmi: True if the sink is an HDMI device.
  

• */
  

• bool is_hdmi;
• /**
  • @has_hdmi_infoframe: Does the sink support the HDMI infoframe?
  */

On Sun, Jul 07, 2019 at 09:07:53PM +0300, Laurent Pinchart wrote:

The drm_display_info structure contains many fields related to HDMI sinks, but none that identifies if a sink compliant with CEA-861 (EDID) shall be treated as an HDMI sink or a DVI sink. Add such a flag, and populate it according to section 8.3.3 ("DVI/HDMI Device Discrimination") of the HDMI v1.3 specification.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

drivers/gpu/drm/drm_edid.c | 3 +++ include/drm/drm_connector.h | 5 +++++ 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 82a4ceed3fcf..d2e7a5334c3f 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4559,6 +4559,8 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db) struct drm_display_info *info = &connector->display_info; u8 len = cea_db_payload_len(db);

• info->is_hdmi = true;

Almost missed this one since it was hidden inside a rather big series.

I was pondering if we should set this in drm_parse_hdmi_forum_vsdb() too, but looks like the spec says we don't have to: "An H14b VSDB shall always be included, regardless of the inclusion of an HF-VSDB, to ensure correct functioning of DVI/HDMI discrimination..."

Also we don't check for the HF-VSDB in drm_detect_hdmi_monitor() either. Unfortunately we can't simply replace drm_detect_hdmi_monitor() in i915 with a check for this flag because we populate display_info way too late.

Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com

if (len >= 6) info->dvi_dual = db[6] & 1; if (len >= 7) @@ -4627,6 +4629,7 @@ drm_reset_display_info(struct drm_connector *connector) info->cea_rev = 0; info->max_tmds_clock = 0; info->dvi_dual = false;

• info->is_hdmi = false; info->has_hdmi_infoframe = false; info->rgb_quant_range_selectable = false; memset(&info->hdmi, 0, sizeof(info->hdmi));

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index ca745d9feaf5..e80ca0d149e5 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -426,6 +426,11 @@ struct drm_display_info { */ bool dvi_dual;

• /**

• * @is_hdmi: True if the sink is an HDMI device.
  

• */
  

• bool is_hdmi;
• /**
  • @has_hdmi_infoframe: Does the sink support the HDMI infoframe?
  */

-- Regards,

Laurent Pinchart

---

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

Sorry, forgot to CC Bartlomiej on this patch.

On Sun, Jul 07, 2019 at 09:07:54PM +0300, Laurent Pinchart wrote:

The hdmi_avi_infoframe_init() never needs to return an error, change its return type to void.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

drivers/gpu/drm/drm_edid.c | 5 +---- drivers/video/hdmi.c | 9 ++------- include/linux/hdmi.h | 2 +- 3 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index d2e7a5334c3f..a7d3189ea8f3 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5086,14 +5086,11 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame, const struct drm_display_mode *mode) { enum hdmi_picture_aspect picture_aspect;

• int err;

  if (!frame || !mode) return -EINVAL;

• err = hdmi_avi_infoframe_init(frame);

• if (err < 0)

• return err;
  

• hdmi_avi_infoframe_init(frame);

  if (mode->flags & DRM_MODE_FLAG_DBLCLK) frame->pixel_repeat = 1;

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index b939bc28d886..54fb7cf11d1a 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -56,15 +56,13 @@ static void hdmi_infoframe_set_checksum(void *buffer, size_t size)

• Returns 0 on success or a negative error code on failure.

*/ -int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame) +void hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame) { memset(frame, 0, sizeof(*frame));

frame->type = HDMI_INFOFRAME_TYPE_AVI; frame->version = 2; frame->length = HDMI_AVI_INFOFRAME_SIZE;

• return 0;

} EXPORT_SYMBOL(hdmi_avi_infoframe_init);

@@ -1553,7 +1551,6 @@ static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame, const void *buffer, size_t size) { const u8 *ptr = buffer;

• int ret;

  if (size < HDMI_INFOFRAME_SIZE(AVI)) return -EINVAL;

@@ -1566,9 +1563,7 @@ static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame, if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(AVI)) != 0) return -EINVAL;

• ret = hdmi_avi_infoframe_init(frame);
• if (ret)

• return ret;
  

• hdmi_avi_infoframe_init(frame);

  ptr += HDMI_INFOFRAME_HEADER_SIZE;

diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index 9918a6c910c5..9613d796cfb1 100644 --- a/include/linux/hdmi.h +++ b/include/linux/hdmi.h @@ -207,7 +207,7 @@ struct hdmi_drm_infoframe { u16 max_fall; };

-int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame); +void hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame); ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer, size_t size); ssize_t hdmi_avi_infoframe_pack_only(const struct hdmi_avi_infoframe *frame, -- Regards,

Laurent Pinchart

---

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

On 07.07.2019 20:07, Laurent Pinchart wrote:

The dumb-vga-dac driver is a simple DRM bridge driver for simple VGA DACs that don't require configuration. Other non-VGA bridges fall in a similar category, and would benefit from a common driver. Prepare for this by renaming the internal symbols from dumb-vga-dac to simple-bridge.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Reviewed-by: Andrzej Hajda a.hajda@samsung.com

-- Regards Andrzej

---

drivers/gpu/drm/bridge/dumb-vga-dac.c | 149 +++++++++++++------------- 1 file changed, 75 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c index d32885b906ae..d46e461ae039 100644 --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c @@ -16,7 +16,7 @@ #include <drm/drm_print.h> #include <drm/drm_probe_helper.h>

-struct dumb_vga { +struct simple_bridge { struct drm_bridge bridge; struct drm_connector connector;

@@ -24,28 +24,28 @@ struct dumb_vga { struct regulator *vdd; };

-static inline struct dumb_vga * -drm_bridge_to_dumb_vga(struct drm_bridge *bridge) +static inline struct simple_bridge * +drm_bridge_to_simple_bridge(struct drm_bridge *bridge) {

• return container_of(bridge, struct dumb_vga, bridge);

• return container_of(bridge, struct simple_bridge, bridge);

}

-static inline struct dumb_vga * -drm_connector_to_dumb_vga(struct drm_connector *connector) +static inline struct simple_bridge * +drm_connector_to_simple_bridge(struct drm_connector *connector) {

• return container_of(connector, struct dumb_vga, connector);

• return container_of(connector, struct simple_bridge, connector);

}

-static int dumb_vga_get_modes(struct drm_connector *connector) +static int simple_bridge_get_modes(struct drm_connector *connector) {

• struct dumb_vga *vga = drm_connector_to_dumb_vga(connector);

• struct simple_bridge *sbridge = drm_connector_to_simple_bridge(connector); struct edid *edid; int ret;

• if (IS_ERR(vga->ddc))

• if (IS_ERR(sbridge->ddc)) goto fallback;

• edid = drm_get_edid(connector, vga->ddc);

• edid = drm_get_edid(connector, sbridge->ddc); if (!edid) { DRM_INFO("EDID readout failed, falling back to standard modes\n"); goto fallback;

@@ -69,14 +69,14 @@ static int dumb_vga_get_modes(struct drm_connector *connector) return ret; }

-static const struct drm_connector_helper_funcs dumb_vga_con_helper_funcs = {

• .get_modes = dumb_vga_get_modes,

+static const struct drm_connector_helper_funcs simple_bridge_con_helper_funcs = {

• .get_modes = simple_bridge_get_modes,

};

static enum drm_connector_status -dumb_vga_connector_detect(struct drm_connector *connector, bool force) +simple_bridge_connector_detect(struct drm_connector *connector, bool force) {

• struct dumb_vga *vga = drm_connector_to_dumb_vga(connector);

• struct simple_bridge *sbridge = drm_connector_to_simple_bridge(connector);

  /*

  • Even if we have an I2C bus, we can't assume that the cable

@@ -84,14 +84,14 @@ dumb_vga_connector_detect(struct drm_connector *connector, bool force) * wire the DDC pins, or the I2C bus might not be working at * all. */

• if (!IS_ERR(vga->ddc) && drm_probe_ddc(vga->ddc))

• if (!IS_ERR(sbridge->ddc) && drm_probe_ddc(sbridge->ddc)) return connector_status_connected;

  return connector_status_unknown;

}

-static const struct drm_connector_funcs dumb_vga_con_funcs = {

• .detect = dumb_vga_connector_detect,

+static const struct drm_connector_funcs simple_bridge_con_funcs = {

• .detect = simple_bridge_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = drm_connector_cleanup, .reset = drm_atomic_helper_connector_reset,

@@ -99,9 +99,9 @@ static const struct drm_connector_funcs dumb_vga_con_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int dumb_vga_attach(struct drm_bridge *bridge) +static int simple_bridge_attach(struct drm_bridge *bridge) {

• struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);

• struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge); int ret;

  if (!bridge->encoder) {

@@ -109,48 +109,49 @@ static int dumb_vga_attach(struct drm_bridge *bridge) return -ENODEV; }

• drm_connector_helper_add(&vga->connector,

• 		 &dumb_vga_con_helper_funcs);
  

• ret = drm_connector_init(bridge->dev, &vga->connector,

• 		 &dumb_vga_con_funcs, DRM_MODE_CONNECTOR_VGA);
  

• drm_connector_helper_add(&sbridge->connector,

• 		 &simple_bridge_con_helper_funcs);
  

• ret = drm_connector_init(bridge->dev, &sbridge->connector,

• 		 &simple_bridge_con_funcs,
  

• 		 DRM_MODE_CONNECTOR_VGA);
  

  if (ret) { DRM_ERROR("Failed to initialize connector\n"); return ret; }

• drm_connector_attach_encoder(&vga->connector,

• drm_connector_attach_encoder(&sbridge->connector, bridge->encoder);

  return 0;

}

-static void dumb_vga_enable(struct drm_bridge *bridge) +static void simple_bridge_enable(struct drm_bridge *bridge) {

• struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);

• struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge); int ret = 0;

• if (vga->vdd)

• ret = regulator_enable(vga->vdd);
  

• if (sbridge->vdd)

• ret = regulator_enable(sbridge->vdd);
  
  

  if (ret) DRM_ERROR("Failed to enable vdd regulator: %d\n", ret);

}

-static void dumb_vga_disable(struct drm_bridge *bridge) +static void simple_bridge_disable(struct drm_bridge *bridge) {

• struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);

• struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge);

• if (vga->vdd)

• regulator_disable(vga->vdd);
  

• if (sbridge->vdd)

• regulator_disable(sbridge->vdd);
  

}

-static const struct drm_bridge_funcs dumb_vga_bridge_funcs = {

• .attach = dumb_vga_attach,
• .enable = dumb_vga_enable,
• .disable = dumb_vga_disable,

+static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {

• .attach = simple_bridge_attach,
• .enable = simple_bridge_enable,
• .disable = simple_bridge_disable,

};

-static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev) +static struct i2c_adapter *simple_bridge_retrieve_ddc(struct device *dev) { struct device_node *phandle, *remote; struct i2c_adapter *ddc; @@ -172,52 +173,52 @@ static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev) return ddc; }

-static int dumb_vga_probe(struct platform_device *pdev) +static int simple_bridge_probe(struct platform_device *pdev) {

• struct dumb_vga *vga;

• struct simple_bridge *sbridge;

• vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL);
• if (!vga)

• sbridge = devm_kzalloc(&pdev->dev, sizeof(*sbridge), GFP_KERNEL);
• if (!sbridge) return -ENOMEM;

• platform_set_drvdata(pdev, vga);

• platform_set_drvdata(pdev, sbridge);

• vga->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
• if (IS_ERR(vga->vdd)) {

• int ret = PTR_ERR(vga->vdd);
  

• sbridge->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
• if (IS_ERR(sbridge->vdd)) {

• int ret = PTR_ERR(sbridge->vdd);
  

  if (ret == -EPROBE_DEFER) return -EPROBE_DEFER;

• vga->vdd = NULL;
  

• sbridge->vdd = NULL;
  

  dev_dbg(&pdev->dev, "No vdd regulator found: %d\n", ret); }

• vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev);
• if (IS_ERR(vga->ddc)) {

• if (PTR_ERR(vga->ddc) == -ENODEV) {
  

• sbridge->ddc = simple_bridge_retrieve_ddc(&pdev->dev);
• if (IS_ERR(sbridge->ddc)) {

• if (PTR_ERR(sbridge->ddc) == -ENODEV) {
  dev_dbg(&pdev->dev,
  	"No i2c bus specified. Disabling EDID readout\n");
  

  } else { dev_err(&pdev->dev, "Couldn't retrieve i2c bus\n");

• 	return PTR_ERR(vga->ddc);
  

• 	return PTR_ERR(sbridge->ddc);
  

  } }

• vga->bridge.funcs = &dumb_vga_bridge_funcs;
• vga->bridge.of_node = pdev->dev.of_node;
• vga->bridge.timings = of_device_get_match_data(&pdev->dev);

• sbridge->bridge.funcs = &simple_bridge_bridge_funcs;
• sbridge->bridge.of_node = pdev->dev.of_node;
• sbridge->bridge.timings = of_device_get_match_data(&pdev->dev);

• drm_bridge_add(&vga->bridge);

• drm_bridge_add(&sbridge->bridge);

  return 0;

}

-static int dumb_vga_remove(struct platform_device *pdev) +static int simple_bridge_remove(struct platform_device *pdev) {

• struct dumb_vga *vga = platform_get_drvdata(pdev);

• struct simple_bridge *sbridge = platform_get_drvdata(pdev);

• drm_bridge_remove(&vga->bridge);

• drm_bridge_remove(&sbridge->bridge);

• if (!IS_ERR(vga->ddc))

• i2c_put_adapter(vga->ddc);
  

• if (!IS_ERR(sbridge->ddc))

• i2c_put_adapter(sbridge->ddc);
  
  

  return 0;

} @@ -228,7 +229,7 @@ static int dumb_vga_remove(struct platform_device *pdev)

• NOTE: the ADV7123EP seems to have other timings and need a new timings
• set if used.

*/ -static const struct drm_bridge_timings default_dac_timings = { +static const struct drm_bridge_timings default_bridge_timings = { /* Timing specifications, datasheet page 7 */ .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, .setup_time_ps = 500, @@ -239,7 +240,7 @@ static const struct drm_bridge_timings default_dac_timings = {

• Information taken from the THS8134, THS8134A, THS8134B datasheet named
• "SLVS205D", dated May 1990, revised March 2000.

*/ -static const struct drm_bridge_timings ti_ths8134_dac_timings = { +static const struct drm_bridge_timings ti_ths8134_bridge_timings = { /* From timing diagram, datasheet page 9 */ .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, /* From datasheet, page 12 */ @@ -252,7 +253,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = {

• Information taken from the THS8135 datasheet named "SLAS343B", dated
• May 2001, revised April 2013.

*/ -static const struct drm_bridge_timings ti_ths8135_dac_timings = { +static const struct drm_bridge_timings ti_ths8135_bridge_timings = { /* From timing diagram, datasheet page 14 */ .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, /* From datasheet, page 16 */ @@ -260,37 +261,37 @@ static const struct drm_bridge_timings ti_ths8135_dac_timings = { .hold_time_ps = 500, };

-static const struct of_device_id dumb_vga_match[] = { +static const struct of_device_id simple_bridge_match[] = { { .compatible = "dumb-vga-dac", .data = NULL, }, { .compatible = "adi,adv7123",

• .data = &default_dac_timings,
  

• .data = &default_bridge_timings,
  

  }, { .compatible = "ti,ths8135",

• .data = &ti_ths8135_dac_timings,
  

• .data = &ti_ths8135_bridge_timings,
  

  }, { .compatible = "ti,ths8134",

• .data = &ti_ths8134_dac_timings,
  

• .data = &ti_ths8134_bridge_timings,
  

  }, {},

}; -MODULE_DEVICE_TABLE(of, dumb_vga_match); +MODULE_DEVICE_TABLE(of, simple_bridge_match);

-static struct platform_driver dumb_vga_driver = {

• .probe = dumb_vga_probe,
• .remove = dumb_vga_remove,

+static struct platform_driver simple_bridge_driver = {

• .probe = simple_bridge_probe,
• .remove = simple_bridge_remove, .driver = { .name = "dumb-vga-dac",

• .of_match_table	= dumb_vga_match,
  

• .of_match_table	= simple_bridge_match,
  

  },

}; -module_platform_driver(dumb_vga_driver); +module_platform_driver(simple_bridge_driver);

MODULE_AUTHOR("Maxime Ripard maxime.ripard@free-electrons.com"); -MODULE_DESCRIPTION("Dumb VGA DAC bridge driver"); +MODULE_DESCRIPTION("Simple DRM bridge driver"); MODULE_LICENSE("GPL");

Create a new simple_bridge_info structure that stores information about the bridge model, and store the bridge timings in there, along with the connector type. Use that new structure for of_device_id data. This enables support for non-VGA bridges.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/bridge/simple-bridge.c | 41 ++++++++++++++++++-------- 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c index da5479bd5878..bff240cf283d 100644 --- a/drivers/gpu/drm/bridge/simple-bridge.c +++ b/drivers/gpu/drm/bridge/simple-bridge.c @@ -16,10 +16,17 @@ #include <drm/drm_print.h> #include <drm/drm_probe_helper.h>

+struct simple_bridge_info { + const struct drm_bridge_timings *timings; + unsigned int type; +}; + struct simple_bridge { struct drm_bridge bridge; struct drm_connector connector;

+ const struct simple_bridge_info *info; + struct i2c_adapter *ddc; struct regulator *vdd; }; @@ -113,7 +120,7 @@ static int simple_bridge_attach(struct drm_bridge *bridge) &simple_bridge_con_helper_funcs); ret = drm_connector_init(bridge->dev, &sbridge->connector, &simple_bridge_con_funcs, - DRM_MODE_CONNECTOR_VGA); + sbridge->info->type); if (ret) { DRM_ERROR("Failed to initialize connector\n"); return ret; @@ -182,6 +189,8 @@ static int simple_bridge_probe(struct platform_device *pdev) return -ENOMEM; platform_set_drvdata(pdev, sbridge);

+ sbridge->info = of_device_get_match_data(&pdev->dev); + sbridge->vdd = devm_regulator_get_optional(&pdev->dev, "vdd"); if (IS_ERR(sbridge->vdd)) { int ret = PTR_ERR(sbridge->vdd); @@ -204,7 +213,7 @@ static int simple_bridge_probe(struct platform_device *pdev)

sbridge->bridge.funcs = &simple_bridge_bridge_funcs; sbridge->bridge.of_node = pdev->dev.of_node; - sbridge->bridge.timings = of_device_get_match_data(&pdev->dev); + sbridge->bridge.timings = sbridge->info->timings;

drm_bridge_add(&sbridge->bridge);

@@ -264,19 +273,27 @@ static const struct drm_bridge_timings ti_ths8135_bridge_timings = { static const struct of_device_id simple_bridge_match[] = { { .compatible = "dumb-vga-dac", - .data = NULL, - }, - { + .data = &(const struct simple_bridge_info) { + .type = DRM_MODE_CONNECTOR_VGA, + }, + }, { .compatible = "adi,adv7123", - .data = &default_bridge_timings, - }, - { + .data = &(const struct simple_bridge_info) { + .timings = &default_bridge_timings, + .type = DRM_MODE_CONNECTOR_VGA, + }, + }, { .compatible = "ti,ths8135", - .data = &ti_ths8135_bridge_timings, - }, - { + .data = &(const struct simple_bridge_info) { + .timings = &ti_ths8135_bridge_timings, + .type = DRM_MODE_CONNECTOR_VGA, + }, + }, { .compatible = "ti,ths8134", - .data = &ti_ths8134_bridge_timings, + .data = &(const struct simple_bridge_info) { + .timings = &ti_ths8134_bridge_timings, + .type = DRM_MODE_CONNECTOR_VGA, + }, }, {}, };

On 2019-07-07 20:18, Laurent Pinchart wrote:

Create a new simple_bridge_info structure that stores information about the bridge model, and store the bridge timings in there, along with the connector type. Use that new structure for of_device_id data. This enables support for non-VGA bridges.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

drivers/gpu/drm/bridge/simple-bridge.c | 41 ++++++++++++++++++-------- 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c index da5479bd5878..bff240cf283d 100644 --- a/drivers/gpu/drm/bridge/simple-bridge.c +++ b/drivers/gpu/drm/bridge/simple-bridge.c @@ -16,10 +16,17 @@ #include <drm/drm_print.h> #include <drm/drm_probe_helper.h>

+struct simple_bridge_info {

• const struct drm_bridge_timings *timings;
• unsigned int type;

How about connector_type? That is how this field is named in other places.

Otherwise looks good to me.

Reviewed-by: Stefan Agner stefan@agner.ch

-- Stefan

+};

struct simple_bridge { struct drm_bridge bridge; struct drm_connector connector;

• const struct simple_bridge_info *info;
• struct i2c_adapter *ddc; struct regulator *vdd;

}; @@ -113,7 +120,7 @@ static int simple_bridge_attach(struct drm_bridge *bridge) &simple_bridge_con_helper_funcs); ret = drm_connector_init(bridge->dev, &sbridge->connector, &simple_bridge_con_funcs,

• 		 DRM_MODE_CONNECTOR_VGA);
  

• 		 sbridge->info->type);
  

  if (ret) { DRM_ERROR("Failed to initialize connector\n"); return ret;

@@ -182,6 +189,8 @@ static int simple_bridge_probe(struct platform_device *pdev) return -ENOMEM; platform_set_drvdata(pdev, sbridge);

• sbridge->info = of_device_get_match_data(&pdev->dev);
• sbridge->vdd = devm_regulator_get_optional(&pdev->dev, "vdd"); if (IS_ERR(sbridge->vdd)) { int ret = PTR_ERR(sbridge->vdd);

@@ -204,7 +213,7 @@ static int simple_bridge_probe(struct platform_device *pdev)

sbridge->bridge.funcs = &simple_bridge_bridge_funcs; sbridge->bridge.of_node = pdev->dev.of_node;

• sbridge->bridge.timings = of_device_get_match_data(&pdev->dev);

• sbridge->bridge.timings = sbridge->info->timings;

  drm_bridge_add(&sbridge->bridge);

@@ -264,19 +273,27 @@ static const struct drm_bridge_timings ti_ths8135_bridge_timings = { static const struct of_device_id simple_bridge_match[] = { { .compatible = "dumb-vga-dac",

• .data = NULL,
  

• },
• {

• .data = &(const struct simple_bridge_info) {
  

• 	.type = DRM_MODE_CONNECTOR_VGA,
  

• },
  

• }, { .compatible = "adi,adv7123",

• .data = &default_bridge_timings,
  

• },
• {

• .data = &(const struct simple_bridge_info) {
  

• 	.timings = &default_bridge_timings,
  

• 	.type = DRM_MODE_CONNECTOR_VGA,
  

• },
  

• }, { .compatible = "ti,ths8135",

• .data = &ti_ths8135_bridge_timings,
  

• },
• {

• .data = &(const struct simple_bridge_info) {
  

• 	.timings = &ti_ths8135_bridge_timings,
  

• 	.type = DRM_MODE_CONNECTOR_VGA,
  

• },
  

• }, { .compatible = "ti,ths8134",

• .data = &ti_ths8134_bridge_timings,
  

• .data = &(const struct simple_bridge_info) {
  

• 	.timings = &ti_ths8134_bridge_timings,
  

• 	.type = DRM_MODE_CONNECTOR_VGA,
  

• },
  

  }, {},

};

On 07.07.2019 20:18, Laurent Pinchart wrote:

If an enable GPIO is declared in the firmware, assert it when enabling the bridge and deassert it when disabling it.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Hmm, simple becomes less simple. I guess we will end-up with sth similar to panel-simple. And then we can merge both :)

Reviewed-by: Andrzej Hajda a.hajda@samsung.com

-- Regards Andrzej

---

drivers/gpu/drm/bridge/simple-bridge.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c index bff240cf283d..a7edf3c39627 100644 --- a/drivers/gpu/drm/bridge/simple-bridge.c +++ b/drivers/gpu/drm/bridge/simple-bridge.c @@ -6,6 +6,7 @@

• Maxime Ripard maxime.ripard@free-electrons.com

*/

+#include <linux/gpio/consumer.h> #include <linux/module.h> #include <linux/of_device.h> #include <linux/of_graph.h> @@ -29,6 +30,7 @@ struct simple_bridge {

struct i2c_adapter *ddc; struct regulator *vdd;

• struct gpio_desc *enable;

};

static inline struct simple_bridge * @@ -135,19 +137,23 @@ static int simple_bridge_attach(struct drm_bridge *bridge) static void simple_bridge_enable(struct drm_bridge *bridge) { struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge);

• int ret = 0;

• int ret;

• if (sbridge->vdd)

• if (sbridge->vdd) { ret = regulator_enable(sbridge->vdd);

• if (ret)
  

• 	DRM_ERROR("Failed to enable vdd regulator: %d\n", ret);
  

• }

• if (ret)

• DRM_ERROR("Failed to enable vdd regulator: %d\n", ret);
  

• gpiod_set_value_cansleep(sbridge->enable, 1);

}

static void simple_bridge_disable(struct drm_bridge *bridge) { struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge);

• gpiod_set_value_cansleep(sbridge->enable, 0);
• if (sbridge->vdd) regulator_disable(sbridge->vdd);

} @@ -200,6 +206,14 @@ static int simple_bridge_probe(struct platform_device *pdev) dev_dbg(&pdev->dev, "No vdd regulator found: %d\n", ret); }

• sbridge->enable = devm_gpiod_get_optional(&pdev->dev, "enable",

• 				  GPIOD_OUT_LOW);
  

• if (IS_ERR(sbridge->enable)) {

• if (PTR_ERR(sbridge->enable) != -EPROBE_DEFER)
  

• 	dev_err(&pdev->dev, "Unable to retrieve enable GPIO\n");
  

• return PTR_ERR(sbridge->enable);
  

• }
• sbridge->ddc = simple_bridge_retrieve_ddc(&pdev->dev); if (IS_ERR(sbridge->ddc)) { if (PTR_ERR(sbridge->ddc) == -ENODEV) {

The TI OP362 is an analog video amplifier controlled through a GPIO. Add support for it to the simple-bridge driver.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/bridge/simple-bridge.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c index a7edf3c39627..7495b9bef865 100644 --- a/drivers/gpu/drm/bridge/simple-bridge.c +++ b/drivers/gpu/drm/bridge/simple-bridge.c @@ -296,6 +296,11 @@ static const struct of_device_id simple_bridge_match[] = { .timings = &default_bridge_timings, .type = DRM_MODE_CONNECTOR_VGA, }, + }, { + .compatible = "ti,opa362", + .data = &(const struct simple_bridge_info) { + .type = DRM_MODE_CONNECTOR_Composite, + }, }, { .compatible = "ti,ths8135", .data = &(const struct simple_bridge_info) {

On 07/07/2019 21:18, Laurent Pinchart wrote:

The TI OP362 is an analog video amplifier controlled through a GPIO. Add support for it to the simple-bridge driver.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

drivers/gpu/drm/bridge/simple-bridge.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c index a7edf3c39627..7495b9bef865 100644 --- a/drivers/gpu/drm/bridge/simple-bridge.c +++ b/drivers/gpu/drm/bridge/simple-bridge.c @@ -296,6 +296,11 @@ static const struct of_device_id simple_bridge_match[] = { .timings = &default_bridge_timings, .type = DRM_MODE_CONNECTOR_VGA, },

• }, {

• .compatible = "ti,opa362",
  

• .data = &(const struct simple_bridge_info) {
  

• 	.type = DRM_MODE_CONNECTOR_Composite,
  

• },
  

I have to say I'm pretty clueless about the analog TV, but OMAP DSS supports also s-video outputs. But I don't know if OPA362 can be used with s-video, or does it dictate composite.

In any case,

Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com

Tomi

Most bridge drivers create a DRM connector to model the connector at the output of the bridge. This model is historical and has worked pretty well so far, but causes several issues:

- It prevents supporting more complex display pipelines where DRM connector operations are split over multiple components. For instance a pipeline with a bridge connected to the DDC signals to read EDID data, and another one connected to the HPD signal to detect connection and disconnection, will not be possible to support through this model.

- It requires every bridge driver to implement similar connector handling code, resulting in code duplication.

- It assumes that a bridge will either be wired to a connector or to another bridge, but doesn't support bridges that can be used in both positions very well (although there is some ad-hoc support for this in the analogix_dp bridge driver).

In order to solve these issues, ownership of the connector should be moved to the display controller driver (where it can be implemented using helpers provided by the core).

Extend the bridge API to allow disabling connector creation in bridge drivers as a first step towards the new model. The new create_connector argument to the bridge .attach() operation tells the bridge driver whether to create a connector. Set the argument to true unconditionally, and modify all existing bridge drivers to return an error when connector creation is not requested as they don't support this feature yet.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/arc/arcpgu_hdmi.c | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 2 +- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++- drivers/gpu/drm/bridge/analogix-anx78xx.c | 6 +++++- drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 ++++++-- drivers/gpu/drm/bridge/cdns-dsi.c | 6 ++++-- drivers/gpu/drm/bridge/lvds-encoder.c | 4 ++-- drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 6 +++++- drivers/gpu/drm/bridge/nxp-ptn3460.c | 6 +++++- drivers/gpu/drm/bridge/panel.c | 5 ++++- drivers/gpu/drm/bridge/parade-ps8622.c | 5 ++++- drivers/gpu/drm/bridge/sii902x.c | 6 +++++- drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- drivers/gpu/drm/bridge/simple-bridge.c | 6 +++++- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 ++++++-- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 8 +++++--- drivers/gpu/drm/bridge/tc358764.c | 5 ++++- drivers/gpu/drm/bridge/tc358767.c | 5 ++++- drivers/gpu/drm/bridge/thc63lvd1024.c | 5 +++-- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 ++++- drivers/gpu/drm/bridge/ti-tfp410.c | 5 ++++- drivers/gpu/drm/drm_bridge.c | 5 +++-- drivers/gpu/drm/drm_simple_kms_helper.c | 2 +- drivers/gpu/drm/exynos/exynos_dp.c | 3 ++- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 ++-- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 2 +- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++-- drivers/gpu/drm/imx/imx-ldb.c | 2 +- drivers/gpu/drm/imx/parallel-display.c | 2 +- drivers/gpu/drm/mcde/mcde_dsi.c | 6 +++++- drivers/gpu/drm/mediatek/mtk_dpi.c | 2 +- drivers/gpu/drm/mediatek/mtk_dsi.c | 2 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 8 ++++++-- drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++-- drivers/gpu/drm/msm/edp/edp_bridge.c | 2 +- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.c | 3 ++- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 2 +- drivers/gpu/drm/rcar-du/rcar_lvds.c | 7 +++++-- drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 +- drivers/gpu/drm/rockchip/rockchip_rgb.c | 2 +- drivers/gpu/drm/sti/sti_dvo.c | 2 +- drivers/gpu/drm/sti/sti_hda.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/stm/ltdc.c | 2 +- drivers/gpu/drm/sun4i/sun4i_lvds.c | 2 +- drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 +- drivers/gpu/drm/tilcdc/tilcdc_external.c | 2 +- drivers/gpu/drm/vc4/vc4_dpi.c | 2 +- drivers/gpu/drm/vc4/vc4_dsi.c | 2 +- include/drm/drm_bridge.h | 4 ++-- 53 files changed, 140 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_hdmi.c b/drivers/gpu/drm/arc/arcpgu_hdmi.c index 98aac743cc26..739f2358f1d5 100644 --- a/drivers/gpu/drm/arc/arcpgu_hdmi.c +++ b/drivers/gpu/drm/arc/arcpgu_hdmi.c @@ -39,7 +39,7 @@ int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np) return ret;

/* Link drm_bridge to encoder */ - ret = drm_bridge_attach(encoder, bridge, NULL); + ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) drm_encoder_cleanup(encoder);

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c index f73d8a92274e..606841d2c0b0 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c @@ -123,7 +123,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint) }

if (bridge) { - ret = drm_bridge_attach(&output->encoder, bridge, NULL); + ret = drm_bridge_attach(&output->encoder, bridge, NULL, true); if (!ret) return 0;

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index f6d2681f6927..c67ba30edec4 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -847,11 +847,15 @@ static void adv7511_bridge_mode_set(struct drm_bridge *bridge, adv7511_mode_set(adv, mode, adj_mode); }

-static int adv7511_bridge_attach(struct drm_bridge *bridge) +static int adv7511_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct adv7511 *adv = bridge_to_adv7511(bridge); int ret;

+ if (!create_connector) + return -EINVAL; + if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV; diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c index 3c7cc5af735c..f72755e59e12 100644 --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c @@ -998,11 +998,15 @@ static const struct drm_connector_funcs anx78xx_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int anx78xx_bridge_attach(struct drm_bridge *bridge) +static int anx78xx_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct anx78xx *anx78xx = bridge_to_anx78xx(bridge); int err;

+ if (!create_connector) + return -EINVAL; + if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV; diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index 3f7f4880be09..f6a1bdcc09d6 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1179,13 +1179,17 @@ static const struct drm_connector_funcs analogix_dp_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int analogix_dp_bridge_attach(struct drm_bridge *bridge) +static int analogix_dp_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct analogix_dp_device *dp = bridge->driver_private; struct drm_encoder *encoder = dp->encoder; struct drm_connector *connector = NULL; int ret = 0;

+ if (!create_connector) + return -EINVAL; + if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV; @@ -1463,7 +1467,7 @@ static int analogix_dp_create_bridge(struct drm_device *drm_dev, bridge->driver_private = dp; bridge->funcs = &analogix_dp_bridge_funcs;

- ret = drm_bridge_attach(dp->encoder, bridge, NULL); + ret = drm_bridge_attach(dp->encoder, bridge, NULL, true); if (ret) { DRM_ERROR("failed to attach drm bridge\n"); return -EINVAL; diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c index 6166dca6be81..45f50852cfbb 100644 --- a/drivers/gpu/drm/bridge/cdns-dsi.c +++ b/drivers/gpu/drm/bridge/cdns-dsi.c @@ -645,7 +645,8 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi, return 0; }

-static int cdns_dsi_bridge_attach(struct drm_bridge *bridge) +static int cdns_dsi_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); struct cdns_dsi *dsi = input_to_dsi(input); @@ -657,7 +658,8 @@ static int cdns_dsi_bridge_attach(struct drm_bridge *bridge) return -ENOTSUPP; }

- return drm_bridge_attach(bridge->encoder, output->bridge, bridge); + return drm_bridge_attach(bridge->encoder, output->bridge, bridge, + create_connector); }

static enum drm_mode_status diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c index 2ab2c234f26c..bafab97521af 100644 --- a/drivers/gpu/drm/bridge/lvds-encoder.c +++ b/drivers/gpu/drm/bridge/lvds-encoder.c @@ -18,14 +18,14 @@ struct lvds_encoder { struct gpio_desc *powerdown_gpio; };

-static int lvds_encoder_attach(struct drm_bridge *bridge) +static int lvds_encoder_attach(struct drm_bridge *bridge, bool create_connector) { struct lvds_encoder *lvds_encoder = container_of(bridge, struct lvds_encoder, bridge);

return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge, - bridge); + bridge, create_connector); }

static void lvds_encoder_enable(struct drm_bridge *bridge) diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c index 79311f8354bd..4250e2235f50 100644 --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c @@ -206,13 +206,17 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id) return IRQ_HANDLED; }

-static int ge_b850v3_lvds_attach(struct drm_bridge *bridge) +static int ge_b850v3_lvds_attach(struct drm_bridge *bridge, + bool create_connector) { struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector; struct i2c_client *stdp4028_i2c = ge_b850v3_lvds_ptr->stdp4028_i2c; int ret;

+ if (!create_connector) + return -EINVAL; + if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV; diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index 98bc650b8c95..6bef439261da 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -238,11 +238,15 @@ static const struct drm_connector_funcs ptn3460_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int ptn3460_bridge_attach(struct drm_bridge *bridge) +static int ptn3460_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge); int ret;

+ if (!create_connector) + return -EINVAL; + if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV; diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index b12ae3a4c5f1..98ad4abf2409 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -52,12 +52,15 @@ static const struct drm_connector_funcs panel_bridge_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int panel_bridge_attach(struct drm_bridge *bridge) +static int panel_bridge_attach(struct drm_bridge *bridge, bool create_connector) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); struct drm_connector *connector = &panel_bridge->connector; int ret;

+ if (!create_connector) + return -EINVAL; + if (!bridge->encoder) { DRM_ERROR("Missing encoder\n"); return -ENODEV; diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c index 2d88146e4836..b9243d51489b 100644 --- a/drivers/gpu/drm/bridge/parade-ps8622.c +++ b/drivers/gpu/drm/bridge/parade-ps8622.c @@ -476,11 +476,14 @@ static const struct drm_connector_funcs ps8622_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int ps8622_attach(struct drm_bridge *bridge) +static int ps8622_attach(struct drm_bridge *bridge, bool create_connector) { struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge); int ret;

+ if (!create_connector) + return -EINVAL; + if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV; diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c index dd7aa466b280..18904b1082b6 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -396,12 +396,16 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, mutex_unlock(&sii902x->mutex); }

-static int sii902x_bridge_attach(struct drm_bridge *bridge) +static int sii902x_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct sii902x *sii902x = bridge_to_sii902x(bridge); struct drm_device *drm = bridge->dev; int ret;

+ if (!create_connector) + return -EINVAL; + drm_connector_helper_add(&sii902x->connector, &sii902x_connector_helper_funcs);

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 0cc293a6ac24..ea6529df7d9c 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -2203,7 +2203,7 @@ static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) return container_of(bridge, struct sii8620, bridge); }

-static int sii8620_attach(struct drm_bridge *bridge) +static int sii8620_attach(struct drm_bridge *bridge, bool create_connector) { struct sii8620 *ctx = bridge_to_sii8620(bridge);

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c index 7495b9bef865..86885eb6e28d 100644 --- a/drivers/gpu/drm/bridge/simple-bridge.c +++ b/drivers/gpu/drm/bridge/simple-bridge.c @@ -108,11 +108,15 @@ static const struct drm_connector_funcs simple_bridge_con_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int simple_bridge_attach(struct drm_bridge *bridge) +static int simple_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge); int ret;

+ if (!create_connector) + return 0; + if (!bridge->encoder) { DRM_ERROR("Missing encoder\n"); return -ENODEV; diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index c6490949d9db..930d67c618dd 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2174,12 +2174,16 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = .get_modes = dw_hdmi_connector_get_modes, };

-static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) +static int dw_hdmi_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct dw_hdmi *hdmi = bridge->driver_private; struct drm_encoder *encoder = bridge->encoder; struct drm_connector *connector = &hdmi->connector;

+ if (!create_connector) + return -EINVAL; + connector->interlace_allowed = 1; connector->polled = DRM_CONNECTOR_POLL_HPD;

@@ -2857,7 +2861,7 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, if (IS_ERR(hdmi)) return hdmi;

- ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL); + ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL, true); if (ret) { dw_hdmi_remove(hdmi); DRM_ERROR("Failed to initialize bridge with drm\n"); diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 281c58bab1a1..05cf97ad524f 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -906,7 +906,8 @@ dw_mipi_dsi_bridge_mode_valid(struct drm_bridge *bridge, return mode_status; }

-static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) +static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);

@@ -919,7 +920,8 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;

/* Attach the panel-bridge to the dsi bridge */ - return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge); + return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge, + create_connector); }

static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = { @@ -1064,7 +1066,7 @@ int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct drm_encoder *encoder) { int ret;

- ret = drm_bridge_attach(encoder, &dsi->bridge, NULL); + ret = drm_bridge_attach(encoder, &dsi->bridge, NULL, true); if (ret) { DRM_ERROR("Failed to initialize bridge with drm\n"); return ret; diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c index 170f162ffa55..6016f3aae42f 100644 --- a/drivers/gpu/drm/bridge/tc358764.c +++ b/drivers/gpu/drm/bridge/tc358764.c @@ -348,12 +348,15 @@ static void tc358764_enable(struct drm_bridge *bridge) dev_err(ctx->dev, "error enabling panel (%d)\n", ret); }

-static int tc358764_attach(struct drm_bridge *bridge) +static int tc358764_attach(struct drm_bridge *bridge, bool create_connector) { struct tc358764 *ctx = bridge_to_tc358764(bridge); struct drm_device *drm = bridge->dev; int ret;

+ if (!create_connector) + return -EINVAL; + ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; ret = drm_connector_init(drm, &ctx->connector, &tc358764_connector_funcs, diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 13ade28a36a8..e2b2d2660adc 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1273,13 +1273,16 @@ static const struct drm_connector_funcs tc_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int tc_bridge_attach(struct drm_bridge *bridge) +static int tc_bridge_attach(struct drm_bridge *bridge, bool create_connector) { u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; struct tc_data *tc = bridge_to_tc(bridge); struct drm_device *drm = bridge->dev; int ret;

+ if (!create_connector) + return -EINVAL; + /* Create DP/eDP connector */ drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs); ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs, diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c index 3d74129b2995..86f3b96f95db 100644 --- a/drivers/gpu/drm/bridge/thc63lvd1024.c +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c @@ -42,11 +42,12 @@ static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) return container_of(bridge, struct thc63_dev, bridge); }

-static int thc63_attach(struct drm_bridge *bridge) +static int thc63_attach(struct drm_bridge *bridge, bool create_connector) { struct thc63_dev *thc63 = to_thc63(bridge);

- return drm_bridge_attach(bridge->encoder, thc63->next, bridge); + return drm_bridge_attach(bridge->encoder, thc63->next, bridge, + create_connector); }

static enum drm_mode_status thc63_mode_valid(struct drm_bridge *bridge, diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index b77a52d05061..dbe265f7112d 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -224,7 +224,7 @@ static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata) pdata->supplies); }

-static int ti_sn_bridge_attach(struct drm_bridge *bridge) +static int ti_sn_bridge_attach(struct drm_bridge *bridge, bool create_connector) { int ret, val; struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); @@ -235,6 +235,9 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge) .node = NULL, };

+ if (!create_connector) + return -EINVAL; + ret = drm_connector_init(bridge->dev, &pdata->connector, &ti_sn_bridge_connector_funcs, DRM_MODE_CONNECTOR_eDP); diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c index 4e76b2b27374..8d4690e436c3 100644 --- a/drivers/gpu/drm/bridge/ti-tfp410.c +++ b/drivers/gpu/drm/bridge/ti-tfp410.c @@ -121,11 +121,14 @@ static const struct drm_connector_funcs tfp410_con_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int tfp410_attach(struct drm_bridge *bridge) +static int tfp410_attach(struct drm_bridge *bridge, bool create_connector) { struct tfp410 *dvi = drm_bridge_to_tfp410(bridge); int ret;

+ if (!create_connector) + return -EINVAL; + if (!bridge->encoder) { dev_err(dvi->dev, "Missing encoder\n"); return -ENODEV; diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..519577f363e3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -95,6 +95,7 @@ EXPORT_SYMBOL(drm_bridge_remove); * @encoder: DRM encoder * @bridge: bridge to attach * @previous: previous bridge in the chain (optional) + * @create_connector: true if the bridge should create a drm_connector * * Called by a kms driver to link the bridge to an encoder's chain. The previous * argument specifies the previous bridge in the chain. If NULL, the bridge is @@ -112,7 +113,7 @@ EXPORT_SYMBOL(drm_bridge_remove); * Zero on success, error code on failure */ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, - struct drm_bridge *previous) + struct drm_bridge *previous, bool create_connector) { int ret;

@@ -129,7 +130,7 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = encoder;

if (bridge->funcs->attach) { - ret = bridge->funcs->attach(bridge); + ret = bridge->funcs->attach(bridge, create_connector); if (ret < 0) { bridge->dev = NULL; bridge->encoder = NULL; diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index b11910f14c46..a367ef1e5081 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -228,7 +228,7 @@ static const struct drm_plane_funcs drm_simple_kms_plane_funcs = { int drm_simple_display_pipe_attach_bridge(struct drm_simple_display_pipe *pipe, struct drm_bridge *bridge) { - return drm_bridge_attach(&pipe->encoder, bridge, NULL); + return drm_bridge_attach(&pipe->encoder, bridge, NULL, true); } EXPORT_SYMBOL(drm_simple_display_pipe_attach_bridge);

diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c index 3a0f0ba8c63a..02b98e6ca52d 100644 --- a/drivers/gpu/drm/exynos/exynos_dp.c +++ b/drivers/gpu/drm/exynos/exynos_dp.c @@ -105,7 +105,8 @@ static int exynos_dp_bridge_attach(struct analogix_dp_plat_data *plat_data,

/* Pre-empt DP connector creation if there's a bridge */ if (dp->ptn_bridge) { - ret = drm_bridge_attach(&dp->encoder, dp->ptn_bridge, bridge); + ret = drm_bridge_attach(&dp->encoder, dp->ptn_bridge, bridge, + true); if (ret) { DRM_DEV_ERROR(dp->dev, "Failed to attach bridge to drm\n"); diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 5f6f523821a2..768acc79d10c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1522,7 +1522,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,

out_bridge = of_drm_find_bridge(device->dev.of_node); if (out_bridge) { - drm_bridge_attach(encoder, out_bridge, NULL); + drm_bridge_attach(encoder, out_bridge, NULL, true); dsi->out_bridge = out_bridge; encoder->bridge = NULL; } else { @@ -1698,7 +1698,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, if (dsi->in_bridge_node) { in_bridge = of_drm_find_bridge(dsi->in_bridge_node); if (in_bridge) - drm_bridge_attach(encoder, in_bridge, NULL); + drm_bridge_attach(encoder, in_bridge, NULL, true); }

return mipi_dsi_host_register(&dsi->dsi_host); diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index bc1565f1822a..808c98101d56 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -952,7 +952,7 @@ static int hdmi_create_connector(struct drm_encoder *encoder) drm_connector_attach_encoder(connector, encoder);

if (hdata->bridge) { - ret = drm_bridge_attach(encoder, hdata->bridge, NULL); + ret = drm_bridge_attach(encoder, hdata->bridge, NULL, true); if (ret) DRM_DEV_ERROR(hdata->dev, "Failed to attach bridge\n"); } diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c index c49e9e3740f8..8d22618ccf2e 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c @@ -159,5 +159,5 @@ int fsl_dcu_create_outputs(struct fsl_dcu_drm_device *fsl_dev) return fsl_dcu_attach_panel(fsl_dev, panel); }

- return drm_bridge_attach(&fsl_dev->encoder, bridge, NULL); + return drm_bridge_attach(&fsl_dev->encoder, bridge, NULL, true); } diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c index 3d6c45097f51..eac8ec1512ab 100644 --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c @@ -780,7 +780,7 @@ static int dsi_bridge_init(struct drm_device *dev, struct dw_dsi *dsi) int ret;

/* associate the bridge to dsi encoder */ - ret = drm_bridge_attach(encoder, bridge, NULL); + ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) { DRM_ERROR("failed to attach external bridge\n"); return ret; diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 3d368c43185f..6b2e648b6c4d 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1366,10 +1366,14 @@ static int tda998x_connector_init(struct tda998x_priv *priv,

/* DRM bridge functions */

-static int tda998x_bridge_attach(struct drm_bridge *bridge) +static int tda998x_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);

+ if (!create_connector) + return -EINVAL; + return tda998x_connector_init(priv, bridge->dev); }

@@ -2033,7 +2037,7 @@ static int tda998x_encoder_init(struct device *dev, struct drm_device *drm) if (ret) goto err_encoder;

- ret = drm_bridge_attach(&priv->encoder, &priv->bridge, NULL); + ret = drm_bridge_attach(&priv->encoder, &priv->bridge, NULL, true); if (ret) goto err_bridge;

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index 383733302280..845ead25ade7 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -446,7 +446,7 @@ static int imx_ldb_register(struct drm_device *drm,

if (imx_ldb_ch->bridge) { ret = drm_bridge_attach(&imx_ldb_ch->encoder, - imx_ldb_ch->bridge, NULL); + imx_ldb_ch->bridge, NULL, true); if (ret) { DRM_ERROR("Failed to initialize bridge with drm\n"); return ret; diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index 1a76de1e8e7b..cd746592d2a7 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -182,7 +182,7 @@ static int imx_pd_register(struct drm_device *drm, drm_panel_attach(imxpd->panel, &imxpd->connector);

if (imxpd->bridge) { - ret = drm_bridge_attach(encoder, imxpd->bridge, NULL); + ret = drm_bridge_attach(encoder, imxpd->bridge, NULL, true); if (ret < 0) { dev_err(imxpd->dev, "failed to attach bridge: %d\n", ret); diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c index 07f7090d08b3..f2deadc980f8 100644 --- a/drivers/gpu/drm/mcde/mcde_dsi.c +++ b/drivers/gpu/drm/mcde/mcde_dsi.c @@ -817,12 +817,16 @@ mcde_dsi_connector_helper_funcs = { .get_modes = mcde_dsi_get_modes, };

-static int mcde_dsi_bridge_attach(struct drm_bridge *bridge) +static int mcde_dsi_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct mcde_dsi *d = bridge_to_mcde_dsi(bridge); struct drm_device *drm = bridge->dev; int ret;

+ if (!create_connector) + return -EINVAL; + drm_connector_helper_add(&d->connector, &mcde_dsi_connector_helper_funcs);

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c index bacd989cc9aa..1ff27bb17016 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi.c +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c @@ -604,7 +604,7 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data) /* Currently DPI0 is fixed to be driven by OVL1 */ dpi->encoder.possible_crtcs = BIT(1);

- ret = drm_bridge_attach(&dpi->encoder, dpi->bridge, NULL); + ret = drm_bridge_attach(&dpi->encoder, dpi->bridge, NULL, true); if (ret) { dev_err(dev, "Failed to attach bridge: %d\n", ret); goto err_cleanup; diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index b91c4616644a..9c5bac48b44f 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -819,7 +819,7 @@ static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi)

/* If there's a bridge, attach to it and let it create the connector */ if (dsi->bridge) { - ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL); + ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL, true); if (ret) { DRM_ERROR("Failed to attach bridge to drm\n"); goto err_encoder_cleanup; diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index 5d6a9f094df5..d3248a881cf0 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1290,11 +1290,15 @@ static void mtk_hdmi_hpd_event(bool hpd, struct device *dev) * Bridge callbacks */

-static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge) +static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge, + bool create_connector) { struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge); int ret;

+ if (!create_connector) + return -EINVAL; + ret = drm_connector_init(bridge->encoder->dev, &hdmi->conn, &mtk_hdmi_connector_funcs, DRM_MODE_CONNECTOR_HDMIA); @@ -1318,7 +1322,7 @@ static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge)

if (hdmi->next_bridge) { ret = drm_bridge_attach(bridge->encoder, hdmi->next_bridge, - bridge); + bridge, create_connector); if (ret) { dev_err(hdmi->dev, "Failed to attach external bridge: %d\n", ret); diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index 271aa7bbca92..ca733086041a 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -664,7 +664,7 @@ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id) bridge = &dsi_bridge->base; bridge->funcs = &dsi_mgr_bridge_funcs;

- ret = drm_bridge_attach(encoder, bridge, NULL); + ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) goto fail;

@@ -693,7 +693,7 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id) encoder = msm_dsi->encoder;

/* link the internal dsi bridge to the external bridge */ - drm_bridge_attach(encoder, ext_bridge, int_bridge); + drm_bridge_attach(encoder, ext_bridge, int_bridge, true);

/* * we need the drm_connector created by the external bridge diff --git a/drivers/gpu/drm/msm/edp/edp_bridge.c b/drivers/gpu/drm/msm/edp/edp_bridge.c index 2950bba4aca9..32a463c84cc1 100644 --- a/drivers/gpu/drm/msm/edp/edp_bridge.c +++ b/drivers/gpu/drm/msm/edp/edp_bridge.c @@ -91,7 +91,7 @@ struct drm_bridge *msm_edp_bridge_init(struct msm_edp *edp) bridge = &edp_bridge->base; bridge->funcs = &edp_bridge_funcs;

- ret = drm_bridge_attach(edp->encoder, bridge, NULL); + ret = drm_bridge_attach(edp->encoder, bridge, NULL, true); if (ret) goto fail;

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c index 03197b8959ba..d7738aafcff8 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c @@ -296,7 +296,7 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi) bridge = &hdmi_bridge->base; bridge->funcs = &msm_hdmi_bridge_funcs;

- ret = drm_bridge_attach(hdmi->encoder, bridge, NULL); + ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, true); if (ret) goto fail;

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 672e0f8ad11c..837d0cd20dd1 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -301,7 +301,8 @@ static int omap_modeset_init(struct drm_device *dev)

if (pipe->output->bridge) { ret = drm_bridge_attach(pipe->encoder, - pipe->output->bridge, NULL); + pipe->output->bridge, NULL, + true); if (ret < 0) return ret; } diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index 0f00bdfe2366..74c2ae5ce687 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -120,7 +120,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, * Attach the bridge to the encoder. The bridge will create the * connector. */ - ret = drm_bridge_attach(encoder, bridge, NULL); + ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) { drm_encoder_cleanup(encoder); return ret; diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 1c62578590f4..a8d8b05c4731 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -605,7 +605,7 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge, rcar_lvds_get_lvds_mode(lvds); }

-static int rcar_lvds_attach(struct drm_bridge *bridge) +static int rcar_lvds_attach(struct drm_bridge *bridge, bool create_connector) { struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge); struct drm_connector *connector = &lvds->connector; @@ -615,7 +615,10 @@ static int rcar_lvds_attach(struct drm_bridge *bridge) /* If we have a next bridge just attach it. */ if (lvds->next_bridge) return drm_bridge_attach(bridge->encoder, lvds->next_bridge, - bridge); + bridge, create_connector); + + if (!create_connector) + return -EINVAL;

/* Otherwise if we have a panel, create a connector. */ if (!lvds->panel) diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c index 830858a809e5..7ca412d294b2 100644 --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c @@ -440,7 +440,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, goto err_free_connector; } } else { - ret = drm_bridge_attach(encoder, lvds->bridge, NULL); + ret = drm_bridge_attach(encoder, lvds->bridge, NULL, true); if (ret) { DRM_DEV_ERROR(drm_dev->dev, "failed to attach bridge: %d\n", ret); diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c index ce4d82d293e4..8218bbd09a72 100644 --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c @@ -143,7 +143,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,

rgb->bridge = bridge;

- ret = drm_bridge_attach(encoder, rgb->bridge, NULL); + ret = drm_bridge_attach(encoder, rgb->bridge, NULL, true); if (ret) { DRM_DEV_ERROR(drm_dev->dev, "failed to attach bridge: %d\n", ret); diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c index 9e6d5d8b7030..f09209621568 100644 --- a/drivers/gpu/drm/sti/sti_dvo.c +++ b/drivers/gpu/drm/sti/sti_dvo.c @@ -468,7 +468,7 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data) bridge->of_node = dvo->dev.of_node; drm_bridge_add(bridge);

- err = drm_bridge_attach(encoder, bridge, NULL); + err = drm_bridge_attach(encoder, bridge, NULL, true); if (err) { DRM_ERROR("Failed to attach bridge\n"); return err; diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c index 94e404f13234..87e0fb742dc8 100644 --- a/drivers/gpu/drm/sti/sti_hda.c +++ b/drivers/gpu/drm/sti/sti_hda.c @@ -700,7 +700,7 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)

bridge->driver_private = hda; bridge->funcs = &sti_hda_bridge_funcs; - drm_bridge_attach(encoder, bridge, NULL); + drm_bridge_attach(encoder, bridge, NULL, true);

connector->encoder = encoder;

diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c index f03d617edc4c..8c0ffe6833f9 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -1276,7 +1276,7 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)

bridge->driver_private = hdmi; bridge->funcs = &sti_hdmi_bridge_funcs; - drm_bridge_attach(encoder, bridge, NULL); + drm_bridge_attach(encoder, bridge, NULL, true);

connector->encoder = encoder;

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 2fe6c4a8d915..10a9f848c5f6 100644 --- a/drivers/gpu/drm/stm/ltdc.c +++ b/drivers/gpu/drm/stm/ltdc.c @@ -1053,7 +1053,7 @@ static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge *bridge) drm_encoder_init(ddev, encoder, &ltdc_encoder_funcs, DRM_MODE_ENCODER_DPI, NULL);

- ret = drm_bridge_attach(encoder, bridge, NULL); + ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) { drm_encoder_cleanup(encoder); return -EINVAL; diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c index 3a3ba99fed22..3e5170fa1e67 100644 --- a/drivers/gpu/drm/sun4i/sun4i_lvds.c +++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c @@ -155,7 +155,7 @@ int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon) }

if (bridge) { - ret = drm_bridge_attach(encoder, bridge, NULL); + ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) { dev_err(drm->dev, "Couldn't attach our bridge\n"); goto err_cleanup_connector; diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index a901ec689b62..3f8629445fbb 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -252,7 +252,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon) }

if (rgb->bridge) { - ret = drm_bridge_attach(encoder, rgb->bridge, NULL); + ret = drm_bridge_attach(encoder, rgb->bridge, NULL, true); if (ret) { dev_err(drm->dev, "Couldn't attach our bridge\n"); goto err_cleanup_connector; diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c index e9969cd36610..ec693c11e455 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_external.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c @@ -168,7 +168,7 @@ int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)

priv->external_encoder->possible_crtcs = BIT(0);

- ret = drm_bridge_attach(priv->external_encoder, bridge, NULL); + ret = drm_bridge_attach(priv->external_encoder, bridge, NULL, true); if (ret) { dev_err(ddev->dev, "drm_bridge_attach() failed %d\n", ret); return ret; diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c index 34f90ca8f479..2d7c5cf0d468 100644 --- a/drivers/gpu/drm/vc4/vc4_dpi.c +++ b/drivers/gpu/drm/vc4/vc4_dpi.c @@ -262,7 +262,7 @@ static int vc4_dpi_init_bridge(struct vc4_dpi *dpi) if (panel) bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DPI);

- return drm_bridge_attach(dpi->encoder, bridge, NULL); + return drm_bridge_attach(dpi->encoder, bridge, NULL, true); }

static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index 2ea4e20b7b8a..3edd7ffc7383 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -1607,7 +1607,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) DRM_MODE_ENCODER_DSI, NULL); drm_encoder_helper_add(dsi->encoder, &vc4_dsi_encoder_helper_funcs);

- ret = drm_bridge_attach(dsi->encoder, dsi->bridge, NULL); + ret = drm_bridge_attach(dsi->encoder, dsi->bridge, NULL, true); if (ret) { dev_err(dev, "bridge attach failed: %d\n", ret); return ret; diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 7616f6562fe4..08dc15f93ded 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -48,7 +48,7 @@ struct drm_bridge_funcs { * * Zero on success, error code on failure. */ - int (*attach)(struct drm_bridge *bridge); + int (*attach)(struct drm_bridge *bridge, bool create_connector);

/** * @detach: @@ -404,7 +404,7 @@ void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, - struct drm_bridge *previous); + struct drm_bridge *previous, bool create_connector);

bool drm_bridge_mode_fixup(struct drm_bridge *bridge, const struct drm_display_mode *mode,

Hi Andrzej,

On Wed, Jul 17, 2019 at 08:39:47AM +0200, Andrzej Hajda wrote:

On 07.07.2019 20:18, Laurent Pinchart wrote:

...

Most bridge drivers create a DRM connector to model the connector at the output of the bridge. This model is historical and has worked pretty well so far, but causes several issues:

• It prevents supporting more complex display pipelines where DRM

connector operations are split over multiple components. For instance a pipeline with a bridge connected to the DDC signals to read EDID data, and another one connected to the HPD signal to detect connection and disconnection, will not be possible to support through this model.

• It requires every bridge driver to implement similar connector

handling code, resulting in code duplication.

• It assumes that a bridge will either be wired to a connector or to

another bridge, but doesn't support bridges that can be used in both positions very well (although there is some ad-hoc support for this in the analogix_dp bridge driver).

In order to solve these issues, ownership of the connector should be moved to the display controller driver (where it can be implemented using helpers provided by the core).

Extend the bridge API to allow disabling connector creation in bridge drivers as a first step towards the new model. The new create_connector argument to the bridge .attach() operation tells the bridge driver whether to create a connector. Set the argument to true unconditionally, and modify all existing bridge drivers to return an error when connector creation is not requested as they don't support this feature yet.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

drivers/gpu/drm/arc/arcpgu_hdmi.c | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 2 +- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++- drivers/gpu/drm/bridge/analogix-anx78xx.c | 6 +++++- drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 ++++++-- drivers/gpu/drm/bridge/cdns-dsi.c | 6 ++++-- drivers/gpu/drm/bridge/lvds-encoder.c | 4 ++-- drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 6 +++++- drivers/gpu/drm/bridge/nxp-ptn3460.c | 6 +++++- drivers/gpu/drm/bridge/panel.c | 5 ++++- drivers/gpu/drm/bridge/parade-ps8622.c | 5 ++++- drivers/gpu/drm/bridge/sii902x.c | 6 +++++- drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- drivers/gpu/drm/bridge/simple-bridge.c | 6 +++++- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 ++++++-- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 8 +++++--- drivers/gpu/drm/bridge/tc358764.c | 5 ++++- drivers/gpu/drm/bridge/tc358767.c | 5 ++++- drivers/gpu/drm/bridge/thc63lvd1024.c | 5 +++-- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 ++++- drivers/gpu/drm/bridge/ti-tfp410.c | 5 ++++- drivers/gpu/drm/drm_bridge.c | 5 +++-- drivers/gpu/drm/drm_simple_kms_helper.c | 2 +- drivers/gpu/drm/exynos/exynos_dp.c | 3 ++- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 ++-- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 2 +- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++-- drivers/gpu/drm/imx/imx-ldb.c | 2 +- drivers/gpu/drm/imx/parallel-display.c | 2 +- drivers/gpu/drm/mcde/mcde_dsi.c | 6 +++++- drivers/gpu/drm/mediatek/mtk_dpi.c | 2 +- drivers/gpu/drm/mediatek/mtk_dsi.c | 2 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 8 ++++++-- drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++-- drivers/gpu/drm/msm/edp/edp_bridge.c | 2 +- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.c | 3 ++- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 2 +- drivers/gpu/drm/rcar-du/rcar_lvds.c | 7 +++++-- drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 +- drivers/gpu/drm/rockchip/rockchip_rgb.c | 2 +- drivers/gpu/drm/sti/sti_dvo.c | 2 +- drivers/gpu/drm/sti/sti_hda.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/stm/ltdc.c | 2 +- drivers/gpu/drm/sun4i/sun4i_lvds.c | 2 +- drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 +- drivers/gpu/drm/tilcdc/tilcdc_external.c | 2 +- drivers/gpu/drm/vc4/vc4_dpi.c | 2 +- drivers/gpu/drm/vc4/vc4_dsi.c | 2 +- include/drm/drm_bridge.h | 4 ++-- 53 files changed, 140 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_hdmi.c b/drivers/gpu/drm/arc/arcpgu_hdmi.c index 98aac743cc26..739f2358f1d5 100644 --- a/drivers/gpu/drm/arc/arcpgu_hdmi.c +++ b/drivers/gpu/drm/arc/arcpgu_hdmi.c @@ -39,7 +39,7 @@ int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np) return ret;

/* Link drm_bridge to encoder */

• ret = drm_bridge_attach(encoder, bridge, NULL);

• ret = drm_bridge_attach(encoder, bridge, NULL, true);

Few suggestions:

1. Maybe it would be more convenient to add flags argument instead of bool:

• code should be more readable: ret = drm_bridge_attach(encoder, bridge,

NULL, DRM_BRIDGE_FLAG_NO_CONNECTOR)

• it can be easily expanded later with other flags, there at least two

drivers which would benefit from DRM_BRIDGE_FLAG_NO_CHAINING flag.

Please note that I think this flag should disappear once drivers get converted to the new model. This will however take some time. I'm not opposed to turning the book into a flag though. I was hoping to receive more review comments on this particular patch, but as that's not the case, I can already proceed with the change.

What would the DRM_BRIDGE_FLAG_NO_CHAINING flag be used for ?

2. If the patch can be applied atomically it is OK as is, if not you can

use preprocessor vararg magic to support new and old syntax, sth like:

#define _drm_bridge_attach(encoder, bridge, prev, flags, optarg...) __drm_bridge_attach(encoder, bridge, prev, flags)

#define drm_bridge_attach(encoder, bridge, prev, optarg...) _drm_bridge_attach(encoder, bridge, prev, ##optarg, 0)

Good point. I'll try to do this atomically, but if it fails I'll follow your suggestion.

3. Maybe more convenient would be to just set the flags directly before

attachment:

bridge->dont_create_connector = true;

ret = drm_bridge_attach(encoder, bridge, NULL);

This way it will be still expandable, and less changes.

Bridges that are chained would need to set the dont_create_connector flag of the next bridge. It would be a bit ugly, but would make this patch smaller. On the other hand we would need to keep the if (!create_connector) check in the .attach() handlers, and it would be easier to miss it in bridge drivers (current or new) than with an explicit argument to the .attach() operation. I would thus have a preference for the new argument to .attach(). Especially if it can help you with DRM_BRIDGE_FLAG_NO_CHAINING :-)

...

if (ret) drm_encoder_cleanup(encoder);

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c index f73d8a92274e..606841d2c0b0 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c @@ -123,7 +123,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint) }

if (bridge) {

• ret = drm_bridge_attach(&output->encoder, bridge, NULL);
  

• ret = drm_bridge_attach(&output->encoder, bridge, NULL, true);
  

  if (!ret) return 0;

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index f6d2681f6927..c67ba30edec4 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -847,11 +847,15 @@ static void adv7511_bridge_mode_set(struct drm_bridge *bridge, adv7511_mode_set(adv, mode, adj_mode); }

-static int adv7511_bridge_attach(struct drm_bridge *bridge) +static int adv7511_bridge_attach(struct drm_bridge *bridge,

• 		 bool create_connector)
  

{ struct adv7511 *adv = bridge_to_adv7511(bridge); int ret;

• if (!create_connector)

• return -EINVAL;
  

• if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV;

diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c index 3c7cc5af735c..f72755e59e12 100644 --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c @@ -998,11 +998,15 @@ static const struct drm_connector_funcs anx78xx_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int anx78xx_bridge_attach(struct drm_bridge *bridge) +static int anx78xx_bridge_attach(struct drm_bridge *bridge,

• 		 bool create_connector)
  

{ struct anx78xx *anx78xx = bridge_to_anx78xx(bridge); int err;

• if (!create_connector)

• return -EINVAL;
  

• if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV;

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index 3f7f4880be09..f6a1bdcc09d6 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1179,13 +1179,17 @@ static const struct drm_connector_funcs analogix_dp_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int analogix_dp_bridge_attach(struct drm_bridge *bridge) +static int analogix_dp_bridge_attach(struct drm_bridge *bridge,

• 		     bool create_connector)
  

{ struct analogix_dp_device *dp = bridge->driver_private; struct drm_encoder *encoder = dp->encoder; struct drm_connector *connector = NULL; int ret = 0;

• if (!create_connector)

• return -EINVAL;
  

• if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV;

@@ -1463,7 +1467,7 @@ static int analogix_dp_create_bridge(struct drm_device *drm_dev, bridge->driver_private = dp; bridge->funcs = &analogix_dp_bridge_funcs;

• ret = drm_bridge_attach(dp->encoder, bridge, NULL);

• ret = drm_bridge_attach(dp->encoder, bridge, NULL, true); if (ret) { DRM_ERROR("failed to attach drm bridge\n"); return -EINVAL;

diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c index 6166dca6be81..45f50852cfbb 100644 --- a/drivers/gpu/drm/bridge/cdns-dsi.c +++ b/drivers/gpu/drm/bridge/cdns-dsi.c @@ -645,7 +645,8 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi, return 0; }

-static int cdns_dsi_bridge_attach(struct drm_bridge *bridge) +static int cdns_dsi_bridge_attach(struct drm_bridge *bridge,

• 		  bool create_connector)
  

{ struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); struct cdns_dsi *dsi = input_to_dsi(input); @@ -657,7 +658,8 @@ static int cdns_dsi_bridge_attach(struct drm_bridge *bridge) return -ENOTSUPP; }

• return drm_bridge_attach(bridge->encoder, output->bridge, bridge);

• return drm_bridge_attach(bridge->encoder, output->bridge, bridge,

• 		 create_connector);
  

}

static enum drm_mode_status diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c index 2ab2c234f26c..bafab97521af 100644 --- a/drivers/gpu/drm/bridge/lvds-encoder.c +++ b/drivers/gpu/drm/bridge/lvds-encoder.c @@ -18,14 +18,14 @@ struct lvds_encoder { struct gpio_desc *powerdown_gpio; };

-static int lvds_encoder_attach(struct drm_bridge *bridge) +static int lvds_encoder_attach(struct drm_bridge *bridge, bool create_connector) { struct lvds_encoder *lvds_encoder = container_of(bridge, struct lvds_encoder, bridge);

return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,

• 		 bridge);
  

• 		 bridge, create_connector);
  

}

static void lvds_encoder_enable(struct drm_bridge *bridge) diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c index 79311f8354bd..4250e2235f50 100644 --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c @@ -206,13 +206,17 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id) return IRQ_HANDLED; }

-static int ge_b850v3_lvds_attach(struct drm_bridge *bridge) +static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,

• 		 bool create_connector)
  

{ struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector; struct i2c_client *stdp4028_i2c = ge_b850v3_lvds_ptr->stdp4028_i2c; int ret;

• if (!create_connector)

• return -EINVAL;
  

• if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV;

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index 98bc650b8c95..6bef439261da 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -238,11 +238,15 @@ static const struct drm_connector_funcs ptn3460_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int ptn3460_bridge_attach(struct drm_bridge *bridge) +static int ptn3460_bridge_attach(struct drm_bridge *bridge,

• 		 bool create_connector)
  

{ struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge); int ret;

• if (!create_connector)

• return -EINVAL;
  

• if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV;

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index b12ae3a4c5f1..98ad4abf2409 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -52,12 +52,15 @@ static const struct drm_connector_funcs panel_bridge_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int panel_bridge_attach(struct drm_bridge *bridge) +static int panel_bridge_attach(struct drm_bridge *bridge, bool create_connector) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); struct drm_connector *connector = &panel_bridge->connector; int ret;

• if (!create_connector)

• return -EINVAL;
  

• if (!bridge->encoder) { DRM_ERROR("Missing encoder\n"); return -ENODEV;

diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c index 2d88146e4836..b9243d51489b 100644 --- a/drivers/gpu/drm/bridge/parade-ps8622.c +++ b/drivers/gpu/drm/bridge/parade-ps8622.c @@ -476,11 +476,14 @@ static const struct drm_connector_funcs ps8622_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int ps8622_attach(struct drm_bridge *bridge) +static int ps8622_attach(struct drm_bridge *bridge, bool create_connector) { struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge); int ret;

• if (!create_connector)

• return -EINVAL;
  

• if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); return -ENODEV;

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c index dd7aa466b280..18904b1082b6 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -396,12 +396,16 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, mutex_unlock(&sii902x->mutex); }

-static int sii902x_bridge_attach(struct drm_bridge *bridge) +static int sii902x_bridge_attach(struct drm_bridge *bridge,

• 		 bool create_connector)
  

{ struct sii902x *sii902x = bridge_to_sii902x(bridge); struct drm_device *drm = bridge->dev; int ret;

• if (!create_connector)

• return -EINVAL;
  

• drm_connector_helper_add(&sii902x->connector, &sii902x_connector_helper_funcs);

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 0cc293a6ac24..ea6529df7d9c 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -2203,7 +2203,7 @@ static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) return container_of(bridge, struct sii8620, bridge); }

-static int sii8620_attach(struct drm_bridge *bridge) +static int sii8620_attach(struct drm_bridge *bridge, bool create_connector) { struct sii8620 *ctx = bridge_to_sii8620(bridge);

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c index 7495b9bef865..86885eb6e28d 100644 --- a/drivers/gpu/drm/bridge/simple-bridge.c +++ b/drivers/gpu/drm/bridge/simple-bridge.c @@ -108,11 +108,15 @@ static const struct drm_connector_funcs simple_bridge_con_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int simple_bridge_attach(struct drm_bridge *bridge) +static int simple_bridge_attach(struct drm_bridge *bridge,

• 		bool create_connector)
  

{ struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge); int ret;

• if (!create_connector)

• return 0;
  

• if (!bridge->encoder) { DRM_ERROR("Missing encoder\n"); return -ENODEV;

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index c6490949d9db..930d67c618dd 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2174,12 +2174,16 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = .get_modes = dw_hdmi_connector_get_modes, };

-static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) +static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,

• 		 bool create_connector)
  

{ struct dw_hdmi *hdmi = bridge->driver_private; struct drm_encoder *encoder = bridge->encoder; struct drm_connector *connector = &hdmi->connector;

• if (!create_connector)

• return -EINVAL;
  

• connector->interlace_allowed = 1; connector->polled = DRM_CONNECTOR_POLL_HPD;

@@ -2857,7 +2861,7 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, if (IS_ERR(hdmi)) return hdmi;

• ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL);

• ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL, true); if (ret) { dw_hdmi_remove(hdmi); DRM_ERROR("Failed to initialize bridge with drm\n");

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 281c58bab1a1..05cf97ad524f 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -906,7 +906,8 @@ dw_mipi_dsi_bridge_mode_valid(struct drm_bridge *bridge, return mode_status; }

-static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) +static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge,

• 		     bool create_connector)
  

{ struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);

@@ -919,7 +920,8 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;

/* Attach the panel-bridge to the dsi bridge */

• return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge);

• return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge,

• 		 create_connector);
  

}

static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = { @@ -1064,7 +1066,7 @@ int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct drm_encoder *encoder) { int ret;

• ret = drm_bridge_attach(encoder, &dsi->bridge, NULL);

• ret = drm_bridge_attach(encoder, &dsi->bridge, NULL, true); if (ret) { DRM_ERROR("Failed to initialize bridge with drm\n"); return ret;

diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c index 170f162ffa55..6016f3aae42f 100644 --- a/drivers/gpu/drm/bridge/tc358764.c +++ b/drivers/gpu/drm/bridge/tc358764.c @@ -348,12 +348,15 @@ static void tc358764_enable(struct drm_bridge *bridge) dev_err(ctx->dev, "error enabling panel (%d)\n", ret); }

-static int tc358764_attach(struct drm_bridge *bridge) +static int tc358764_attach(struct drm_bridge *bridge, bool create_connector) { struct tc358764 *ctx = bridge_to_tc358764(bridge); struct drm_device *drm = bridge->dev; int ret;

• if (!create_connector)

• return -EINVAL;
  

• ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; ret = drm_connector_init(drm, &ctx->connector, &tc358764_connector_funcs,

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 13ade28a36a8..e2b2d2660adc 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1273,13 +1273,16 @@ static const struct drm_connector_funcs tc_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int tc_bridge_attach(struct drm_bridge *bridge) +static int tc_bridge_attach(struct drm_bridge *bridge, bool create_connector) { u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; struct tc_data *tc = bridge_to_tc(bridge); struct drm_device *drm = bridge->dev; int ret;

• if (!create_connector)

• return -EINVAL;
  

• /* Create DP/eDP connector */ drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs); ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,

diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c index 3d74129b2995..86f3b96f95db 100644 --- a/drivers/gpu/drm/bridge/thc63lvd1024.c +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c @@ -42,11 +42,12 @@ static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) return container_of(bridge, struct thc63_dev, bridge); }

-static int thc63_attach(struct drm_bridge *bridge) +static int thc63_attach(struct drm_bridge *bridge, bool create_connector) { struct thc63_dev *thc63 = to_thc63(bridge);

• return drm_bridge_attach(bridge->encoder, thc63->next, bridge);

• return drm_bridge_attach(bridge->encoder, thc63->next, bridge,

• 		 create_connector);
  

}

static enum drm_mode_status thc63_mode_valid(struct drm_bridge *bridge, diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index b77a52d05061..dbe265f7112d 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -224,7 +224,7 @@ static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata) pdata->supplies); }

-static int ti_sn_bridge_attach(struct drm_bridge *bridge) +static int ti_sn_bridge_attach(struct drm_bridge *bridge, bool create_connector) { int ret, val; struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); @@ -235,6 +235,9 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge) .node = NULL, };

• if (!create_connector)

• return -EINVAL;
  

• ret = drm_connector_init(bridge->dev, &pdata->connector, &ti_sn_bridge_connector_funcs, DRM_MODE_CONNECTOR_eDP);

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c index 4e76b2b27374..8d4690e436c3 100644 --- a/drivers/gpu/drm/bridge/ti-tfp410.c +++ b/drivers/gpu/drm/bridge/ti-tfp410.c @@ -121,11 +121,14 @@ static const struct drm_connector_funcs tfp410_con_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static int tfp410_attach(struct drm_bridge *bridge) +static int tfp410_attach(struct drm_bridge *bridge, bool create_connector) { struct tfp410 *dvi = drm_bridge_to_tfp410(bridge); int ret;

• if (!create_connector)

• return -EINVAL;
  

• if (!bridge->encoder) { dev_err(dvi->dev, "Missing encoder\n"); return -ENODEV;

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..519577f363e3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -95,6 +95,7 @@ EXPORT_SYMBOL(drm_bridge_remove);

• @encoder: DRM encoder
• @bridge: bridge to attach
• @previous: previous bridge in the chain (optional)

• • @create_connector: true if the bridge should create a drm_connector
  • Called by a kms driver to link the bridge to an encoder's chain. The previous
  • argument specifies the previous bridge in the chain. If NULL, the bridge is

@@ -112,7 +113,7 @@ EXPORT_SYMBOL(drm_bridge_remove);

• Zero on success, error code on failure

*/ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,

•       struct drm_bridge *previous)
  

•       struct drm_bridge *previous, bool create_connector)
  

{ int ret;

@@ -129,7 +130,7 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = encoder;

if (bridge->funcs->attach) {

• ret = bridge->funcs->attach(bridge);
  

• ret = bridge->funcs->attach(bridge, create_connector);
  

  if (ret < 0) { bridge->dev = NULL; bridge->encoder = NULL;

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index b11910f14c46..a367ef1e5081 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -228,7 +228,7 @@ static const struct drm_plane_funcs drm_simple_kms_plane_funcs = { int drm_simple_display_pipe_attach_bridge(struct drm_simple_display_pipe *pipe, struct drm_bridge *bridge) {

• return drm_bridge_attach(&pipe->encoder, bridge, NULL);

• return drm_bridge_attach(&pipe->encoder, bridge, NULL, true);

} EXPORT_SYMBOL(drm_simple_display_pipe_attach_bridge);

diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c index 3a0f0ba8c63a..02b98e6ca52d 100644 --- a/drivers/gpu/drm/exynos/exynos_dp.c +++ b/drivers/gpu/drm/exynos/exynos_dp.c @@ -105,7 +105,8 @@ static int exynos_dp_bridge_attach(struct analogix_dp_plat_data *plat_data,

/* Pre-empt DP connector creation if there's a bridge */ if (dp->ptn_bridge) {

• ret = drm_bridge_attach(&dp->encoder, dp->ptn_bridge, bridge);
  

• ret = drm_bridge_attach(&dp->encoder, dp->ptn_bridge, bridge,
  

• 			true);
  

  if (ret) { DRM_DEV_ERROR(dp->dev, "Failed to attach bridge to drm\n");

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 5f6f523821a2..768acc79d10c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1522,7 +1522,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,

out_bridge = of_drm_find_bridge(device->dev.of_node); if (out_bridge) {

• drm_bridge_attach(encoder, out_bridge, NULL);
  

• drm_bridge_attach(encoder, out_bridge, NULL, true);
  

  dsi->out_bridge = out_bridge; encoder->bridge = NULL; } else {

@@ -1698,7 +1698,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, if (dsi->in_bridge_node) { in_bridge = of_drm_find_bridge(dsi->in_bridge_node); if (in_bridge)

• 	drm_bridge_attach(encoder, in_bridge, NULL);
  

• 	drm_bridge_attach(encoder, in_bridge, NULL, true);
  

  }

  return mipi_dsi_host_register(&dsi->dsi_host);

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index bc1565f1822a..808c98101d56 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -952,7 +952,7 @@ static int hdmi_create_connector(struct drm_encoder *encoder) drm_connector_attach_encoder(connector, encoder);

if (hdata->bridge) {

• ret = drm_bridge_attach(encoder, hdata->bridge, NULL);
  

• ret = drm_bridge_attach(encoder, hdata->bridge, NULL, true);
  

  if (ret) DRM_DEV_ERROR(hdata->dev, "Failed to attach bridge\n"); }

diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c index c49e9e3740f8..8d22618ccf2e 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c @@ -159,5 +159,5 @@ int fsl_dcu_create_outputs(struct fsl_dcu_drm_device *fsl_dev) return fsl_dcu_attach_panel(fsl_dev, panel); }

• return drm_bridge_attach(&fsl_dev->encoder, bridge, NULL);

• return drm_bridge_attach(&fsl_dev->encoder, bridge, NULL, true);

} diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c index 3d6c45097f51..eac8ec1512ab 100644 --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c @@ -780,7 +780,7 @@ static int dsi_bridge_init(struct drm_device *dev, struct dw_dsi *dsi) int ret;

/* associate the bridge to dsi encoder */

• ret = drm_bridge_attach(encoder, bridge, NULL);

• ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) { DRM_ERROR("failed to attach external bridge\n"); return ret;

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 3d368c43185f..6b2e648b6c4d 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1366,10 +1366,14 @@ static int tda998x_connector_init(struct tda998x_priv *priv,

/* DRM bridge functions */

-static int tda998x_bridge_attach(struct drm_bridge *bridge) +static int tda998x_bridge_attach(struct drm_bridge *bridge,

• 		 bool create_connector)
  

{ struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);

• if (!create_connector)

• return -EINVAL;
  

• return tda998x_connector_init(priv, bridge->dev);

}

@@ -2033,7 +2037,7 @@ static int tda998x_encoder_init(struct device *dev, struct drm_device *drm) if (ret) goto err_encoder;

• ret = drm_bridge_attach(&priv->encoder, &priv->bridge, NULL);

• ret = drm_bridge_attach(&priv->encoder, &priv->bridge, NULL, true); if (ret) goto err_bridge;

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index 383733302280..845ead25ade7 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -446,7 +446,7 @@ static int imx_ldb_register(struct drm_device *drm,

if (imx_ldb_ch->bridge) { ret = drm_bridge_attach(&imx_ldb_ch->encoder,

• 			imx_ldb_ch->bridge, NULL);
  

• 			imx_ldb_ch->bridge, NULL, true);
  

  if (ret) { DRM_ERROR("Failed to initialize bridge with drm\n"); return ret;

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index 1a76de1e8e7b..cd746592d2a7 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -182,7 +182,7 @@ static int imx_pd_register(struct drm_device *drm, drm_panel_attach(imxpd->panel, &imxpd->connector);

if (imxpd->bridge) {

• ret = drm_bridge_attach(encoder, imxpd->bridge, NULL);
  

• ret = drm_bridge_attach(encoder, imxpd->bridge, NULL, true);
  

  if (ret < 0) { dev_err(imxpd->dev, "failed to attach bridge: %d\n", ret);

diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c index 07f7090d08b3..f2deadc980f8 100644 --- a/drivers/gpu/drm/mcde/mcde_dsi.c +++ b/drivers/gpu/drm/mcde/mcde_dsi.c @@ -817,12 +817,16 @@ mcde_dsi_connector_helper_funcs = { .get_modes = mcde_dsi_get_modes, };

-static int mcde_dsi_bridge_attach(struct drm_bridge *bridge) +static int mcde_dsi_bridge_attach(struct drm_bridge *bridge,

• 		  bool create_connector)
  

{ struct mcde_dsi *d = bridge_to_mcde_dsi(bridge); struct drm_device *drm = bridge->dev; int ret;

• if (!create_connector)

• return -EINVAL;
  

• drm_connector_helper_add(&d->connector, &mcde_dsi_connector_helper_funcs);

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c index bacd989cc9aa..1ff27bb17016 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi.c +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c @@ -604,7 +604,7 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data) /* Currently DPI0 is fixed to be driven by OVL1 */ dpi->encoder.possible_crtcs = BIT(1);

• ret = drm_bridge_attach(&dpi->encoder, dpi->bridge, NULL);

• ret = drm_bridge_attach(&dpi->encoder, dpi->bridge, NULL, true); if (ret) { dev_err(dev, "Failed to attach bridge: %d\n", ret); goto err_cleanup;

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index b91c4616644a..9c5bac48b44f 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -819,7 +819,7 @@ static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi)

/* If there's a bridge, attach to it and let it create the connector */ if (dsi->bridge) {

• ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL);
  

• ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL, true);
  

  if (ret) { DRM_ERROR("Failed to attach bridge to drm\n"); goto err_encoder_cleanup;

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index 5d6a9f094df5..d3248a881cf0 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1290,11 +1290,15 @@ static void mtk_hdmi_hpd_event(bool hpd, struct device *dev)

• Bridge callbacks

*/

-static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge) +static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge,

• 		  bool create_connector)
  

{ struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge); int ret;

• if (!create_connector)

• return -EINVAL;
  

• ret = drm_connector_init(bridge->encoder->dev, &hdmi->conn, &mtk_hdmi_connector_funcs, DRM_MODE_CONNECTOR_HDMIA);

@@ -1318,7 +1322,7 @@ static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge)

if (hdmi->next_bridge) { ret = drm_bridge_attach(bridge->encoder, hdmi->next_bridge,

• 			bridge);
  

• 			bridge, create_connector);
  

  if (ret) { dev_err(hdmi->dev, "Failed to attach external bridge: %d\n", ret);

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index 271aa7bbca92..ca733086041a 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -664,7 +664,7 @@ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id) bridge = &dsi_bridge->base; bridge->funcs = &dsi_mgr_bridge_funcs;

• ret = drm_bridge_attach(encoder, bridge, NULL);

• ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) goto fail;

@@ -693,7 +693,7 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id) encoder = msm_dsi->encoder;

/* link the internal dsi bridge to the external bridge */

• drm_bridge_attach(encoder, ext_bridge, int_bridge);

• drm_bridge_attach(encoder, ext_bridge, int_bridge, true);

  /*

  • we need the drm_connector created by the external bridge

diff --git a/drivers/gpu/drm/msm/edp/edp_bridge.c b/drivers/gpu/drm/msm/edp/edp_bridge.c index 2950bba4aca9..32a463c84cc1 100644 --- a/drivers/gpu/drm/msm/edp/edp_bridge.c +++ b/drivers/gpu/drm/msm/edp/edp_bridge.c @@ -91,7 +91,7 @@ struct drm_bridge *msm_edp_bridge_init(struct msm_edp *edp) bridge = &edp_bridge->base; bridge->funcs = &edp_bridge_funcs;

• ret = drm_bridge_attach(edp->encoder, bridge, NULL);

• ret = drm_bridge_attach(edp->encoder, bridge, NULL, true); if (ret) goto fail;

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c index 03197b8959ba..d7738aafcff8 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c @@ -296,7 +296,7 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi) bridge = &hdmi_bridge->base; bridge->funcs = &msm_hdmi_bridge_funcs;

• ret = drm_bridge_attach(hdmi->encoder, bridge, NULL);

• ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, true); if (ret) goto fail;

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 672e0f8ad11c..837d0cd20dd1 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -301,7 +301,8 @@ static int omap_modeset_init(struct drm_device *dev)

if (pipe->output->bridge) {
	ret = drm_bridge_attach(pipe->encoder,

• 				pipe->output->bridge, NULL);
  

• 				pipe->output->bridge, NULL,
  

• 				true);
  if (ret < 0)
  	return ret;
  

  }

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index 0f00bdfe2366..74c2ae5ce687 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -120,7 +120,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, * Attach the bridge to the encoder. The bridge will create the * connector. */

• ret = drm_bridge_attach(encoder, bridge, NULL);

• ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) { drm_encoder_cleanup(encoder); return ret;

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 1c62578590f4..a8d8b05c4731 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -605,7 +605,7 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge, rcar_lvds_get_lvds_mode(lvds); }

-static int rcar_lvds_attach(struct drm_bridge *bridge) +static int rcar_lvds_attach(struct drm_bridge *bridge, bool create_connector) { struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge); struct drm_connector *connector = &lvds->connector; @@ -615,7 +615,10 @@ static int rcar_lvds_attach(struct drm_bridge *bridge) /* If we have a next bridge just attach it. */ if (lvds->next_bridge) return drm_bridge_attach(bridge->encoder, lvds->next_bridge,

• 			 bridge);
  

• 			 bridge, create_connector);
  

• if (!create_connector)

• return -EINVAL;
  
  

  /* Otherwise if we have a panel, create a connector. */ if (!lvds->panel)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c index 830858a809e5..7ca412d294b2 100644 --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c @@ -440,7 +440,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, goto err_free_connector; } } else {

• ret = drm_bridge_attach(encoder, lvds->bridge, NULL);
  

• ret = drm_bridge_attach(encoder, lvds->bridge, NULL, true);
  

  if (ret) { DRM_DEV_ERROR(drm_dev->dev, "failed to attach bridge: %d\n", ret);

diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c index ce4d82d293e4..8218bbd09a72 100644 --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c @@ -143,7 +143,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,

rgb->bridge = bridge;

• ret = drm_bridge_attach(encoder, rgb->bridge, NULL);

• ret = drm_bridge_attach(encoder, rgb->bridge, NULL, true); if (ret) { DRM_DEV_ERROR(drm_dev->dev, "failed to attach bridge: %d\n", ret);

diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c index 9e6d5d8b7030..f09209621568 100644 --- a/drivers/gpu/drm/sti/sti_dvo.c +++ b/drivers/gpu/drm/sti/sti_dvo.c @@ -468,7 +468,7 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data) bridge->of_node = dvo->dev.of_node; drm_bridge_add(bridge);

• err = drm_bridge_attach(encoder, bridge, NULL);

• err = drm_bridge_attach(encoder, bridge, NULL, true); if (err) { DRM_ERROR("Failed to attach bridge\n"); return err;

diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c index 94e404f13234..87e0fb742dc8 100644 --- a/drivers/gpu/drm/sti/sti_hda.c +++ b/drivers/gpu/drm/sti/sti_hda.c @@ -700,7 +700,7 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)

bridge->driver_private = hda; bridge->funcs = &sti_hda_bridge_funcs;

• drm_bridge_attach(encoder, bridge, NULL);

• drm_bridge_attach(encoder, bridge, NULL, true);

  connector->encoder = encoder;

diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c index f03d617edc4c..8c0ffe6833f9 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -1276,7 +1276,7 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)

bridge->driver_private = hdmi; bridge->funcs = &sti_hdmi_bridge_funcs;

• drm_bridge_attach(encoder, bridge, NULL);

• drm_bridge_attach(encoder, bridge, NULL, true);

  connector->encoder = encoder;

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 2fe6c4a8d915..10a9f848c5f6 100644 --- a/drivers/gpu/drm/stm/ltdc.c +++ b/drivers/gpu/drm/stm/ltdc.c @@ -1053,7 +1053,7 @@ static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge *bridge) drm_encoder_init(ddev, encoder, &ltdc_encoder_funcs, DRM_MODE_ENCODER_DPI, NULL);

• ret = drm_bridge_attach(encoder, bridge, NULL);

• ret = drm_bridge_attach(encoder, bridge, NULL, true); if (ret) { drm_encoder_cleanup(encoder); return -EINVAL;

diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c index 3a3ba99fed22..3e5170fa1e67 100644 --- a/drivers/gpu/drm/sun4i/sun4i_lvds.c +++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c @@ -155,7 +155,7 @@ int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon) }

if (bridge) {

• ret = drm_bridge_attach(encoder, bridge, NULL);
  

• ret = drm_bridge_attach(encoder, bridge, NULL, true);
  

  if (ret) { dev_err(drm->dev, "Couldn't attach our bridge\n"); goto err_cleanup_connector;

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index a901ec689b62..3f8629445fbb 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -252,7 +252,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon) }

if (rgb->bridge) {

• ret = drm_bridge_attach(encoder, rgb->bridge, NULL);
  

• ret = drm_bridge_attach(encoder, rgb->bridge, NULL, true);
  

  if (ret) { dev_err(drm->dev, "Couldn't attach our bridge\n"); goto err_cleanup_connector;

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c index e9969cd36610..ec693c11e455 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_external.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c @@ -168,7 +168,7 @@ int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)

priv->external_encoder->possible_crtcs = BIT(0);

• ret = drm_bridge_attach(priv->external_encoder, bridge, NULL);

• ret = drm_bridge_attach(priv->external_encoder, bridge, NULL, true); if (ret) { dev_err(ddev->dev, "drm_bridge_attach() failed %d\n", ret); return ret;

diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c index 34f90ca8f479..2d7c5cf0d468 100644 --- a/drivers/gpu/drm/vc4/vc4_dpi.c +++ b/drivers/gpu/drm/vc4/vc4_dpi.c @@ -262,7 +262,7 @@ static int vc4_dpi_init_bridge(struct vc4_dpi *dpi) if (panel) bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DPI);

• return drm_bridge_attach(dpi->encoder, bridge, NULL);

• return drm_bridge_attach(dpi->encoder, bridge, NULL, true);

}

static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index 2ea4e20b7b8a..3edd7ffc7383 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -1607,7 +1607,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) DRM_MODE_ENCODER_DSI, NULL); drm_encoder_helper_add(dsi->encoder, &vc4_dsi_encoder_helper_funcs);

• ret = drm_bridge_attach(dsi->encoder, dsi->bridge, NULL);

• ret = drm_bridge_attach(dsi->encoder, dsi->bridge, NULL, true); if (ret) { dev_err(dev, "bridge attach failed: %d\n", ret); return ret;

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 7616f6562fe4..08dc15f93ded 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -48,7 +48,7 @@ struct drm_bridge_funcs { * * Zero on success, error code on failure. */

• int (*attach)(struct drm_bridge *bridge);

• int (*attach)(struct drm_bridge *bridge, bool create_connector);

  /**

  • @detach:

@@ -404,7 +404,7 @@ void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,

•       struct drm_bridge *previous);
  

•       struct drm_bridge *previous, bool create_connector);
  
  

bool drm_bridge_mode_fixup(struct drm_bridge *bridge, const struct drm_display_mode *mode,

Hi Andrzej,

On Thu, Aug 08, 2019 at 07:36:49PM +0200, Andrzej Hajda wrote:

On 08.08.2019 16:25, Laurent Pinchart wrote:

...

On Wed, Jul 17, 2019 at 08:39:47AM +0200, Andrzej Hajda wrote:

...

On 07.07.2019 20:18, Laurent Pinchart wrote:

...

Most bridge drivers create a DRM connector to model the connector at the output of the bridge. This model is historical and has worked pretty well so far, but causes several issues:

• It prevents supporting more complex display pipelines where DRM

connector operations are split over multiple components. For instance a pipeline with a bridge connected to the DDC signals to read EDID data, and another one connected to the HPD signal to detect connection and disconnection, will not be possible to support through this model.

• It requires every bridge driver to implement similar connector

handling code, resulting in code duplication.

• It assumes that a bridge will either be wired to a connector or to

another bridge, but doesn't support bridges that can be used in both positions very well (although there is some ad-hoc support for this in the analogix_dp bridge driver).

In order to solve these issues, ownership of the connector should be moved to the display controller driver (where it can be implemented using helpers provided by the core).

Extend the bridge API to allow disabling connector creation in bridge drivers as a first step towards the new model. The new create_connector argument to the bridge .attach() operation tells the bridge driver whether to create a connector. Set the argument to true unconditionally, and modify all existing bridge drivers to return an error when connector creation is not requested as they don't support this feature yet.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

drivers/gpu/drm/arc/arcpgu_hdmi.c | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 2 +- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++- drivers/gpu/drm/bridge/analogix-anx78xx.c | 6 +++++- drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 ++++++-- drivers/gpu/drm/bridge/cdns-dsi.c | 6 ++++-- drivers/gpu/drm/bridge/lvds-encoder.c | 4 ++-- drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 6 +++++- drivers/gpu/drm/bridge/nxp-ptn3460.c | 6 +++++- drivers/gpu/drm/bridge/panel.c | 5 ++++- drivers/gpu/drm/bridge/parade-ps8622.c | 5 ++++- drivers/gpu/drm/bridge/sii902x.c | 6 +++++- drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- drivers/gpu/drm/bridge/simple-bridge.c | 6 +++++- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 ++++++-- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 8 +++++--- drivers/gpu/drm/bridge/tc358764.c | 5 ++++- drivers/gpu/drm/bridge/tc358767.c | 5 ++++- drivers/gpu/drm/bridge/thc63lvd1024.c | 5 +++-- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 ++++- drivers/gpu/drm/bridge/ti-tfp410.c | 5 ++++- drivers/gpu/drm/drm_bridge.c | 5 +++-- drivers/gpu/drm/drm_simple_kms_helper.c | 2 +- drivers/gpu/drm/exynos/exynos_dp.c | 3 ++- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 ++-- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 2 +- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++-- drivers/gpu/drm/imx/imx-ldb.c | 2 +- drivers/gpu/drm/imx/parallel-display.c | 2 +- drivers/gpu/drm/mcde/mcde_dsi.c | 6 +++++- drivers/gpu/drm/mediatek/mtk_dpi.c | 2 +- drivers/gpu/drm/mediatek/mtk_dsi.c | 2 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 8 ++++++-- drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++-- drivers/gpu/drm/msm/edp/edp_bridge.c | 2 +- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.c | 3 ++- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 2 +- drivers/gpu/drm/rcar-du/rcar_lvds.c | 7 +++++-- drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 +- drivers/gpu/drm/rockchip/rockchip_rgb.c | 2 +- drivers/gpu/drm/sti/sti_dvo.c | 2 +- drivers/gpu/drm/sti/sti_hda.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/stm/ltdc.c | 2 +- drivers/gpu/drm/sun4i/sun4i_lvds.c | 2 +- drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 +- drivers/gpu/drm/tilcdc/tilcdc_external.c | 2 +- drivers/gpu/drm/vc4/vc4_dpi.c | 2 +- drivers/gpu/drm/vc4/vc4_dsi.c | 2 +- include/drm/drm_bridge.h | 4 ++-- 53 files changed, 140 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_hdmi.c b/drivers/gpu/drm/arc/arcpgu_hdmi.c index 98aac743cc26..739f2358f1d5 100644 --- a/drivers/gpu/drm/arc/arcpgu_hdmi.c +++ b/drivers/gpu/drm/arc/arcpgu_hdmi.c @@ -39,7 +39,7 @@ int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np) return ret;

/* Link drm_bridge to encoder */

• ret = drm_bridge_attach(encoder, bridge, NULL);

• ret = drm_bridge_attach(encoder, bridge, NULL, true);

Few suggestions:

1. Maybe it would be more convenient to add flags argument instead of bool:

• code should be more readable: ret = drm_bridge_attach(encoder, bridge,

NULL, DRM_BRIDGE_FLAG_NO_CONNECTOR)

• it can be easily expanded later with other flags, there at least two

drivers which would benefit from DRM_BRIDGE_FLAG_NO_CHAINING flag.

Please note that I think this flag should disappear once drivers get converted to the new model. This will however take some time. I'm not opposed to turning the book into a flag though. I was hoping to receive more review comments on this particular patch, but as that's not the case, I can already proceed with the change.

What would the DRM_BRIDGE_FLAG_NO_CHAINING flag be used for ?

To avoid setting encoder->bridge or previous_bridge->next field to attached bridge (last lines of drm_bridge_attach code).

This is for sure the case of exynos_dsi and vc4_dsi, but I guess it can affect other drivers as well, probably they just use other workarounds or have more flexible hardware.

Generally idea that order of calling pre_enable/enable/disable/post_disable callbacks in chained encoder/bridges is fixed is wrong IMHO, only video source component knows in which moment it should enable its sink and if it should do something after. And it does not work at all with sources/sinks having more than one output/input.

Makes sense. I think I would however still set the ->next field, in order to track bridges, and only skip chaining of the enable/disable operations.

...

...

2. If the patch can be applied atomically it is OK as is, if not you can

use preprocessor vararg magic to support new and old syntax, sth like:

#define _drm_bridge_attach(encoder, bridge, prev, flags, optarg...) __drm_bridge_attach(encoder, bridge, prev, flags)

#define drm_bridge_attach(encoder, bridge, prev, optarg...) _drm_bridge_attach(encoder, bridge, prev, ##optarg, 0)

Good point. I'll try to do this atomically, but if it fails I'll follow your suggestion.

...

3. Maybe more convenient would be to just set the flags directly before

attachment:

bridge->dont_create_connector = true;

ret = drm_bridge_attach(encoder, bridge, NULL);

This way it will be still expandable, and less changes.

Bridges that are chained would need to set the dont_create_connector flag of the next bridge. It would be a bit ugly, but would make this patch smaller. On the other hand we would need to keep the if (!create_connector) check in the .attach() handlers, and it would be easier to miss it in bridge drivers (current or new) than with an explicit argument to the .attach() operation. I would thus have a preference for the new argument to .attach(). Especially if it can help you with DRM_BRIDGE_FLAG_NO_CHAINING :-)

bridge->dont_chain would work as well :)

Btw I wonder if it could be possible to disallow creating connectors at all by new bridges - it would speed-up transition.

If we can agree on that, that would be wonderful !

Another long term idea. Since bridges can be attached to:

• encoder,
• another bridge,
• crtc (I have one example, but I guess there could be more),
• even before crtc (image postprocessing)

And since bridge output goes to:

• another bridge,
• panel.

Wouldn't be better to create drm_source and drm_sink (do not respond with xkcd picture :) ):

• drm_source will be embedded in source device context,
• drm_sink will be embedded in sink device context.

We could make then transitions of bridges to drm_sink with drm_source embeded in its context, and panels to drm_sink. This way we could drop these crazy constructs:

• if sink is panel then do sth, elsif is bridge then do sth_else,
• if src is bridge then do sth, elsif is encoder ... elsif ....
• helpers of_find_panel_or_bridge,
• drm_panel_bridge,

Also we could implement easily multi input/output bridges/panels/crtcs whatever. And hpd callbacks you have proposed in another patch would fit better to drm_source.ops. ...

My best answer to this is: one step at a time :-) Yes, that's the direction I want to take (there will of course be lots of bikeshedding on the details), and this series is one step in that direction. What I'm really trying to do here is simplify bridge implementations by creating a clear API that shrinks as much as possible the duties of bridge drivers. This moves existing complexity outside of bridges, and we of course don't want to duplicate it in all display controller drivers, so I'm also creating helpers that centralise the complexity. Their use is not mandatory (Daniel has hammered the "no midlayer" message enough), but for the vast majority of display controllers, it should greatly simplify the code.

My next planned steps are:

- Convert more bridge drivers to support DRM_BRIDGE_FLAG_NO_CONNECTOR - Convert more display controller drivers to use DRM_BRIDGE_FLAG_NO_CONNECTOR - Drop support for !DRM_BRIDGE_FLAG_NO_CONNECTOR in bridge drivers once they are only used by display controllers that set DRM_BRIDGE_FLAG_NO_CONNECTOR

In parallel, I want to address drm_panel ad drm_panel_bridge. With the extension of the bridge API to expose connector-related operations done in this series, the distinction between panel and bridges becomes much thinner, opening the door to lots of simplifications.

On Wed, Aug 14, 2019 at 10:18:40AM +0200, Daniel Vetter wrote:

On Thu, Aug 08, 2019 at 07:36:49PM +0200, Andrzej Hajda wrote:

...

On 08.08.2019 16:25, Laurent Pinchart wrote:

...

On Wed, Jul 17, 2019 at 08:39:47AM +0200, Andrzej Hajda wrote:

...

On 07.07.2019 20:18, Laurent Pinchart wrote:

...

Most bridge drivers create a DRM connector to model the connector at the output of the bridge. This model is historical and has worked pretty well so far, but causes several issues:

• It prevents supporting more complex display pipelines where DRM

connector operations are split over multiple components. For instance a pipeline with a bridge connected to the DDC signals to read EDID data, and another one connected to the HPD signal to detect connection and disconnection, will not be possible to support through this model.

• It requires every bridge driver to implement similar connector

handling code, resulting in code duplication.

• It assumes that a bridge will either be wired to a connector or to

another bridge, but doesn't support bridges that can be used in both positions very well (although there is some ad-hoc support for this in the analogix_dp bridge driver).

In order to solve these issues, ownership of the connector should be moved to the display controller driver (where it can be implemented using helpers provided by the core).

Extend the bridge API to allow disabling connector creation in bridge drivers as a first step towards the new model. The new create_connector argument to the bridge .attach() operation tells the bridge driver whether to create a connector. Set the argument to true unconditionally, and modify all existing bridge drivers to return an error when connector creation is not requested as they don't support this feature yet.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

drivers/gpu/drm/arc/arcpgu_hdmi.c | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 2 +- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++- drivers/gpu/drm/bridge/analogix-anx78xx.c | 6 +++++- drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 ++++++-- drivers/gpu/drm/bridge/cdns-dsi.c | 6 ++++-- drivers/gpu/drm/bridge/lvds-encoder.c | 4 ++-- drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 6 +++++- drivers/gpu/drm/bridge/nxp-ptn3460.c | 6 +++++- drivers/gpu/drm/bridge/panel.c | 5 ++++- drivers/gpu/drm/bridge/parade-ps8622.c | 5 ++++- drivers/gpu/drm/bridge/sii902x.c | 6 +++++- drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- drivers/gpu/drm/bridge/simple-bridge.c | 6 +++++- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 ++++++-- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 8 +++++--- drivers/gpu/drm/bridge/tc358764.c | 5 ++++- drivers/gpu/drm/bridge/tc358767.c | 5 ++++- drivers/gpu/drm/bridge/thc63lvd1024.c | 5 +++-- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 ++++- drivers/gpu/drm/bridge/ti-tfp410.c | 5 ++++- drivers/gpu/drm/drm_bridge.c | 5 +++-- drivers/gpu/drm/drm_simple_kms_helper.c | 2 +- drivers/gpu/drm/exynos/exynos_dp.c | 3 ++- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 ++-- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 2 +- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++-- drivers/gpu/drm/imx/imx-ldb.c | 2 +- drivers/gpu/drm/imx/parallel-display.c | 2 +- drivers/gpu/drm/mcde/mcde_dsi.c | 6 +++++- drivers/gpu/drm/mediatek/mtk_dpi.c | 2 +- drivers/gpu/drm/mediatek/mtk_dsi.c | 2 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 8 ++++++-- drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++-- drivers/gpu/drm/msm/edp/edp_bridge.c | 2 +- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.c | 3 ++- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 2 +- drivers/gpu/drm/rcar-du/rcar_lvds.c | 7 +++++-- drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 +- drivers/gpu/drm/rockchip/rockchip_rgb.c | 2 +- drivers/gpu/drm/sti/sti_dvo.c | 2 +- drivers/gpu/drm/sti/sti_hda.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/stm/ltdc.c | 2 +- drivers/gpu/drm/sun4i/sun4i_lvds.c | 2 +- drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 +- drivers/gpu/drm/tilcdc/tilcdc_external.c | 2 +- drivers/gpu/drm/vc4/vc4_dpi.c | 2 +- drivers/gpu/drm/vc4/vc4_dsi.c | 2 +- include/drm/drm_bridge.h | 4 ++-- 53 files changed, 140 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_hdmi.c b/drivers/gpu/drm/arc/arcpgu_hdmi.c index 98aac743cc26..739f2358f1d5 100644 --- a/drivers/gpu/drm/arc/arcpgu_hdmi.c +++ b/drivers/gpu/drm/arc/arcpgu_hdmi.c @@ -39,7 +39,7 @@ int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np) return ret;

/* Link drm_bridge to encoder */

• ret = drm_bridge_attach(encoder, bridge, NULL);

• ret = drm_bridge_attach(encoder, bridge, NULL, true);

Few suggestions:

1. Maybe it would be more convenient to add flags argument instead of bool:

• code should be more readable: ret = drm_bridge_attach(encoder, bridge,

NULL, DRM_BRIDGE_FLAG_NO_CONNECTOR)

• it can be easily expanded later with other flags, there at least two

drivers which would benefit from DRM_BRIDGE_FLAG_NO_CHAINING flag.

Please note that I think this flag should disappear once drivers get converted to the new model. This will however take some time. I'm not opposed to turning the book into a flag though. I was hoping to receive more review comments on this particular patch, but as that's not the case, I can already proceed with the change.

What would the DRM_BRIDGE_FLAG_NO_CHAINING flag be used for ?

To avoid setting encoder->bridge or previous_bridge->next field to attached bridge (last lines of drm_bridge_attach code).

This is for sure the case of exynos_dsi and vc4_dsi, but I guess it can affect other drivers as well, probably they just use other workarounds or have more flexible hardware.

Generally idea that order of calling pre_enable/enable/disable/post_disable callbacks in chained encoder/bridges is fixed is wrong IMHO, only video source component knows in which moment it should enable its sink and if it should do something after. And it does not work at all with sources/sinks having more than one output/input.

This doesn't make sense to me ... if you don't want to have a chained bridge, don't attach it? You can just open-code the attach and call the bridge functions directly when appropriate. I think that's the better long-term plan than trying to have flags for every possible topology existing out there ...

I think the whole topology should be known to the display driver, so the next (and possibly previous if we later need it) pointers should be set in all cases. Otherwise we can't, for instance, notify all bridges of HPD events. Sure, a bridge could decide not to attach at all, but then it would have to reimplement lots of logic to forward everything manually, when the need for manual forwarding is usually limited to a certain category of operations.

...

...

...

2. If the patch can be applied atomically it is OK as is, if not you can

use preprocessor vararg magic to support new and old syntax, sth like:

#define _drm_bridge_attach(encoder, bridge, prev, flags, optarg...) __drm_bridge_attach(encoder, bridge, prev, flags)

#define drm_bridge_attach(encoder, bridge, prev, optarg...) _drm_bridge_attach(encoder, bridge, prev, ##optarg, 0)

Good point. I'll try to do this atomically, but if it fails I'll follow your suggestion.

...

3. Maybe more convenient would be to just set the flags directly before

attachment:

bridge->dont_create_connector = true;

ret = drm_bridge_attach(encoder, bridge, NULL);

This way it will be still expandable, and less changes.

Bridges that are chained would need to set the dont_create_connector flag of the next bridge. It would be a bit ugly, but would make this patch smaller. On the other hand we would need to keep the if (!create_connector) check in the .attach() handlers, and it would be easier to miss it in bridge drivers (current or new) than with an explicit argument to the .attach() operation. I would thus have a preference for the new argument to .attach(). Especially if it can help you with DRM_BRIDGE_FLAG_NO_CHAINING :-)

bridge->dont_chain would work as well :)

Btw I wonder if it could be possible to disallow creating connectors at all by new bridges - it would speed-up transition.

Another long term idea. Since bridges can be attached to:

• encoder,
• another bridge,
• crtc (I have one example, but I guess there could be more),
• even before crtc (image postprocessing)

And since bridge output goes to:

• another bridge,
• panel.

Wouldn't be better to create drm_source and drm_sink (do not respond with xkcd picture :) ):

• drm_source will be embedded in source device context,
• drm_sink will be embedded in sink device context.

We could make then transitions of bridges to drm_sink with drm_source embeded in its context, and panels to drm_sink. This way we could drop these crazy constructs:

• if sink is panel then do sth, elsif is bridge then do sth_else,
• if src is bridge then do sth, elsif is encoder ... elsif ....
• helpers of_find_panel_or_bridge,
• drm_panel_bridge,

Also we could implement easily multi input/output bridges/panels/crtcs whatever. And hpd callbacks you have proposed in another patch would fit better to drm_source.ops. ...

Regards Andrzej

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

To support implementation of DRM connectors on top of DRM bridges instead of by bridges, the drm_bridge needs to expose new operations and data:

- Output detection, hot-plug notification, mode retrieval and EDID retrieval operations - Bitmask of supported operations - Bridge output type

Add and document these.

Three new bridge helper functions are also added to handle hot plug notification in a way that is as transparent as possible for the bridges.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/drm_bridge.c | 92 +++++++++++++++++++ include/drm/drm_bridge.h | 170 ++++++++++++++++++++++++++++++++++- 2 files changed, 261 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 519577f363e3..3c2a255df7af 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -70,6 +70,8 @@ static LIST_HEAD(bridge_list); */ void drm_bridge_add(struct drm_bridge *bridge) { + mutex_init(&bridge->hpd_mutex); + mutex_lock(&bridge_lock); list_add_tail(&bridge->list, &bridge_list); mutex_unlock(&bridge_lock); @@ -86,6 +88,8 @@ void drm_bridge_remove(struct drm_bridge *bridge) mutex_lock(&bridge_lock); list_del_init(&bridge->list); mutex_unlock(&bridge_lock); + + mutex_destroy(&bridge->hpd_mutex); } EXPORT_SYMBOL(drm_bridge_remove);

@@ -463,6 +467,94 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_atomic_bridge_enable);

+/** + * drm_bridge_hpd_enable - enable hot plug detection for the bridge + * @bridge: bridge control structure + * @cb: hot-plug detection callback + * @data: data to be passed to the hot-plug detection callback + * + * Call &drm_bridge_funcs.hpd_enable and register the given @cb and @data as + * hot plug notification callback. From now on the @cb will be called with + * @data when an output status change is detected by the bridge, until hot plug + * notification gets disabled with drm_bridge_hpd_disable(). + * + * Hot plug detection is supported only if the DRM_BRIDGE_OP_HPD flag is set in + * bridge->ops. This function shall not be called when the flag is not set. + * + * Only one hot plug detection callback can be registered at a time, it is an + * error to call this function when hot plug detection is already enabled for + * the bridge. + */ +void drm_bridge_hpd_enable(struct drm_bridge *bridge, + void (*cb)(void *data, + enum drm_connector_status status), + void *data) +{ + if (!bridge || !bridge->funcs->hpd_enable) + return; + + mutex_lock(&bridge->hpd_mutex); + + if (WARN(bridge->hpd_cb, "Hot plug detection already enabled\n")) + goto unlock; + + bridge->hpd_cb = cb; + bridge->hpd_data = data; + + bridge->funcs->hpd_enable(bridge); + +unlock: + mutex_unlock(&bridge->hpd_mutex); +} +EXPORT_SYMBOL_GPL(drm_bridge_hpd_enable); + +/** + * drm_bridge_hpd_disable - disable hot plug detection for the bridge + * @bridge: bridge control structure + * + * Call &drm_bridge_funcs.hpd_disable and unregister the hot plug detection + * callback previously registered with drm_bridge_hpd_enable(). Once this + * function returns the callback will not be called by the bridge when an + * output status change occurs. + * + * Hot plug detection is supported only if the DRM_BRIDGE_OP_HPD flag is set in + * bridge->ops. This function shall not be called when the flag is not set. + */ +void drm_bridge_hpd_disable(struct drm_bridge *bridge) +{ + if (!bridge || !bridge->funcs->hpd_disable) + return; + + mutex_lock(&bridge->hpd_mutex); + bridge->funcs->hpd_disable(bridge); + + bridge->hpd_cb = NULL; + bridge->hpd_data = NULL; + mutex_unlock(&bridge->hpd_mutex); +} +EXPORT_SYMBOL_GPL(drm_bridge_hpd_disable); + +/** + * drm_bridge_hpd_notify - notify hot plug detection events + * @bridge: bridge control structure + * @status: output connection status + * + * Bridge drivers shall call this function to report hot plug events when they + * detect a change in the output status, when hot plug detection has been + * enabled by the &drm_bridge_funcs.hpd_enable callback. + * + * This function shall be called in a context that can sleep. + */ +void drm_bridge_hpd_notify(struct drm_bridge *bridge, + enum drm_connector_status status) +{ + mutex_lock(&bridge->hpd_mutex); + if (bridge->hpd_cb) + bridge->hpd_cb(bridge->hpd_data, status); + mutex_unlock(&bridge->hpd_mutex); +} +EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify); + #ifdef CONFIG_OF /** * of_drm_find_bridge - find the bridge corresponding to the device node in diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 08dc15f93ded..b9445aa5b1ef 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -23,8 +23,9 @@ #ifndef __DRM_BRIDGE_H__ #define __DRM_BRIDGE_H__

-#include <linux/list.h> #include <linux/ctype.h> +#include <linux/list.h> +#include <linux/mutex.h> #include <drm/drm_mode_object.h> #include <drm/drm_modes.h>

@@ -334,6 +335,110 @@ struct drm_bridge_funcs { */ void (*atomic_post_disable)(struct drm_bridge *bridge, struct drm_atomic_state *state); + + /** + * @detect: + * + * Check if anything is attached to the bridge output. + * + * This callback is optional, if not implemented the bridge will be + * considered as always having a component attached to its output. + * Bridges that implement this callback shall set the + * DRM_BRIDGE_OP_DETECT flag in their &drm_bridge->ops. + * + * RETURNS: + * + * drm_connector_status indicating the bridge output status. + */ + enum drm_connector_status (*detect)(struct drm_bridge *bridge); + + /** + * @get_modes: + * + * Fill all modes currently valid for the sink into the &drm_connector + * with drm_mode_probed_add(). + * + * The @get_modes callback is mostly intended to support non-probable + * displays such as many fixed panels. Bridges that support reading + * EDID shall leave @get_modes unimplemented and implement the + * &drm_bridge_funcs->get_edid callback instead. + * + * This callback is optional. Bridges that implement it shall set the + * DRM_BRIDGE_OP_MODES flag in their &drm_bridge->ops. + * + * RETURNS: + * + * The number of modes added by calling drm_mode_probed_add(). + */ + int (*get_modes)(struct drm_bridge *bridge, + struct drm_connector *connector); + + /** + * @get_edid: + * + * Read and parse the EDID data of the connected display. + * + * The @get_edid callback is the preferred way of reporting mode + * information for a display connected to the bridge output. Bridges + * that support readind EDID shall implement this callback and leave + * the @get_modes callback unimplemented. + * + * The caller of this operation shall first verify the output + * connection status and refrain from reading EDID from a disconnected + * output. + * + * This callback is optional. Bridges that implement it shall set the + * DRM_BRIDGE_OP_EDID flag in their &drm_bridge->ops. + * + * RETURNS: + * + * An edid structure newly allocated with kmalloc() (or similar) on + * success, or NULL otherwise. The caller is responsible for freeing + * the returned edid structure with kfree(). + */ + struct edid *(*get_edid)(struct drm_bridge *bridge, + struct drm_connector *connector); + + /** + * @lost_hotplug: + * + * Notify the bridge of display disconnection. + * + * This callback is optional, it may be implemented by bridges that + * need to be notified of display disconnection for internal reasons. + * One use case is to reset the internal state of CEC controllers for + * HDMI bridges. + */ + void (*lost_hotplug)(struct drm_bridge *bridge); + + /** + * @hpd_enable: + * + * Enable hot plug detection. From now on the bridge shall call + * drm_bridge_hpd_notify() each time a change is detected in the output + * connection status, until hot plug detection gets disabled with + * @hpd_disable. + * + * This callback is optional and shall only be implemented by bridges + * that support hot-plug notification without polling. Bridges that + * implement it shall also implement the @hpd_disable callback and set + * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops. + */ + void (*hpd_enable)(struct drm_bridge *bridge); + + /** + * @hpd_disable: + * + * Disable hot plug detection. Once this function returns the bridge + * shall not call drm_bridge_hpd_notify() when a change in the output + * connection status occurs. + * + * This callback is optional and shall only be implemented by bridges + * that support hot-plug notification without polling. Bridges that + * implement it shall also implement the @hpd_enable callback and set + * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops. + */ + void (*hpd_disable)(struct drm_bridge *bridge); };

/** @@ -372,6 +477,38 @@ struct drm_bridge_timings { bool dual_link; };

+/** + * enum drm_bridge_ops - Bitmask of operations supported by the bridge + */ +enum drm_bridge_ops { + /** + * @DRM_BRIDGE_OP_DETECT: The bridge can detect displays connected to + * its output. Bridges that set this flag shall implement the + * &drm_bridge_funcs->detect callback. + */ + DRM_BRIDGE_OP_DETECT = BIT(0), + /** + * @DRM_BRIDGE_OP_EDID: The bridge can retrieve the EDID of the display + * connected to its output. Bridges that set this flag shall implement + * the &drm_bridge_funcs->get_edid callback. + */ + DRM_BRIDGE_OP_EDID = BIT(1), + /** + * @DRM_BRIDGE_OP_HPD: The bridge can detect hot-plug and hot-unplug + * without requiring polling. Bridges that set this flag shall + * implement the &drm_bridge_funcs->hpd_enable and + * &drm_bridge_funcs->disable_hpd_cb callbacks. + */ + DRM_BRIDGE_OP_HPD = BIT(2), + /** + * @DRM_BRIDGE_OP_MODES: The bridge can retrieving the modes supported + * by the display at its output. This does not include readind EDID + * which is separately covered by @DRM_BRIDGE_OP_EDID. Bridges that set + * this flag shall implement the &drm_bridge_funcs->get_modes callback. + */ + DRM_BRIDGE_OP_MODES = BIT(3), +}; + /** * struct drm_bridge - central DRM bridge control structure */ @@ -398,6 +535,29 @@ struct drm_bridge { const struct drm_bridge_funcs *funcs; /** @driver_private: pointer to the bridge driver's internal context */ void *driver_private; + /** @ops: bitmask of operations supported by the bridge */ + enum drm_bridge_ops ops; + /** + * @type: Type of the connection at the bridge output + * (DRM_MODE_CONNECTOR_*). For bridges at the end of this chain this + * identifies the type of connected display. + */ + int type; + /** private: */ + /** + * @hpd_mutex: Protects the @hpd_cb and @hpd_data fields. + */ + struct mutex hpd_mutex; + /** + * @hpd_cb: Hot plug detection callback, registered with + * drm_bridge_hpd_enable(). + */ + void (*hpd_cb)(void *data, enum drm_connector_status status); + /** + * @hpd_data: Private data passed to the Hot plug detection callback + * @hpd_cb. + */ + void *hpd_data; };

void drm_bridge_add(struct drm_bridge *bridge); @@ -428,6 +588,14 @@ void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge, void drm_atomic_bridge_enable(struct drm_bridge *bridge, struct drm_atomic_state *state);

+void drm_bridge_hpd_enable(struct drm_bridge *bridge, + void (*cb)(void *data, + enum drm_connector_status status), + void *data); +void drm_bridge_hpd_disable(struct drm_bridge *bridge); +void drm_bridge_hpd_notify(struct drm_bridge *bridge, + enum drm_connector_status status); + #ifdef CONFIG_DRM_PANEL_BRIDGE struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, u32 connector_type);

On Wed, Jul 10, 2019 at 02:12:14PM +0200, Andrzej Hajda wrote:

Hi Laurent,

I like the approach, current practice when almost every bridge should optionally implement connector, or alternatively downstream bridge or panel is very painful.

Yeah I think this looks mostly reasonable. Some api design comments on top of Andrzej', with the fair warning that I didn't bother to read up on how it's all used in the end. I probably should go and do that, at least to get a feeling for what your hpd_cb usually does.

More comments inlined.

On 07.07.2019 20:18, Laurent Pinchart wrote:

...

To support implementation of DRM connectors on top of DRM bridges instead of by bridges, the drm_bridge needs to expose new operations and data:

• Output detection, hot-plug notification, mode retrieval and EDID retrieval operations
• Bitmask of supported operations

Why do we need these bitmask at all? Why cannot we rely on presence of operation's callback?

Yeah also not a huge fan of these bitmasks. Smells like DRIVER_GEM|DRIVER_MODESET, and I personally really hate those. Easy to add, generally good excuse to not have to think through the design between different parts of drivers - "just" add another flag.

...

• Bridge output type

Add and document these.

Three new bridge helper functions are also added to handle hot plug notification in a way that is as transparent as possible for the bridges.

Documentation of new opses does not explain how it should cooperate with bridge chaining, I suppose they should be chained explicitly, am I right? More comments about it later.

...

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

drivers/gpu/drm/drm_bridge.c | 92 +++++++++++++++++++ include/drm/drm_bridge.h | 170 ++++++++++++++++++++++++++++++++++- 2 files changed, 261 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 519577f363e3..3c2a255df7af 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -70,6 +70,8 @@ static LIST_HEAD(bridge_list); */ void drm_bridge_add(struct drm_bridge *bridge) {

• mutex_init(&bridge->hpd_mutex);
• mutex_lock(&bridge_lock); list_add_tail(&bridge->list, &bridge_list); mutex_unlock(&bridge_lock);

@@ -86,6 +88,8 @@ void drm_bridge_remove(struct drm_bridge *bridge) mutex_lock(&bridge_lock); list_del_init(&bridge->list); mutex_unlock(&bridge_lock);

• mutex_destroy(&bridge->hpd_mutex);

} EXPORT_SYMBOL(drm_bridge_remove);

@@ -463,6 +467,94 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_atomic_bridge_enable);

+/**

• • drm_bridge_hpd_enable - enable hot plug detection for the bridge
• • @bridge: bridge control structure
• • @cb: hot-plug detection callback
• • @data: data to be passed to the hot-plug detection callback
• • Call &drm_bridge_funcs.hpd_enable and register the given @cb and @data as
• • hot plug notification callback. From now on the @cb will be called with
• • @data when an output status change is detected by the bridge, until hot plug
• • notification gets disabled with drm_bridge_hpd_disable().
• • Hot plug detection is supported only if the DRM_BRIDGE_OP_HPD flag is set in
• • bridge->ops. This function shall not be called when the flag is not set.
• • Only one hot plug detection callback can be registered at a time, it is an
• • error to call this function when hot plug detection is already enabled for
• • the bridge.
• */

To simplify architecture maybe would be better to enable hpd just on bridge attach:

bridge->hpd_cb = cb;

bridge->hpd_data = data;

ret = drm_bridge_attach(...);

Yeah I like this more. The other problem here is, what if you need more than 1 callback registers on the same bridge hdp signal?

This way we could avoid adding new callbacks hpd_(enable|disable) without big sacrifices.

One more thing: HPD in DisplayPort/HDMI beside signalling plug/unplug, notifies about sink status change, how it translates to this cb?

...

+void drm_bridge_hpd_enable(struct drm_bridge *bridge,

• 	   void (*cb)(void *data,
  

• 		      enum drm_connector_status status),
  

• 	   void *data)
  

+{

• if (!bridge || !bridge->funcs->hpd_enable)

• return;
  

• mutex_lock(&bridge->hpd_mutex);
• if (WARN(bridge->hpd_cb, "Hot plug detection already enabled\n"))

• goto unlock;
  

• bridge->hpd_cb = cb;
• bridge->hpd_data = data;
• bridge->funcs->hpd_enable(bridge);

+unlock:

• mutex_unlock(&bridge->hpd_mutex);

+} +EXPORT_SYMBOL_GPL(drm_bridge_hpd_enable);

+/**

• • drm_bridge_hpd_disable - disable hot plug detection for the bridge
• • @bridge: bridge control structure
• • Call &drm_bridge_funcs.hpd_disable and unregister the hot plug detection
• • callback previously registered with drm_bridge_hpd_enable(). Once this
• • function returns the callback will not be called by the bridge when an
• • output status change occurs.
• • Hot plug detection is supported only if the DRM_BRIDGE_OP_HPD flag is set in
• • bridge->ops. This function shall not be called when the flag is not set.
• */

+void drm_bridge_hpd_disable(struct drm_bridge *bridge) +{

• if (!bridge || !bridge->funcs->hpd_disable)

• return;
  

• mutex_lock(&bridge->hpd_mutex);
• bridge->funcs->hpd_disable(bridge);
• bridge->hpd_cb = NULL;
• bridge->hpd_data = NULL;
• mutex_unlock(&bridge->hpd_mutex);

+} +EXPORT_SYMBOL_GPL(drm_bridge_hpd_disable);

+/**

• • drm_bridge_hpd_notify - notify hot plug detection events
• • @bridge: bridge control structure
• • @status: output connection status
• • Bridge drivers shall call this function to report hot plug events when they
• • detect a change in the output status, when hot plug detection has been
• • enabled by the &drm_bridge_funcs.hpd_enable callback.
• • This function shall be called in a context that can sleep.
• */

+void drm_bridge_hpd_notify(struct drm_bridge *bridge,

• 	   enum drm_connector_status status)
  

+{

• mutex_lock(&bridge->hpd_mutex);
• if (bridge->hpd_cb)

• bridge->hpd_cb(bridge->hpd_data, status);
  

So this isn't quite what I had in mind. Instead something like this:

/* iterates over all bridges in the chain containing @bridge */ for_each_bridge(tmp_bridge, bridge) { if (tmp_bridge == bridge) continue; if (bridge->hpd_notify); bridge->hpd_notify(tmp_bridge, bridge, status); }

encoder = encoder_for_bridge(bridge); if (encoder->helper_private->bridge_hpd_notify) encoder->helper_private->bridge_hpd_notify(encoder, bridge, status);

dev = bridge->dev if (dev->mode_config.helper_private->bridge_hpd_notify) dev->mode_config.helper_private->bridge_hpd_notify(dev, bridge, status)

No register callback needed, no locking needed, everyone gets exactly the hpd they want/need.

...

• mutex_unlock(&bridge->hpd_mutex);

+} +EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);

#ifdef CONFIG_OF /**

• of_drm_find_bridge - find the bridge corresponding to the device node in

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 08dc15f93ded..b9445aa5b1ef 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -23,8 +23,9 @@ #ifndef __DRM_BRIDGE_H__ #define __DRM_BRIDGE_H__

-#include <linux/list.h> #include <linux/ctype.h> +#include <linux/list.h> +#include <linux/mutex.h> #include <drm/drm_mode_object.h> #include <drm/drm_modes.h>

@@ -334,6 +335,110 @@ struct drm_bridge_funcs { */ void (*atomic_post_disable)(struct drm_bridge *bridge, struct drm_atomic_state *state);

• /**

• * @detect:
  

• *
  

• * Check if anything is attached to the bridge output.
  

• *
  

• * This callback is optional, if not implemented the bridge will be
  

• * considered as always having a component attached to its output.
  

• * Bridges that implement this callback shall set the
  

• * DRM_BRIDGE_OP_DETECT flag in their &drm_bridge->ops.
  

• *
  

• * RETURNS:
  

• *
  

• * drm_connector_status indicating the bridge output status.
  

• */
  

• enum drm_connector_status (*detect)(struct drm_bridge *bridge);
• /**

• * @get_modes:
  

• *
  

• * Fill all modes currently valid for the sink into the &drm_connector
  

• * with drm_mode_probed_add().
  

• *
  

• * The @get_modes callback is mostly intended to support non-probable
  

• * displays such as many fixed panels. Bridges that support reading
  

• * EDID shall leave @get_modes unimplemented and implement the
  

• * &drm_bridge_funcs->get_edid callback instead.
  

• *
  

• * This callback is optional. Bridges that implement it shall set the
  

• * DRM_BRIDGE_OP_MODES flag in their &drm_bridge->ops.
  

• *
  

• * RETURNS:
  

• *
  

• * The number of modes added by calling drm_mode_probed_add().
  

• */
  

• int (*get_modes)(struct drm_bridge *bridge,

• 	 struct drm_connector *connector);
  

• /**

• * @get_edid:
  

• *
  

• * Read and parse the EDID data of the connected display.
  

• *
  

• * The @get_edid callback is the preferred way of reporting mode
  

• * information for a display connected to the bridge output. Bridges
  

• * that support readind EDID shall implement this callback and leave
  

• * the @get_modes callback unimplemented.
  

• *
  

• * The caller of this operation shall first verify the output
  

• * connection status and refrain from reading EDID from a disconnected
  

• * output.
  

• *
  

• * This callback is optional. Bridges that implement it shall set the
  

• * DRM_BRIDGE_OP_EDID flag in their &drm_bridge->ops.
  

• *
  

• * RETURNS:
  

• *
  

• * An edid structure newly allocated with kmalloc() (or similar) on
  

• * success, or NULL otherwise. The caller is responsible for freeing
  

• * the returned edid structure with kfree().
  

• */
  

• struct edid *(*get_edid)(struct drm_bridge *bridge,

• 		 struct drm_connector *connector);
  

It overlaps with get_modes, I guess presence of one ops should disallow presence of another one?

I am not really convinced we need this op at all, cannot we just assign some helper function to .get_modes cb, which will do the same?

Plan B): ditch ->get_edid, require that the driver has ->get_modes in that case, and require that if it has an edid it must fill out connector->info and connector->edid correctly.

Btw if a hpd happens, who's responible for making sure the edid/mode list in the connector is up-to-date? With your current callback design that's up to the callback, which doesn't feel great. Maybe drm_bridge_hpd_notify should guarantee that it'll first walk the connectors to update status and edid/mode list for the final drm_connector. And then instead of just passing the simple "status", it'll pass the connector, with everything correctly updated.

Otherwise everyone interested in that hpd signal will go and re-fetch the edid, which is not so awesome :-) -Daniel

Regards

Andrzej

...

• /**

• * @lost_hotplug:
  

• *
  

• * Notify the bridge of display disconnection.
  

• *
  

• * This callback is optional, it may be implemented by bridges that
  

• * need to be notified of display disconnection for internal reasons.
  

• * One use case is to reset the internal state of CEC controllers for
  

• * HDMI bridges.
  

• */
  

• void (*lost_hotplug)(struct drm_bridge *bridge);
• /**

• * @hpd_enable:
  

• *
  

• * Enable hot plug detection. From now on the bridge shall call
  

• * drm_bridge_hpd_notify() each time a change is detected in the output
  

• * connection status, until hot plug detection gets disabled with
  

• * @hpd_disable.
  

• *
  

• * This callback is optional and shall only be implemented by bridges
  

• * that support hot-plug notification without polling. Bridges that
  

• * implement it shall also implement the @hpd_disable callback and set
  

• * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops.
  

• */
  

• void (*hpd_enable)(struct drm_bridge *bridge);
• /**

• * @hpd_disable:
  

• *
  

• * Disable hot plug detection. Once this function returns the bridge
  

• * shall not call drm_bridge_hpd_notify() when a change in the output
  

• * connection status occurs.
  

• *
  

• * This callback is optional and shall only be implemented by bridges
  

• * that support hot-plug notification without polling. Bridges that
  

• * implement it shall also implement the @hpd_enable callback and set
  

• * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops.
  

• */
  

• void (*hpd_disable)(struct drm_bridge *bridge);

};

/** @@ -372,6 +477,38 @@ struct drm_bridge_timings { bool dual_link; };

+/**

• • enum drm_bridge_ops - Bitmask of operations supported by the bridge
• */

+enum drm_bridge_ops {

• /**

• * @DRM_BRIDGE_OP_DETECT: The bridge can detect displays connected to
  

• * its output. Bridges that set this flag shall implement the
  

• * &drm_bridge_funcs->detect callback.
  

• */
  

• DRM_BRIDGE_OP_DETECT = BIT(0),
• /**

• * @DRM_BRIDGE_OP_EDID: The bridge can retrieve the EDID of the display
  

• * connected to its output. Bridges that set this flag shall implement
  

• * the &drm_bridge_funcs->get_edid callback.
  

• */
  

• DRM_BRIDGE_OP_EDID = BIT(1),
• /**

• * @DRM_BRIDGE_OP_HPD: The bridge can detect hot-plug and hot-unplug
  

• * without requiring polling. Bridges that set this flag shall
  

• * implement the &drm_bridge_funcs->hpd_enable and
  

• * &drm_bridge_funcs->disable_hpd_cb callbacks.
  

• */
  

• DRM_BRIDGE_OP_HPD = BIT(2),
• /**

• * @DRM_BRIDGE_OP_MODES: The bridge can retrieving the modes supported
  

• * by the display at its output. This does not include readind EDID
  

• * which is separately covered by @DRM_BRIDGE_OP_EDID. Bridges that set
  

• * this flag shall implement the &drm_bridge_funcs->get_modes callback.
  

• */
  

• DRM_BRIDGE_OP_MODES = BIT(3),

+};

/**

• struct drm_bridge - central DRM bridge control structure

*/ @@ -398,6 +535,29 @@ struct drm_bridge { const struct drm_bridge_funcs *funcs; /** @driver_private: pointer to the bridge driver's internal context */ void *driver_private;

• /** @ops: bitmask of operations supported by the bridge */
• enum drm_bridge_ops ops;
• /**

• * @type: Type of the connection at the bridge output
  

• * (DRM_MODE_CONNECTOR_*). For bridges at the end of this chain this
  

• * identifies the type of connected display.
  

• */
  

• int type;
• /** private: */
• /**

• * @hpd_mutex: Protects the @hpd_cb and @hpd_data fields.
  

• */
  

• struct mutex hpd_mutex;
• /**

• * @hpd_cb: Hot plug detection callback, registered with
  

• * drm_bridge_hpd_enable().
  

• */
  

• void (*hpd_cb)(void *data, enum drm_connector_status status);
• /**

• * @hpd_data: Private data passed to the Hot plug detection callback
  

• * @hpd_cb.
  

• */
  

• void *hpd_data;

};

void drm_bridge_add(struct drm_bridge *bridge); @@ -428,6 +588,14 @@ void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge, void drm_atomic_bridge_enable(struct drm_bridge *bridge, struct drm_atomic_state *state);

+void drm_bridge_hpd_enable(struct drm_bridge *bridge,

• 	   void (*cb)(void *data,
  

• 		      enum drm_connector_status status),
  

• 	   void *data);
  

+void drm_bridge_hpd_disable(struct drm_bridge *bridge); +void drm_bridge_hpd_notify(struct drm_bridge *bridge,

• 	   enum drm_connector_status status);
  

#ifdef CONFIG_DRM_PANEL_BRIDGE struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, u32 connector_type);

On Thu, Jul 11, 2019 at 02:41:01PM +0200, Andrzej Hajda wrote:

On 11.07.2019 09:35, Daniel Vetter wrote:

...

On Wed, Jul 10, 2019 at 02:12:14PM +0200, Andrzej Hajda wrote:

...

Hi Laurent,

I like the approach, current practice when almost every bridge should optionally implement connector, or alternatively downstream bridge or panel is very painful.

Yeah I think this looks mostly reasonable. Some api design comments on top of Andrzej', with the fair warning that I didn't bother to read up on how it's all used in the end. I probably should go and do that, at least to get a feeling for what your hpd_cb usually does.

...

More comments inlined.

On 07.07.2019 20:18, Laurent Pinchart wrote:

...

To support implementation of DRM connectors on top of DRM bridges instead of by bridges, the drm_bridge needs to expose new operations and data:

• Output detection, hot-plug notification, mode retrieval and EDID retrieval operations
• Bitmask of supported operations

Why do we need these bitmask at all? Why cannot we rely on presence of operation's callback?

Yeah also not a huge fan of these bitmasks. Smells like DRIVER_GEM|DRIVER_MODESET, and I personally really hate those. Easy to add, generally good excuse to not have to think through the design between different parts of drivers - "just" add another flag.

...

...

• Bridge output type

Add and document these.

Three new bridge helper functions are also added to handle hot plug notification in a way that is as transparent as possible for the bridges.

Documentation of new opses does not explain how it should cooperate with bridge chaining, I suppose they should be chained explicitly, am I right? More comments about it later.

...

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

drivers/gpu/drm/drm_bridge.c | 92 +++++++++++++++++++ include/drm/drm_bridge.h | 170 ++++++++++++++++++++++++++++++++++- 2 files changed, 261 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 519577f363e3..3c2a255df7af 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -70,6 +70,8 @@ static LIST_HEAD(bridge_list); */ void drm_bridge_add(struct drm_bridge *bridge) {

• mutex_init(&bridge->hpd_mutex);
• mutex_lock(&bridge_lock); list_add_tail(&bridge->list, &bridge_list); mutex_unlock(&bridge_lock);

@@ -86,6 +88,8 @@ void drm_bridge_remove(struct drm_bridge *bridge) mutex_lock(&bridge_lock); list_del_init(&bridge->list); mutex_unlock(&bridge_lock);

• mutex_destroy(&bridge->hpd_mutex);

} EXPORT_SYMBOL(drm_bridge_remove);

@@ -463,6 +467,94 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_atomic_bridge_enable);

+/**

• • drm_bridge_hpd_enable - enable hot plug detection for the bridge
• • @bridge: bridge control structure
• • @cb: hot-plug detection callback
• • @data: data to be passed to the hot-plug detection callback
• • Call &drm_bridge_funcs.hpd_enable and register the given @cb and @data as
• • hot plug notification callback. From now on the @cb will be called with
• • @data when an output status change is detected by the bridge, until hot plug
• • notification gets disabled with drm_bridge_hpd_disable().
• • Hot plug detection is supported only if the DRM_BRIDGE_OP_HPD flag is set in
• • bridge->ops. This function shall not be called when the flag is not set.
• • Only one hot plug detection callback can be registered at a time, it is an
• • error to call this function when hot plug detection is already enabled for
• • the bridge.
• */

To simplify architecture maybe would be better to enable hpd just on bridge attach:

bridge->hpd_cb = cb;

bridge->hpd_data = data;

ret = drm_bridge_attach(...);

Yeah I like this more. The other problem here is, what if you need more than 1 callback registers on the same bridge hdp signal?

...

This way we could avoid adding new callbacks hpd_(enable|disable) without big sacrifices.

One more thing: HPD in DisplayPort/HDMI beside signalling plug/unplug, notifies about sink status change, how it translates to this cb?

...

+void drm_bridge_hpd_enable(struct drm_bridge *bridge,

• 	   void (*cb)(void *data,
  

• 		      enum drm_connector_status status),
  

• 	   void *data)
  

+{

• if (!bridge || !bridge->funcs->hpd_enable)

• return;
  

• mutex_lock(&bridge->hpd_mutex);
• if (WARN(bridge->hpd_cb, "Hot plug detection already enabled\n"))

• goto unlock;
  

• bridge->hpd_cb = cb;
• bridge->hpd_data = data;
• bridge->funcs->hpd_enable(bridge);

+unlock:

• mutex_unlock(&bridge->hpd_mutex);

+} +EXPORT_SYMBOL_GPL(drm_bridge_hpd_enable);

+/**

• • drm_bridge_hpd_disable - disable hot plug detection for the bridge
• • @bridge: bridge control structure
• • Call &drm_bridge_funcs.hpd_disable and unregister the hot plug detection
• • callback previously registered with drm_bridge_hpd_enable(). Once this
• • function returns the callback will not be called by the bridge when an
• • output status change occurs.
• • Hot plug detection is supported only if the DRM_BRIDGE_OP_HPD flag is set in
• • bridge->ops. This function shall not be called when the flag is not set.
• */

+void drm_bridge_hpd_disable(struct drm_bridge *bridge) +{

• if (!bridge || !bridge->funcs->hpd_disable)

• return;
  

• mutex_lock(&bridge->hpd_mutex);
• bridge->funcs->hpd_disable(bridge);
• bridge->hpd_cb = NULL;
• bridge->hpd_data = NULL;
• mutex_unlock(&bridge->hpd_mutex);

+} +EXPORT_SYMBOL_GPL(drm_bridge_hpd_disable);

+/**

• • drm_bridge_hpd_notify - notify hot plug detection events
• • @bridge: bridge control structure
• • @status: output connection status
• • Bridge drivers shall call this function to report hot plug events when they
• • detect a change in the output status, when hot plug detection has been
• • enabled by the &drm_bridge_funcs.hpd_enable callback.
• • This function shall be called in a context that can sleep.
• */

+void drm_bridge_hpd_notify(struct drm_bridge *bridge,

• 	   enum drm_connector_status status)
  

+{

• mutex_lock(&bridge->hpd_mutex);
• if (bridge->hpd_cb)

• bridge->hpd_cb(bridge->hpd_data, status);
  

So this isn't quite what I had in mind. Instead something like this:

/* iterates over all bridges in the chain containing @bridge */ for_each_bridge(tmp_bridge, bridge) { if (tmp_bridge == bridge) continue; if (bridge->hpd_notify); bridge->hpd_notify(tmp_bridge, bridge, status); }

encoder = encoder_for_bridge(bridge); if (encoder->helper_private->bridge_hpd_notify) encoder->helper_private->bridge_hpd_notify(encoder, bridge, status);

dev = bridge->dev if (dev->mode_config.helper_private->bridge_hpd_notify) dev->mode_config.helper_private->bridge_hpd_notify(dev, bridge, status)

No register callback needed, no locking needed, everyone gets exactly the hpd they want/need.

As I understand you want to notify every member of the pipeline.

I think it should be enough to notify only the source, and then source should decide if/when the hpd should be propagated upstream.

It looks more generic for me.

I'm not parsing ... do you think my idea is more generic and useful, or the one from Laurent? Kinda confused here. -Daniel

Regards

Andrzej

...

...

...

• mutex_unlock(&bridge->hpd_mutex);

+} +EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);

#ifdef CONFIG_OF /**

• of_drm_find_bridge - find the bridge corresponding to the device node in

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 08dc15f93ded..b9445aa5b1ef 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -23,8 +23,9 @@ #ifndef __DRM_BRIDGE_H__ #define __DRM_BRIDGE_H__

-#include <linux/list.h> #include <linux/ctype.h> +#include <linux/list.h> +#include <linux/mutex.h> #include <drm/drm_mode_object.h> #include <drm/drm_modes.h>

@@ -334,6 +335,110 @@ struct drm_bridge_funcs { */ void (*atomic_post_disable)(struct drm_bridge *bridge, struct drm_atomic_state *state);

• /**

• * @detect:
  

• *
  

• * Check if anything is attached to the bridge output.
  

• *
  

• * This callback is optional, if not implemented the bridge will be
  

• * considered as always having a component attached to its output.
  

• * Bridges that implement this callback shall set the
  

• * DRM_BRIDGE_OP_DETECT flag in their &drm_bridge->ops.
  

• *
  

• * RETURNS:
  

• *
  

• * drm_connector_status indicating the bridge output status.
  

• */
  

• enum drm_connector_status (*detect)(struct drm_bridge *bridge);
• /**

• * @get_modes:
  

• *
  

• * Fill all modes currently valid for the sink into the &drm_connector
  

• * with drm_mode_probed_add().
  

• *
  

• * The @get_modes callback is mostly intended to support non-probable
  

• * displays such as many fixed panels. Bridges that support reading
  

• * EDID shall leave @get_modes unimplemented and implement the
  

• * &drm_bridge_funcs->get_edid callback instead.
  

• *
  

• * This callback is optional. Bridges that implement it shall set the
  

• * DRM_BRIDGE_OP_MODES flag in their &drm_bridge->ops.
  

• *
  

• * RETURNS:
  

• *
  

• * The number of modes added by calling drm_mode_probed_add().
  

• */
  

• int (*get_modes)(struct drm_bridge *bridge,

• 	 struct drm_connector *connector);
  

• /**

• * @get_edid:
  

• *
  

• * Read and parse the EDID data of the connected display.
  

• *
  

• * The @get_edid callback is the preferred way of reporting mode
  

• * information for a display connected to the bridge output. Bridges
  

• * that support readind EDID shall implement this callback and leave
  

• * the @get_modes callback unimplemented.
  

• *
  

• * The caller of this operation shall first verify the output
  

• * connection status and refrain from reading EDID from a disconnected
  

• * output.
  

• *
  

• * This callback is optional. Bridges that implement it shall set the
  

• * DRM_BRIDGE_OP_EDID flag in their &drm_bridge->ops.
  

• *
  

• * RETURNS:
  

• *
  

• * An edid structure newly allocated with kmalloc() (or similar) on
  

• * success, or NULL otherwise. The caller is responsible for freeing
  

• * the returned edid structure with kfree().
  

• */
  

• struct edid *(*get_edid)(struct drm_bridge *bridge,

• 		 struct drm_connector *connector);
  

It overlaps with get_modes, I guess presence of one ops should disallow presence of another one?

I am not really convinced we need this op at all, cannot we just assign some helper function to .get_modes cb, which will do the same?

Plan B): ditch ->get_edid, require that the driver has ->get_modes in that case, and require that if it has an edid it must fill out connector->info and connector->edid correctly.

Btw if a hpd happens, who's responible for making sure the edid/mode list in the connector is up-to-date? With your current callback design that's up to the callback, which doesn't feel great. Maybe drm_bridge_hpd_notify should guarantee that it'll first walk the connectors to update status and edid/mode list for the final drm_connector. And then instead of just passing the simple "status", it'll pass the connector, with everything correctly updated.

Otherwise everyone interested in that hpd signal will go and re-fetch the edid, which is not so awesome :-) -Daniel

...

Regards

Andrzej

...

• /**

• * @lost_hotplug:
  

• *
  

• * Notify the bridge of display disconnection.
  

• *
  

• * This callback is optional, it may be implemented by bridges that
  

• * need to be notified of display disconnection for internal reasons.
  

• * One use case is to reset the internal state of CEC controllers for
  

• * HDMI bridges.
  

• */
  

• void (*lost_hotplug)(struct drm_bridge *bridge);
• /**

• * @hpd_enable:
  

• *
  

• * Enable hot plug detection. From now on the bridge shall call
  

• * drm_bridge_hpd_notify() each time a change is detected in the output
  

• * connection status, until hot plug detection gets disabled with
  

• * @hpd_disable.
  

• *
  

• * This callback is optional and shall only be implemented by bridges
  

• * that support hot-plug notification without polling. Bridges that
  

• * implement it shall also implement the @hpd_disable callback and set
  

• * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops.
  

• */
  

• void (*hpd_enable)(struct drm_bridge *bridge);
• /**

• * @hpd_disable:
  

• *
  

• * Disable hot plug detection. Once this function returns the bridge
  

• * shall not call drm_bridge_hpd_notify() when a change in the output
  

• * connection status occurs.
  

• *
  

• * This callback is optional and shall only be implemented by bridges
  

• * that support hot-plug notification without polling. Bridges that
  

• * implement it shall also implement the @hpd_enable callback and set
  

• * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops.
  

• */
  

• void (*hpd_disable)(struct drm_bridge *bridge);

};

/** @@ -372,6 +477,38 @@ struct drm_bridge_timings { bool dual_link; };

+/**

• • enum drm_bridge_ops - Bitmask of operations supported by the bridge
• */

+enum drm_bridge_ops {

• /**

• * @DRM_BRIDGE_OP_DETECT: The bridge can detect displays connected to
  

• * its output. Bridges that set this flag shall implement the
  

• * &drm_bridge_funcs->detect callback.
  

• */
  

• DRM_BRIDGE_OP_DETECT = BIT(0),
• /**

• * @DRM_BRIDGE_OP_EDID: The bridge can retrieve the EDID of the display
  

• * connected to its output. Bridges that set this flag shall implement
  

• * the &drm_bridge_funcs->get_edid callback.
  

• */
  

• DRM_BRIDGE_OP_EDID = BIT(1),
• /**

• * @DRM_BRIDGE_OP_HPD: The bridge can detect hot-plug and hot-unplug
  

• * without requiring polling. Bridges that set this flag shall
  

• * implement the &drm_bridge_funcs->hpd_enable and
  

• * &drm_bridge_funcs->disable_hpd_cb callbacks.
  

• */
  

• DRM_BRIDGE_OP_HPD = BIT(2),
• /**

• * @DRM_BRIDGE_OP_MODES: The bridge can retrieving the modes supported
  

• * by the display at its output. This does not include readind EDID
  

• * which is separately covered by @DRM_BRIDGE_OP_EDID. Bridges that set
  

• * this flag shall implement the &drm_bridge_funcs->get_modes callback.
  

• */
  

• DRM_BRIDGE_OP_MODES = BIT(3),

+};

/**

• struct drm_bridge - central DRM bridge control structure

*/ @@ -398,6 +535,29 @@ struct drm_bridge { const struct drm_bridge_funcs *funcs; /** @driver_private: pointer to the bridge driver's internal context */ void *driver_private;

• /** @ops: bitmask of operations supported by the bridge */
• enum drm_bridge_ops ops;
• /**

• * @type: Type of the connection at the bridge output
  

• * (DRM_MODE_CONNECTOR_*). For bridges at the end of this chain this
  

• * identifies the type of connected display.
  

• */
  

• int type;
• /** private: */
• /**

• * @hpd_mutex: Protects the @hpd_cb and @hpd_data fields.
  

• */
  

• struct mutex hpd_mutex;
• /**

• * @hpd_cb: Hot plug detection callback, registered with
  

• * drm_bridge_hpd_enable().
  

• */
  

• void (*hpd_cb)(void *data, enum drm_connector_status status);
• /**

• * @hpd_data: Private data passed to the Hot plug detection callback
  

• * @hpd_cb.
  

• */
  

• void *hpd_data;

};

void drm_bridge_add(struct drm_bridge *bridge); @@ -428,6 +588,14 @@ void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge, void drm_atomic_bridge_enable(struct drm_bridge *bridge, struct drm_atomic_state *state);

+void drm_bridge_hpd_enable(struct drm_bridge *bridge,

• 	   void (*cb)(void *data,
  

• 		      enum drm_connector_status status),
  

• 	   void *data);
  

+void drm_bridge_hpd_disable(struct drm_bridge *bridge); +void drm_bridge_hpd_notify(struct drm_bridge *bridge,

• 	   enum drm_connector_status status);
  

#ifdef CONFIG_DRM_PANEL_BRIDGE struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, u32 connector_type);