diff mbox series

[v5,29/29] drm/omap: dsi: allow DSI commands to be sent early

Message ID 20201208122855.254819-30-tomi.valkeinen@ti.com
State New
Headers show
Series [v5,01/29] drm/panel: panel-dsi-cm: cleanup tear enable | expand

Commit Message

Tomi Valkeinen Dec. 8, 2020, 12:28 p.m. UTC 
Panel drivers can send DSI commands in panel's prepare(), which happens
before the bridge's enable() is called. The OMAP DSI driver currently
only sets up the DSI interface at bridge's enable(), so prepare() cannot
be used to send DSI commands.

This patch fixes the issue by making it possible to enable the DSI
interface any time a command is about to be sent. Disabling the
interface is be done via delayed work.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dsi.c | 49 +++++++++++++++++++++++++++----
 drivers/gpu/drm/omapdrm/dss/dsi.h |  3 ++
 2 files changed, 47 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart Dec. 8, 2020, 3:48 p.m. UTC | #1 
Hi Tomi,

Thank you for the patch.

On Tue, Dec 08, 2020 at 02:28:55PM +0200, Tomi Valkeinen wrote:
> Panel drivers can send DSI commands in panel's prepare(), which happens
> before the bridge's enable() is called. The OMAP DSI driver currently
> only sets up the DSI interface at bridge's enable(), so prepare() cannot
> be used to send DSI commands.
> 
> This patch fixes the issue by making it possible to enable the DSI
> interface any time a command is about to be sent. Disabling the
> interface is be done via delayed work.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dsi.c | 49 +++++++++++++++++++++++++++----
>  drivers/gpu/drm/omapdrm/dss/dsi.h |  3 ++
>  2 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 53a64bc91867..34f665aa9a59 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -3503,6 +3503,9 @@ static void dsi_enable(struct dsi_data *dsi)
>  
>  	WARN_ON(!dsi_bus_is_locked(dsi));
>  
> +	if (WARN_ON(dsi->iface_enabled))
> +		return;
> +
>  	mutex_lock(&dsi->lock);
>  
>  	r = dsi_runtime_get(dsi);
> @@ -3515,6 +3518,8 @@ static void dsi_enable(struct dsi_data *dsi)
>  	if (r)
>  		goto err_init_dsi;
>  
> +	dsi->iface_enabled = true;
> +
>  	mutex_unlock(&dsi->lock);
>  
>  	return;
> @@ -3530,6 +3535,9 @@ static void dsi_disable(struct dsi_data *dsi)
>  {
>  	WARN_ON(!dsi_bus_is_locked(dsi));
>  
> +	if (WARN_ON(!dsi->iface_enabled))
> +		return;
> +
>  	mutex_lock(&dsi->lock);
>  
>  	dsi_sync_vc(dsi, 0);
> @@ -3541,6 +3549,8 @@ static void dsi_disable(struct dsi_data *dsi)
>  
>  	dsi_runtime_put(dsi);
>  
> +	dsi->iface_enabled = false;
> +
>  	mutex_unlock(&dsi->lock);
>  }
>  
> @@ -4229,10 +4239,12 @@ static ssize_t omap_dsi_host_transfer(struct mipi_dsi_host *host,
>  
>  	dsi_bus_lock(dsi);
>  
> -	if (dsi->video_enabled)
> -		r = _omap_dsi_host_transfer(dsi, vc, msg);
> -	else
> -		r = -EIO;
> +	if (!dsi->iface_enabled) {
> +		dsi_enable(dsi);
> +		schedule_delayed_work(&dsi->dsi_disable_work, msecs_to_jiffies(2000));
> +	}
> +
> +	r = _omap_dsi_host_transfer(dsi, vc, msg);
>  
>  	dsi_bus_unlock(dsi);
>  
> @@ -4397,6 +4409,14 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host,
>  	if (WARN_ON(dsi->dsidev != client))
>  		return -EINVAL;
>  
> +	cancel_delayed_work_sync(&dsi->dsi_disable_work);
> +
> +	if (dsi->iface_enabled) {
> +		dsi_bus_lock(dsi);
> +		dsi_disable(dsi);
> +		dsi_bus_unlock(dsi);
> +	}
> +
>  	omap_dsi_unregister_te_irq(dsi);
>  	dsi->dsidev = NULL;
>  	return 0;
> @@ -4632,9 +4652,12 @@ static void dsi_bridge_enable(struct drm_bridge *bridge)
>  	struct dsi_data *dsi = drm_bridge_to_dsi(bridge);
>  	struct omap_dss_device *dssdev = &dsi->output;
>  
> +	cancel_delayed_work_sync(&dsi->dsi_disable_work);
> +

Is there a risk of a race condition if omap_dsi_host_transfer() is
called right here, before locking the bus ? Or is there a guarantee that
the two functions can't be executed concurrently ? Same for
dsi_bridge_disable() below.

>  	dsi_bus_lock(dsi);
>  
> -	dsi_enable(dsi);
> +	if (!dsi->iface_enabled)
> +		dsi_enable(dsi);
>  
>  	dsi_enable_video_output(dssdev, VC_VIDEO);
>  
> @@ -4648,6 +4671,8 @@ static void dsi_bridge_disable(struct drm_bridge *bridge)
>  	struct dsi_data *dsi = drm_bridge_to_dsi(bridge);
>  	struct omap_dss_device *dssdev = &dsi->output;
>  
> +	cancel_delayed_work_sync(&dsi->dsi_disable_work);
> +
>  	dsi_bus_lock(dsi);
>  
>  	dsi->video_enabled = false;
> @@ -4840,6 +4865,18 @@ static const struct soc_device_attribute dsi_soc_devices[] = {
>  	{ /* sentinel */ }
>  };
>  
> +static void omap_dsi_disable_work_callback(struct work_struct *work)
> +{
> +	struct dsi_data *dsi = container_of(work, struct dsi_data, dsi_disable_work.work);
> +
> +	dsi_bus_lock(dsi);
> +
> +	if (dsi->iface_enabled && !dsi->video_enabled)
> +		dsi_disable(dsi);
> +
> +	dsi_bus_unlock(dsi);
> +}
> +
>  static int dsi_probe(struct platform_device *pdev)
>  {
>  	const struct soc_device_attribute *soc;
> @@ -4873,6 +4910,8 @@ static int dsi_probe(struct platform_device *pdev)
>  	INIT_DEFERRABLE_WORK(&dsi->framedone_timeout_work,
>  			     dsi_framedone_timeout_work_callback);
>  
> +	INIT_DEFERRABLE_WORK(&dsi->dsi_disable_work, omap_dsi_disable_work_callback);
> +
>  #ifdef DSI_CATCH_MISSING_TE
>  	timer_setup(&dsi->te_timer, dsi_te_timeout, 0);
>  #endif
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.h b/drivers/gpu/drm/omapdrm/dss/dsi.h
> index de9411067ba2..601707c0ecc4 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.h
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.h
> @@ -394,6 +394,7 @@ struct dsi_data {
>  	atomic_t do_ext_te_update;
>  
>  	bool te_enabled;
> +	bool iface_enabled;
>  	bool video_enabled;
>  
>  	struct delayed_work framedone_timeout_work;
> @@ -443,6 +444,8 @@ struct dsi_data {
>  
>  	struct omap_dss_device output;
>  	struct drm_bridge bridge;
> +
> +	struct delayed_work dsi_disable_work;
>  };
>  
>  struct dsi_packet_sent_handler_data {
Tomi Valkeinen Dec. 10, 2020, 7:34 a.m. UTC | #2 
On 08/12/2020 17:48, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Dec 08, 2020 at 02:28:55PM +0200, Tomi Valkeinen wrote:
>> Panel drivers can send DSI commands in panel's prepare(), which happens
>> before the bridge's enable() is called. The OMAP DSI driver currently
>> only sets up the DSI interface at bridge's enable(), so prepare() cannot
>> be used to send DSI commands.
>>
>> This patch fixes the issue by making it possible to enable the DSI
>> interface any time a command is about to be sent. Disabling the
>> interface is be done via delayed work.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/dsi.c | 49 +++++++++++++++++++++++++++----
>>  drivers/gpu/drm/omapdrm/dss/dsi.h |  3 ++
>>  2 files changed, 47 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
>> index 53a64bc91867..34f665aa9a59 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
>> @@ -3503,6 +3503,9 @@ static void dsi_enable(struct dsi_data *dsi)
>>  
>>  	WARN_ON(!dsi_bus_is_locked(dsi));
>>  
>> +	if (WARN_ON(dsi->iface_enabled))
>> +		return;
>> +
>>  	mutex_lock(&dsi->lock);
>>  
>>  	r = dsi_runtime_get(dsi);
>> @@ -3515,6 +3518,8 @@ static void dsi_enable(struct dsi_data *dsi)
>>  	if (r)
>>  		goto err_init_dsi;
>>  
>> +	dsi->iface_enabled = true;
>> +
>>  	mutex_unlock(&dsi->lock);
>>  
>>  	return;
>> @@ -3530,6 +3535,9 @@ static void dsi_disable(struct dsi_data *dsi)
>>  {
>>  	WARN_ON(!dsi_bus_is_locked(dsi));
>>  
>> +	if (WARN_ON(!dsi->iface_enabled))
>> +		return;
>> +
>>  	mutex_lock(&dsi->lock);
>>  
>>  	dsi_sync_vc(dsi, 0);
>> @@ -3541,6 +3549,8 @@ static void dsi_disable(struct dsi_data *dsi)
>>  
>>  	dsi_runtime_put(dsi);
>>  
>> +	dsi->iface_enabled = false;
>> +
>>  	mutex_unlock(&dsi->lock);
>>  }
>>  
>> @@ -4229,10 +4239,12 @@ static ssize_t omap_dsi_host_transfer(struct mipi_dsi_host *host,
>>  
>>  	dsi_bus_lock(dsi);
>>  
>> -	if (dsi->video_enabled)
>> -		r = _omap_dsi_host_transfer(dsi, vc, msg);
>> -	else
>> -		r = -EIO;
>> +	if (!dsi->iface_enabled) {
>> +		dsi_enable(dsi);
>> +		schedule_delayed_work(&dsi->dsi_disable_work, msecs_to_jiffies(2000));
>> +	}
>> +
>> +	r = _omap_dsi_host_transfer(dsi, vc, msg);
>>  
>>  	dsi_bus_unlock(dsi);
>>  
>> @@ -4397,6 +4409,14 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host,
>>  	if (WARN_ON(dsi->dsidev != client))
>>  		return -EINVAL;
>>  
>> +	cancel_delayed_work_sync(&dsi->dsi_disable_work);
>> +
>> +	if (dsi->iface_enabled) {
>> +		dsi_bus_lock(dsi);
>> +		dsi_disable(dsi);
>> +		dsi_bus_unlock(dsi);
>> +	}
>> +
>>  	omap_dsi_unregister_te_irq(dsi);
>>  	dsi->dsidev = NULL;
>>  	return 0;
>> @@ -4632,9 +4652,12 @@ static void dsi_bridge_enable(struct drm_bridge *bridge)
>>  	struct dsi_data *dsi = drm_bridge_to_dsi(bridge);
>>  	struct omap_dss_device *dssdev = &dsi->output;
>>  
>> +	cancel_delayed_work_sync(&dsi->dsi_disable_work);
>> +
> 
> Is there a risk of a race condition if omap_dsi_host_transfer() is
> called right here, before locking the bus ? Or is there a guarantee that
> the two functions can't be executed concurrently ? Same for
> dsi_bridge_disable() below.

Yes, there's a possibility for a race, if the panel driver does dsi command transactions via, say, a
timer, and doesn't take DRM locks that are shared with bridge-enable/disable/detach.

For bridge-enable, it shouldn't matter: If the disable callback is called just before bridge_enable
takes the dsi_bus_lock, no problem, bridge_enable just enables the interface again. If the callback
is ran just after bridge_enable's dsi_bus_unlock, no problem, dsi->video_enabled == true so the
callback does nothing.

Similarly for bridge-disable, the callback won't do anything if video_enabled == true, and after
bridge-disable has turned the video and the interface off, there's nothing to do for the callback.

The detach is a bit more unclear. Is the panel driver allowed to do "stuff" with bridges while
bridge detach is going on? If yes, it's probably broken. We should move the bus_locks to cover the
whole if() so that dsi->iface_enabled is inside the locks. With that change the delayed disable
itself should work fine.

But we don't have anything stopping omap_dsi_host_transfer being called after the whole bridge has
been detached (or called before attach). So, if we have a guarantee that the panels won't be doing
dsi transfers before/during bridge attach or after/during bridge detach, we have no issue. If we
don't have such a guarantee, it's broken.

I'll try to figure out if there's such a guarantee, but maybe it's safer to add a flag to indicate
if the bridge is available, and check that during omap_dsi_host_transfer.

 Tomi
Tomi Valkeinen Dec. 10, 2020, 8:17 a.m. UTC | #3 
On 10/12/2020 09:34, Tomi Valkeinen wrote:

> But we don't have anything stopping omap_dsi_host_transfer being called after the whole bridge has
> been detached (or called before attach). So, if we have a guarantee that the panels won't be doing
> dsi transfers before/during bridge attach or after/during bridge detach, we have no issue. If we
> don't have such a guarantee, it's broken.
> 
> I'll try to figure out if there's such a guarantee, but maybe it's safer to add a flag to indicate
> if the bridge is available, and check that during omap_dsi_host_transfer.

I don't think this can happen. I mixed up the bridge attach/detach and the dsi host attach/detach.
The cancel_delayed_work_sync happens in omap_dsi_host_detach, and I think it's a sensible
expectation that the panel won't first do mipi_dsi_detach(), and then try to do DSI transfers.

 Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 53a64bc91867..34f665aa9a59 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -3503,6 +3503,9 @@  static void dsi_enable(struct dsi_data *dsi)
 
 	WARN_ON(!dsi_bus_is_locked(dsi));
 
+	if (WARN_ON(dsi->iface_enabled))
+		return;
+
 	mutex_lock(&dsi->lock);
 
 	r = dsi_runtime_get(dsi);
@@ -3515,6 +3518,8 @@  static void dsi_enable(struct dsi_data *dsi)
 	if (r)
 		goto err_init_dsi;
 
+	dsi->iface_enabled = true;
+
 	mutex_unlock(&dsi->lock);
 
 	return;
@@ -3530,6 +3535,9 @@  static void dsi_disable(struct dsi_data *dsi)
 {
 	WARN_ON(!dsi_bus_is_locked(dsi));
 
+	if (WARN_ON(!dsi->iface_enabled))
+		return;
+
 	mutex_lock(&dsi->lock);
 
 	dsi_sync_vc(dsi, 0);
@@ -3541,6 +3549,8 @@  static void dsi_disable(struct dsi_data *dsi)
 
 	dsi_runtime_put(dsi);
 
+	dsi->iface_enabled = false;
+
 	mutex_unlock(&dsi->lock);
 }
 
@@ -4229,10 +4239,12 @@  static ssize_t omap_dsi_host_transfer(struct mipi_dsi_host *host,
 
 	dsi_bus_lock(dsi);
 
-	if (dsi->video_enabled)
-		r = _omap_dsi_host_transfer(dsi, vc, msg);
-	else
-		r = -EIO;
+	if (!dsi->iface_enabled) {
+		dsi_enable(dsi);
+		schedule_delayed_work(&dsi->dsi_disable_work, msecs_to_jiffies(2000));
+	}
+
+	r = _omap_dsi_host_transfer(dsi, vc, msg);
 
 	dsi_bus_unlock(dsi);
 
@@ -4397,6 +4409,14 @@  static int omap_dsi_host_detach(struct mipi_dsi_host *host,
 	if (WARN_ON(dsi->dsidev != client))
 		return -EINVAL;
 
+	cancel_delayed_work_sync(&dsi->dsi_disable_work);
+
+	if (dsi->iface_enabled) {
+		dsi_bus_lock(dsi);
+		dsi_disable(dsi);
+		dsi_bus_unlock(dsi);
+	}
+
 	omap_dsi_unregister_te_irq(dsi);
 	dsi->dsidev = NULL;
 	return 0;
@@ -4632,9 +4652,12 @@  static void dsi_bridge_enable(struct drm_bridge *bridge)
 	struct dsi_data *dsi = drm_bridge_to_dsi(bridge);
 	struct omap_dss_device *dssdev = &dsi->output;
 
+	cancel_delayed_work_sync(&dsi->dsi_disable_work);
+
 	dsi_bus_lock(dsi);
 
-	dsi_enable(dsi);
+	if (!dsi->iface_enabled)
+		dsi_enable(dsi);
 
 	dsi_enable_video_output(dssdev, VC_VIDEO);
 
@@ -4648,6 +4671,8 @@  static void dsi_bridge_disable(struct drm_bridge *bridge)
 	struct dsi_data *dsi = drm_bridge_to_dsi(bridge);
 	struct omap_dss_device *dssdev = &dsi->output;
 
+	cancel_delayed_work_sync(&dsi->dsi_disable_work);
+
 	dsi_bus_lock(dsi);
 
 	dsi->video_enabled = false;
@@ -4840,6 +4865,18 @@  static const struct soc_device_attribute dsi_soc_devices[] = {
 	{ /* sentinel */ }
 };
 
+static void omap_dsi_disable_work_callback(struct work_struct *work)
+{
+	struct dsi_data *dsi = container_of(work, struct dsi_data, dsi_disable_work.work);
+
+	dsi_bus_lock(dsi);
+
+	if (dsi->iface_enabled && !dsi->video_enabled)
+		dsi_disable(dsi);
+
+	dsi_bus_unlock(dsi);
+}
+
 static int dsi_probe(struct platform_device *pdev)
 {
 	const struct soc_device_attribute *soc;
@@ -4873,6 +4910,8 @@  static int dsi_probe(struct platform_device *pdev)
 	INIT_DEFERRABLE_WORK(&dsi->framedone_timeout_work,
 			     dsi_framedone_timeout_work_callback);
 
+	INIT_DEFERRABLE_WORK(&dsi->dsi_disable_work, omap_dsi_disable_work_callback);
+
 #ifdef DSI_CATCH_MISSING_TE
 	timer_setup(&dsi->te_timer, dsi_te_timeout, 0);
 #endif
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.h b/drivers/gpu/drm/omapdrm/dss/dsi.h
index de9411067ba2..601707c0ecc4 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.h
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.h
@@ -394,6 +394,7 @@  struct dsi_data {
 	atomic_t do_ext_te_update;
 
 	bool te_enabled;
+	bool iface_enabled;
 	bool video_enabled;
 
 	struct delayed_work framedone_timeout_work;
@@ -443,6 +444,8 @@  struct dsi_data {
 
 	struct omap_dss_device output;
 	struct drm_bridge bridge;
+
+	struct delayed_work dsi_disable_work;
 };
 
 struct dsi_packet_sent_handler_data {