[09/53] drm/i915/bdw: Initialization for Logical Ring Contexts

Commit Message

[e-mail] June 13, 2014, 3:37 p.m. UTC

From: Oscar Mateo <[e-mail]>

Early in the series we had our own gen8_gem_context_init/fini
functions, but the truth is they now look almost the same as the
legacy hw context init/fini functions. We can always split them
later if this ceases to be the case.

Also, we do not fall back to legacy ringbuffers when logical ring
context initialization fails (not very likely to happen and, even
if it does, hw contexts would probably fail as well).

Signed-off-by: Oscar Mateo <[e-mail]>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Comments

Daniel Vetter June 18, 2014, 8:24 p.m. UTC | #1

On Fri, Jun 13, 2014 at 04:37:27PM +0100, [e-mail] wrote:
> From: Oscar Mateo <[e-mail]>
> 
> Early in the series we had our own gen8_gem_context_init/fini
> functions, but the truth is they now look almost the same as the
> legacy hw context init/fini functions. We can always split them
> later if this ceases to be the case.
> 
> Also, we do not fall back to legacy ringbuffers when logical ring
> context initialization fails (not very likely to happen and, even
> if it does, hw contexts would probably fail as well).
> 
> Signed-off-by: Oscar Mateo <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 801b891..3f3fb36 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -416,7 +416,13 @@ int i915_gem_context_init(struct drm_device *dev)
>  	if (WARN_ON(dev_priv->ring[RCS].default_context))
>  		return 0;
>  
> -	if (HAS_HW_CONTEXTS(dev)) {
> +	dev_priv->lrc_enabled = intel_enable_execlists(dev);
> +
> +	if (dev_priv->lrc_enabled) {
> +		/* NB: intentionally left blank. We will allocate our own
> +		 * backing objects as we need them, thank you very much */
> +		dev_priv->hw_context_size = 0;
> +	} else if (HAS_HW_CONTEXTS(dev)) {
>  		dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
>  		if (dev_priv->hw_context_size > (1<<20)) {
>  			DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
> @@ -436,7 +442,9 @@ int i915_gem_context_init(struct drm_device *dev)
>  	for (i = 0; i < I915_NUM_RINGS; i++)
>  		dev_priv->ring[i].default_context = ctx;
>  
> -	DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
> +	DRM_DEBUG_DRIVER("%s context support initialized\n",
> +			dev_priv->lrc_enabled ? "LR" :
> +			dev_priv->hw_context_size ? "HW" : "fake");
>  	return 0;
>  }
>  
> @@ -765,9 +773,12 @@ int i915_switch_context(struct intel_engine_cs *ring,
>  	return do_switch(ring, to);
>  }
>  
> -static bool hw_context_enabled(struct drm_device *dev)
> +static bool contexts_enabled(struct drm_device *dev)
>  {
> -	return to_i915(dev)->hw_context_size;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	/* FIXME: this would be cleaner with a "context type" enum */
> +	return dev_priv->lrc_enabled || dev_priv->hw_context_size;

Since you have a bunch of if ladders the usual approach isn't an enum but
a vfunc table to abstract behaviour. Think object types instead of switch
statements. Style bikeshed though (presume code later on doesn't have
excesses here).
-Daniel

>  }
>  
>  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> @@ -778,7 +789,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  	struct intel_context *ctx;
>  	int ret;
>  
> -	if (!hw_context_enabled(dev))
> +	if (!contexts_enabled(dev))
>  		return -ENODEV;
>  
>  	ret = i915_mutex_lock_interruptible(dev);
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[e-mail] June 19, 2014, 9:23 a.m. UTC | #2

> -----Original Message-----
> From: Daniel Vetter [mailto:[e-mail]] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, June 18, 2014 9:25 PM
> To: Mateo Lozano, Oscar
> Cc: [e-mail]
> Subject: Re: [Intel-gfx] [PATCH 09/53] drm/i915/bdw: Initialization for
> Logical Ring Contexts
> 
> On Fri, Jun 13, 2014 at 04:37:27PM +0100, [e-mail] wrote:
> > From: Oscar Mateo <[e-mail]>
> >
> > Early in the series we had our own gen8_gem_context_init/fini
> > functions, but the truth is they now look almost the same as the
> > legacy hw context init/fini functions. We can always split them later
> > if this ceases to be the case.
> >
> > Also, we do not fall back to legacy ringbuffers when logical ring
> > context initialization fails (not very likely to happen and, even if
> > it does, hw contexts would probably fail as well).
> >
> > Signed-off-by: Oscar Mateo <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 801b891..3f3fb36 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -416,7 +416,13 @@ int i915_gem_context_init(struct drm_device
> *dev)
> >  	if (WARN_ON(dev_priv->ring[RCS].default_context))
> >  		return 0;
> >
> > -	if (HAS_HW_CONTEXTS(dev)) {
> > +	dev_priv->lrc_enabled = intel_enable_execlists(dev);
> > +
> > +	if (dev_priv->lrc_enabled) {
> > +		/* NB: intentionally left blank. We will allocate our own
> > +		 * backing objects as we need them, thank you very much */
> > +		dev_priv->hw_context_size = 0;
> > +	} else if (HAS_HW_CONTEXTS(dev)) {
> >  		dev_priv->hw_context_size =
> round_up(get_context_size(dev), 4096);
> >  		if (dev_priv->hw_context_size > (1<<20)) {
> >  			DRM_DEBUG_DRIVER("Disabling HW Contexts;
> invalid size %d\n", @@
> > -436,7 +442,9 @@ int i915_gem_context_init(struct drm_device *dev)
> >  	for (i = 0; i < I915_NUM_RINGS; i++)
> >  		dev_priv->ring[i].default_context = ctx;
> >
> > -	DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv-
> >hw_context_size ? "HW" : "fake");
> > +	DRM_DEBUG_DRIVER("%s context support initialized\n",
> > +			dev_priv->lrc_enabled ? "LR" :
> > +			dev_priv->hw_context_size ? "HW" : "fake");
> >  	return 0;
> >  }
> >
> > @@ -765,9 +773,12 @@ int i915_switch_context(struct intel_engine_cs
> *ring,
> >  	return do_switch(ring, to);
> >  }
> >
> > -static bool hw_context_enabled(struct drm_device *dev)
> > +static bool contexts_enabled(struct drm_device *dev)
> >  {
> > -	return to_i915(dev)->hw_context_size;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > +	/* FIXME: this would be cleaner with a "context type" enum */
> > +	return dev_priv->lrc_enabled || dev_priv->hw_context_size;
> 
> Since you have a bunch of if ladders the usual approach isn't an enum but a
> vfunc table to abstract behaviour. Think object types instead of switch
> statements. Style bikeshed though (presume code later on doesn't have
> excesses here).
> -Daniel

Hmmmm... I offered to do this with vfuncs early on, but you mentioned special-casing should be enough. And I agreed: at the end, the LR contexts are not that different from traditional HW contexts. This is what I had in mind:

CTX_TYPE_FAKE: no backing objects.
CTX_TYPE_HW: one render backing object at creation time.
CTX_TYPE_LR: n backing objects, with deferred creation. A few functions are moot (e.g. switch, reset).

The current system (looking at dev_priv->hw_context_size to distinguish fake from hw contexts) is imo a bit obfuscated.
And we can always abstract this away with vfuncs if it becomes too complex in the future...

What do you think? can special casing made do for the time being?

Daniel Vetter June 19, 2014, 10:08 a.m. UTC | #3

On Thu, Jun 19, 2014 at 11:23 AM, Mateo Lozano, Oscar
<[e-mail]> wrote:
>> > -static bool hw_context_enabled(struct drm_device *dev)
>> > +static bool contexts_enabled(struct drm_device *dev)
>> >  {
>> > -   return to_i915(dev)->hw_context_size;
>> > +   struct drm_i915_private *dev_priv = to_i915(dev);
>> > +
>> > +   /* FIXME: this would be cleaner with a "context type" enum */
>> > +   return dev_priv->lrc_enabled || dev_priv->hw_context_size;
>>
>> Since you have a bunch of if ladders the usual approach isn't an enum but a
>> vfunc table to abstract behaviour. Think object types instead of switch
>> statements. Style bikeshed though (presume code later on doesn't have
>> excesses here).
>> -Daniel
>
> Hmmmm... I offered to do this with vfuncs early on, but you mentioned special-casing should be enough. And I agreed: at the end, the LR contexts are not that different from traditional HW contexts. This is what I had in mind:
>
> CTX_TYPE_FAKE: no backing objects.
> CTX_TYPE_HW: one render backing object at creation time.
> CTX_TYPE_LR: n backing objects, with deferred creation. A few functions are moot (e.g. switch, reset).
>
> The current system (looking at dev_priv->hw_context_size to distinguish fake from hw contexts) is imo a bit obfuscated.
> And we can always abstract this away with vfuncs if it becomes too complex in the future...
>
> What do you think? can special casing made do for the time being?

Yeah I generally promote the rule-of-thumb to only do vfuncs once we
have the 3rd case (and I don't think we should count fake contexts
really). Until then if-ladders and hacks are good enough. Actually
better since usually you need a few completely different platforms to
know what's required of your vfunc intefaces to cover it all.

I really only latched onto this because of your FIXME comment, no
other reason at all. So if we decide that some reorg helps the code a
lot we can do that as a follow-up, but really not required upfront
imo.
-Daniel

[e-mail] June 19, 2014, 10:10 a.m. UTC | #4

> -----Original Message-----

> From: [e-mail] [mailto:[e-mail]] On Behalf Of

> Daniel Vetter

> Sent: Thursday, June 19, 2014 11:08 AM

> To: Mateo Lozano, Oscar

> Cc: [e-mail]

> Subject: Re: [Intel-gfx] [PATCH 09/53] drm/i915/bdw: Initialization for

> Logical Ring Contexts

> 

> On Thu, Jun 19, 2014 at 11:23 AM, Mateo Lozano, Oscar

> <[e-mail]> wrote:

> >> > -static bool hw_context_enabled(struct drm_device *dev)

> >> > +static bool contexts_enabled(struct drm_device *dev)

> >> >  {

> >> > -   return to_i915(dev)->hw_context_size;

> >> > +   struct drm_i915_private *dev_priv = to_i915(dev);

> >> > +

> >> > +   /* FIXME: this would be cleaner with a "context type" enum */

> >> > +   return dev_priv->lrc_enabled || dev_priv->hw_context_size;

> >>

> >> Since you have a bunch of if ladders the usual approach isn't an enum

> >> but a vfunc table to abstract behaviour. Think object types instead

> >> of switch statements. Style bikeshed though (presume code later on

> >> doesn't have excesses here).

> >> -Daniel

> >

> > Hmmmm... I offered to do this with vfuncs early on, but you mentioned

> special-casing should be enough. And I agreed: at the end, the LR contexts

> are not that different from traditional HW contexts. This is what I had in

> mind:

> >

> > CTX_TYPE_FAKE: no backing objects.

> > CTX_TYPE_HW: one render backing object at creation time.

> > CTX_TYPE_LR: n backing objects, with deferred creation. A few functions

> are moot (e.g. switch, reset).

> >

> > The current system (looking at dev_priv->hw_context_size to distinguish

> fake from hw contexts) is imo a bit obfuscated.

> > And we can always abstract this away with vfuncs if it becomes too

> complex in the future...

> >

> > What do you think? can special casing made do for the time being?

> 

> Yeah I generally promote the rule-of-thumb to only do vfuncs once we have

> the 3rd case (and I don't think we should count fake contexts really). Until

> then if-ladders and hacks are good enough. Actually better since usually you

> need a few completely different platforms to know what's required of your

> vfunc intefaces to cover it all.

> 

> I really only latched onto this because of your FIXME comment, no other

> reason at all. So if we decide that some reorg helps the code a lot we can do

> that as a follow-up, but really not required upfront imo.


So green light to the enum idea? It´ll look better than the current dev_priv->hw_context_size + dev_priv->lrc_enabled and I can send it early as prep-work...

Daniel Vetter June 19, 2014, 10:34 a.m. UTC | #5

On Thu, Jun 19, 2014 at 12:10 PM, Mateo Lozano, Oscar
<[e-mail]> wrote:
> So green light to the enum idea? It´ll look better than the current dev_priv->hw_context_size + dev_priv->lrc_enabled and I can send it early as prep-work...

Yeah if you like it I'm ok with it, so please go ahead.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 801b891..3f3fb36 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -416,7 +416,13 @@  int i915_gem_context_init(struct drm_device *dev)
 	if (WARN_ON(dev_priv->ring[RCS].default_context))
 		return 0;
 
-	if (HAS_HW_CONTEXTS(dev)) {
+	dev_priv->lrc_enabled = intel_enable_execlists(dev);
+
+	if (dev_priv->lrc_enabled) {
+		/* NB: intentionally left blank. We will allocate our own
+		 * backing objects as we need them, thank you very much */
+		dev_priv->hw_context_size = 0;
+	} else if (HAS_HW_CONTEXTS(dev)) {
 		dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
 		if (dev_priv->hw_context_size > (1<<20)) {
 			DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
@@ -436,7 +442,9 @@  int i915_gem_context_init(struct drm_device *dev)
 	for (i = 0; i < I915_NUM_RINGS; i++)
 		dev_priv->ring[i].default_context = ctx;
 
-	DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
+	DRM_DEBUG_DRIVER("%s context support initialized\n",
+			dev_priv->lrc_enabled ? "LR" :
+			dev_priv->hw_context_size ? "HW" : "fake");
 	return 0;
 }
 
@@ -765,9 +773,12 @@  int i915_switch_context(struct intel_engine_cs *ring,
 	return do_switch(ring, to);
 }
 
-static bool hw_context_enabled(struct drm_device *dev)
+static bool contexts_enabled(struct drm_device *dev)
 {
-	return to_i915(dev)->hw_context_size;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	/* FIXME: this would be cleaner with a "context type" enum */
+	return dev_priv->lrc_enabled || dev_priv->hw_context_size;
 }
 
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
@@ -778,7 +789,7 @@  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	struct intel_context *ctx;
 	int ret;
 
-	if (!hw_context_enabled(dev))
+	if (!contexts_enabled(dev))
 		return -ENODEV;
 
 	ret = i915_mutex_lock_interruptible(dev);