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@xxxxxxxxxxxxx>
> > > ---
> > > 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).
--
Regards,
Laurent Pinchart
