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.
HTH o/
-Emil
