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

[Mikko Perttunen <cyndis@kapsi.fi>]

On 3/23/21 12:36 PM, Thierry Reding wrote:
> 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?

I replaced it because in the past the check was there really to just 
check if the ID is valid (the pointer was thrown away) -- now we 
actually pass the pointer into the job structure, so it serves a more 
general "get the syncpoint" purpose which is clear based on the name of 
the function. The new comment is then a new comment to clarify the 
lifetime of the reference.

> 
>>   	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.

Having a reference to a syncpoint indicates ownership of the syncpoint. 
Since here we are just reading it, we don't want ownership. (The non 
_noref functions will fail if the syncpoint is not currently allocated, 
which would break this interface.) The host1x_syncpt structure itself 
always exists even if the refcount drops to zero.

> 
>>   	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?

As above, though here we actually mutate the syncpoint even though we 
don't have a reference and as such ownership. But that's just a quirk of 
this old interface allowing incrementing of syncpoints you don't own.

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

Mikko