[PATCH 1/2] drm/omap: add omap_gem_put_paddr_locked()

[Tomi Valkeinen]


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 at ti.com>
>> ---
>>  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 at ideasonboard.com>
> 
> 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