Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 11/08/17 17:09, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Monday 08 May 2017 11:51:21 Tomi Valkeinen wrote:
>> Add omap_gem_put_paddr_locked() which is a version of
>> omap_gem_put_paddr() that expects the caller to hold the struct_mutex.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
>> ---
>> drivers/gpu/drm/omapdrm/omap_gem.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
>> b/drivers/gpu/drm/omapdrm/omap_gem.c index 68a75b829b71..5d73dccc1383
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>> @@ -873,12 +873,12 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
>> /* Release physical address, when DMA is no longer being performed.. this
>> * could potentially unpin and unmap buffers from TILER
>> */
>> -void omap_gem_put_paddr(struct drm_gem_object *obj)
>> +
>> +static void omap_gem_put_paddr_locked(struct drm_gem_object *obj)
>> {
>> struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> int ret;
>>
>> - mutex_lock(&obj->dev->struct_mutex);
>> if (omap_obj->paddr_cnt > 0) {
>> omap_obj->paddr_cnt--;
>> if (omap_obj->paddr_cnt == 0) {
>
> You could now decrease the indentation level in this function by adding a few
> returns. This doesn't call for a v2, but as you'll have to rebase anyway due
> to the function name change, you can throw that change in at the same time :-)
I actually already have the function name changed, I just didn't send an
updated patch only for that...
I don't want to make this patch more confusing by changing indents etc,
but yes, that can be done in a separate patch.
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> On a side note, it might be worth it changing the count field to a refcount_t,
> this would give us a WARN_ON() if the pin/unpin calls are unbalanced.
I wonder why we have a plain if() there for the case where this function
is called for paddr_cnt == 0. That should only happen when we have a
kernel bug, I believe, so I would have expected an least a warning
print. So yes, if refcount_t gives us warnings, it's a good change.
Tomi
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
