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

[Emil Velikov <emil.l.velikov@gmail.com>]

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.

HTH o/
-Emil