On Tue, Mar 23, 2021 at 12:44:28PM +0200, Mikko Perttunen wrote: > 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 > > > --- > > > 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. Alright, makes sense. > > > > > 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. Ah... you're right. host1x_syncpt_put() on the last reference doesn't actually cause the backing memory to be freed. That's a bit counter- intuitive, but I don't see why that can't work. > > > 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. Yeah, doesn't actually make anything worse. Thierry