Re: [PATCH v5 06/21] gpu: host1x: Cleanup and refcounting for syncpoints

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4640 bytes --]

On Mon, Jan 11, 2021 at 03:00:04PM +0200, Mikko Perttunen wrote:
> Add reference counting for allocated syncpoints to allow keeping
> them allocated while jobs are referencing them. Additionally,
> clean up various places using syncpoint IDs to use host1x_syncpt
> pointers instead.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> v5:
> - Remove host1x_syncpt_put in submit code, as job_put already
>   puts the syncpoint.
> - Changes due to rebase in VI driver.
> v4:
> - Update from _free to _put in VI driver as well
> ---
>  drivers/gpu/drm/tegra/dc.c             |  4 +-
>  drivers/gpu/drm/tegra/drm.c            | 14 ++---
>  drivers/gpu/drm/tegra/gr2d.c           |  4 +-
>  drivers/gpu/drm/tegra/gr3d.c           |  4 +-
>  drivers/gpu/drm/tegra/vic.c            |  4 +-
>  drivers/gpu/host1x/cdma.c              | 11 ++--
>  drivers/gpu/host1x/dev.h               |  7 ++-
>  drivers/gpu/host1x/hw/cdma_hw.c        |  2 +-
>  drivers/gpu/host1x/hw/channel_hw.c     | 10 ++--
>  drivers/gpu/host1x/hw/debug_hw.c       |  2 +-
>  drivers/gpu/host1x/job.c               |  5 +-
>  drivers/gpu/host1x/syncpt.c            | 75 +++++++++++++++++++-------
>  drivers/gpu/host1x/syncpt.h            |  3 ++
>  drivers/staging/media/tegra-video/vi.c |  4 +-
>  include/linux/host1x.h                 |  8 +--
>  15 files changed, 98 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 85dd7131553a..033032dfc4b9 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -2129,7 +2129,7 @@ static int tegra_dc_init(struct host1x_client *client)
>  		drm_plane_cleanup(primary);
>  
>  	host1x_client_iommu_detach(client);
> -	host1x_syncpt_free(dc->syncpt);
> +	host1x_syncpt_put(dc->syncpt);
>  
>  	return err;
>  }
> @@ -2154,7 +2154,7 @@ static int tegra_dc_exit(struct host1x_client *client)
>  	}
>  
>  	host1x_client_iommu_detach(client);
> -	host1x_syncpt_free(dc->syncpt);
> +	host1x_syncpt_put(dc->syncpt);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index e45c8414e2a3..5a6037eff37f 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -171,7 +171,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  	struct drm_tegra_syncpt syncpt;
>  	struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
>  	struct drm_gem_object **refs;
> -	struct host1x_syncpt *sp;
> +	struct host1x_syncpt *sp = NULL;
>  	struct host1x_job *job;
>  	unsigned int num_refs;
>  	int err;
> @@ -298,8 +298,8 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  		goto fail;
>  	}
>  
> -	/* check whether syncpoint ID is valid */
> -	sp = host1x_syncpt_get(host1x, syncpt.id);
> +	/* Syncpoint ref will be dropped on job release. */
> +	sp = host1x_syncpt_get_by_id(host1x, syncpt.id);

It's a bit odd to replace the comment like that. Perhaps instead of
replacing it, just extend it with the note about the lifetime?

>  	if (!sp) {
>  		err = -ENOENT;
>  		goto fail;
> @@ -308,7 +308,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  	job->is_addr_reg = context->client->ops->is_addr_reg;
>  	job->is_valid_class = context->client->ops->is_valid_class;
>  	job->syncpt_incrs = syncpt.incrs;
> -	job->syncpt_id = syncpt.id;
> +	job->syncpt = sp;
>  	job->timeout = 10000;
>  
>  	if (args->timeout && args->timeout < 10000)
> @@ -380,7 +380,7 @@ static int tegra_syncpt_read(struct drm_device *drm, void *data,
>  	struct drm_tegra_syncpt_read *args = data;
>  	struct host1x_syncpt *sp;
>  
> -	sp = host1x_syncpt_get(host, args->id);
> +	sp = host1x_syncpt_get_by_id_noref(host, args->id);

Why don't we need a reference here? It's perhaps unlikely, because this
function is short-lived, but the otherwise last reference to this could
go away at any point after this line and cause sp to become invalid.

In general it's very rare to not have to keep a reference to a reference
counted object.

>  	if (!sp)
>  		return -EINVAL;
>  
> @@ -395,7 +395,7 @@ static int tegra_syncpt_incr(struct drm_device *drm, void *data,
>  	struct drm_tegra_syncpt_incr *args = data;
>  	struct host1x_syncpt *sp;
>  
> -	sp = host1x_syncpt_get(host1x, args->id);
> +	sp = host1x_syncpt_get_by_id_noref(host1x, args->id);

Same here. Or am I missing some other way by which it is ensured that
the reference stays around?

Generally I like this because it makes the handling of syncpoints much
more consistent.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]