Re: [PATCH v6 06/10] gpu: host1x: Cleanup and refcounting for syncpoints

29.03.2021 16:38, Mikko Perttunen пишет:

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 5635fac01e3e..416d6036cf47 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -2141,7 +2141,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;

} @@ -2166,7 +2166,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 90709c38c993..ce5bdc58d315 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -174,7 +174,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;

@@ -301,8 +301,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); if (!sp) { err = -ENOENT; goto fail;

@@ -311,7 +311,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)

@@ -383,7 +383,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); if (!sp) return -EINVAL;

@@ -398,7 +398,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); if (!sp) return -EINVAL;

@@ -412,7 +412,7 @@ static int tegra_syncpt_wait(struct drm_device *drm, void *data, struct drm_tegra_syncpt_wait *args = data; struct host1x_syncpt *sp;

• sp = host1x_syncpt_get(host1x, args->id);

• sp = host1x_syncpt_get_by_id_noref(host1x, args->id); if (!sp) return -EINVAL;

diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c index adbe2ddcda19..de288cba3905 100644 --- a/drivers/gpu/drm/tegra/gr2d.c +++ b/drivers/gpu/drm/tegra/gr2d.c @@ -67,7 +67,7 @@ static int gr2d_init(struct host1x_client *client) detach: host1x_client_iommu_detach(client); free:

• host1x_syncpt_free(client->syncpts[0]);

• host1x_syncpt_put(client->syncpts[0]);

put: host1x_channel_put(gr2d->channel); return err; @@ -86,7 +86,7 @@ static int gr2d_exit(struct host1x_client *client) return err;

host1x_client_iommu_detach(client);

• host1x_syncpt_free(client->syncpts[0]);

• host1x_syncpt_put(client->syncpts[0]); host1x_channel_put(gr2d->channel);

  return 0;

diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c index b0b8154e8104..24442ade0da3 100644 --- a/drivers/gpu/drm/tegra/gr3d.c +++ b/drivers/gpu/drm/tegra/gr3d.c @@ -76,7 +76,7 @@ static int gr3d_init(struct host1x_client *client) detach: host1x_client_iommu_detach(client); free:

• host1x_syncpt_free(client->syncpts[0]);

• host1x_syncpt_put(client->syncpts[0]);

put: host1x_channel_put(gr3d->channel); return err; @@ -94,7 +94,7 @@ static int gr3d_exit(struct host1x_client *client) return err;

host1x_client_iommu_detach(client);

• host1x_syncpt_free(client->syncpts[0]);

• host1x_syncpt_put(client->syncpts[0]); host1x_channel_put(gr3d->channel);

  return 0;

diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index 77e128832920..72aea1cc0cfa 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -214,7 +214,7 @@ static int vic_init(struct host1x_client *client) return 0;

free_syncpt:

• host1x_syncpt_free(client->syncpts[0]);

• host1x_syncpt_put(client->syncpts[0]);

free_channel: host1x_channel_put(vic->channel); detach: @@ -238,7 +238,7 @@ static int vic_exit(struct host1x_client *client) if (err < 0) return err;

• host1x_syncpt_free(client->syncpts[0]);

• host1x_syncpt_put(client->syncpts[0]); host1x_channel_put(vic->channel); host1x_client_iommu_detach(client);

diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c index e8d3fda91d8a..6e6ca774f68d 100644 --- a/drivers/gpu/host1x/cdma.c +++ b/drivers/gpu/host1x/cdma.c @@ -273,15 +273,13 @@ static int host1x_cdma_wait_pushbuffer_space(struct host1x *host1x, static void cdma_start_timer_locked(struct host1x_cdma *cdma, struct host1x_job *job) {

• struct host1x *host = cdma_to_host1x(cdma);

• if (cdma->timeout.client) { /* timer already started */ return; }

  cdma->timeout.client = job->client;

• cdma->timeout.syncpt = host1x_syncpt_get(host, job->syncpt_id);

• cdma->timeout.syncpt = job->syncpt; cdma->timeout.syncpt_val = job->syncpt_end; cdma->timeout.start_ktime = ktime_get();

@@ -312,7 +310,6 @@ static void stop_cdma_timer_locked(struct host1x_cdma *cdma) static void update_cdma_locked(struct host1x_cdma *cdma) { bool signal = false;

• struct host1x *host1x = cdma_to_host1x(cdma); struct host1x_job *job, *n;

  /* If CDMA is stopped, queue is cleared and we can return */

@@ -324,8 +321,7 @@ static void update_cdma_locked(struct host1x_cdma *cdma) * to consume as many sync queue entries as possible without blocking */ list_for_each_entry_safe(job, n, &cdma->sync_queue, list) {

• struct host1x_syncpt *sp =
  

• 	host1x_syncpt_get(host1x, job->syncpt_id);
  

• struct host1x_syncpt *sp = job->syncpt;
  
  

  /* Check whether this syncpt has completed, and bail if not */ if (!host1x_syncpt_is_expired(sp, job->syncpt_end)) {

@@ -499,8 +495,7 @@ int host1x_cdma_begin(struct host1x_cdma *cdma, struct host1x_job *job) if (!cdma->timeout.initialized) { int err;

• 	err = host1x_hw_cdma_timeout_init(host1x, cdma,
  

• 					  job->syncpt_id);
  

• 	err = host1x_hw_cdma_timeout_init(host1x, cdma);
  if (err) {
  	mutex_unlock(&cdma->lock);
  	return err;
  

diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h index f781a9b0f39d..63010ae37a97 100644 --- a/drivers/gpu/host1x/dev.h +++ b/drivers/gpu/host1x/dev.h @@ -37,7 +37,7 @@ struct host1x_cdma_ops { void (*start)(struct host1x_cdma *cdma); void (*stop)(struct host1x_cdma *cdma); void (*flush)(struct host1x_cdma *cdma);

• int (*timeout_init)(struct host1x_cdma *cdma, unsigned int syncpt);

• int (*timeout_init)(struct host1x_cdma *cdma); void (*timeout_destroy)(struct host1x_cdma *cdma); void (*freeze)(struct host1x_cdma *cdma); void (*resume)(struct host1x_cdma *cdma, u32 getptr);

@@ -261,10 +261,9 @@ static inline void host1x_hw_cdma_flush(struct host1x *host, }

static inline int host1x_hw_cdma_timeout_init(struct host1x *host,

• 			      struct host1x_cdma *cdma,
  

• 			      unsigned int syncpt)
  

• 			      struct host1x_cdma *cdma)
  

{

• return host->cdma_op->timeout_init(cdma, syncpt);

• return host->cdma_op->timeout_init(cdma);

}

static inline void host1x_hw_cdma_timeout_destroy(struct host1x *host, diff --git a/drivers/gpu/host1x/hw/cdma_hw.c b/drivers/gpu/host1x/hw/cdma_hw.c index 2f3bf94cf365..e49cd5b8f735 100644 --- a/drivers/gpu/host1x/hw/cdma_hw.c +++ b/drivers/gpu/host1x/hw/cdma_hw.c @@ -295,7 +295,7 @@ static void cdma_timeout_handler(struct work_struct *work) /*

• Init timeout resources

*/ -static int cdma_timeout_init(struct host1x_cdma *cdma, unsigned int syncpt) +static int cdma_timeout_init(struct host1x_cdma *cdma) { INIT_DELAYED_WORK(&cdma->timeout.wq, cdma_timeout_handler); cdma->timeout.initialized = true; diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c index 5eaa29d171c9..d4c28faf27d1 100644 --- a/drivers/gpu/host1x/hw/channel_hw.c +++ b/drivers/gpu/host1x/hw/channel_hw.c @@ -86,8 +86,7 @@ static void submit_gathers(struct host1x_job *job)

static inline void synchronize_syncpt_base(struct host1x_job *job) {

• struct host1x *host = dev_get_drvdata(job->channel->dev->parent);
• struct host1x_syncpt *sp = host->syncpt + job->syncpt_id;

• struct host1x_syncpt *sp = job->syncpt; unsigned int id; u32 value;

@@ -118,7 +117,7 @@ static void host1x_channel_set_streamid(struct host1x_channel *channel) static int channel_submit(struct host1x_job *job) { struct host1x_channel *ch = job->channel;

• struct host1x_syncpt *sp;

• struct host1x_syncpt *sp = job->syncpt; u32 user_syncpt_incrs = job->syncpt_incrs; u32 prev_max = 0; u32 syncval;

@@ -126,10 +125,9 @@ static int channel_submit(struct host1x_job *job) struct host1x_waitlist *completed_waiter = NULL; struct host1x *host = dev_get_drvdata(ch->dev->parent);

• sp = host->syncpt + job->syncpt_id; trace_host1x_channel_submit(dev_name(ch->dev), job->num_gathers, job->num_relocs,

• 		    job->syncpt_id, job->syncpt_incrs);
  

• 		    job->syncpt->id, job->syncpt_incrs);
  
  

  /* before error checks, return current max */ prev_max = job->syncpt_end = host1x_syncpt_read_max(sp);

@@ -163,7 +161,7 @@ static int channel_submit(struct host1x_job *job) host1x_cdma_push(&ch->cdma, host1x_opcode_setclass(HOST1X_CLASS_HOST1X, host1x_uclass_wait_syncpt_r(), 1),

• 		 host1x_class_host_wait_syncpt(job->syncpt_id,
  

• 		 host1x_class_host_wait_syncpt(job->syncpt->id,
  		host1x_syncpt_read_max(sp)));
  

  }

diff --git a/drivers/gpu/host1x/hw/debug_hw.c b/drivers/gpu/host1x/hw/debug_hw.c index f31bcfa1b837..ceb48229d14b 100644 --- a/drivers/gpu/host1x/hw/debug_hw.c +++ b/drivers/gpu/host1x/hw/debug_hw.c @@ -204,7 +204,7 @@ static void show_channel_gathers(struct output *o, struct host1x_cdma *cdma) unsigned int i;

host1x_debug_output(o, "\n%p: JOB, syncpt_id=%d, syncpt_val=%d, first_get=%08x, timeout=%d num_slots=%d, num_handles=%d\n",

• 		    job, job->syncpt_id, job->syncpt_end,
  

• 		    job, job->syncpt->id, job->syncpt_end,
  	    job->first_get, job->timeout,
  	    job->num_slots, job->num_unpins);
  
  

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c index 82d0a60ba3f7..adbdc225de8d 100644 --- a/drivers/gpu/host1x/job.c +++ b/drivers/gpu/host1x/job.c @@ -79,6 +79,9 @@ static void job_free(struct kref *ref) { struct host1x_job *job = container_of(ref, struct host1x_job, ref);

• if (job->syncpt)

• host1x_syncpt_put(job->syncpt);
  

• kfree(job);

}

@@ -674,7 +677,7 @@ EXPORT_SYMBOL(host1x_job_unpin); */ void host1x_job_dump(struct device *dev, struct host1x_job *job) {

• dev_dbg(dev, " SYNCPT_ID %d\n", job->syncpt_id);

• dev_dbg(dev, " SYNCPT_ID %d\n", job->syncpt->id); dev_dbg(dev, " SYNCPT_VAL %d\n", job->syncpt_end); dev_dbg(dev, " FIRST_GET 0x%x\n", job->first_get); dev_dbg(dev, " TIMEOUT %d\n", job->timeout);

diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c index 8da4bbce8b9d..7bb5de8c3d63 100644 --- a/drivers/gpu/host1x/syncpt.c +++ b/drivers/gpu/host1x/syncpt.c @@ -90,6 +90,8 @@ struct host1x_syncpt *host1x_syncpt_alloc(struct host1x *host, else sp->client_managed = false;

• kref_init(&sp->ref);
• mutex_unlock(&host->syncpt_mutex); return sp;

@@ -383,7 +385,7 @@ int host1x_syncpt_init(struct host1x *host)

• host1x client drivers can use this function to allocate a syncpoint for
• subsequent use. A syncpoint returned by this function will be reserved for
• use by the client exclusively. When no longer using a syncpoint, a host1x

• • client driver needs to release it using host1x_syncpt_free().

• • client driver needs to release it using host1x_syncpt_put().
  */

struct host1x_syncpt *host1x_syncpt_request(struct host1x_client *client, unsigned long flags) @@ -394,20 +396,9 @@ struct host1x_syncpt *host1x_syncpt_request(struct host1x_client *client, } EXPORT_SYMBOL(host1x_syncpt_request);

-/**

• • host1x_syncpt_free() - free a requested syncpoint
• • @sp: host1x syncpoint
• • Release a syncpoint previously allocated using host1x_syncpt_request(). A
• • host1x client driver should call this when the syncpoint is no longer in
• • use. Note that client drivers must ensure that the syncpoint doesn't remain
• • under the control of hardware after calling this function, otherwise two
• • clients may end up trying to access the same syncpoint concurrently.
• */

-void host1x_syncpt_free(struct host1x_syncpt *sp) +static void syncpt_release(struct kref *ref) {

• if (!sp)

• return;
  

• struct host1x_syncpt *sp = container_of(ref, struct host1x_syncpt, ref);

  mutex_lock(&sp->host->syncpt_mutex);

@@ -419,7 +410,23 @@ void host1x_syncpt_free(struct host1x_syncpt *sp)

mutex_unlock(&sp->host->syncpt_mutex); } -EXPORT_SYMBOL(host1x_syncpt_free);

+/**

• • host1x_syncpt_put() - free a requested syncpoint
• • @sp: host1x syncpoint
• • Release a syncpoint previously allocated using host1x_syncpt_request(). A
• • host1x client driver should call this when the syncpoint is no longer in
• • use.
• */

+void host1x_syncpt_put(struct host1x_syncpt *sp) +{

• if (!sp)

• return;
  

• kref_put(&sp->ref, syncpt_release);

+} +EXPORT_SYMBOL(host1x_syncpt_put);

void host1x_syncpt_deinit(struct host1x *host) { @@ -486,16 +493,48 @@ unsigned int host1x_syncpt_nb_mlocks(struct host1x *host) }

/**

• • host1x_syncpt_get() - obtain a syncpoint by ID

• • host1x_syncpt_get_by_id() - obtain a syncpoint by ID
• • @host: host1x controller
• • @id: syncpoint ID
• */

+struct host1x_syncpt *host1x_syncpt_get_by_id(struct host1x *host,

• 			      unsigned int id)
  

+{

• if (id >= host->info->nb_pts)

• return NULL;
  

• if (kref_get_unless_zero(&host->syncpt[id].ref))

• return &host->syncpt[id];
  

• else

• return NULL;
  

+} +EXPORT_SYMBOL(host1x_syncpt_get_by_id);

+/**

• • host1x_syncpt_get_by_id_noref() - obtain a syncpoint by ID but don't
• • increase the refcount.
  • @host: host1x controller
  • @id: syncpoint ID
  */

-struct host1x_syncpt *host1x_syncpt_get(struct host1x *host, unsigned int id) +struct host1x_syncpt *host1x_syncpt_get_by_id_noref(struct host1x *host,

• 				    unsigned int id)
  

{ if (id >= host->info->nb_pts) return NULL;

• return host->syncpt + id;

• return &host->syncpt[id];

+} +EXPORT_SYMBOL(host1x_syncpt_get_by_id_noref);

+/**

• • host1x_syncpt_get() - increment syncpoint refcount
• • @sp: syncpoint
• */

+struct host1x_syncpt *host1x_syncpt_get(struct host1x_syncpt *sp) +{

• kref_get(&sp->ref);
• return sp;

} EXPORT_SYMBOL(host1x_syncpt_get);

diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h index 3aa6b25b1b9c..a6766f8d55ee 100644 --- a/drivers/gpu/host1x/syncpt.h +++ b/drivers/gpu/host1x/syncpt.h @@ -11,6 +11,7 @@ #include <linux/atomic.h> #include <linux/host1x.h> #include <linux/kernel.h> +#include <linux/kref.h> #include <linux/sched.h>

#include "intr.h" @@ -26,6 +27,8 @@ struct host1x_syncpt_base { };

struct host1x_syncpt {

• struct kref ref;
• unsigned int id; atomic_t min_val; atomic_t max_val;

diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c index 7a09061cda57..7e0cb5529b49 100644 --- a/drivers/staging/media/tegra-video/vi.c +++ b/drivers/staging/media/tegra-video/vi.c @@ -1131,8 +1131,8 @@ static void tegra_channel_host1x_syncpts_free(struct tegra_vi_channel *chan) int i;

for (i = 0; i < chan->numgangports; i++) {

• host1x_syncpt_free(chan->mw_ack_sp[i]);
  

• host1x_syncpt_free(chan->frame_start_sp[i]);
  

• host1x_syncpt_put(chan->mw_ack_sp[i]);
  

• host1x_syncpt_put(chan->frame_start_sp[i]);
  

  }

}

diff --git a/include/linux/host1x.h b/include/linux/host1x.h index 7137ce0e35d4..107aea29bccb 100644 --- a/include/linux/host1x.h +++ b/include/linux/host1x.h @@ -142,7 +142,9 @@ struct host1x_syncpt_base; struct host1x_syncpt; struct host1x;

-struct host1x_syncpt *host1x_syncpt_get(struct host1x *host, u32 id); +struct host1x_syncpt *host1x_syncpt_get_by_id(struct host1x *host, u32 id); +struct host1x_syncpt *host1x_syncpt_get_by_id_noref(struct host1x *host, u32 id); +struct host1x_syncpt *host1x_syncpt_get(struct host1x_syncpt *sp); u32 host1x_syncpt_id(struct host1x_syncpt *sp); u32 host1x_syncpt_read_min(struct host1x_syncpt *sp); u32 host1x_syncpt_read_max(struct host1x_syncpt *sp); @@ -153,7 +155,7 @@ int host1x_syncpt_wait(struct host1x_syncpt *sp, u32 thresh, long timeout, u32 *value); struct host1x_syncpt *host1x_syncpt_request(struct host1x_client *client, unsigned long flags); -void host1x_syncpt_free(struct host1x_syncpt *sp); +void host1x_syncpt_put(struct host1x_syncpt *sp); struct host1x_syncpt *host1x_syncpt_alloc(struct host1x *host, unsigned long flags, const char *name); @@ -221,7 +223,7 @@ struct host1x_job { dma_addr_t *reloc_addr_phys;

/* Sync point id, number of increments and end related to the submit */

• u32 syncpt_id;

• struct host1x_syncpt *syncpt; u32 syncpt_incrs; u32 syncpt_end;

Tested-by: Dmitry Osipenko digetx@gmail.com