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@xxxxxxxxxx>
---
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
