Hi Boris,
On Sun, Nov 24, 2019 at 02:17:27PM +0100, Boris Brezillon wrote:
On Sun, 24 Nov 2019 12:24:33 +0200 Laurent Pinchart wrote:
On Wed, Oct 23, 2019 at 05:44:54PM +0200, Boris Brezillon wrote:
Encoder drivers will progressively transition to the drm_bridge interface in place of the drm_encoder one.
Converting the Exynos DSI encoder driver to this approach allows us to use the ->pre_{enable,disable}() hooks and get rid of the hack resetting encoder->bridge.next (which was needed to control the encoder/bridge enable/disable sequence).
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com
Changes in v3:
- Embed a drm_bridge object in exynos_dsi since drm_encoder no longer has a dummy bridge
Changes in v2:
- New patch (replacement for "drm/exynos: Get rid of exynos_dsi->out_bridge")
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 89 +++++++++++++++---------- 1 file changed, 55 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 72726f2c7a9f..3915f50b005e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -252,10 +252,10 @@ struct exynos_dsi_driver_data {
struct exynos_dsi { struct drm_encoder encoder;
- struct drm_bridge bridge; struct mipi_dsi_host dsi_host; struct drm_connector connector; struct drm_panel *panel;
struct drm_bridge *out_bridge; struct device *dev;
void __iomem *reg_base;
@@ -291,6 +291,11 @@ static inline struct exynos_dsi *encoder_to_dsi(struct drm_encoder *e) return container_of(e, struct exynos_dsi, encoder); }
+static inline struct exynos_dsi *bridge_to_dsi(struct drm_bridge *b) +{
- return container_of(b, struct exynos_dsi, bridge);
+}
enum reg_idx { DSIM_STATUS_REG, /* Status register */ DSIM_SWRST_REG, /* Software reset register */ @@ -1374,25 +1379,38 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi) } }
-static void exynos_dsi_enable(struct drm_encoder *encoder) +static void exynos_dsi_pre_enable(struct drm_bridge *bridge) {
- struct exynos_dsi *dsi = encoder_to_dsi(encoder);
struct exynos_dsi *dsi = bridge_to_dsi(bridge); int ret;
if (dsi->state & DSIM_STATE_ENABLED) return;
This can probably be removed now as the core should ensure that double-enable or double-disable never occurs, but it can be done in a separate patch.
Except the enable/disable() implementations handle failures (the framework does not expect those to fails BTW), and I guess it's important to know the actual HW state in order to keep runtime PM get/put calls balanced.
pm_runtime_get_sync(dsi->dev);
dsi->state |= DSIM_STATE_ENABLED;
if (dsi->panel) { ret = drm_panel_prepare(dsi->panel); if (ret < 0) goto err_put_sync;
} else {
drm_bridge_pre_enable(dsi->out_bridge);}
It would be nice to switch to the drm panel bridge, but that can also be done on top of this series.
I agree, just didn't want to add more stuff to this series.
- dsi->state |= DSIM_STATE_ENABLED;
- return;
+err_put_sync:
- pm_runtime_put(dsi->dev);
+}
+static void exynos_dsi_enable(struct drm_bridge *bridge) +{
- struct exynos_dsi *dsi = bridge_to_dsi(bridge);
- int ret;
- if (!(dsi->state & DSIM_STATE_ENABLED) ||
(dsi->state & DSIM_STATE_VIDOUT_AVAILABLE))return;- exynos_dsi_set_display_mode(dsi); exynos_dsi_set_display_enable(dsi, true);
@@ -1400,8 +1418,6 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) ret = drm_panel_enable(dsi->panel); if (ret < 0) goto err_display_disable;
} else {
drm_bridge_enable(dsi->out_bridge);}
dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
@@ -1410,28 +1426,30 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) err_display_disable: exynos_dsi_set_display_enable(dsi, false); drm_panel_unprepare(dsi->panel);
Does this belong here, as drm_panel_prepare() was called in exynos_dsi_pre_enable() ?
Nope, this one should be dropped.
-err_put_sync:
- dsi->state &= ~DSIM_STATE_ENABLED;
- pm_runtime_put(dsi->dev);
}
-static void exynos_dsi_disable(struct drm_encoder *encoder) +static void exynos_dsi_disable(struct drm_bridge *bridge) {
- struct exynos_dsi *dsi = encoder_to_dsi(encoder);
- struct exynos_dsi *dsi = bridge_to_dsi(bridge);
- if (!(dsi->state & DSIM_STATE_VIDOUT_AVAILABLE))
return;- drm_panel_disable(dsi->panel);
- exynos_dsi_set_display_enable(dsi, false);
- dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
+}
+static void exynos_dsi_post_disable(struct drm_bridge *bridge) +{
struct exynos_dsi *dsi = bridge_to_dsi(bridge);
if (!(dsi->state & DSIM_STATE_ENABLED)) return;
- dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
- drm_panel_disable(dsi->panel);
- drm_bridge_disable(dsi->out_bridge);
- exynos_dsi_set_display_enable(dsi, false); drm_panel_unprepare(dsi->panel);
- drm_bridge_post_disable(dsi->out_bridge);
- dsi->state &= ~DSIM_STATE_ENABLED; pm_runtime_put_sync(dsi->dev);
- dsi->state &= ~DSIM_STATE_ENABLED;
}
static enum drm_connector_status @@ -1499,9 +1517,11 @@ static int exynos_dsi_create_connector(struct drm_encoder *encoder) return 0; }
-static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = { +static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {
- .pre_enable = exynos_dsi_pre_enable, .enable = exynos_dsi_enable, .disable = exynos_dsi_disable,
- .post_disable = exynos_dsi_post_disable,
};
static const struct drm_encoder_funcs exynos_dsi_encoder_funcs = { @@ -1520,9 +1540,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);dsi->out_bridge = out_bridge;encoder->bridge = NULL;
drm_bridge_attach(encoder, out_bridge, &dsi->bridge);@@ -1575,19 +1593,19 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct exynos_dsi *dsi = host_to_dsi(host);
struct drm_bridge *out_bridge = dsi->bridge.next; struct drm_device *drm = dsi->encoder.dev;
if (dsi->panel) { mutex_lock(&drm->mode_config.mutex);
exynos_dsi_disable(&dsi->encoder);
exynos_dsi_disable(&dsi->bridge);exynos_dsi_post_disable(&dsi->bridge);
- } else {
if (dsi->out_bridge->funcs->detach)dsi->out_bridge->funcs->detach(dsi->out_bridge);dsi->out_bridge = NULL;
- } else if (out_bridge && out_bridge->funcs->detach) {
out_bridge->funcs->detach(out_bridge);Maybe drm_bridge_detach() ?
This function is not exported, and I suppose that's why they used the function pointer in this driver. I bet there's a good reason for not exposing this function...
Indeed, my bad. It's called by drm_encoder_cleanup(), I assume it's not enough ? If there's a good use case for exporting it, then I think it should be exported.
}
if (drm->mode_config.poll_enabled) @@ -1687,16 +1705,18 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, drm_encoder_init(drm_dev, encoder, &exynos_dsi_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL);
- drm_encoder_helper_add(encoder, &exynos_dsi_encoder_helper_funcs);
- ret = exynos_drm_set_possible_crtcs(encoder, EXYNOS_DISPLAY_TYPE_LCD); if (ret < 0) return ret;
- /* Declare ourself as the first bridge element. */
- dsi->bridge.funcs = &exynos_dsi_bridge_funcs;
- drm_bridge_attach(encoder, &dsi->bridge, NULL);
- 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, &dsi->bridge);Same as for patch 01/21, maybe this could be moved to this bridge's attach operation ? Actually, now that I've read the code, this in_bridge part looks weird. Why would the DSI encoder have an input bridge that is has to manage itself ?
Yes, I know, it doesn't make any sense. Either we're dealing with a bridge which can be chained to other bridges (can be placed in the middle of a chain as well), or we're dealing with an encoder which precedes any bridges. In the latter case (which is how exynos_dsi is implemented) in_bridge doesn't have any meaning, and that's even worse since we're placing the so-called input bridge (AKA previous bridge) after our encoder (that's what drm_bridge_attach(encoder, in_bridge, NULL) does).
Can we get input from the exynos maintainers ? Or is the driver not actively maintained anymore ?
TBH, I didn't want to go that far and fix existing drivers when I started this series, so I think I'll rework the patchset to get rid of the VC4 and exynos patches, even if that means having 2 drivers that mess up with the encoder->bridge_chain list.
I don't mind the above changes really (and the one for VC4 seems pretty fine so far).