Re: [PATCH v3 27/56] drm/omap: dsi: do bus locking in host driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Thu, Nov 05, 2020 at 02:03:04PM +0200, Tomi Valkeinen wrote:
> From: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> This moves the bus locking into the host driver and unexports
> the custom API in preparation for drm_panel support.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  .../gpu/drm/omapdrm/displays/panel-dsi-cm.c   | 46 +------------------
>  drivers/gpu/drm/omapdrm/dss/dsi.c             | 33 ++++++++-----
>  drivers/gpu/drm/omapdrm/dss/omapdss.h         |  3 --
>  3 files changed, 23 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> index dc2c045cc6b0..4be0c9dbcc43 100644
> --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> @@ -298,7 +298,6 @@ static int dsicm_wake_up(struct panel_drv_data *ddata)
>  static int dsicm_bl_update_status(struct backlight_device *dev)
>  {
>  	struct panel_drv_data *ddata = dev_get_drvdata(&dev->dev);
> -	struct omap_dss_device *src = ddata->src;
>  	int r = 0;
>  	int level;
>  
> @@ -313,15 +312,11 @@ static int dsicm_bl_update_status(struct backlight_device *dev)
>  	mutex_lock(&ddata->lock);
>  
>  	if (ddata->enabled) {
> -		src->ops->dsi.bus_lock(src);
> -
>  		r = dsicm_wake_up(ddata);
>  		if (!r) {
>  			r = dsicm_dcs_write_1(ddata,
>  				MIPI_DCS_SET_DISPLAY_BRIGHTNESS, level);
>  		}
> -
> -		src->ops->dsi.bus_unlock(src);
>  	}
>  
>  	mutex_unlock(&ddata->lock);
> @@ -347,21 +342,16 @@ static ssize_t dsicm_num_errors_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct panel_drv_data *ddata = dev_get_drvdata(dev);
> -	struct omap_dss_device *src = ddata->src;
>  	u8 errors = 0;
>  	int r;
>  
>  	mutex_lock(&ddata->lock);
>  
>  	if (ddata->enabled) {
> -		src->ops->dsi.bus_lock(src);
> -
>  		r = dsicm_wake_up(ddata);
>  		if (!r)
>  			r = dsicm_dcs_read_1(ddata, DCS_READ_NUM_ERRORS,
>  					&errors);
> -
> -		src->ops->dsi.bus_unlock(src);
>  	} else {
>  		r = -ENODEV;
>  	}
> @@ -378,20 +368,15 @@ static ssize_t dsicm_hw_revision_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct panel_drv_data *ddata = dev_get_drvdata(dev);
> -	struct omap_dss_device *src = ddata->src;
>  	u8 id1, id2, id3;
>  	int r;
>  
>  	mutex_lock(&ddata->lock);
>  
>  	if (ddata->enabled) {
> -		src->ops->dsi.bus_lock(src);
> -
>  		r = dsicm_wake_up(ddata);
>  		if (!r)
>  			r = dsicm_get_id(ddata, &id1, &id2, &id3);
> -
> -		src->ops->dsi.bus_unlock(src);
>  	} else {
>  		r = -ENODEV;
>  	}
> @@ -409,7 +394,6 @@ static ssize_t dsicm_store_ulps(struct device *dev,
>  		const char *buf, size_t count)
>  {
>  	struct panel_drv_data *ddata = dev_get_drvdata(dev);
> -	struct omap_dss_device *src = ddata->src;
>  	unsigned long t;
>  	int r;
>  
> @@ -420,14 +404,10 @@ static ssize_t dsicm_store_ulps(struct device *dev,
>  	mutex_lock(&ddata->lock);
>  
>  	if (ddata->enabled) {
> -		src->ops->dsi.bus_lock(src);
> -
>  		if (t)
>  			r = dsicm_enter_ulps(ddata);
>  		else
>  			r = dsicm_wake_up(ddata);
> -
> -		src->ops->dsi.bus_unlock(src);
>  	}
>  
>  	mutex_unlock(&ddata->lock);
> @@ -457,7 +437,6 @@ static ssize_t dsicm_store_ulps_timeout(struct device *dev,
>  		const char *buf, size_t count)
>  {
>  	struct panel_drv_data *ddata = dev_get_drvdata(dev);
> -	struct omap_dss_device *src = ddata->src;
>  	unsigned long t;
>  	int r;
>  
> @@ -470,9 +449,7 @@ static ssize_t dsicm_store_ulps_timeout(struct device *dev,
>  
>  	if (ddata->enabled) {
>  		/* dsicm_wake_up will restart the timer */
> -		src->ops->dsi.bus_lock(src);
>  		r = dsicm_wake_up(ddata);
> -		src->ops->dsi.bus_unlock(src);
>  	}
>  
>  	mutex_unlock(&ddata->lock);
> @@ -673,17 +650,11 @@ static void dsicm_disconnect(struct omap_dss_device *src,
>  static void dsicm_enable(struct omap_dss_device *dssdev)
>  {
>  	struct panel_drv_data *ddata = to_panel_data(dssdev);
> -	struct omap_dss_device *src = ddata->src;
>  	int r;
>  
>  	mutex_lock(&ddata->lock);
>  
> -	src->ops->dsi.bus_lock(src);
> -
>  	r = dsicm_power_on(ddata);
> -
> -	src->ops->dsi.bus_unlock(src);
> -
>  	if (r)
>  		goto err;
>  
> @@ -700,7 +671,6 @@ static void dsicm_enable(struct omap_dss_device *dssdev)
>  static void dsicm_disable(struct omap_dss_device *dssdev)
>  {
>  	struct panel_drv_data *ddata = to_panel_data(dssdev);
> -	struct omap_dss_device *src = ddata->src;
>  	int r;
>  
>  	dsicm_bl_power(ddata, false);
> @@ -709,24 +679,19 @@ static void dsicm_disable(struct omap_dss_device *dssdev)
>  
>  	dsicm_cancel_ulps_work(ddata);
>  
> -	src->ops->dsi.bus_lock(src);
> -
>  	r = dsicm_wake_up(ddata);
>  	if (!r)
>  		dsicm_power_off(ddata);
>  
> -	src->ops->dsi.bus_unlock(src);
> -
>  	mutex_unlock(&ddata->lock);
>  }
>  
>  static void dsicm_framedone_cb(int err, void *data)
>  {
>  	struct panel_drv_data *ddata = data;
> -	struct omap_dss_device *src = ddata->src;
>  
>  	dev_dbg(&ddata->dsi->dev, "framedone, err %d\n", err);
> -	src->ops->dsi.bus_unlock(src);
> +	mutex_unlock(&ddata->lock);
>  }
>  
>  static int dsicm_update(struct omap_dss_device *dssdev,
> @@ -739,7 +704,6 @@ static int dsicm_update(struct omap_dss_device *dssdev,
>  	dev_dbg(&ddata->dsi->dev, "update %d, %d, %d x %d\n", x, y, w, h);
>  
>  	mutex_lock(&ddata->lock);
> -	src->ops->dsi.bus_lock(src);
>  
>  	r = dsicm_wake_up(ddata);
>  	if (r)
> @@ -761,11 +725,9 @@ static int dsicm_update(struct omap_dss_device *dssdev,
>  	if (r)
>  		goto err;
>  
> -	/* note: no bus_unlock here. unlock is src framedone_cb */
> -	mutex_unlock(&ddata->lock);
> +	/* note: no unlock here. unlock is src framedone_cb */

This change isn't described in the commit message. Could you explain why
it's needed ? Locking a mutex in a function and unlocking it elsewhere
always scares me.

>  	return 0;
>  err:
> -	src->ops->dsi.bus_unlock(src);
>  	mutex_unlock(&ddata->lock);
>  	return r;
>  }
> @@ -791,7 +753,6 @@ static void dsicm_ulps_work(struct work_struct *work)
>  	struct panel_drv_data *ddata = container_of(work, struct panel_drv_data,
>  			ulps_work.work);
>  	struct omap_dss_device *dssdev = &ddata->dssdev;
> -	struct omap_dss_device *src = ddata->src;
>  
>  	mutex_lock(&ddata->lock);
>  
> @@ -800,11 +761,8 @@ static void dsicm_ulps_work(struct work_struct *work)
>  		return;
>  	}
>  
> -	src->ops->dsi.bus_lock(src);
> -
>  	dsicm_enter_ulps(ddata);
>  
> -	src->ops->dsi.bus_unlock(src);
>  	mutex_unlock(&ddata->lock);
>  }
>  
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 41431ca34568..d54b743c2b48 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -476,17 +476,13 @@ static inline u32 dsi_read_reg(struct dsi_data *dsi, const struct dsi_reg idx)
>  	return __raw_readl(base + idx.idx);
>  }
>  
> -static void dsi_bus_lock(struct omap_dss_device *dssdev)
> +static void dsi_bus_lock(struct dsi_data *dsi)
>  {
> -	struct dsi_data *dsi = to_dsi_data(dssdev);
> -
>  	down(&dsi->bus_lock);

Nothing to be addressed in this patch, but is there a reason to use a
semaphore instead of a mutex ?

>  }
>  
> -static void dsi_bus_unlock(struct omap_dss_device *dssdev)
> +static void dsi_bus_unlock(struct dsi_data *dsi)
>  {
> -	struct dsi_data *dsi = to_dsi_data(dssdev);
> -
>  	up(&dsi->bus_lock);
>  }
>  
> @@ -3798,6 +3794,8 @@ static void dsi_handle_framedone(struct dsi_data *dsi, int error)
>  		REG_FLD_MOD(dsi, DSI_TIMING2, 1, 15, 15); /* LP_RX_TO */
>  	}
>  
> +	dsi_bus_unlock(dsi);
> +
>  	dsi->framedone_callback(error, dsi->framedone_data);
>  
>  	if (!error)
> @@ -3857,6 +3855,8 @@ static int dsi_update(struct omap_dss_device *dssdev, int channel,
>  {
>  	struct dsi_data *dsi = to_dsi_data(dssdev);
>  
> +	dsi_bus_lock(dsi);
> +
>  	dsi->update_channel = channel;
>  	dsi->framedone_callback = callback;
>  	dsi->framedone_data = data;
> @@ -4716,10 +4716,9 @@ static enum omap_channel dsi_get_channel(struct dsi_data *dsi)
>  	}
>  }
>  
> -static ssize_t omap_dsi_host_transfer(struct mipi_dsi_host *host,
> -				      const struct mipi_dsi_msg *msg)
> +static ssize_t _omap_dsi_host_transfer(struct dsi_data *dsi,
> +				       const struct mipi_dsi_msg *msg)
>  {
> -	struct dsi_data *dsi = host_to_omap(host);
>  	struct omap_dss_device *dssdev = &dsi->output;
>  	int r;
>  
> @@ -4769,6 +4768,19 @@ static ssize_t omap_dsi_host_transfer(struct mipi_dsi_host *host,
>  	return 0;
>  }
>  
> +static ssize_t omap_dsi_host_transfer(struct mipi_dsi_host *host,
> +				      const struct mipi_dsi_msg *msg)
> +{
> +	struct dsi_data *dsi = host_to_omap(host);
> +	int r;
> +
> +	dsi_bus_lock(dsi);
> +	r = _omap_dsi_host_transfer(dsi, msg);
> +	dsi_bus_unlock(dsi);
> +
> +	return r;
> +}
> +
>  static int dsi_get_clocks(struct dsi_data *dsi)
>  {
>  	struct clk *clk;
> @@ -4802,9 +4814,6 @@ static const struct omap_dss_device_ops dsi_ops = {
>  	.enable = dsi_display_enable,
>  
>  	.dsi = {
> -		.bus_lock = dsi_bus_lock,
> -		.bus_unlock = dsi_bus_unlock,
> -
>  		.disable = dsi_display_disable,
>  
>  		.set_config = dsi_set_config,
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index 1520a5f752b7..43eba2ea1f96 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -291,9 +291,6 @@ struct omapdss_dsi_ops {
>  	int (*update)(struct omap_dss_device *dssdev, int channel,
>  			void (*callback)(int, void *), void *data);
>  
> -	void (*bus_lock)(struct omap_dss_device *dssdev);
> -	void (*bus_unlock)(struct omap_dss_device *dssdev);
> -
>  	int (*enable_video_output)(struct omap_dss_device *dssdev, int channel);
>  	void (*disable_video_output)(struct omap_dss_device *dssdev,
>  			int channel);

-- 
Regards,

Laurent Pinchart