On 05.11.2017 14:01, Mikko Perttunen wrote:

Host1x has a feature called MLOCKs which allow a certain class (~HW unit) to be locked (in the mutex sense) and unlocked during command execution, preventing other channels from accessing the class while it is locked. This is necessary to prevent concurrent jobs from messing up class state.

This has not been necessary so far since due to our channel allocation model, there has only been a single hardware channel submitting commands to each class. Future patches, however, change the channel allocation model to allow hardware-scheduled concurrency, and as such we need to start locking.

This patch implements locking on all platforms from Tegra20 to Tegra186.

Signed-off-by: Mikko Perttunen mperttunen@nvidia.com

drivers/gpu/host1x/cdma.c | 1 + drivers/gpu/host1x/cdma.h | 1 + drivers/gpu/host1x/hw/cdma_hw.c | 122 +++++++++++++++++++++++++ drivers/gpu/host1x/hw/channel_hw.c | 71 ++++++++++---- drivers/gpu/host1x/hw/host1x01_hardware.h | 10 ++ drivers/gpu/host1x/hw/host1x02_hardware.h | 10 ++ drivers/gpu/host1x/hw/host1x04_hardware.h | 10 ++ drivers/gpu/host1x/hw/host1x05_hardware.h | 10 ++ drivers/gpu/host1x/hw/host1x06_hardware.h | 10 ++ drivers/gpu/host1x/hw/hw_host1x01_sync.h | 6 ++ drivers/gpu/host1x/hw/hw_host1x02_sync.h | 6 ++ drivers/gpu/host1x/hw/hw_host1x04_sync.h | 6 ++ drivers/gpu/host1x/hw/hw_host1x05_sync.h | 6 ++ drivers/gpu/host1x/hw/hw_host1x06_hypervisor.h | 5 + 14 files changed, 257 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c index 28541b280739..f787cfe69c11 100644 --- a/drivers/gpu/host1x/cdma.c +++ b/drivers/gpu/host1x/cdma.c @@ -232,6 +232,7 @@ static void cdma_start_timer_locked(struct host1x_cdma *cdma, }

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

• cdma->timeout.class = job->class; cdma->timeout.syncpt = host1x_syncpt_get(host, job->syncpt_id); cdma->timeout.syncpt_val = job->syncpt_end; cdma->timeout.start_ktime = ktime_get();

diff --git a/drivers/gpu/host1x/cdma.h b/drivers/gpu/host1x/cdma.h index 286d49386be9..e72660fc83c9 100644 --- a/drivers/gpu/host1x/cdma.h +++ b/drivers/gpu/host1x/cdma.h @@ -59,6 +59,7 @@ struct buffer_timeout { ktime_t start_ktime; /* starting time */ /* context timeout information */ int client;

• u32 class;

};

enum cdma_event { diff --git a/drivers/gpu/host1x/hw/cdma_hw.c b/drivers/gpu/host1x/hw/cdma_hw.c index ce320534cbed..4d5970d863d5 100644 --- a/drivers/gpu/host1x/hw/cdma_hw.c +++ b/drivers/gpu/host1x/hw/cdma_hw.c @@ -16,6 +16,7 @@

• along with this program. If not, see http://www.gnu.org/licenses/.

*/

+#include <linux/iopoll.h> #include <linux/slab.h> #include <linux/scatterlist.h> #include <linux/dma-mapping.h> @@ -243,6 +244,125 @@ static void cdma_resume(struct host1x_cdma *cdma, u32 getptr) cdma_timeout_restart(cdma, getptr); }

+static int mlock_id_for_class(unsigned int class) +{ +#if HOST1X_HW >= 6

• switch (class)
• {
• case HOST1X_CLASS_HOST1X:

• return 0;
  

• case HOST1X_CLASS_VIC:

• return 17;
  

What is the meaning of returned ID values that you have defined here? Why VIC should have different ID on T186?

• default:

• return -EINVAL;
  

• }

+#else

• switch (class)
• {
• case HOST1X_CLASS_HOST1X:

• return 0;
  

• case HOST1X_CLASS_GR2D:

• return 1;
  

• case HOST1X_CLASS_GR2D_SB:

• return 2;
  

Note that we are allowing to switch 2d classes in the same jobs context and currently jobs class is somewhat hardcoded to GR2D.

Even though that GR2D and GR2D_SB use different register banks, is it okay to trigger execution of different classes simultaneously? Would syncpoint differentiate classes on OP_DONE event?

I suppose that MLOCK (the module lock) implies the whole module locking, wouldn't it make sense to just use the module ID's defined in the TRM?

• case HOST1X_CLASS_VIC:

• return 3;
  

• case HOST1X_CLASS_GR3D:

• return 4;
  

• default:

• return -EINVAL;
  

• }

+#endif +}

+static void timeout_release_mlock(struct host1x_cdma *cdma) +{ +#if HOST1X_HW >= 6

• struct host1x_channel *ch = cdma_to_channel(cdma);
• struct host1x *host = cdma_to_host1x(cdma);
• u32 pb_pos, pb_temp[3], val;
• int err, mlock_id;
• if (!host->hv_regs)

• return;
  

• mlock_id = mlock_id_for_class(cdma->timeout.class);
• if (WARN(mlock_id < 0, "Invalid class ID"))

• return;
  

• val = host1x_hypervisor_readl(host, HOST1X_HV_MLOCK(mlock_id));
• if (!HOST1X_HV_MLOCK_LOCKED_V(val) ||

•    HOST1X_HV_MLOCK_CH_V(val) != ch->id)
  

• {

• /* Channel is not holding the MLOCK, nothing to release. */
  

• return;
  

• }
• /*

• * On Tegra186, there is no register to unlock an MLOCK (don't ask me
  

• * why). As such, we have to execute a release_mlock instruction to
  

• * do it. We do this by backing up the first three opcodes of the
  

• * pushbuffer and replacing them with our own short sequence to do
  

• * the unlocking. We set the .pos field to 12, which causes DMAEND
  

• * to be set accordingly such that only the three opcodes we set
  

• * here are executed before CDMA stops. Finally we restore the value
  

• * of pos and pushbuffer contents.
  

• */
  

• pb_pos = cdma->push_buffer.pos;
• memcpy(pb_temp, cdma->push_buffer.mapped, ARRAY_SIZE(pb_temp) * 4);
• {

• u32 *pb = cdma->push_buffer.mapped;
  

• pb[0] = host1x_opcode_acquire_mlock(cdma->timeout.class);
  

• pb[1] = host1x_opcode_setclass(cdma->timeout.class, 0, 0);
  

• pb[2] = host1x_opcode_release_mlock(cdma->timeout.class);
  

• }
• /* Flush writecombine buffer */
• wmb();
• cdma->push_buffer.pos = ARRAY_SIZE(pb_temp) * 4;
• cdma_resume(cdma, 0);
• /* Wait until the release_mlock opcode has been executed */
• err = readl_relaxed_poll_timeout(

• host->hv_regs + HOST1X_HV_MLOCK(mlock_id), val,
  

• !HOST1X_HV_MLOCK_LOCKED_V(val) ||
  

• 	HOST1X_HV_MLOCK_CH_V(val) != ch->id,
  

• 10, 10000);
  

• WARN(err, "Failed to unlock mlock %u\n", mlock_id);
• cdma_freeze(cdma);
• /* Restore original pushbuffer state */
• cdma->push_buffer.pos = pb_pos;
• memcpy(cdma->push_buffer.mapped, pb_temp, ARRAY_SIZE(pb_temp) * 4);
• wmb();

+#else

• struct host1x_channel *ch = cdma_to_channel(cdma);
• struct host1x *host = cdma_to_host1x(cdma);
• int mlock_id;
• u32 val;
• mlock_id = mlock_id_for_class(cdma->timeout.class);
• if (WARN(mlock_id < 0, "Invalid class ID"))

• return;
  

• val = host1x_sync_readl(host, HOST1X_SYNC_MLOCK_OWNER(mlock_id));
• if (!HOST1X_SYNC_MLOCK_OWNER_CH_OWNS_V(val) ||

•    HOST1X_SYNC_MLOCK_OWNER_CHID_V(val) != ch->id)
  

• {

• /* Channel is not holding the MLOCK, nothing to release. */
  

• return;
  

• }
• /* Unlock MLOCK */
• host1x_sync_writel(host, 0x0, HOST1X_SYNC_MLOCK(mlock_id));

+#endif +}

/*

• If this timeout fires, it indicates the current sync_queue entry has
• exceeded its TTL and the userctx should be timed out and remaining

@@ -293,6 +413,8 @@ static void cdma_timeout_handler(struct work_struct *work) /* stop HW, resetting channel/module */ host1x_hw_cdma_freeze(host1x, cdma);

• timeout_release_mlock(cdma);
• host1x_cdma_update_sync_queue(cdma, ch->dev); mutex_unlock(&cdma->lock);

} diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c index 246b78c41281..f80fb8be38e6 100644 --- a/drivers/gpu/host1x/hw/channel_hw.c +++ b/drivers/gpu/host1x/hw/channel_hw.c @@ -89,6 +89,34 @@ static inline void synchronize_syncpt_base(struct host1x_job *job) HOST1X_UCLASS_LOAD_SYNCPT_BASE_VALUE_F(value)); }

+static void channel_submit_serialize(struct host1x_channel *ch,

• 		     struct host1x_syncpt *sp)
  

+{ +#if HOST1X_HW >= 6

• /*

• * On T186, due to a hw issue, we need to issue an mlock acquire
  

• * for HOST1X class. It is still interpreted as a no-op in
  

• * hardware.
  

• */
  

• host1x_cdma_push(&ch->cdma,

• host1x_opcode_acquire_mlock(HOST1X_CLASS_HOST1X),
  

• host1x_opcode_setclass(HOST1X_CLASS_HOST1X, 0, 0));
  

• host1x_cdma_push(&ch->cdma,

• host1x_opcode_nonincr(host1x_uclass_wait_syncpt_r(), 1),
  

• host1x_class_host_wait_syncpt(host1x_syncpt_id(sp),
  

• 			      host1x_syncpt_read_max(sp)));
  

• host1x_cdma_push(&ch->cdma,

• host1x_opcode_release_mlock(HOST1X_CLASS_HOST1X),
  

• HOST1X_OPCODE_NOP);
  

+#else

• host1x_cdma_push(&ch->cdma,

• host1x_opcode_setclass(HOST1X_CLASS_HOST1X,
  

• 		       host1x_uclass_wait_syncpt_r(), 1),
  

• host1x_class_host_wait_syncpt(host1x_syncpt_id(sp),
  

• 			      host1x_syncpt_read_max(sp)));
  

+#endif +}

static int channel_submit(struct host1x_job *job) { struct host1x_channel *ch = job->channel; @@ -96,6 +124,7 @@ static int channel_submit(struct host1x_job *job) u32 user_syncpt_incrs = job->syncpt_incrs; u32 prev_max = 0; u32 syncval;

• u32 mlock; int err; struct host1x_waitlist *completed_waiter = NULL; struct host1x *host = dev_get_drvdata(ch->dev->parent);

@@ -128,17 +157,12 @@ static int channel_submit(struct host1x_job *job) goto error; }

• if (job->serialize) {

• /*
  

•  * Force serialization by inserting a host wait for the
  

•  * previous job to finish before this one can commence.
  

•  */
  

• 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_syncpt_read_max(sp)));
  

• }

• /*

• * Force serialization by inserting a host wait for the
  

• * previous job to finish before this one can commence.
  

• */
  

• if (job->serialize)

• channel_submit_serialize(ch, sp);
  
  

  /* Synchronize base register to allow using it for relative waiting */ if (sp->base)

@@ -149,16 +173,29 @@ static int channel_submit(struct host1x_job *job) /* assign syncpoint to channel */ host1x_hw_syncpt_assign_channel(host, sp, ch);

• job->syncpt_end = syncval;
• /* add a setclass for modules that require it */
• if (job->class)

• /* acquire MLOCK and set channel class to specified class */
• mlock = HOST1X_HW >= 6 ? job->class : mlock_id_for_class(job->class);
• if (job->class) { host1x_cdma_push(&ch->cdma,

• 		 host1x_opcode_setclass(job->class, 0, 0),
  

• 		 HOST1X_OPCODE_NOP);
  

• 		 host1x_opcode_acquire_mlock(mlock),
  

• 		 host1x_opcode_setclass(job->class, 0, 0));
  

• }

  submit_gathers(job);

• if (job->class) {

• /*
  

•  * Push additional increment to catch jobs that crash before
  

•  * finishing their gather, not reaching the unlock opcode.
  

•  */
  

• syncval = host1x_syncpt_incr_max(sp, 1);
  

• host1x_cdma_push(&ch->cdma,
  

• 	host1x_opcode_imm_incr_syncpt(0, job->syncpt_id),
  

• 	host1x_opcode_release_mlock(mlock));
  

• }

• job->syncpt_end = syncval;

• /* end CDMA submit & stash pinned hMems into sync queue */ host1x_cdma_end(&ch->cdma, job);

diff --git a/drivers/gpu/host1x/hw/host1x01_hardware.h b/drivers/gpu/host1x/hw/host1x01_hardware.h index 5f0fb866efa8..6a2c9f905acc 100644 --- a/drivers/gpu/host1x/hw/host1x01_hardware.h +++ b/drivers/gpu/host1x/hw/host1x01_hardware.h @@ -138,6 +138,16 @@ static inline u32 host1x_opcode_gather_incr(unsigned offset, unsigned count) return (6 << 28) | (offset << 16) | BIT(15) | BIT(14) | count; }

+static inline u32 host1x_opcode_acquire_mlock(unsigned id) +{

• return (14 << 28) | id;

+}

+static inline u32 host1x_opcode_release_mlock(unsigned id) +{

• return (14 << 28) | (1 << 24) | id;

+}

#define HOST1X_OPCODE_NOP host1x_opcode_nonincr(0, 0)

#endif diff --git a/drivers/gpu/host1x/hw/host1x02_hardware.h b/drivers/gpu/host1x/hw/host1x02_hardware.h index 154901860bc6..c524c6c8d82f 100644 --- a/drivers/gpu/host1x/hw/host1x02_hardware.h +++ b/drivers/gpu/host1x/hw/host1x02_hardware.h @@ -137,6 +137,16 @@ static inline u32 host1x_opcode_gather_incr(unsigned offset, unsigned count) return (6 << 28) | (offset << 16) | BIT(15) | BIT(14) | count; }

+static inline u32 host1x_opcode_acquire_mlock(unsigned id) +{

• return (14 << 28) | id;

+}

+static inline u32 host1x_opcode_release_mlock(unsigned id) +{

• return (14 << 28) | (1 << 24) | id;

+}

#define HOST1X_OPCODE_NOP host1x_opcode_nonincr(0, 0)

#endif diff --git a/drivers/gpu/host1x/hw/host1x04_hardware.h b/drivers/gpu/host1x/hw/host1x04_hardware.h index de1a38175328..8dedd77b7a1b 100644 --- a/drivers/gpu/host1x/hw/host1x04_hardware.h +++ b/drivers/gpu/host1x/hw/host1x04_hardware.h @@ -137,6 +137,16 @@ static inline u32 host1x_opcode_gather_incr(unsigned offset, unsigned count) return (6 << 28) | (offset << 16) | BIT(15) | BIT(14) | count; }

+static inline u32 host1x_opcode_acquire_mlock(unsigned id) +{

• return (14 << 28) | id;

+}

+static inline u32 host1x_opcode_release_mlock(unsigned id) +{

• return (14 << 28) | (1 << 24) | id;

+}

#define HOST1X_OPCODE_NOP host1x_opcode_nonincr(0, 0)

#endif diff --git a/drivers/gpu/host1x/hw/host1x05_hardware.h b/drivers/gpu/host1x/hw/host1x05_hardware.h index 2937ebb6be11..6aafcb5e97e6 100644 --- a/drivers/gpu/host1x/hw/host1x05_hardware.h +++ b/drivers/gpu/host1x/hw/host1x05_hardware.h @@ -137,6 +137,16 @@ static inline u32 host1x_opcode_gather_incr(unsigned offset, unsigned count) return (6 << 28) | (offset << 16) | BIT(15) | BIT(14) | count; }

+static inline u32 host1x_opcode_acquire_mlock(unsigned id) +{

• return (14 << 28) | id;

+}

+static inline u32 host1x_opcode_release_mlock(unsigned id) +{

• return (14 << 28) | (1 << 24) | id;

+}

#define HOST1X_OPCODE_NOP host1x_opcode_nonincr(0, 0)

#endif diff --git a/drivers/gpu/host1x/hw/host1x06_hardware.h b/drivers/gpu/host1x/hw/host1x06_hardware.h index 3039c92ea605..60147d26ad9b 100644 --- a/drivers/gpu/host1x/hw/host1x06_hardware.h +++ b/drivers/gpu/host1x/hw/host1x06_hardware.h @@ -137,6 +137,16 @@ static inline u32 host1x_opcode_gather_incr(unsigned offset, unsigned count) return (6 << 28) | (offset << 16) | BIT(15) | BIT(14) | count; }

+static inline u32 host1x_opcode_acquire_mlock(unsigned id) +{

• return (14 << 28) | id;

+}

+static inline u32 host1x_opcode_release_mlock(unsigned id) +{

• return (14 << 28) | (1 << 24) | id;

+}

#define HOST1X_OPCODE_NOP host1x_opcode_nonincr(0, 0)

#endif diff --git a/drivers/gpu/host1x/hw/hw_host1x01_sync.h b/drivers/gpu/host1x/hw/hw_host1x01_sync.h index 31238c285d46..4630fec2237a 100644 --- a/drivers/gpu/host1x/hw/hw_host1x01_sync.h +++ b/drivers/gpu/host1x/hw/hw_host1x01_sync.h @@ -125,6 +125,12 @@ static inline u32 host1x_sync_ip_busy_timeout_r(void) } #define HOST1X_SYNC_IP_BUSY_TIMEOUT \ host1x_sync_ip_busy_timeout_r() +static inline u32 host1x_sync_mlock_r(unsigned int id) +{

• return 0x2c0 + id * REGISTER_STRIDE;

+} +#define HOST1X_SYNC_MLOCK(id) \

• host1x_sync_mlock_r(id)

static inline u32 host1x_sync_mlock_owner_r(unsigned int id) { return 0x340 + id * REGISTER_STRIDE; diff --git a/drivers/gpu/host1x/hw/hw_host1x02_sync.h b/drivers/gpu/host1x/hw/hw_host1x02_sync.h index 540c7b65995f..dec8e812a114 100644 --- a/drivers/gpu/host1x/hw/hw_host1x02_sync.h +++ b/drivers/gpu/host1x/hw/hw_host1x02_sync.h @@ -125,6 +125,12 @@ static inline u32 host1x_sync_ip_busy_timeout_r(void) } #define HOST1X_SYNC_IP_BUSY_TIMEOUT \ host1x_sync_ip_busy_timeout_r() +static inline u32 host1x_sync_mlock_r(unsigned int id) +{

• return 0x2c0 + id * REGISTER_STRIDE;

+} +#define HOST1X_SYNC_MLOCK(id) \

• host1x_sync_mlock_r(id)

static inline u32 host1x_sync_mlock_owner_r(unsigned int id) { return 0x340 + id * REGISTER_STRIDE; diff --git a/drivers/gpu/host1x/hw/hw_host1x04_sync.h b/drivers/gpu/host1x/hw/hw_host1x04_sync.h index 3d6c8ec65934..9a951a4db07f 100644 --- a/drivers/gpu/host1x/hw/hw_host1x04_sync.h +++ b/drivers/gpu/host1x/hw/hw_host1x04_sync.h @@ -125,6 +125,12 @@ static inline u32 host1x_sync_ip_busy_timeout_r(void) } #define HOST1X_SYNC_IP_BUSY_TIMEOUT \ host1x_sync_ip_busy_timeout_r() +static inline u32 host1x_sync_mlock_r(unsigned int id) +{

• return 0x2c0 + id * REGISTER_STRIDE;

+} +#define HOST1X_SYNC_MLOCK(id) \

• host1x_sync_mlock_r(id)

static inline u32 host1x_sync_mlock_owner_r(unsigned int id) { return 0x340 + id * REGISTER_STRIDE; diff --git a/drivers/gpu/host1x/hw/hw_host1x05_sync.h b/drivers/gpu/host1x/hw/hw_host1x05_sync.h index ca10eee5045c..e29e2e19a60b 100644 --- a/drivers/gpu/host1x/hw/hw_host1x05_sync.h +++ b/drivers/gpu/host1x/hw/hw_host1x05_sync.h @@ -125,6 +125,12 @@ static inline u32 host1x_sync_ip_busy_timeout_r(void) } #define HOST1X_SYNC_IP_BUSY_TIMEOUT \ host1x_sync_ip_busy_timeout_r() +static inline u32 host1x_sync_mlock_r(unsigned int id) +{

• return 0x2c0 + id * REGISTER_STRIDE;

+} +#define HOST1X_SYNC_MLOCK(id) \

• host1x_sync_mlock_r(id)

static inline u32 host1x_sync_mlock_owner_r(unsigned int id) { return 0x340 + id * REGISTER_STRIDE; diff --git a/drivers/gpu/host1x/hw/hw_host1x06_hypervisor.h b/drivers/gpu/host1x/hw/hw_host1x06_hypervisor.h index c05dab8a178b..be73530c3585 100644 --- a/drivers/gpu/host1x/hw/hw_host1x06_hypervisor.h +++ b/drivers/gpu/host1x/hw/hw_host1x06_hypervisor.h @@ -18,6 +18,11 @@ #define HOST1X_HV_SYNCPT_PROT_EN 0x1ac4 #define HOST1X_HV_SYNCPT_PROT_EN_CH_EN BIT(1) #define HOST1X_HV_CH_KERNEL_FILTER_GBUFFER(x) (0x2020 + (x * 4)) +#define HOST1X_HV_MLOCK(x) (0x2030 + (x * 4)) +#define HOST1X_HV_MLOCK_CH(x) (((x) & 0x3f) << 2) +#define HOST1X_HV_MLOCK_CH_V(x) (((x) >> 2) & 0x3f) +#define HOST1X_HV_MLOCK_LOCKED BIT(0) +#define HOST1X_HV_MLOCK_LOCKED_V(x) ((x) & 0x1) #define HOST1X_HV_CMDFIFO_PEEK_CTRL 0x233c #define HOST1X_HV_CMDFIFO_PEEK_CTRL_ADDR(x) (x) #define HOST1X_HV_CMDFIFO_PEEK_CTRL_CHANNEL(x) ((x) << 16)

On 05.11.2017 14:01, Mikko Perttunen wrote:

To allow client drivers to free resources when jobs have completed, deliver job completion callbacks to them. This requires adding reference counting to context objects, as job completion can happen after the userspace application has closed the context. As such, also add kref-based refcounting for contexts.

Signed-off-by: Mikko Perttunen mperttunen@nvidia.com

drivers/gpu/drm/tegra/drm.c | 27 ++++++++++++++++++++++++--- drivers/gpu/drm/tegra/drm.h | 4 ++++ 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 2cdd054520bf..3e2a4a19412e 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -281,8 +281,11 @@ static int tegra_drm_open(struct drm_device *drm, struct drm_file *filp) return 0; }

-static void tegra_drm_context_free(struct tegra_drm_context *context) +static void tegra_drm_context_free(struct kref *ref) {

• struct tegra_drm_context *context =

• container_of(ref, struct tegra_drm_context, ref);
  

• context->client->ops->close_channel(context); kfree(context);

} @@ -379,6 +382,16 @@ static int host1x_waitchk_copy_from_user(struct host1x_waitchk *dest, return 0; }

+static void tegra_drm_job_done(struct host1x_job *job) +{

• struct tegra_drm_context *context = job->callback_data;
• if (context->client->ops->submit_done)

• context->client->ops->submit_done(context);
  

• kref_put(&context->ref, tegra_drm_context_free);

+}

int tegra_drm_submit(struct tegra_drm_context *context, struct drm_tegra_submit *args, struct drm_device *drm, struct drm_file *file) @@ -560,6 +573,9 @@ int tegra_drm_submit(struct tegra_drm_context *context, job->syncpt_id = syncpt.id; job->timeout = 10000;

• job->done = tegra_drm_job_done;
• job->callback_data = context;
• if (args->timeout && args->timeout < 10000) job->timeout = args->timeout;

@@ -567,8 +583,11 @@ int tegra_drm_submit(struct tegra_drm_context *context, if (err) goto fail;

• kref_get(&context->ref);
• err = host1x_job_submit(job); if (err) {

• kref_put(&context->ref, tegra_drm_context_free);
  

  host1x_job_unpin(job); goto fail; }

@@ -717,6 +736,8 @@ static int tegra_open_channel(struct drm_device *drm, void *data, if (err < 0) kfree(context);

• kref_init(&context->ref);
• mutex_unlock(&fpriv->lock); return err;

} @@ -738,7 +759,7 @@ static int tegra_close_channel(struct drm_device *drm, void *data, }

idr_remove(&fpriv->contexts, context->id);

• tegra_drm_context_free(context);

• kref_put(&context->ref, tegra_drm_context_free);

unlock: mutex_unlock(&fpriv->lock); @@ -1026,7 +1047,7 @@ static int tegra_drm_context_cleanup(int id, void *p, void *data) { struct tegra_drm_context *context = p;

• tegra_drm_context_free(context);

• kref_put(&context->ref, tegra_drm_context_free);

Probably won't hurt to add and use tegra_drm_context_get()/tegra_drm_context_put().

return 0; } diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index 063f5d397526..079aebb3fb38 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -13,6 +13,7 @@ #include <uapi/drm/tegra_drm.h> #include <linux/host1x.h> #include <linux/iova.h> +#include <linux/kref.h> #include <linux/of_gpio.h>

#include <drm/drmP.h> @@ -74,6 +75,8 @@ struct tegra_drm { struct tegra_drm_client;

struct tegra_drm_context {

• struct kref ref;
• struct tegra_drm_client *client; struct host1x_channel *channel; unsigned int id;

@@ -88,6 +91,7 @@ struct tegra_drm_client_ops { int (*submit)(struct tegra_drm_context *context, struct drm_tegra_submit *args, struct drm_device *drm, struct drm_file *file);

• void (*submit_done)(struct tegra_drm_context *context);

};

int tegra_drm_submit(struct tegra_drm_context *context,

On 05.11.2017 14:01, Mikko Perttunen wrote:

In the traditional channel allocation model, a single hardware channel was allocated for each client. This is simple from an implementation perspective but prevents use of hardware scheduling.

This patch implements a channel allocation model where when a user submits a job for a context, a hardware channel is allocated for that context. The same channel is kept for as long as there are incomplete jobs for that context. This way we can use hardware scheduling and channel isolation between userspace processes, but also prevent idling contexts from taking up hardware resources.

The dynamic channels resources (pushbuf) allocation is very expensive, neglecting all benefits that this model should bring at least in non-IOMMU case. We could have statically preallocated channels resources or defer resources freeing.

For now, this patch only adapts VIC to the new model.

I think VIC's conversion should be a distinct patch.

Signed-off-by: Mikko Perttunen mperttunen@nvidia.com

drivers/gpu/drm/tegra/drm.c | 46 ++++++++++++++++++++++++++ drivers/gpu/drm/tegra/drm.h | 7 +++- drivers/gpu/drm/tegra/vic.c | 79 +++++++++++++++++++++++---------------------- 3 files changed, 92 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index b964e18e3058..658bc8814f38 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -382,6 +382,51 @@ static int host1x_waitchk_copy_from_user(struct host1x_waitchk *dest, return 0; }

+/**

• • tegra_drm_context_get_channel() - Get a channel for submissions
• • @context: Context for which to get a channel for
• • Request a free hardware host1x channel for this user context, or if the
• • context already has one, bump its refcount.
• • Returns 0 on success, or -EBUSY if there were no free hardware channels.
• */

+int tegra_drm_context_get_channel(struct tegra_drm_context *context) +{

• struct host1x_client *client = &context->client->base;
• mutex_lock(&context->lock);
• if (context->pending_jobs == 0) {

• context->channel = host1x_channel_request(client->dev);
  

• if (!context->channel) {
  

• 	mutex_unlock(&context->lock);
  

• 	return -EBUSY;
  

• }
  

• }
• context->pending_jobs++;
• mutex_unlock(&context->lock);
• return 0;

+}

+/**

• • tegra_drm_context_put_channel() - Put a previously gotten channel
• • @context: Context which channel is no longer needed
• • Decrease the refcount of the channel associated with this context,
• • freeing it if the refcount drops to zero.
• */

+void tegra_drm_context_put_channel(struct tegra_drm_context *context) +{

• mutex_lock(&context->lock);
• if (--context->pending_jobs == 0)

• host1x_channel_put(context->channel);
  

• mutex_unlock(&context->lock);

+}

static void tegra_drm_job_done(struct host1x_job *job) { struct tegra_drm_context *context = job->callback_data; @@ -737,6 +782,7 @@ static int tegra_open_channel(struct drm_device *drm, void *data, kfree(context);

kref_init(&context->ref);

• mutex_init(&context->lock);

  mutex_unlock(&fpriv->lock); return err;

diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index 11d690846fd0..d0c3f1f779f6 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -78,9 +78,12 @@ struct tegra_drm_context { struct kref ref;

struct tegra_drm_client *client;

• unsigned int id;
• struct mutex lock; struct host1x_channel *channel; struct host1x_syncpt *syncpt;

• unsigned int id;

• unsigned int pending_jobs;

};

struct tegra_drm_client_ops { @@ -95,6 +98,8 @@ struct tegra_drm_client_ops { void (*submit_done)(struct tegra_drm_context *context); };

+int tegra_drm_context_get_channel(struct tegra_drm_context *context); +void tegra_drm_context_put_channel(struct tegra_drm_context *context); int tegra_drm_submit(struct tegra_drm_context *context, struct drm_tegra_submit *args, struct drm_device *drm, struct drm_file *file); diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index efe5f3af933e..0cacf023a890 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -33,7 +33,6 @@ struct vic {

void __iomem *regs; struct tegra_drm_client client;

• struct host1x_channel *channel; struct iommu_domain *domain; struct device *dev; struct clk *clk;

@@ -161,28 +160,12 @@ static int vic_init(struct host1x_client *client) goto detach_device; }

• vic->channel = host1x_channel_request(client->dev);
• if (!vic->channel) {

• err = -ENOMEM;
  

• goto detach_device;
  

• }
• client->syncpts[0] = host1x_syncpt_request(client->dev, 0);
• if (!client->syncpts[0]) {

• err = -ENOMEM;
  

• goto free_channel;
  

• }
• err = tegra_drm_register_client(tegra, drm); if (err < 0)

• goto free_syncpt;
  

• goto detach_device;
  
  

  return 0;

-free_syncpt:

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

-free_channel:

• host1x_channel_put(vic->channel);

detach_device: if (tegra->domain) iommu_detach_device(tegra->domain, vic->dev); @@ -202,9 +185,6 @@ static int vic_exit(struct host1x_client *client) if (err < 0) return err;

• host1x_syncpt_free(client->syncpts[0]);
• host1x_channel_put(vic->channel);
• if (vic->domain) { iommu_detach_device(vic->domain, vic->dev); vic->domain = NULL;

@@ -221,7 +201,24 @@ static const struct host1x_client_ops vic_client_ops = { static int vic_open_channel(struct tegra_drm_client *client, struct tegra_drm_context *context) {

• struct vic *vic = to_vic(client);

• context->syncpt = host1x_syncpt_request(client->base.dev, 0);
• if (!context->syncpt)

• return -ENOMEM;
  

• return 0;

+}

+static void vic_close_channel(struct tegra_drm_context *context) +{

• host1x_syncpt_free(context->syncpt);

+}

+static int vic_submit(struct tegra_drm_context *context,

•       struct drm_tegra_submit *args, struct drm_device *drm,
  

•       struct drm_file *file)
  

+{

• struct host1x_client *client = &context->client->base;

• struct vic *vic = dev_get_drvdata(client->dev); int err;

  err = pm_runtime_get_sync(vic->dev);

@@ -229,35 +226,41 @@ static int vic_open_channel(struct tegra_drm_client *client, return err;

err = vic_boot(vic);

• if (err < 0) {

• pm_runtime_put(vic->dev);
  

• return err;
  

• }

• if (err < 0)

• goto put_vic;
  
  

• context->channel = host1x_channel_get(vic->channel);
• if (!context->channel) {

• pm_runtime_put(vic->dev);
  

• return -ENOMEM;
  

• }

• err = tegra_drm_context_get_channel(context);
• if (err < 0)

• goto put_vic;
  
  

• context->syncpt = client->base.syncpts[0];

• err = tegra_drm_submit(context, args, drm, file);

• if (err)

• goto put_channel;
  
  

  return 0;

+put_channel:

• tegra_drm_context_put_channel(context);

+put_vic:

• pm_runtime_put(vic->dev);
• return err;

}

-static void vic_close_channel(struct tegra_drm_context *context) +static void vic_submit_done(struct tegra_drm_context *context) {

• struct vic *vic = to_vic(context->client);
• host1x_channel_put(context->channel);

• struct host1x_client *client = &context->client->base;

• struct vic *vic = dev_get_drvdata(client->dev);

• tegra_drm_context_put_channel(context); pm_runtime_put(vic->dev);

}

static const struct tegra_drm_client_ops vic_ops = { .open_channel = vic_open_channel, .close_channel = vic_close_channel,

• .submit = tegra_drm_submit,

• .submit = vic_submit,
• .submit_done = vic_submit_done,

};

#define NVIDIA_TEGRA_124_VIC_FIRMWARE "nvidia/tegra124/vic03_ucode.bin" @@ -340,8 +343,6 @@ static int vic_probe(struct platform_device *pdev) vic->client.base.ops = &vic_client_ops; vic->client.base.dev = dev; vic->client.base.class = HOST1X_CLASS_VIC;

• vic->client.base.syncpts = syncpts;
• vic->client.base.num_syncpts = 1; vic->dev = dev; vic->config = vic_config;

On 05.11.2017 19:14, Dmitry Osipenko wrote:

On 05.11.2017 14:01, Mikko Perttunen wrote:

...

Add an option to host1x_channel_request to interruptibly wait for a free channel. This allows IOCTLs that acquire a channel to block the userspace.

Wouldn't it be more optimal to request channel and block after job's pining, when all patching and checks are completed? Note that right now we have locking around submission in DRM, which I suppose should go away by making locking fine grained.

That would be possible, but I don't think it should matter much since contention here should not be the common case.

Or maybe it would be more optimal to just iterate over channels, like I suggested before [0]?

Somehow I hadn't noticed this before, but this would break the invariant of having one client/class per channel.

In general since we haven't seen any issues downstream with the model implemented here, I'd like to try to go with this and if we have problems with channel allocation then we could revisit.

Mikko

[0] https://github.com/cyndis/linux/commit/9e6d87f40afb01fbe13ba65c73cb617bdfcd8...

...

Signed-off-by: Mikko Perttunen mperttunen@nvidia.com

drivers/gpu/drm/tegra/drm.c | 9 +++++---- drivers/gpu/drm/tegra/gr2d.c | 6 +++--- drivers/gpu/drm/tegra/gr3d.c | 6 +++--- drivers/gpu/host1x/channel.c | 40 ++++++++++++++++++++++++++++++---------- drivers/gpu/host1x/channel.h | 1 + include/linux/host1x.h | 2 +- 6 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 658bc8814f38..19f77c1a76c0 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -389,7 +389,8 @@ static int host1x_waitchk_copy_from_user(struct host1x_waitchk *dest,

• Request a free hardware host1x channel for this user context, or if the
• context already has one, bump its refcount.

• • Returns 0 on success, or -EBUSY if there were no free hardware channels.

• • Returns 0 on success, -EINTR if wait for a free channel was interrupted,
• • or other error.
  */

int tegra_drm_context_get_channel(struct tegra_drm_context *context) { @@ -398,10 +399,10 @@ int tegra_drm_context_get_channel(struct tegra_drm_context *context) mutex_lock(&context->lock);

if (context->pending_jobs == 0) {

• context->channel = host1x_channel_request(client->dev);
  

• if (!context->channel) {
  

• context->channel = host1x_channel_request(client->dev, true);
  

• if (IS_ERR(context->channel)) {
  mutex_unlock(&context->lock);
  

• 	return -EBUSY;
  

• 	return PTR_ERR(context->channel);
  

  } }

diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c index 3db3bcac48b9..c1853402f69b 100644 --- a/drivers/gpu/drm/tegra/gr2d.c +++ b/drivers/gpu/drm/tegra/gr2d.c @@ -32,9 +32,9 @@ static int gr2d_init(struct host1x_client *client) unsigned long flags = HOST1X_SYNCPT_HAS_BASE; struct gr2d *gr2d = to_gr2d(drm);

• gr2d->channel = host1x_channel_request(client->dev);
• if (!gr2d->channel)

• return -ENOMEM;
  

• gr2d->channel = host1x_channel_request(client->dev, false);

• if (IS_ERR(gr2d->channel))

• return PTR_ERR(gr2d->channel);
  
  

  client->syncpts[0] = host1x_syncpt_request(client->dev, flags); if (!client->syncpts[0]) {

diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c index 279438342c8c..793a91d577cb 100644 --- a/drivers/gpu/drm/tegra/gr3d.c +++ b/drivers/gpu/drm/tegra/gr3d.c @@ -42,9 +42,9 @@ static int gr3d_init(struct host1x_client *client) unsigned long flags = HOST1X_SYNCPT_HAS_BASE; struct gr3d *gr3d = to_gr3d(drm);

• gr3d->channel = host1x_channel_request(client->dev);
• if (!gr3d->channel)

• return -ENOMEM;
  

• gr3d->channel = host1x_channel_request(client->dev, false);

• if (IS_ERR(gr3d->channel))

• return PTR_ERR(gr3d->channel);
  
  

  client->syncpts[0] = host1x_syncpt_request(client->dev, flags); if (!client->syncpts[0]) {

diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c index 9d8cad12f9d8..eebcd51261df 100644 --- a/drivers/gpu/host1x/channel.c +++ b/drivers/gpu/host1x/channel.c @@ -43,6 +43,7 @@ int host1x_channel_list_init(struct host1x_channel_list *chlist, bitmap_zero(chlist->allocated_channels, num_channels);

mutex_init(&chlist->lock);

• sema_init(&chlist->sema, num_channels);

  return 0;

} @@ -99,6 +100,8 @@ static void release_channel(struct kref *kref) host1x_cdma_deinit(&channel->cdma);

clear_bit(channel->id, chlist->allocated_channels);

• up(&chlist->sema);

}

void host1x_channel_put(struct host1x_channel *channel) @@ -107,19 +110,30 @@ void host1x_channel_put(struct host1x_channel *channel) } EXPORT_SYMBOL(host1x_channel_put);

-static struct host1x_channel *acquire_unused_channel(struct host1x *host) +static struct host1x_channel *acquire_unused_channel(struct host1x *host,

• 				     bool wait)
  

{ struct host1x_channel_list *chlist = &host->channel_list; unsigned int max_channels = host->info->nb_channels; unsigned int index;

• int err;

• if (wait) {

• err = down_interruptible(&chlist->sema);
  

• if (err)
  

• 	return ERR_PTR(err);
  

• } else {

• if (down_trylock(&chlist->sema))
  

• 	return ERR_PTR(-EBUSY);
  

• }

  mutex_lock(&chlist->lock);

  index = find_first_zero_bit(chlist->allocated_channels, max_channels);

• if (index >= max_channels) {

• if (WARN(index >= max_channels, "failed to find free channel")) { mutex_unlock(&chlist->lock); dev_err(host->dev, "failed to find free channel\n");

• return NULL;
  

• return ERR_PTR(-EBUSY);
  

  }

  chlist->channels[index].id = index;

@@ -134,20 +148,26 @@ static struct host1x_channel *acquire_unused_channel(struct host1x *host) /**

• host1x_channel_request() - Allocate a channel
• @device: Host1x unit this channel will be used to send commands to

• • @wait: Whether to wait for a free channels if all are reserved
• • Allocates a new host1x channel for @device. If all channels are in use,
• • and @wait is true, does an interruptible wait until one is available.

• • Allocates a new host1x channel for @device. May return NULL if CDMA
• • initialization fails.

• • If a channel was acquired, returns a pointer to it. Otherwise returns
• • an error pointer with -EINTR if the wait was interrupted, -EBUSY
• • if a channel could not be acquired or another error code if channel
• • initialization failed.
  */

-struct host1x_channel *host1x_channel_request(struct device *dev) +struct host1x_channel *host1x_channel_request(struct device *dev, bool wait) { struct host1x *host = dev_get_drvdata(dev->parent); struct host1x_channel_list *chlist = &host->channel_list; struct host1x_channel *channel; int err;

• channel = acquire_unused_channel(host);
• if (!channel)

• return NULL;
  

• channel = acquire_unused_channel(host, wait);

• if (IS_ERR(channel))

• return channel;
  
  

  kref_init(&channel->refcount); mutex_init(&channel->submitlock);

@@ -168,6 +188,6 @@ struct host1x_channel *host1x_channel_request(struct device *dev)

dev_err(dev, "failed to initialize channel\n");

• return NULL;

• return ERR_PTR(err);

} EXPORT_SYMBOL(host1x_channel_request); diff --git a/drivers/gpu/host1x/channel.h b/drivers/gpu/host1x/channel.h index e68a8ae9a670..1f5cf8029b62 100644 --- a/drivers/gpu/host1x/channel.h +++ b/drivers/gpu/host1x/channel.h @@ -31,6 +31,7 @@ struct host1x_channel_list { struct host1x_channel *channels;

struct mutex lock;

• struct semaphore sema; unsigned long *allocated_channels;

};

diff --git a/include/linux/host1x.h b/include/linux/host1x.h index f931d28a68ff..2a34905d4408 100644 --- a/include/linux/host1x.h +++ b/include/linux/host1x.h @@ -171,7 +171,7 @@ u32 host1x_syncpt_base_id(struct host1x_syncpt_base *base); struct host1x_channel; struct host1x_job;

-struct host1x_channel *host1x_channel_request(struct device *dev); +struct host1x_channel *host1x_channel_request(struct device *dev, bool wait); struct host1x_channel *host1x_channel_get(struct host1x_channel *channel); void host1x_channel_put(struct host1x_channel *channel); int host1x_job_submit(struct host1x_job *job);

-- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html