linux-kernel - Re: [PATCH v5 4/4] drm/vc4: Allocate binner bo when starting to use the V3D

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87tvez802s.fsf@anholt.net>
Date:   Mon, 15 Apr 2019 13:50:35 -0700
From:   Eric Anholt <eric@...olt.net>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Cc:     David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Eben Upton <eben@...pberrypi.org>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Subject: Re: [PATCH v5 4/4] drm/vc4: Allocate binner bo when starting to use the V3D

Paul Kocialkowski <paul.kocialkowski@...tlin.com> writes:

> The binner bo is not required until the V3D is in use, so avoid
> allocating it at probe and do it on the first non-dumb BO allocation.
> Keep track of which clients are using the V3D and liberate the buffer
> when there is none left (through a kref) and protect it with a mutex to
> avoid race conditions.
>
> The Out-Of-Memory (OOM) interrupt also gets some tweaking, to avoid
> enabling it before having allocated a binner bo.

I love your solution to this!

> We also want to keep the BO alive during runtime suspend/resume to avoid
> failing to allocate it at resume. This happens when the CMA pool is
> full at that point and results in a hard crash.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@...tlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_bo.c  | 30 ++++++++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_drv.c | 17 +++++++++++++
>  drivers/gpu/drm/vc4/vc4_drv.h |  9 +++++++
>  drivers/gpu/drm/vc4/vc4_irq.c |  6 +++--
>  drivers/gpu/drm/vc4/vc4_v3d.c | 47 +++++++++++++++++++++++++----------
>  5 files changed, 94 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 88ebd681d7eb..5a5276cce9a2 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -799,6 +799,28 @@ vc4_prime_import_sg_table(struct drm_device *dev,
>  	return obj;
>  }
>  
> +static int vc4_grab_bin_bo(struct drm_device *dev,
> +			   struct drm_file *file_priv)
> +{
> +	struct vc4_file *vc4file = file_priv->driver_priv;
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +
> +	if (!vc4->v3d)
> +		return -ENODEV;
> +
> +	if (vc4file->bin_bo_kref)
> +		return 0;
> +
> +	mutex_lock(&vc4->bin_bo_lock);
> +	vc4file->bin_bo_kref = vc4_v3d_bin_bo_get(vc4);
> +	mutex_unlock(&vc4->bin_bo_lock);

Interesting.  I think I would have stored a bool for whether he had the
kref, instead of a pointer to vc4->bin_bo_kref.  Either way is fine with
me, though.

> +
> +	if (!vc4file->bin_bo_kref)
> +		return -ENOMEM;
> +
> +	return 0;
> +}

> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index d840b52b9805..b09f771384be 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -126,10 +126,25 @@ static int vc4_open(struct drm_device *dev, struct drm_file *file)
>  	return 0;
>  }
>  
> +static void vc4_close_bin_bo_release(struct kref *ref)
> +{
> +	struct vc4_dev *vc4 = container_of(ref, struct vc4_dev, bin_bo_kref);
> +
> +	return vc4_v3d_bin_bo_free(vc4);
> +}

Could we have this be the function that vc4_v3d.c publishes instead of
_free, and then we don't need two separate functions for the free path?

>  static void vc4_close(struct drm_device *dev, struct drm_file *file)
>  {
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>  	struct vc4_file *vc4file = file->driver_priv;
>  
> +	mutex_lock(&vc4->bin_bo_lock);
> +
> +	if (vc4file->bin_bo_kref)
> +		kref_put(vc4file->bin_bo_kref, vc4_close_bin_bo_release);
> +
> +	mutex_unlock(&vc4->bin_bo_lock);
> +
>  	vc4_perfmon_close_file(vc4file);
>  	kfree(vc4file);
>  }


> @@ -313,6 +338,15 @@ int vc4_v3d_bin_bo_alloc(struct vc4_dev *vc4)
>  	return ret;
>  }
>  
> +void vc4_v3d_bin_bo_free(struct vc4_dev *vc4)
> +{
> +	if (!vc4->bin_bo)
> +		return;
> +
> +	drm_gem_object_put_unlocked(&vc4->bin_bo->base.base);
> +	vc4->bin_bo = NULL;

Hmm.  I'm thinking:

We free the bin BO from the DRM fd close operation, but we could have
jobs still being executed after the close if userspace didn't happen to
wait on them before finishing (closing doesn't wait for execution to
complete, and we couldn't reliably wait during close even if we wanted
to).  I think to resolve that we need the vc4_exec_info to keep a ref on
the bin BO as well (at least, if they had a binning component to their
job).

>  #ifdef CONFIG_PM
>  static int vc4_v3d_runtime_suspend(struct device *dev)
>  {
> @@ -321,9 +355,6 @@ static int vc4_v3d_runtime_suspend(struct device *dev)
>  
>  	vc4_irq_uninstall(vc4->dev);
>  
> -	drm_gem_object_put_unlocked(&vc4->bin_bo->base.base);
> -	vc4->bin_bo = NULL;
> -
>  	clk_disable_unprepare(v3d->clk);
>  
>  	return 0;
> @@ -335,10 +366,6 @@ static int vc4_v3d_runtime_resume(struct device *dev)
>  	struct vc4_dev *vc4 = v3d->vc4;
>  	int ret;
>  
> -	ret = vc4_v3d_bin_bo_alloc(vc4);
> -	if (ret)
> -		return ret;
> -
>  	ret = clk_prepare_enable(v3d->clk);
>  	if (ret != 0)
>  		return ret;

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)