Re: [Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state
On ke, 2017-05-10 at 08:33 +, Oscar Mateo wrote: > > > On 05/10/2017 01:28 PM, Daniel Vetter wrote: > > > > On Wed, May 10, 2017 at 2:59 PM, Joonas Lahtinen > > wrote: > > > > > > > > > > > @@ -841,6 +847,11 @@ static int gen9_init_workarounds(struct > > > > intel_engine_cs *engine) > > > > if (ret) > > > > return ret; > > > > > > > > + /* Allow the UMD to configure their own power clock state > > > > */ > > > > + ret = wa_ring_whitelist_reg(engine, > > > > GEN8_R_PWR_CLK_STATE); > > > > + if (ret) > > > > + return ret; > > > This is not a workaround, so it should be part of the cmd parser > > > and > > > have an userspace. > > cmd parser? This is Gen8+, so the cmd parser shouldn't even be > active. > Or am I missing something? That's correct, my bad. > > > > > Yeah, the "This allows userspace ..." start of the commit message > > should have been a dead giveaway that we do indeed need the > > userspace > > patch for this too. > > -Daniel > > Yes, I got the memo :) > Dmitry has opened a ticket against intel-vaapi-driver > (https://github.com/01org/intel-vaapi-driver/issues/152) so hopefully > we > will have a userspace soon. So with the userspace, we can merge. Regards, Joonas -- Joonas Lahtinen Open Source Technology Center Intel Corporation ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state
On 05/10/2017 01:28 PM, Daniel Vetter wrote: On Wed, May 10, 2017 at 2:59 PM, Joonas Lahtinen wrote: @@ -841,6 +847,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *engine) if (ret) return ret; + /* Allow the UMD to configure their own power clock state */ + ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE); + if (ret) + return ret; This is not a workaround, so it should be part of the cmd parser and have an userspace. cmd parser? This is Gen8+, so the cmd parser shouldn't even be active. Or am I missing something? Yeah, the "This allows userspace ..." start of the commit message should have been a dead giveaway that we do indeed need the userspace patch for this too. -Daniel Yes, I got the memo :) Dmitry has opened a ticket against intel-vaapi-driver (https://github.com/01org/intel-vaapi-driver/issues/152) so hopefully we will have a userspace soon. -- Oscar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state
On 05/10/2017 02:09 PM, Michał Winiarski wrote:
On Wed, May 10, 2017 at 04:47:50PM +0300, Mika Kuoppala wrote:
Oscar Mateo writes:
This allows userspace to shutdown slices at will for performance/power reasons
(because it doesn't have a use for more slices).
Cc: Dmitry Rogozhkin
Cc: Chris Wilson
Signed-off-by: Oscar Mateo
---
drivers/gpu/drm/i915/intel_engine_cs.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
b/drivers/gpu/drm/i915/intel_engine_cs.c
index 402769d..17ff88d 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -628,6 +628,7 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs
*engine,
static int gen8_init_workarounds(struct intel_engine_cs *engine)
{
struct drm_i915_private *dev_priv = engine->i915;
+ int ret;
WA_SET_BIT_MASKED(INSTPM, INSTPM_FORCE_ORDERING);
@@ -673,6 +674,11 @@ static int gen8_init_workarounds(struct intel_engine_cs *engine)
GEN6_WIZ_HASHING_MASK,
GEN6_WIZ_HASHING_16x4);
+ /* Allow the UMD to configure their own power clock state */
+ ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
+ if (ret)
+ return ret;
+
How will the different userspace clients coordinate with the slice
state? I guess in another words, is this part of the context?
-Mika
As Michal said, this is context-saved/restored, so no need to coordinate
between clients.
Spec says (see IHD-OS-SKL-Vol2c-05.16 page 614):
"This register must be initialized correctly when the context is submitted for
the first time. This register is context save/restored as part of Exec-List
context image in both Exec-List and Ring-Buffer mode of scheduling."
So we're good, right?
Ditto :)
Except there's also:
"This register must not be programmed using MI_LOAD_REGISTER_IMM command in ring
buffer or in batch buffer, however programming "NON - SLM Indication" field
through MI_LOAD_REGISTER_IMM is an exception defined below. If a need arises to
change the render configuration for a context being executed in HW, Scheduler
must preempt the context and update the desired render configuration in the
logical render context image in memory and resubmit the context"
So... are we sure that we're fine with giving userspace control over this
register? If so - it would still be a good idea to mention that the info in the
spec is bogus (commit message or just a comment).
This approach has been double-checked with HW and they believe there
should be no problem, as long as the update is done as:
- PIPECONTROL - Stalling flish, flush all caches (color, depth, DC$)
- LOAD_REGISTER_IMMEDIATE - R_PWR_CLK_STATE
- Reprogram complete state
I'm trying to get them to change the BSpec accordingly.
return 0;
}
@@ -841,6 +847,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *engine)
if (ret)
return ret;
+ /* Allow the UMD to configure their own power clock state */
+ ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
+ if (ret)
+ return ret;
+
return 0;
}
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state
On Wed, May 10, 2017 at 04:09:34PM +0200, Michał Winiarski wrote:
> On Wed, May 10, 2017 at 04:47:50PM +0300, Mika Kuoppala wrote:
> > Oscar Mateo writes:
> >
> > > This allows userspace to shutdown slices at will for performance/power
> > > reasons
> > > (because it doesn't have a use for more slices).
> > >
> > > Cc: Dmitry Rogozhkin
> > > Cc: Chris Wilson
> > > Signed-off-by: Oscar Mateo
> > > ---
> > > drivers/gpu/drm/i915/intel_engine_cs.c | 11 +++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> > > b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > index 402769d..17ff88d 100644
> > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > @@ -628,6 +628,7 @@ static int wa_ring_whitelist_reg(struct
> > > intel_engine_cs *engine,
> > > static int gen8_init_workarounds(struct intel_engine_cs *engine)
> > > {
> > > struct drm_i915_private *dev_priv = engine->i915;
> > > + int ret;
> > >
> > > WA_SET_BIT_MASKED(INSTPM, INSTPM_FORCE_ORDERING);
> > >
> > > @@ -673,6 +674,11 @@ static int gen8_init_workarounds(struct
> > > intel_engine_cs *engine)
> > > GEN6_WIZ_HASHING_MASK,
> > > GEN6_WIZ_HASHING_16x4);
> > >
> > > + /* Allow the UMD to configure their own power clock state */
> > > + ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
> > > + if (ret)
> > > + return ret;
> > > +
> >
> > How will the different userspace clients coordinate with the slice
> > state? I guess in another words, is this part of the context?
> >
> > -Mika
> >
>
> Spec says (see IHD-OS-SKL-Vol2c-05.16 page 614):
> "This register must be initialized correctly when the context is submitted for
> the first time. This register is context save/restored as part of Exec-List
> context image in both Exec-List and Ring-Buffer mode of scheduling."
>
> So we're good, right?
>
> Except there's also:
> "This register must not be programmed using MI_LOAD_REGISTER_IMM command in
> ring
> buffer or in batch buffer, however programming "NON - SLM Indication" field
> through MI_LOAD_REGISTER_IMM is an exception defined below. If a need arises
> to
> change the render configuration for a context being executed in HW, Scheduler
> must preempt the context and update the desired render configuration in the
> logical render context image in memory and resubmit the context"
>
> So... are we sure that we're fine with giving userspace control over this
> register? If so - it would still be a good idea to mention that the info in
> the
> spec is bogus (commit message or just a comment).
Doesn't sound like it. So we are back to doing the preempt and changing
the context image as suggested in the other patch. Still we have the
issue that OA config conflicts with changing the slice eu config.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state
On Wed, May 10, 2017 at 04:47:50PM +0300, Mika Kuoppala wrote:
> Oscar Mateo writes:
>
> > This allows userspace to shutdown slices at will for performance/power
> > reasons
> > (because it doesn't have a use for more slices).
> >
> > Cc: Dmitry Rogozhkin
> > Cc: Chris Wilson
> > Signed-off-by: Oscar Mateo
> > ---
> > drivers/gpu/drm/i915/intel_engine_cs.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> > b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 402769d..17ff88d 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -628,6 +628,7 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs
> > *engine,
> > static int gen8_init_workarounds(struct intel_engine_cs *engine)
> > {
> > struct drm_i915_private *dev_priv = engine->i915;
> > + int ret;
> >
> > WA_SET_BIT_MASKED(INSTPM, INSTPM_FORCE_ORDERING);
> >
> > @@ -673,6 +674,11 @@ static int gen8_init_workarounds(struct
> > intel_engine_cs *engine)
> > GEN6_WIZ_HASHING_MASK,
> > GEN6_WIZ_HASHING_16x4);
> >
> > + /* Allow the UMD to configure their own power clock state */
> > + ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
> > + if (ret)
> > + return ret;
> > +
>
> How will the different userspace clients coordinate with the slice
> state? I guess in another words, is this part of the context?
>
> -Mika
>
Spec says (see IHD-OS-SKL-Vol2c-05.16 page 614):
"This register must be initialized correctly when the context is submitted for
the first time. This register is context save/restored as part of Exec-List
context image in both Exec-List and Ring-Buffer mode of scheduling."
So we're good, right?
Except there's also:
"This register must not be programmed using MI_LOAD_REGISTER_IMM command in ring
buffer or in batch buffer, however programming "NON - SLM Indication" field
through MI_LOAD_REGISTER_IMM is an exception defined below. If a need arises to
change the render configuration for a context being executed in HW, Scheduler
must preempt the context and update the desired render configuration in the
logical render context image in memory and resubmit the context"
So... are we sure that we're fine with giving userspace control over this
register? If so - it would still be a good idea to mention that the info in the
spec is bogus (commit message or just a comment).
-Michał
>
> > return 0;
> > }
> >
> > @@ -841,6 +847,11 @@ static int gen9_init_workarounds(struct
> > intel_engine_cs *engine)
> > if (ret)
> > return ret;
> >
> > + /* Allow the UMD to configure their own power clock state */
> > + ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
> > + if (ret)
> > + return ret;
> > +
> > return 0;
> > }
> >
> > --
> > 1.9.1
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state
Oscar Mateo writes:
> This allows userspace to shutdown slices at will for performance/power reasons
> (because it doesn't have a use for more slices).
>
> Cc: Dmitry Rogozhkin
> Cc: Chris Wilson
> Signed-off-by: Oscar Mateo
> ---
> drivers/gpu/drm/i915/intel_engine_cs.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 402769d..17ff88d 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -628,6 +628,7 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs
> *engine,
> static int gen8_init_workarounds(struct intel_engine_cs *engine)
> {
> struct drm_i915_private *dev_priv = engine->i915;
> + int ret;
>
> WA_SET_BIT_MASKED(INSTPM, INSTPM_FORCE_ORDERING);
>
> @@ -673,6 +674,11 @@ static int gen8_init_workarounds(struct intel_engine_cs
> *engine)
> GEN6_WIZ_HASHING_MASK,
> GEN6_WIZ_HASHING_16x4);
>
> + /* Allow the UMD to configure their own power clock state */
> + ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
> + if (ret)
> + return ret;
> +
How will the different userspace clients coordinate with the slice
state? I guess in another words, is this part of the context?
-Mika
> return 0;
> }
>
> @@ -841,6 +847,11 @@ static int gen9_init_workarounds(struct intel_engine_cs
> *engine)
> if (ret)
> return ret;
>
> + /* Allow the UMD to configure their own power clock state */
> + ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
> + if (ret)
> + return ret;
> +
> return 0;
> }
>
> --
> 1.9.1
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state
On Wed, May 10, 2017 at 2:59 PM, Joonas Lahtinen wrote: >> @@ -841,6 +847,11 @@ static int gen9_init_workarounds(struct intel_engine_cs >> *engine) >> if (ret) >> return ret; >> >> + /* Allow the UMD to configure their own power clock state */ >> + ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE); >> + if (ret) >> + return ret; > > This is not a workaround, so it should be part of the cmd parser and > have an userspace. Yeah, the "This allows userspace ..." start of the commit message should have been a dead giveaway that we do indeed need the userspace patch for this too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state
On ti, 2017-05-02 at 15:07 +, Oscar Mateo wrote: > This allows userspace to shutdown slices at will for performance/power reasons > (because it doesn't have a use for more slices). > > Cc: Dmitry Rogozhkin > Cc: Chris Wilson > Signed-off-by: Oscar Mateo > @@ -673,6 +674,11 @@ static int gen8_init_workarounds(struct intel_engine_cs > *engine) > GEN6_WIZ_HASHING_MASK, > GEN6_WIZ_HASHING_16x4); > > + /* Allow the UMD to configure their own power clock state */ > + ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE); > + if (ret) > + return ret; > + > return 0; > } > > @@ -841,6 +847,11 @@ static int gen9_init_workarounds(struct intel_engine_cs > *engine) > if (ret) > return ret; > > + /* Allow the UMD to configure their own power clock state */ > + ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE); > + if (ret) > + return ret; This is not a workaround, so it should be part of the cmd parser and have an userspace. Regards, Joonas -- Joonas Lahtinen Open Source Technology Center Intel Corporation ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state
This allows userspace to shutdown slices at will for performance/power reasons
(because it doesn't have a use for more slices).
Cc: Dmitry Rogozhkin
Cc: Chris Wilson
Signed-off-by: Oscar Mateo
---
drivers/gpu/drm/i915/intel_engine_cs.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
b/drivers/gpu/drm/i915/intel_engine_cs.c
index 402769d..17ff88d 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -628,6 +628,7 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs
*engine,
static int gen8_init_workarounds(struct intel_engine_cs *engine)
{
struct drm_i915_private *dev_priv = engine->i915;
+ int ret;
WA_SET_BIT_MASKED(INSTPM, INSTPM_FORCE_ORDERING);
@@ -673,6 +674,11 @@ static int gen8_init_workarounds(struct intel_engine_cs
*engine)
GEN6_WIZ_HASHING_MASK,
GEN6_WIZ_HASHING_16x4);
+ /* Allow the UMD to configure their own power clock state */
+ ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
+ if (ret)
+ return ret;
+
return 0;
}
@@ -841,6 +847,11 @@ static int gen9_init_workarounds(struct intel_engine_cs
*engine)
if (ret)
return ret;
+ /* Allow the UMD to configure their own power clock state */
+ ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
+ if (ret)
+ return ret;
+
return 0;
}
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
