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

From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Sebastian Reichel <sre@kernel.org>,
	Nikhil Devshatwar <nikhil.nd@ti.com>,
	<dri-devel@lists.freedesktop.org>, <linux-omap@vger.kernel.org>,
	Sekhar Nori <nsekhar@ti.com>, Tony Lindgren <tony@atomide.com>,
	<hns@goldelico.com>, Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH v5 29/29] drm/omap: dsi: allow DSI commands to be sent early
Date: Thu, 10 Dec 2020 09:34:17 +0200	[thread overview]
Message-ID: <c5139c54-78c1-fe16-7f50-c60efd1f447b@ti.com> (raw)
In-Reply-To: <X8+gXWBwLItZA7gA@pendragon.ideasonboard.com>

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki