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