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

[Tomi Valkeinen]

On 09/11/2020 15:27, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Nov 09, 2020 at 12:08:33PM +0200, Tomi Valkeinen wrote:
>> On 09/11/2020 11:52, Laurent Pinchart wrote:
>>> 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 at 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 at collabora.com>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
>>
>> <snip>
>>
>>>>  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.
>>
>> Good catch. I don't know why it is needed. I don't think it is, as
>> the dsi driver handles the bus lock.
>>
>> Sebastian, what was the reason for this lock?
>>
>> Note that this goes away in the series, and there's no such lock
>> in the end.
> 
> It's not really a change. What this patch basically does is to fold
> src->ops->dsi.bus_lock(src) into mutex_lock(&ddata->lock), so that
> there is only a single locking mechanism. This function previously
> had a matching pair of mutex_lock/unlock for ddata->lock, but the
> bus was not locked paired. So after conversion the lock must not be
> free'd here.

Hmm, but taking the bus_lock is moved into dsi.c (dsi_bus_lock/unlock). Previously the panel called
that directly via the dsi_ops->bus_lock(), but afaics bus_lock is now taken in dsi_update(), which
is called from the panel.

But in addition to that, this patch makes the panel driver keep the ddata->lock during the update,
which is meant to only protect the ddata.

> My understanding is, that this is because the bus must not be used
> until the update has been done.

Yes, and not only about the update, but e.g. prevent sending backlight commands while reading the
framebuffer memory.

 Tomi

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