Re: [PATCH RFC 10/13] drm/powervr_sgx: Add render jobs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/powervr_sgx: unclamped default PB page count in
  pb_create()
- [Medium] drm/powervr_sgx: premature sdev->pb publish and PB
  displacement on error
- [Medium] drm/powervr_sgx: graveyard deferral of unused kobjs exhausts
  krange space
--

commit c2a5d476f9621e03f6e1668986df566b2eded3be
Author: Joseph Telaak <joseph@telaak.dev>

drm/powervr_sgx: Add render jobs

This commit adds render contexts, parameter buffer management, render target
datasets, and render and transfer job submission and command building for
PowerVR SGX GPUs.

> diff --git a/drivers/gpu/drm/powervr_sgx/pvrsgx_render.c b/drivers/gpu/drm/powervr_sgx/pvrsgx_render.c
> new file mode 100644
> index 0000000000000..00b444d05a605
> --- /dev/null
> +++ b/drivers/gpu/drm/powervr_sgx/pvrsgx_render.c
[ ... ]
> +static struct pvrsgx_pb *pb_create(struct pvrsgx_device *sdev, u32 req_pages)
> +{
> +	struct pvrsgx_pb *pb = kzalloc_obj(*pb);
> +	u32 pages = (req_pages ?: PVRSGX_PB_DEFAULT_PAGES) + PB_EXTRA_PAGES;
> +	int err;

[Severity: Medium]
Can pb_create() allocate a parameter buffer larger than the core's
pb_max_pages when req_pages is 0?

PVRSGX_PB_DEFAULT_PAGES is 0x1000, whereas sgx530_heaps sets pb_max_pages to
0x800.  When the first render context on SGX530 passes param_buffer_pages == 0,
sdev->pb is created with 0x1000 + PB_EXTRA_PAGES pages, which both exceeds the
core limit and causes dev_pb_get() to reject any later CREATE_CONTEXT call that
passes a valid non-zero param_buffer_pages (<= 0x800).  Clamping
PVRSGX_PB_DEFAULT_PAGES to pvrsgx_heaps(sdev)->pb_max_pages would avoid this.

> +
> +	if (!pb)
> +		return ERR_PTR(-ENOMEM);
> +	kref_init(&pb->ref);
> +	pb->sdev = sdev;
> +
> +	/* The PB, and what is addressed from its 1 MiB-aligned base, in the VM. */
> +	err = pvrsgx_kobj_new(sdev, &pb->buf, PVRSGX_KRANGE_3DPARAMS, pages << PAGE_SHIFT, 0, NULL);
> +	if (err)
> +		goto fail;
> +	pb->base = kobj_va(pb->buf) & 0xfff00000;
> +	err = pvrsgx_kobj_new(sdev, &pb->freelist, PVRSGX_KRANGE_3DPARAMS, FREELIST_BYTES, pb->base,
> +			      NULL);
> +	err = err ?: pvrsgx_kobj_new(sdev, &pb->desc, PVRSGX_KRANGE_KDATA,
> +				     sizeof(struct pvrsgx_fw_pb_desc), 0, NULL);
> +	if (pb->desc)
> +		pb->desc->until_restart = true;
> +	err = err ?: pvrsgx_kobj_new(sdev, &pb->blocks, PVRSGX_KRANGE_KDATA, 0x10, 0, NULL);
> +	if (err)
> +		goto fail;
> +
> +	pb->pages = pages;
> +	pb_init(pb);
> +	pb->fw_gen = sdev->fw_gen;
> +	return pb;
> +
> +fail:
> +	pvrsgx_pb_put(pb);
> +	return ERR_PTR(err);
> +}
[ ... ]
> +/* The device's PB, created by the first render context; the device keeps one reference. */
> +static struct pvrsgx_pb *dev_pb_get(struct pvrsgx_device *sdev, u32 req_pages)
> +{
> +	struct pvrsgx_pb *pb;
> +
> +	mutex_lock(&sdev->pb_lock);
> +	if (!sdev->pb) {
> +		pb = pb_create(sdev, req_pages);
> +		if (IS_ERR(pb)) {
> +			mutex_unlock(&sdev->pb_lock);
> +			return pb;
> +		}
> +		sdev->pb = pb;
> +		drm_info(&sdev->base, "parameter buffer: %#x pages\n", pb->pages);
> +	} else if (req_pages && req_pages != sdev->pb->pages - PB_EXTRA_PAGES) {
> +		/* A size the live buffer doesn't have is refused, not ignored. */
> +		mutex_unlock(&sdev->pb_lock);
> +		return ERR_PTR(-EINVAL);
> +	}
> +	pb = sdev->pb;
> +	kref_get(&pb->ref);
> +	mutex_unlock(&sdev->pb_lock);
> +	return pb;
> +}

[Severity: Medium]
What happens if pb_create() fails after allocating pb->buf, or if
pvrsgx_render_ctx_init() fails after dev_pb_get() publishes sdev->pb?

If pb->freelist, pb->desc, or pb->blocks fails to allocate, pvrsgx_pb_put()
drops pb->buf into sdev->graveyard while no render context exists to run a
render job and reap it.  A retry of pb_create() then allocates the new pb->buf
after the unreaped one in PVRSGX_KRANGE_3DPARAMS, so pb->base
(kobj_va(pb->buf) & 0xfff00000) no longer matches params_3d.base, which
pb_init() uses to index free-list page numbers.

Similarly, dev_pb_get() assigns sdev->pb with the device's permanent reference
before pvrsgx_pb_map() and the remaining allocations in
pvrsgx_render_ctx_init() succeed, so a failure there leaves sdev->pb
permanently locked to the failed context's requested page count.

[ ... ]
> +static void rtds_release(struct kref *ref)
> +{
> +	struct pvrsgx_rt_dataset *s = container_of(ref, struct pvrsgx_rt_dataset, ref);
> +	struct pvrsgx_device *sdev = s->ctx->sdev;
> +
> +	pvrsgx_kobj_drop(sdev, &s->rtword);
> +	pvrsgx_kobj_drop(sdev, &s->rtdata);
> +	pvrsgx_kobj_drop(sdev, &s->done);
> +	pvrsgx_kobj_drop(sdev, &s->tailptr);
> +	pvrsgx_kobj_drop(sdev, &s->ta40);
> +	pvrsgx_kobj_drop(sdev, &s->ta50);
> +	for (int i = 0; i < 2; i++) {
> +		pvrsgx_kobj_drop(sdev, &s->bgblock[i]);
> +		pvrsgx_kobj_drop(sdev, &s->rd[i]);
> +		pvrsgx_kobj_drop(sdev, &s->rd_w8[i]);
> +		pvrsgx_kobj_drop(sdev, &s->rd_w4[i]);
> +		pvrsgx_kobj_drop(sdev, &s->hwsync[i]);
> +		pvrsgx_kobj_drop(sdev, &s->rd_sync[i]);
> +		pvrsgx_kobj_drop(sdev, &s->hwsync_access[i]);
> +		pvrsgx_kobj_drop(sdev, &s->entword[i]);
> +		pvrsgx_kobj_drop(sdev, &s->rgn[i]);
> +		pvrsgx_kobj_drop(sdev, &s->ta88[i]);
> +	}
> +	pvrsgx_context_put(s->ctx);
> +	kfree(s);
> +}
[ ... ]
> +int pvrsgx_ioctl_destroy_rt_dataset(struct drm_device *drm, void *data, struct drm_file *file)
> +{
> +	struct drm_powervr_sgx_ioctl_destroy_rt_dataset_args *args = data;
> +	struct pvrsgx_rt_dataset *s;
> +
> +	if (args->_padding_4)
> +		return -EINVAL;
> +	s = xa_erase(&to_pvrsgx_file(file)->rtds, args->handle);
> +	if (!s)
> +		return -ENOENT;
> +	pvrsgx_rtds_put(s);
> +	return 0;
> +}

[Severity: Medium]
Can a client exhaust the device-wide krange allocators by repeatedly creating
and destroying RT datasets or contexts without submitting a render job?

pvrsgx_ioctl_destroy_rt_dataset() erases the handle from sfile->rtds
immediately, allowing the client to allocate new datasets past
PVRSGX_MAX_FILE_RTDS, while rtds_release() (and similarly
pvrsgx_render_ctx_fini() and pvrsgx_transfer_ctx_fini()) places all kernel
objects onto sdev->graveyard via pvrsgx_kobj_drop() even when no render job has
ever used them.  Because pvrsgx_kobj_reap() only frees graveyard objects once
sdev->renders_done advances past o->grave_seq or the GPU resets, creating and
destroying unused datasets or contexts in a loop fills PVRSGX_KRANGE_TADATA and
PVRSGX_KRANGE_KDATA with unreaped objects and causes -ENOSPC for all clients.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-powervr-sgx-v1-0-c69b84bac011@telaak.dev?part=10