Re: [PATCH v3 03/21] drm/exynos: Declare the DSI encoder as a bridge element

[Boris Brezillon <boris.brezillon@collabora.com>]

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