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