Re: [Intel-gfx] [PATCH 01/43] drm/i915: Reorder the actual workload submission so that args checking is done earlier
On Fri, Jul 25, 2014 at 10:30:09AM +0200, Daniel Vetter wrote: > On Thu, Jul 24, 2014 at 05:04:09PM +0100, Thomas Daniel wrote: > > From: Oscar Mateo > > > > In this patch: > > > > commit 78382593e921c88371abd019aca8978db3248a8f > > Author: Oscar Mateo > > Date: Thu Jul 3 16:28:05 2014 +0100 > > > > drm/i915: Extract the actual workload submission mechanism from > > execbuffer > > > > So that we isolate the legacy ringbuffer submission mechanism, which > > becomes > > a good candidate to be abstracted away. This is prep-work for Execlists > > (which > > will its own workload submission mechanism). > > > > No functional changes. > > > > I changed the order in which the args checking is done. I don't know why I > > did (brain > > fade?) but itś not right. I haven't seen any ill effect from this, but the > > Execlists > > version of this function will have problems if the order is not correct. > > > > Signed-off-by: Oscar Mateo > > I don't think this matters - the point of no return for legacy execbuf is > the call to ring->dispatch. After that nothing may fail any more. But as > long as we track state correctly (e.g. if we've switched the context > already) we'll be fine. Right. Except that I think our tracking is buggy - or at least insufficient to address the needs of future dispatch mechanisms. I think that we confuse some bookkeeping that should be at the request level and place it on the ring. At the moment, we have one request per-ring and so it doesn't matter, but transitioning to one request per-logical-ring we start to have issues as that state is being tracked on the wrong struct. Anyway, that's part of the motivation to fixing up requests and making them central to accessing the rings/dispatch (whereas at the moment they are behind the scenes). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/43] drm/i915: Reorder the actual workload submission so that args checking is done earlier
On Thu, Jul 24, 2014 at 05:04:09PM +0100, Thomas Daniel wrote:
> From: Oscar Mateo
>
> In this patch:
>
> commit 78382593e921c88371abd019aca8978db3248a8f
> Author: Oscar Mateo
> Date: Thu Jul 3 16:28:05 2014 +0100
>
> drm/i915: Extract the actual workload submission mechanism from execbuffer
>
> So that we isolate the legacy ringbuffer submission mechanism, which
> becomes
> a good candidate to be abstracted away. This is prep-work for Execlists
> (which
> will its own workload submission mechanism).
>
> No functional changes.
>
> I changed the order in which the args checking is done. I don't know why I
> did (brain
> fade?) but itś not right. I haven't seen any ill effect from this, but the
> Execlists
> version of this function will have problems if the order is not correct.
>
> Signed-off-by: Oscar Mateo
I don't think this matters - the point of no return for legacy execbuf is
the call to ring->dispatch. After that nothing may fail any more. But as
long as we track state correctly (e.g. if we've switched the context
already) we'll be fine.
So presuming I'm not blind I dont' think this is needed. But maybe Chris
spots something.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 86
> ++--
> 1 file changed, 43 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 60998fc..c5115957 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1042,6 +1042,43 @@ legacy_ringbuffer_submission(struct drm_device *dev,
> struct drm_file *file,
> u32 instp_mask;
> int i, ret = 0;
>
> + instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> + instp_mask = I915_EXEC_CONSTANTS_MASK;
> + switch (instp_mode) {
> + case I915_EXEC_CONSTANTS_REL_GENERAL:
> + case I915_EXEC_CONSTANTS_ABSOLUTE:
> + case I915_EXEC_CONSTANTS_REL_SURFACE:
> + if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
> + DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + if (instp_mode != dev_priv->relative_constants_mode) {
> + if (INTEL_INFO(dev)->gen < 4) {
> + DRM_DEBUG("no rel constants on pre-gen4\n");
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + if (INTEL_INFO(dev)->gen > 5 &&
> + instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
> + DRM_DEBUG("rel surface constants mode invalid
> on gen5+\n");
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + /* The HW changed the meaning on this bit on gen6 */
> + if (INTEL_INFO(dev)->gen >= 6)
> + instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
> + }
> + break;
> + default:
> + DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
> + ret = -EINVAL;
> + goto error;
> + }
> +
> if (args->num_cliprects != 0) {
> if (ring != &dev_priv->ring[RCS]) {
> DRM_DEBUG("clip rectangles are only valid with the
> render ring\n");
> @@ -1085,6 +1122,12 @@ legacy_ringbuffer_submission(struct drm_device *dev,
> struct drm_file *file,
> }
> }
>
> + if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
> + ret = i915_reset_gen7_sol_offsets(dev, ring);
> + if (ret)
> + goto error;
> + }
> +
> ret = i915_gem_execbuffer_move_to_gpu(ring, vmas);
> if (ret)
> goto error;
> @@ -1093,43 +1136,6 @@ legacy_ringbuffer_submission(struct drm_device *dev,
> struct drm_file *file,
> if (ret)
> goto error;
>
> - instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> - instp_mask = I915_EXEC_CONSTANTS_MASK;
> - switch (instp_mode) {
> - case I915_EXEC_CONSTANTS_REL_GENERAL:
> - case I915_EXEC_CONSTANTS_ABSOLUTE:
> - case I915_EXEC_CONSTANTS_REL_SURFACE:
> - if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
> - DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
> - ret = -EINVAL;
> - goto error;
> - }
> -
> - if (instp_mode != dev_priv->relative_constants_mode) {
> - if (INTEL_INFO(dev)->gen < 4) {
> - DRM_DEBUG("no rel constants on pre-gen4\n");
> - ret = -EINVAL;
> - goto error;
> - }
> -
> - if (INTEL
[Intel-gfx] [PATCH 01/43] drm/i915: Reorder the actual workload submission so that args checking is done earlier
From: Oscar Mateo
In this patch:
commit 78382593e921c88371abd019aca8978db3248a8f
Author: Oscar Mateo
Date: Thu Jul 3 16:28:05 2014 +0100
drm/i915: Extract the actual workload submission mechanism from execbuffer
So that we isolate the legacy ringbuffer submission mechanism, which becomes
a good candidate to be abstracted away. This is prep-work for Execlists
(which
will its own workload submission mechanism).
No functional changes.
I changed the order in which the args checking is done. I don't know why I did
(brain
fade?) but itś not right. I haven't seen any ill effect from this, but the
Execlists
version of this function will have problems if the order is not correct.
Signed-off-by: Oscar Mateo
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 86 ++--
1 file changed, 43 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 60998fc..c5115957 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1042,6 +1042,43 @@ legacy_ringbuffer_submission(struct drm_device *dev,
struct drm_file *file,
u32 instp_mask;
int i, ret = 0;
+ instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
+ instp_mask = I915_EXEC_CONSTANTS_MASK;
+ switch (instp_mode) {
+ case I915_EXEC_CONSTANTS_REL_GENERAL:
+ case I915_EXEC_CONSTANTS_ABSOLUTE:
+ case I915_EXEC_CONSTANTS_REL_SURFACE:
+ if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
+ DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
+ ret = -EINVAL;
+ goto error;
+ }
+
+ if (instp_mode != dev_priv->relative_constants_mode) {
+ if (INTEL_INFO(dev)->gen < 4) {
+ DRM_DEBUG("no rel constants on pre-gen4\n");
+ ret = -EINVAL;
+ goto error;
+ }
+
+ if (INTEL_INFO(dev)->gen > 5 &&
+ instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
+ DRM_DEBUG("rel surface constants mode invalid
on gen5+\n");
+ ret = -EINVAL;
+ goto error;
+ }
+
+ /* The HW changed the meaning on this bit on gen6 */
+ if (INTEL_INFO(dev)->gen >= 6)
+ instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+ }
+ break;
+ default:
+ DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
+ ret = -EINVAL;
+ goto error;
+ }
+
if (args->num_cliprects != 0) {
if (ring != &dev_priv->ring[RCS]) {
DRM_DEBUG("clip rectangles are only valid with the
render ring\n");
@@ -1085,6 +1122,12 @@ legacy_ringbuffer_submission(struct drm_device *dev,
struct drm_file *file,
}
}
+ if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
+ ret = i915_reset_gen7_sol_offsets(dev, ring);
+ if (ret)
+ goto error;
+ }
+
ret = i915_gem_execbuffer_move_to_gpu(ring, vmas);
if (ret)
goto error;
@@ -1093,43 +1136,6 @@ legacy_ringbuffer_submission(struct drm_device *dev,
struct drm_file *file,
if (ret)
goto error;
- instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
- instp_mask = I915_EXEC_CONSTANTS_MASK;
- switch (instp_mode) {
- case I915_EXEC_CONSTANTS_REL_GENERAL:
- case I915_EXEC_CONSTANTS_ABSOLUTE:
- case I915_EXEC_CONSTANTS_REL_SURFACE:
- if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
- DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
- ret = -EINVAL;
- goto error;
- }
-
- if (instp_mode != dev_priv->relative_constants_mode) {
- if (INTEL_INFO(dev)->gen < 4) {
- DRM_DEBUG("no rel constants on pre-gen4\n");
- ret = -EINVAL;
- goto error;
- }
-
- if (INTEL_INFO(dev)->gen > 5 &&
- instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
- DRM_DEBUG("rel surface constants mode invalid
on gen5+\n");
- ret = -EINVAL;
- goto error;
- }
-
- /* The HW changed the meaning on this bit on gen6 */
- if (INTEL_INFO(dev)->gen >= 6)
- instp_mask &= ~I915_EXEC_CONSTANTS_
