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@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
<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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
