Re: [Intel-gfx] [PATCH v3 6/6] drm/shmem-helper: Switch to reservation lock

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 5/22/23 16:02, Emil Velikov wrote:
> Hi Dmitry,
> 
> Saw v3 fly by, so I had a quick look. Original RB still stands,
> although I noticed a couple of non-blocking nitpicks.
> 
> On Sun, 21 May 2023 at 22:00, Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
> 
>> -static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
>> +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
>>  {
> 
> Should this getter have a dma_resv_assert_held(shmem->base.resv); like
> it's put brethren?
> 
> 
>> -void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
>> +static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
>> +{
>> +       int ret;
>> +
>> +       dma_resv_assert_held(shmem->base.resv);
>> +
>> +       ret = drm_gem_shmem_get_pages(shmem);
>> +
>> +       return ret;
> 
> With the assert_held in the getter, it would be less confusing to
> inline this and the unpin_locked functions.
> 
>> +}
>> +
>> +static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem)
>>  {
>> -       mutex_lock(&shmem->pages_lock);
>> -       drm_gem_shmem_put_pages_locked(shmem);
>> -       mutex_unlock(&shmem->pages_lock);
>> +       dma_resv_assert_held(shmem->base.resv);
>> +
>> +       drm_gem_shmem_put_pages(shmem);
> 
> Side note: the putter has an assert_held so the extra one here seems quite odd.
> 
> As said at the top - with or w/o these nitpicks, the original RB still stands.

Good catch. I actually added assert_held to get_pages(), but in a later
patch that is not part of this series.

-- 
Best regards,
Dmitry