Re: [PATCH v7 13/15] drm/tegra: Implement job submission part of new UAPI

[Mikko Perttunen <cyndis@kapsi.fi>]

On 6/15/21 10:00 PM, Jon Hunter wrote:
> 
> On 10/06/2021 12:04, Mikko Perttunen wrote:
>> Implement the job submission IOCTL with a minimum feature set.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>> v7:
>> * Allocate gather BO with DMA API to get page-aligned
>>    memory
>> * Add error prints to a few places where they were missing
>> v6:
>> * Remove sgt bypass path in gather_bo - this would cause
>>    cache maintenance to be skipped and is unnecessary in
>>    general.
>> * Changes related to moving to using syncpoint IDs
>> * Add syncobj related code
>> * Print warning on submit failure describing the issue
>> * Use ARCH_DMA_ADDR_T_64BIT to check if that is indeed
>>    the case
>> * Add support for relative syncpoint wait
>> * Use pm_runtime_resume_and_get
>> * Only try to resume engines that support runtime PM
>> * Removed uapi subdirectory
>> * Don't use "copy_err" variables for copy_from_user
>>    return value
>> * Fix setting of blocklinear flag
>> v5:
>> * Add 16K size limit to copies from userspace.
>> * Guard RELOC_BLOCKLINEAR flag handling to only exist in ARM64
>>    to prevent oversized shift on 32-bit platforms.
>> v4:
>> * Remove all features that are not strictly necessary.
>> * Split into two patches.
>> v3:
>> * Remove WRITE_RELOC. Relocations are now patched implicitly
>>    when patching is needed.
>> * Directly call PM runtime APIs on devices instead of using
>>    power_on/power_off callbacks.
>> * Remove incorrect mutex unlock in tegra_drm_ioctl_channel_open
>> * Use XA_FLAGS_ALLOC1 instead of XA_FLAGS_ALLOC
>> * Accommodate for removal of timeout field and inlining of
>>    syncpt_incrs array.
>> * Copy entire user arrays at a time instead of going through
>>    elements one-by-one.
>> * Implement waiting of DMA reservations.
>> * Split out gather_bo implementation into a separate file.
>> * Fix length parameter passed to sg_init_one in gather_bo
>> * Cosmetic cleanup.
>> ---
>>   drivers/gpu/drm/tegra/Makefile    |   2 +
>>   drivers/gpu/drm/tegra/drm.c       |   4 +-
>>   drivers/gpu/drm/tegra/gather_bo.c |  82 +++++
>>   drivers/gpu/drm/tegra/gather_bo.h |  24 ++
>>   drivers/gpu/drm/tegra/submit.c    | 549 ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tegra/submit.h    |  17 +
>>   6 files changed, 677 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/tegra/gather_bo.c
>>   create mode 100644 drivers/gpu/drm/tegra/gather_bo.h
>>   create mode 100644 drivers/gpu/drm/tegra/submit.c
>>   create mode 100644 drivers/gpu/drm/tegra/submit.h
> 
> ...
> 
>> +int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
>> +				   struct drm_file *file)
>> +{
>> +	struct tegra_drm_file *fpriv = file->driver_priv;
>> +	struct drm_tegra_channel_submit *args = data;
>> +	struct tegra_drm_submit_data *job_data;
>> +	struct drm_syncobj *syncobj = NULL;
>> +	struct tegra_drm_context *ctx;
>> +	struct host1x_job *job;
>> +	struct gather_bo *bo;
>> +	u32 i;
>> +	int err;
>> +
>> +	mutex_lock(&fpriv->lock);
>> +	ctx = xa_load(&fpriv->contexts, args->channel_ctx);
>> +	if (!ctx) {
>> +		mutex_unlock(&fpriv->lock);
>> +		pr_err_ratelimited("%s: %s: invalid channel_ctx '%d'", __func__,
>> +			current->comm, args->channel_ctx);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (args->syncobj_in) {
>> +		struct dma_fence *fence;
>> +
>> +		err = drm_syncobj_find_fence(file, args->syncobj_in, 0, 0, &fence);
>> +		if (err) {
>> +			SUBMIT_ERR(ctx, "invalid syncobj_in '%d'", args->syncobj_in);
>> +			goto unlock;
>> +		}
>> +
>> +		err = dma_fence_wait_timeout(fence, true, msecs_to_jiffies(10000));
>> +		dma_fence_put(fence);
>> +		if (err) {
>> +			SUBMIT_ERR(ctx, "wait for syncobj_in timed out");
>> +			goto unlock;
>> +		}
>> +	}
>> +
>> +	if (args->syncobj_out) {
>> +		syncobj = drm_syncobj_find(file, args->syncobj_out);
>> +		if (!syncobj) {
>> +			SUBMIT_ERR(ctx, "invalid syncobj_out '%d'", args->syncobj_out);
>> +			err = -ENOENT;
>> +			goto unlock;
>> +		}
>> +	}
>> +
>> +	/* Allocate gather BO and copy gather words in. */
>> +	err = submit_copy_gather_data(&bo, drm->dev, ctx, args);
>> +	if (err)
>> +		goto unlock;
>> +
>> +	job_data = kzalloc(sizeof(*job_data), GFP_KERNEL);
>> +	if (!job_data) {
>> +		SUBMIT_ERR(ctx, "failed to allocate memory for job data");
>> +		err = -ENOMEM;
>> +		goto put_bo;
>> +	}
>> +
>> +	/* Get data buffer mappings and do relocation patching. */
>> +	err = submit_process_bufs(ctx, bo, args, job_data);
>> +	if (err)
>> +		goto free_job_data;
>> +
>> +	/* Allocate host1x_job and add gathers and waits to it. */
>> +	err = submit_create_job(&job, ctx, bo, args, job_data,
>> +				&fpriv->syncpoints);
>> +	if (err)
>> +		goto free_job_data;
>> +
>> +	/* Map gather data for Host1x. */
>> +	err = host1x_job_pin(job, ctx->client->base.dev);
>> +	if (err) {
>> +		SUBMIT_ERR(ctx, "failed to pin job: %d", err);
>> +		goto put_job;
>> +	}
>> +
>> +	/* Boot engine. */
>> +	if (pm_runtime_enabled(ctx->client->base.dev)) {
>> +		err = pm_runtime_resume_and_get(ctx->client->base.dev);
>> +		if (err < 0) {
>> +			SUBMIT_ERR(ctx, "could not power up engine: %d", err);
>> +			goto unpin_job;
>> +		}
>> +	}
>> +
>> +	job->user_data = job_data;
>> +	job->release = release_job;
>> +	job->timeout = 10000;
>> +
>> +	/*
>> +	 * job_data is now part of job reference counting, so don't release
>> +	 * it from here.
>> +	 */
>> +	job_data = NULL;
>> +
>> +	/* Submit job to hardware. */
>> +	err = host1x_job_submit(job);
>> +	if (err) {
>> +		SUBMIT_ERR(ctx, "host1x job submission failed: %d", err);
>> +		goto unpin_job;
>> +	}
> 
> 
> If we fail here, it appears that we may leave rpm enabled. Should we be
> calling pm_runtime_put() for any failures from here on?

host1x_job_put will call the release callback, which will do the PM 
runtime put. So this should be taken care of.

thanks,
Mikko

> 
> Cheers
> Jon
>