All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alyssa Rosenzweig <alyssa@collabora.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Jason Ekstrand <jason@jlekstrand.net>,
	Tomeu Vizoso <tomeu.vizoso@collabora.com>,
	dri-devel@lists.freedesktop.org,
	Steven Price <steven.price@arm.com>,
	Rob Herring <robh+dt@kernel.org>,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH v3 5/7] drm/panfrost: Add a new ioctl to submit batches
Date: Fri, 2 Jul 2021 11:13:16 -0400	[thread overview]
Message-ID: <YN8tDD6tRF85cR4z@maud> (raw)
In-Reply-To: <20210702143225.3347980-6-boris.brezillon@collabora.com>

```
> +/* Syncobj reference passed at job submission time to encode explicit
> + * input/output fences.
> + */
> +struct drm_panfrost_syncobj_ref {
> +	__u32 handle;
> +	__u32 pad;
> +	__u64 point;
> +};
```

What is handle? What is point? Why is there padding instead of putting
point first?

```
>  #define PANFROST_BO_REF_EXCLUSIVE	0x1
> +#define PANFROST_BO_REF_NO_IMPLICIT_DEP	0x2
```

This seems logically backwards. NO_IMPLICIT_DEP makes sense if we're
trying to keep backwards compatibility, but here you're crafting a new
interface totally from scratch. If anything, isn't BO_REF_IMPLICIT_DEP
the flag you'd want?

```
> +	/**
> +	 * Stride of the jobs array (needed to ease extension of the
> +	 * BATCH_SUBMIT ioctl). Should be set to
> +	 * sizeof(struct drm_panfrost_job).
> +	 */
> +	__u32 job_stride;
...
> +	/**
> +	 * Stride of the BO and syncobj reference arrays (needed to ease
> +	 * extension of the BATCH_SUBMIT ioctl). Should be set to
> +	 * sizeof(struct drm_panfrost_bo_ref).
> +	 */
> +	__u32 bo_ref_stride;
> +	__u32 syncobj_ref_stride;
```

Hmm. I'm not /opposed/ and I know kbase uses strides but it seems like
somewhat unwarranted complexity, and there is a combinatoric explosion
here (if jobs, bo refs, and syncobj refs use 3 different versions, as
this encoding permits... as opposed to just specifying a UABI version or
something like that)

```
> +	/**
> +	 * If the submission fails, this encodes the index of the job
> +	 * failed.
> +	 */
> +	__u32 fail_idx;
```

What if multiple jobs fail?

```
> +	/**
> +	 * ID of the queue to submit those jobs to. 0 is the default
> +	 * submit queue and should always exists. If you need a dedicated
> +	 * queue, create it with DRM_IOCTL_PANFROST_CREATE_SUBMITQUEUE.
> +	 */
> +	__u32 queue;
```

s/exists/exist/

  reply	other threads:[~2021-07-02 15:13 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-02 14:32 [PATCH v3 0/7] drm/panfrost: drm/panfrost: Add a new submit ioctl Boris Brezillon
2021-07-02 14:32 ` [PATCH v3 1/7] drm/panfrost: Pass a job to panfrost_{acquire, attach}_object_fences() Boris Brezillon
2021-07-02 14:32 ` [PATCH v3 2/7] drm/panfrost: Move the mappings collection out of panfrost_lookup_bos() Boris Brezillon
2021-07-02 14:32 ` [PATCH v3 3/7] drm/panfrost: Add BO access flags to relax dependencies between jobs Boris Brezillon
2021-07-02 14:32 ` [PATCH v3 4/7] drm/panfrost: Add the ability to create submit queues Boris Brezillon
2021-07-02 15:05   ` Steven Price
2021-07-02 15:49     ` Boris Brezillon
2021-07-02 16:01       ` Boris Brezillon
2021-07-02 14:32 ` [PATCH v3 5/7] drm/panfrost: Add a new ioctl to submit batches Boris Brezillon
2021-07-02 15:13   ` Alyssa Rosenzweig [this message]
2021-07-02 15:38     ` Boris Brezillon
2021-07-02 16:49       ` Alyssa Rosenzweig
2021-07-02 17:54         ` Boris Brezillon
2021-07-02 18:11         ` Boris Brezillon
2021-07-05  8:22           ` Steven Price
2021-07-05  8:43             ` Boris Brezillon
2021-07-05  8:51               ` Steven Price
2021-07-06 12:48                 ` Alyssa Rosenzweig
2021-07-07  9:09                   ` Steven Price
2021-07-02 15:31   ` Steven Price
2021-07-02 15:34     ` Alyssa Rosenzweig
2021-07-02 14:32 ` [PATCH v3 6/7] drm/panfrost: Advertise the SYNCOBJ_TIMELINE feature Boris Brezillon
2021-07-02 14:32 ` [PATCH v3 7/7] drm/panfrost: Bump minor version to reflect the feature additions Boris Brezillon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YN8tDD6tRF85cR4z@maud \
    --to=alyssa@collabora.com \
    --cc=alyssa.rosenzweig@collabora.com \
    --cc=boris.brezillon@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jason@jlekstrand.net \
    --cc=robh+dt@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=steven.price@arm.com \
    --cc=tomeu.vizoso@collabora.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.