Hello,
This patch series converts the R-Car DU driver to use the DRM bridge connector helper drm_bridge_connector_init().
The bulk of the v1 series was converting the adv7511, simple-bridge and dw-hdmi drivers to make connector creation optional (through the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag), and have already been merged. v2 includes the remaining patches.
Patch 1/4 addresses the rcar-lvds driver. Instead of implementing direct support for DRM_BRIDGE_ATTACH_NO_CONNECTOR, it simply removes code that shouldn't have been in the driver in the first place by switching to the panel bridge helper.
Patches 2/4 then adds support to the dw-hdmi driver to attach to a downstream bridge if one is specified in DT. As the DT port number corresponding to the video output differs between platforms that integrate the dw-hdmi (some of them even don't have a video output port, which should probably be fixed, but that's out of scope for this series), the port number has to be specified by the platform glue layer. Patch 3/4 does so for the R-Car dw-hdmi driver.
Patch 4/4 finally makes use of the drm_bridge_connector_init() helper.
The series depends on "[PATCH 27/27] drm: Add default modes for connectors in unknown state" ([1]) to avoid a breakage on the VGA output on R-Car Gen3 platforms. That patch has already been approved, and I'll get it merged as a prerequisite.
The series has been tested on the Renesas R-Car Salvator-XS and Draak boards with the VGA, HDMI and LVDS outputs.
[1] https://lore.kernel.org/dri-devel/20200526011505.31884-28-laurent.pinchart+r...
Laurent Pinchart (4): drm: rcar-du: lvds: Convert to DRM panel bridge helper drm: bridge: dw-hdmi: Attach to next bridge if available drm: rcar-du: dw-hdmi: Set output port number drm: rcar-du: Use drm_bridge_connector_init() helper
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 54 +++++++++- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++- drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 1 + drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++------------------- include/drm/bridge/dw_hdmi.h | 2 + 5 files changed, 88 insertions(+), 114 deletions(-)
Replace the manual panel handling with usage of the DRM panel bridge helper. This simplifies the driver, and brings support for DRM_BRIDGE_ATTACH_NO_CONNECTOR as an added bonus.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++------------------------- 1 file changed, 12 insertions(+), 108 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 70dbbe44bb23..1b360e06658c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -63,7 +63,6 @@ struct rcar_lvds { struct drm_bridge bridge;
struct drm_bridge *next_bridge; - struct drm_connector connector; struct drm_panel *panel;
void __iomem *mmio; @@ -80,73 +79,11 @@ struct rcar_lvds { #define bridge_to_rcar_lvds(b) \ container_of(b, struct rcar_lvds, bridge)
-#define connector_to_rcar_lvds(c) \ - container_of(c, struct rcar_lvds, connector) - static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data) { iowrite32(data, lvds->mmio + reg); }
-/* ----------------------------------------------------------------------------- - * Connector & Panel - */ - -static int rcar_lvds_connector_get_modes(struct drm_connector *connector) -{ - struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); - - return drm_panel_get_modes(lvds->panel, connector); -} - -static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, - struct drm_atomic_state *state) -{ - struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); - const struct drm_display_mode *panel_mode; - struct drm_connector_state *conn_state; - struct drm_crtc_state *crtc_state; - - conn_state = drm_atomic_get_new_connector_state(state, connector); - if (!conn_state->crtc) - return 0; - - if (list_empty(&connector->modes)) { - dev_dbg(lvds->dev, "connector: empty modes list\n"); - return -EINVAL; - } - - panel_mode = list_first_entry(&connector->modes, - struct drm_display_mode, head); - - /* We're not allowed to modify the resolution. */ - crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); - - if (crtc_state->mode.hdisplay != panel_mode->hdisplay || - crtc_state->mode.vdisplay != panel_mode->vdisplay) - return -EINVAL; - - /* The flat panel mode is fixed, just copy it to the adjusted mode. */ - drm_mode_copy(&crtc_state->adjusted_mode, panel_mode); - - return 0; -} - -static const struct drm_connector_helper_funcs rcar_lvds_conn_helper_funcs = { - .get_modes = rcar_lvds_connector_get_modes, - .atomic_check = rcar_lvds_connector_atomic_check, -}; - -static const struct drm_connector_funcs rcar_lvds_conn_funcs = { - .reset = drm_atomic_helper_connector_reset, - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - /* ----------------------------------------------------------------------------- * PLL Setup */ @@ -583,11 +520,6 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge, /* Turn the output on. */ lvdcr0 |= LVDCR0_LVRES; rcar_lvds_write(lvds, LVDCR0, lvdcr0); - - if (lvds->panel) { - drm_panel_prepare(lvds->panel); - drm_panel_enable(lvds->panel); - } }
static void rcar_lvds_atomic_enable(struct drm_bridge *bridge, @@ -609,11 +541,6 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge, { struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
- if (lvds->panel) { - drm_panel_disable(lvds->panel); - drm_panel_unprepare(lvds->panel); - } - rcar_lvds_write(lvds, LVDCR0, 0); rcar_lvds_write(lvds, LVDCR1, 0); rcar_lvds_write(lvds, LVDPLLCR, 0); @@ -648,45 +575,13 @@ static int rcar_lvds_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge); - struct drm_connector *connector = &lvds->connector; - struct drm_encoder *encoder = bridge->encoder; - int ret;
- /* If we have a next bridge just attach it. */ - if (lvds->next_bridge) - return drm_bridge_attach(bridge->encoder, lvds->next_bridge, - bridge, flags); - - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } - - /* Otherwise if we have a panel, create a connector. */ - if (!lvds->panel) - return 0; - - ret = drm_connector_init(bridge->dev, connector, &rcar_lvds_conn_funcs, - DRM_MODE_CONNECTOR_LVDS); - if (ret < 0) - return ret; - - drm_connector_helper_add(connector, &rcar_lvds_conn_helper_funcs); - - ret = drm_connector_attach_encoder(connector, encoder); - if (ret < 0) - return ret; - - return 0; -} - -static void rcar_lvds_detach(struct drm_bridge *bridge) -{ + return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge, + flags); }
static const struct drm_bridge_funcs rcar_lvds_bridge_ops = { .attach = rcar_lvds_attach, - .detach = rcar_lvds_detach, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset, @@ -759,7 +654,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) * that we are expected to generate even pixels from the primary * encoder, and odd pixels from the companion encoder. */ - if (lvds->next_bridge && lvds->next_bridge->timings && + if (lvds->next_bridge->timings && lvds->next_bridge->timings->dual_link) lvds->link_type = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS; else @@ -811,6 +706,15 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds) if (ret) goto done;
+ if (lvds->panel) { + lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev, + lvds->panel); + if (IS_ERR_OR_NULL(lvds->next_bridge)) { + ret = -EINVAL; + goto done; + } + } + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) ret = rcar_lvds_parse_dt_companion(lvds);
Hi Laurent,
On Wed, Dec 16, 2020 at 02:50:18AM +0200, Laurent Pinchart wrote:
Replace the manual panel handling with usage of the DRM panel bridge helper. This simplifies the driver, and brings support for DRM_BRIDGE_ATTACH_NO_CONNECTOR as an added bonus.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++------------------------- 1 file changed, 12 insertions(+), 108 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 70dbbe44bb23..1b360e06658c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -63,7 +63,6 @@ struct rcar_lvds { struct drm_bridge bridge;
struct drm_bridge *next_bridge;
struct drm_connector connector; struct drm_panel *panel;
void __iomem *mmio;
@@ -80,73 +79,11 @@ struct rcar_lvds { #define bridge_to_rcar_lvds(b) \ container_of(b, struct rcar_lvds, bridge)
-#define connector_to_rcar_lvds(c) \
- container_of(c, struct rcar_lvds, connector)
static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data) { iowrite32(data, lvds->mmio + reg); }
-/* -----------------------------------------------------------------------------
- Connector & Panel
- */
-static int rcar_lvds_connector_get_modes(struct drm_connector *connector) -{
- struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
- return drm_panel_get_modes(lvds->panel, connector);
-}
-static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
struct drm_atomic_state *state)
-{
- struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
- const struct drm_display_mode *panel_mode;
- struct drm_connector_state *conn_state;
- struct drm_crtc_state *crtc_state;
- conn_state = drm_atomic_get_new_connector_state(state, connector);
- if (!conn_state->crtc)
return 0;
- if (list_empty(&connector->modes)) {
dev_dbg(lvds->dev, "connector: empty modes list\n");
return -EINVAL;
- }
- panel_mode = list_first_entry(&connector->modes,
struct drm_display_mode, head);
- /* We're not allowed to modify the resolution. */
- crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
- if (IS_ERR(crtc_state))
return PTR_ERR(crtc_state);
- if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
crtc_state->mode.vdisplay != panel_mode->vdisplay)
return -EINVAL;
- /* The flat panel mode is fixed, just copy it to the adjusted mode. */
- drm_mode_copy(&crtc_state->adjusted_mode, panel_mode);
- return 0;
-}
-static const struct drm_connector_helper_funcs rcar_lvds_conn_helper_funcs = {
- .get_modes = rcar_lvds_connector_get_modes,
- .atomic_check = rcar_lvds_connector_atomic_check,
-};
-static const struct drm_connector_funcs rcar_lvds_conn_funcs = {
- .reset = drm_atomic_helper_connector_reset,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = drm_connector_cleanup,
- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
/* -----------------------------------------------------------------------------
- PLL Setup
*/ @@ -583,11 +520,6 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge, /* Turn the output on. */ lvdcr0 |= LVDCR0_LVRES; rcar_lvds_write(lvds, LVDCR0, lvdcr0);
- if (lvds->panel) {
drm_panel_prepare(lvds->panel);
drm_panel_enable(lvds->panel);
- }
}
static void rcar_lvds_atomic_enable(struct drm_bridge *bridge, @@ -609,11 +541,6 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge, { struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
- if (lvds->panel) {
drm_panel_disable(lvds->panel);
drm_panel_unprepare(lvds->panel);
- }
- rcar_lvds_write(lvds, LVDCR0, 0); rcar_lvds_write(lvds, LVDCR1, 0); rcar_lvds_write(lvds, LVDPLLCR, 0);
@@ -648,45 +575,13 @@ static int rcar_lvds_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
struct drm_connector *connector = &lvds->connector;
struct drm_encoder *encoder = bridge->encoder;
int ret;
/* If we have a next bridge just attach it. */
if (lvds->next_bridge)
return drm_bridge_attach(bridge->encoder, lvds->next_bridge,
bridge, flags);
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
DRM_ERROR("Fix bridge driver to make connector optional!");
return -EINVAL;
}
/* Otherwise if we have a panel, create a connector. */
if (!lvds->panel)
return 0;
ret = drm_connector_init(bridge->dev, connector, &rcar_lvds_conn_funcs,
DRM_MODE_CONNECTOR_LVDS);
if (ret < 0)
return ret;
drm_connector_helper_add(connector, &rcar_lvds_conn_helper_funcs);
ret = drm_connector_attach_encoder(connector, encoder);
if (ret < 0)
return ret;
return 0;
-}
-static void rcar_lvds_detach(struct drm_bridge *bridge) -{
- return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge,
flags);
}
static const struct drm_bridge_funcs rcar_lvds_bridge_ops = { .attach = rcar_lvds_attach,
- .detach = rcar_lvds_detach, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset,
@@ -759,7 +654,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) * that we are expected to generate even pixels from the primary * encoder, and odd pixels from the companion encoder. */
if (lvds->next_bridge && lvds->next_bridge->timings &&
lvds->next_bridge->timings->dual_link) lvds->link_type = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS; elseif (lvds->next_bridge->timings &&
@@ -811,6 +706,15 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds) if (ret) goto done;
- if (lvds->panel) {
lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
lvds->panel);
Reading the devm_drm_panel_bridge_add() function documentation:
* devm_drm_panel_bridge_add - Creates a managed &drm_bridge and &drm_connector
Doesn't this conflict with the drm_bridge_connector_init() called by the encoder in [4/4] ?
if (IS_ERR_OR_NULL(lvds->next_bridge)) {
ret = -EINVAL;
goto done;
}
- }
- if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) ret = rcar_lvds_parse_dt_companion(lvds);
-- Regards,
Laurent Pinchart
Hi Jacopo,
On Wed, Dec 16, 2020 at 02:16:56PM +0100, Jacopo Mondi wrote:
On Wed, Dec 16, 2020 at 02:50:18AM +0200, Laurent Pinchart wrote:
Replace the manual panel handling with usage of the DRM panel bridge helper. This simplifies the driver, and brings support for DRM_BRIDGE_ATTACH_NO_CONNECTOR as an added bonus.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++------------------------- 1 file changed, 12 insertions(+), 108 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 70dbbe44bb23..1b360e06658c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -63,7 +63,6 @@ struct rcar_lvds { struct drm_bridge bridge;
struct drm_bridge *next_bridge;
struct drm_connector connector; struct drm_panel *panel;
void __iomem *mmio;
@@ -80,73 +79,11 @@ struct rcar_lvds { #define bridge_to_rcar_lvds(b) \ container_of(b, struct rcar_lvds, bridge)
-#define connector_to_rcar_lvds(c) \
- container_of(c, struct rcar_lvds, connector)
static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data) { iowrite32(data, lvds->mmio + reg); }
-/* -----------------------------------------------------------------------------
- Connector & Panel
- */
-static int rcar_lvds_connector_get_modes(struct drm_connector *connector) -{
- struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
- return drm_panel_get_modes(lvds->panel, connector);
-}
-static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
struct drm_atomic_state *state)
-{
- struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
- const struct drm_display_mode *panel_mode;
- struct drm_connector_state *conn_state;
- struct drm_crtc_state *crtc_state;
- conn_state = drm_atomic_get_new_connector_state(state, connector);
- if (!conn_state->crtc)
return 0;
- if (list_empty(&connector->modes)) {
dev_dbg(lvds->dev, "connector: empty modes list\n");
return -EINVAL;
- }
- panel_mode = list_first_entry(&connector->modes,
struct drm_display_mode, head);
- /* We're not allowed to modify the resolution. */
- crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
- if (IS_ERR(crtc_state))
return PTR_ERR(crtc_state);
- if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
crtc_state->mode.vdisplay != panel_mode->vdisplay)
return -EINVAL;
- /* The flat panel mode is fixed, just copy it to the adjusted mode. */
- drm_mode_copy(&crtc_state->adjusted_mode, panel_mode);
- return 0;
-}
-static const struct drm_connector_helper_funcs rcar_lvds_conn_helper_funcs = {
- .get_modes = rcar_lvds_connector_get_modes,
- .atomic_check = rcar_lvds_connector_atomic_check,
-};
-static const struct drm_connector_funcs rcar_lvds_conn_funcs = {
- .reset = drm_atomic_helper_connector_reset,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = drm_connector_cleanup,
- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
/* -----------------------------------------------------------------------------
- PLL Setup
*/ @@ -583,11 +520,6 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge, /* Turn the output on. */ lvdcr0 |= LVDCR0_LVRES; rcar_lvds_write(lvds, LVDCR0, lvdcr0);
- if (lvds->panel) {
drm_panel_prepare(lvds->panel);
drm_panel_enable(lvds->panel);
- }
}
static void rcar_lvds_atomic_enable(struct drm_bridge *bridge, @@ -609,11 +541,6 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge, { struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
- if (lvds->panel) {
drm_panel_disable(lvds->panel);
drm_panel_unprepare(lvds->panel);
- }
- rcar_lvds_write(lvds, LVDCR0, 0); rcar_lvds_write(lvds, LVDCR1, 0); rcar_lvds_write(lvds, LVDPLLCR, 0);
@@ -648,45 +575,13 @@ static int rcar_lvds_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
struct drm_connector *connector = &lvds->connector;
struct drm_encoder *encoder = bridge->encoder;
int ret;
/* If we have a next bridge just attach it. */
if (lvds->next_bridge)
return drm_bridge_attach(bridge->encoder, lvds->next_bridge,
bridge, flags);
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
DRM_ERROR("Fix bridge driver to make connector optional!");
return -EINVAL;
}
/* Otherwise if we have a panel, create a connector. */
if (!lvds->panel)
return 0;
ret = drm_connector_init(bridge->dev, connector, &rcar_lvds_conn_funcs,
DRM_MODE_CONNECTOR_LVDS);
if (ret < 0)
return ret;
drm_connector_helper_add(connector, &rcar_lvds_conn_helper_funcs);
ret = drm_connector_attach_encoder(connector, encoder);
if (ret < 0)
return ret;
return 0;
-}
-static void rcar_lvds_detach(struct drm_bridge *bridge) -{
- return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge,
flags);
}
static const struct drm_bridge_funcs rcar_lvds_bridge_ops = { .attach = rcar_lvds_attach,
- .detach = rcar_lvds_detach, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset,
@@ -759,7 +654,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) * that we are expected to generate even pixels from the primary * encoder, and odd pixels from the companion encoder. */
if (lvds->next_bridge && lvds->next_bridge->timings &&
lvds->next_bridge->timings->dual_link) lvds->link_type = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS; elseif (lvds->next_bridge->timings &&
@@ -811,6 +706,15 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds) if (ret) goto done;
- if (lvds->panel) {
lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
lvds->panel);
Reading the devm_drm_panel_bridge_add() function documentation:
- devm_drm_panel_bridge_add - Creates a managed &drm_bridge and &drm_connector
Doesn't this conflict with the drm_bridge_connector_init() called by the encoder in [4/4] ?
It would, if the documentation was right :-) The function only creates a bridge. A connector will only be created when the bridge is attached without DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Would you like to send a patch to fix the documentation ?
if (IS_ERR_OR_NULL(lvds->next_bridge)) {
ret = -EINVAL;
goto done;
}
- }
- if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) ret = rcar_lvds_parse_dt_companion(lvds);
Hi Laurent,
On Wed, Dec 16, 2020 at 03:49:24PM +0200, Laurent Pinchart wrote:
Hi Jacopo,
- if (lvds->panel) {
lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
lvds->panel);
Reading the devm_drm_panel_bridge_add() function documentation:
- devm_drm_panel_bridge_add - Creates a managed &drm_bridge and &drm_connector
Doesn't this conflict with the drm_bridge_connector_init() called by the encoder in [4/4] ?
It would, if the documentation was right :-) The function only creates a bridge. A connector will only be created when the bridge is attached without DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Well, reading it again, it is kind of implied that if NO_CONNECTOR is given to the bridge, no connector will be registered at all.
Would you like to send a patch to fix the documentation ?
if (IS_ERR_OR_NULL(lvds->next_bridge)) {
ret = -EINVAL;
goto done;
}
- }
- if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) ret = rcar_lvds_parse_dt_companion(lvds);
-- Regards,
Laurent Pinchart
On all platforms except i.MX and Rockchip, the dw-hdmi DT bindings require a video output port connected to an HDMI sink (most likely an HDMI connector, in rare cases another bridges converting HDMI to another protocol). For those platforms, retrieve the next bridge and attach it from the dw-hdmi bridge attach handler.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Reviewed-by: Neil Armstrong narmstrong@baylibre.com --- Changes since v1:
- Make missing endpoint a fatal error --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 54 ++++++++++++++++++++++- include/drm/bridge/dw_hdmi.h | 2 + 2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 0c79a9ba48bb..a8393ecd3d19 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -143,6 +143,7 @@ struct dw_hdmi_phy_data { struct dw_hdmi { struct drm_connector connector; struct drm_bridge bridge; + struct drm_bridge *next_bridge;
unsigned int version;
@@ -2791,7 +2792,8 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge, struct dw_hdmi *hdmi = bridge->driver_private;
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) - return 0; + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, + bridge, flags);
return dw_hdmi_connector_create(hdmi); } @@ -3176,6 +3178,52 @@ static void dw_hdmi_init_hw(struct dw_hdmi *hdmi) /* ----------------------------------------------------------------------------- * Probe/remove API, used from platforms based on the DRM bridge API. */ + +static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi) +{ + struct device_node *endpoint; + struct device_node *remote; + + if (!hdmi->plat_data->output_port) + return 0; + + endpoint = of_graph_get_endpoint_by_regs(hdmi->dev->of_node, + hdmi->plat_data->output_port, + -1); + if (!endpoint) { + /* + * On platforms whose bindings don't make the output port + * mandatory (such as Rockchip) the plat_data->output_port + * field isn't set, so it's safe to make this a fatal error. + */ + dev_err(hdmi->dev, "Missing endpoint in port@%u\n", + hdmi->plat_data->output_port); + return -ENODEV; + } + + remote = of_graph_get_remote_port_parent(endpoint); + of_node_put(endpoint); + if (!remote) { + dev_err(hdmi->dev, "Endpoint in port@%u unconnected\n", + hdmi->plat_data->output_port); + return -ENODEV; + } + + if (!of_device_is_available(remote)) { + dev_err(hdmi->dev, "port@%u remote device is disabled\n", + hdmi->plat_data->output_port); + of_node_put(remote); + return -ENODEV; + } + + hdmi->next_bridge = of_drm_find_bridge(remote); + of_node_put(remote); + if (!hdmi->next_bridge) + return -EPROBE_DEFER; + + return 0; +} + struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, const struct dw_hdmi_plat_data *plat_data) { @@ -3212,6 +3260,10 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, mutex_init(&hdmi->cec_notifier_mutex); spin_lock_init(&hdmi->audio_lock);
+ ret = dw_hdmi_parse_dt(hdmi); + if (ret < 0) + return ERR_PTR(ret); + ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); if (ddc_node) { hdmi->ddc = of_get_i2c_adapter_by_node(ddc_node); diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index ea34ca146b82..8ebeb65d6371 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -126,6 +126,8 @@ struct dw_hdmi_phy_ops { struct dw_hdmi_plat_data { struct regmap *regm;
+ unsigned int output_port; + unsigned long input_bus_encoding; bool use_drm_infoframe; bool ycbcr_420_allowed;
Report the DT output port number in dw_hdmi_plat_data to connect to the next bridge in the dw-hdmi driver.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c index 7b8ec8310699..18ed14911b98 100644 --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c @@ -75,6 +75,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi, void *data, }
static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = { + .output_port = 1, .mode_valid = rcar_hdmi_mode_valid, .configure_phy = rcar_hdmi_phy_configure, };
Use the drm_bridge_connector_init() helper to create a drm_connector for each output, instead of relying on the bridge drivers doing so. Attach the bridges with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to instruct them not to create a connector.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index ba8c6038cd63..10a66091391a 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -11,6 +11,7 @@ #include <linux/slab.h>
#include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_crtc.h> #include <drm/drm_managed.h> #include <drm/drm_modeset_helper_vtables.h> @@ -61,6 +62,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, struct device_node *enc_node) { struct rcar_du_encoder *renc; + struct drm_connector *connector; struct drm_bridge *bridge; int ret;
@@ -122,9 +124,22 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, if (ret) return ret;
- /* - * Attach the bridge to the encoder. The bridge will create the - * connector. - */ - return drm_bridge_attach(&renc->base, bridge, NULL, 0); + /* Attach the bridge to the encoder. */ + ret = drm_bridge_attach(&renc->base, bridge, NULL, + DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (ret) { + dev_err(rcdu->dev, "failed to attach bridge for output %u\n", + output); + return ret; + } + + /* Create the connector for the chain of bridges. */ + connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base); + if (IS_ERR(connector)) { + dev_err(rcdu->dev, + "failed to created connector for output %u\n", output); + return PTR_ERR(connector); + } + + return drm_connector_attach_encoder(connector, &renc->base); }
Hi Laurent,
On Wed, Dec 16, 2020 at 02:50:21AM +0200, Laurent Pinchart wrote:
Use the drm_bridge_connector_init() helper to create a drm_connector for each output, instead of relying on the bridge drivers doing so. Attach the bridges with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to instruct them not to create a connector.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index ba8c6038cd63..10a66091391a 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -11,6 +11,7 @@ #include <linux/slab.h>
#include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_crtc.h> #include <drm/drm_managed.h> #include <drm/drm_modeset_helper_vtables.h> @@ -61,6 +62,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, struct device_node *enc_node) { struct rcar_du_encoder *renc;
- struct drm_connector *connector; struct drm_bridge *bridge; int ret;
@@ -122,9 +124,22 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, if (ret) return ret;
- /*
* Attach the bridge to the encoder. The bridge will create the
* connector.
*/
- return drm_bridge_attach(&renc->base, bridge, NULL, 0);
- /* Attach the bridge to the encoder. */
- ret = drm_bridge_attach(&renc->base, bridge, NULL,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
- if (ret) {
dev_err(rcdu->dev, "failed to attach bridge for output %u\n",
output);
return ret;
- }
- /* Create the connector for the chain of bridges. */
- connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
- if (IS_ERR(connector)) {
dev_err(rcdu->dev,
"failed to created connector for output %u\n", output);
return PTR_ERR(connector);
- }
- return drm_connector_attach_encoder(connector, &renc->base);
Looks good Reviewed-by: Jacopo Mondi jacopo+renesas@jmondi.org
I'm trying to figure out how deferred probe of a panel directly connected to the lvds encoder work. I assume there's no risk of creating the connector before the panel is probed, or this is not an issue.
}
Regards,
Laurent Pinchart
Hi Jacopo,
On Wed, Dec 16, 2020 at 12:53:19PM +0100, Jacopo Mondi wrote:
On Wed, Dec 16, 2020 at 02:50:21AM +0200, Laurent Pinchart wrote:
Use the drm_bridge_connector_init() helper to create a drm_connector for each output, instead of relying on the bridge drivers doing so. Attach the bridges with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to instruct them not to create a connector.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index ba8c6038cd63..10a66091391a 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -11,6 +11,7 @@ #include <linux/slab.h>
#include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_crtc.h> #include <drm/drm_managed.h> #include <drm/drm_modeset_helper_vtables.h> @@ -61,6 +62,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, struct device_node *enc_node) { struct rcar_du_encoder *renc;
- struct drm_connector *connector; struct drm_bridge *bridge; int ret;
@@ -122,9 +124,22 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, if (ret) return ret;
- /*
* Attach the bridge to the encoder. The bridge will create the
* connector.
*/
- return drm_bridge_attach(&renc->base, bridge, NULL, 0);
- /* Attach the bridge to the encoder. */
- ret = drm_bridge_attach(&renc->base, bridge, NULL,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
- if (ret) {
dev_err(rcdu->dev, "failed to attach bridge for output %u\n",
output);
return ret;
- }
- /* Create the connector for the chain of bridges. */
- connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
- if (IS_ERR(connector)) {
dev_err(rcdu->dev,
"failed to created connector for output %u\n", output);
return PTR_ERR(connector);
- }
- return drm_connector_attach_encoder(connector, &renc->base);
Looks good Reviewed-by: Jacopo Mondi jacopo+renesas@jmondi.org
I'm trying to figure out how deferred probe of a panel directly connected to the lvds encoder work. I assume there's no risk of creating the connector before the panel is probed, or this is not an issue.
If the panel isn't probed yet, the call to drm_of_find_panel_or_bridge() in rcar_lvds_parse_dt() will defer probing of the LVDS encoder, which in turn will defer probind of the DU as the LVDS bridge won't be registered.
}
dri-devel@lists.freedesktop.org