On 11.08.2017 21:16, Dmitry Osipenko wrote:
> On 11.08.2017 20:59, Thierry Reding wrote:
>> From: Dmitry Osipenko <digetx@xxxxxxxxx>
>>
>> Since DRM IOCTL's are lockless, there is a chance that BOs could be
>> released while a job submission is in progress. To avoid that, keep the
>> GEM reference until the job has been pinned, part of which will be to
>> take another reference.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
>> ---
>> drivers/gpu/drm/tegra/drm.c | 42 +++++++++++++++++++++++++++++++++---------
>> 1 file changed, 33 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>> index f01db33fa20f..e5d19e1c9bc8 100644
>> --- a/drivers/gpu/drm/tegra/drm.c
>> +++ b/drivers/gpu/drm/tegra/drm.c
>> @@ -320,8 +320,6 @@ host1x_bo_lookup(struct drm_file *file, u32 handle)
>> if (!gem)
>> return NULL;
>>
>> - drm_gem_object_unreference_unlocked(gem);
>> -
>> bo = to_tegra_bo(gem);
>> return &bo->base;
>> }
>> @@ -410,8 +408,10 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>> (void __user *)(uintptr_t)args->waitchks;
>> 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_job *job;
>> + unsigned int num_refs;
>> int err;
>>
>> /* We don't yet support other than one syncpt_incr struct per submit */
>> @@ -433,6 +433,26 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>> job->class = context->client->base.class;
>> job->serialize = true;
>>
>> + /*
>> + * Track referenced BOs so that they can be unreferenced after the
>> + * submission is complete.
>> + */
>> + num_refs = num_cmdbufs + num_relocs * 2 + num_waitchks;
>> +
>> + if (sizeof(*refs) * num_refs > ULONG_MAX) {
>
> Thierry, since you've omitted (u64) here (comparing to the original patch), I
> think it should be better to write as:
>
> if (num_refs > ULONG_MAX / sizeof(*refs)) {
>
> To avoid integer overflow in the check.
>
Moreover, this check isn't needed at all because it duplicates same checking
done by kmalloc_array().
>> + err = -EINVAL;
>> + goto fail;
>> + }
>> +
>> + refs = kmalloc_array(num_refs, sizeof(*refs), GFP_KERNEL);
>> + if (!refs) {
>> + err = -ENOMEM;
>> + goto fail;
>> + }
>> +
>> + /* reuse as an iterator later */
>> + num_refs = 0;
>> +
>> while (num_cmdbufs) {
>> struct drm_tegra_cmdbuf cmdbuf;
>> struct host1x_bo *bo;
>> @@ -461,6 +481,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>
>> offset = (u64)cmdbuf.offset + (u64)cmdbuf.words * sizeof(u32);
>> obj = host1x_to_tegra_bo(bo);
>> + refs[num_refs++] = &obj->gem;
>>
>> /*
>> * Gather buffer base address must be 4-bytes aligned,
>> @@ -490,6 +511,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>
>> reloc = &job->relocarray[num_relocs];
>> obj = host1x_to_tegra_bo(reloc->cmdbuf.bo);
>> + refs[num_refs++] = &obj->gem;
>>
>> /*
>> * The unaligned cmdbuf offset will cause an unaligned write
>> @@ -503,6 +525,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>> }
>>
>> obj = host1x_to_tegra_bo(reloc->target.bo);
>> + refs[num_refs++] = &obj->gem;
>>
>> if (reloc->target.offset >= obj->gem.size) {
>> err = -EINVAL;
>> @@ -522,6 +545,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>> goto fail;
>>
>> obj = host1x_to_tegra_bo(wait->bo);
>> + refs[num_refs++] = &obj->gem;
>>
>> /*
>> * The unaligned offset will cause an unaligned write during
>> @@ -561,17 +585,17 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>> goto fail;
>>
>> err = host1x_job_submit(job);
>> - if (err)
>> - goto fail_submit;
>> + if (err) {
>> + host1x_job_unpin(job);
>> + goto fail;
>> + }
>>
>> args->fence = job->syncpt_end;
>>
>> - host1x_job_put(job);
>> - return 0;
>> -
>> -fail_submit:
>> - host1x_job_unpin(job);
>> fail:
>> + while (num_refs--)
>> + drm_gem_object_put_unlocked(refs[num_refs]);
>> +
>> host1x_job_put(job);
>> return err;
>> }
>>
>
>
--
Dmitry
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
