| Message ID | 20191023154512.9762-4-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for bus-format negotiation | expand |
Hi Boris, Thank you for the patch. 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. > > 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. > > + 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() ? > - > -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); > } else { > int ret = exynos_dsi_create_connector(encoder); > > @@ -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); > drm_panel_detach(dsi->panel); > dsi->panel = NULL; > dsi->connector.status = connector_status_disconnected; > mutex_unlock(&drm->mode_config.mutex); > - } 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() ? > } > > 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 ? I don't feel confident enough to ack this patch. It goes in the right direction as far as the API evolution is concerned, so if you get an ack from the Exynos maintainers, I'm happy enough to see it merged. > > return mipi_dsi_host_register(&dsi->dsi_host); > @@ -1708,7 +1728,8 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master, > struct drm_encoder *encoder = dev_get_drvdata(dev); > struct exynos_dsi *dsi = encoder_to_dsi(encoder); > > - exynos_dsi_disable(encoder); > + exynos_dsi_disable(&dsi->bridge); > + exynos_dsi_post_disable(&dsi->bridge); > > mipi_dsi_host_unregister(&dsi->dsi_host); > }
Hi Laurent, On Sun, 24 Nov 2019 12:24:33 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > Thank you for the patch. > > 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); > > } else { > > int ret = exynos_dsi_create_connector(encoder); > > > > @@ -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); > > drm_panel_detach(dsi->panel); > > dsi->panel = NULL; > > dsi->connector.status = connector_status_disconnected; > > mutex_unlock(&drm->mode_config.mutex); > > - } 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... > > > } > > > > 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). 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. Regards, Boris
On Sun, 24 Nov 2019 14:17:27 +0100 Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > @@ -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). More on that topic: I checked the exynos dts we have in mainline and no one is making use of the ports part of the exynos_dsim bindings, so maybe we should just deprecate it. The other option would be to patch the driver to act as a real bridge, but I can't do that without someone testing my changes and I didn't get much feedback from Exynos maintainers so far...
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); > > > } else { > > > int ret = exynos_dsi_create_connector(encoder); > > > > > > @@ -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); > > > drm_panel_detach(dsi->panel); > > > dsi->panel = NULL; > > > dsi->connector.status = connector_status_disconnected; > > > mutex_unlock(&drm->mode_config.mutex); > > > - } 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).
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; 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); } + 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); - -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); } else { int ret = exynos_dsi_create_connector(encoder); @@ -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); drm_panel_detach(dsi->panel); dsi->panel = NULL; dsi->connector.status = connector_status_disconnected; mutex_unlock(&drm->mode_config.mutex); - } 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); } 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); } return mipi_dsi_host_register(&dsi->dsi_host); @@ -1708,7 +1728,8 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master, struct drm_encoder *encoder = dev_get_drvdata(dev); struct exynos_dsi *dsi = encoder_to_dsi(encoder); - exynos_dsi_disable(encoder); + exynos_dsi_disable(&dsi->bridge); + exynos_dsi_post_disable(&dsi->bridge); mipi_dsi_host_unregister(&dsi->dsi_host); }
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(-)