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

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(-)

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@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>

<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.

>>  	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 ?

It's been a long time, but I think the reason was that mutex gave a warning after being locked for a
bit longer time, and semaphore didn't. The resource is reserved while a DSI transfer is active, so
it could be almost 2 frames (wait for vsync and then transfer frame). Or reading the frame buffer
back from the panel, which could take a long time (seconds).

There are better ways to implement it (after this series =).

 Tomi

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@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>
> 
> <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.

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

-- Sebastian

> >>  	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 ?
> 
> It's been a long time, but I think the reason was that mutex gave a warning after being locked for a
> bit longer time, and semaphore didn't. The resource is reserved while a DSI transfer is active, so
> it could be almost 2 frames (wait for vsync and then transfer frame). Or reading the frame buffer
> back from the panel, which could take a long time (seconds).
> 
> There are better ways to implement it (after this series =).
> 
>  Tomi
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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@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>
>>
>> <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.
> 
> My understanding is, that this is because the bus must not be used
> until the update has been done.

So as I said, I think keeping ddata->lock is not correct. This code also goes away some patches
later. So I'll drop the "keep ddata->lock" part.

 Tomi