| Message ID | 20180914092158.7248-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Check engine->default_state mapping on module load | expand |
On 14/09/2018 10:21, Chris Wilson wrote: > Check we can indeed acquire a WB mapping of the context image on module > load. Later this will give us the opportunity to validate that we can > switch from WC to WB as required. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index be9d012d851b..37353afec66e 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -5424,6 +5424,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) > > for_each_engine(engine, i915, id) { > struct i915_vma *state; > + void *vaddr; > > state = to_intel_context(ctx, engine)->state; > if (!state) > @@ -5446,6 +5447,16 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) > goto err_active; > > engine->default_state = i915_gem_object_get(state->obj); > + > + /* Check we can acquire the image of the context state */ > + vaddr = i915_gem_object_pin_map(engine->default_state, > + I915_MAP_WB); > + if (IS_ERR(vaddr)) { > + err = PTR_ERR(vaddr); > + goto err_active; > + } > + > + i915_gem_object_unpin_map(engine->default_state); Ah this perhaps strengthens the argument to have __intel_engines_pin_default_state? (And the unpin pair.) And I guess you made this patch come before the switch to WC since it is not FORCE_WB here? Regards, Tvrtko > } > > if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) { >
Quoting Tvrtko Ursulin (2018-09-14 10:43:12) > > On 14/09/2018 10:21, Chris Wilson wrote: > > Check we can indeed acquire a WB mapping of the context image on module > > load. Later this will give us the opportunity to validate that we can > > switch from WC to WB as required. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index be9d012d851b..37353afec66e 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -5424,6 +5424,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) > > > > for_each_engine(engine, i915, id) { > > struct i915_vma *state; > > + void *vaddr; > > > > state = to_intel_context(ctx, engine)->state; > > if (!state) > > @@ -5446,6 +5447,16 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) > > goto err_active; > > > > engine->default_state = i915_gem_object_get(state->obj); > > + > > + /* Check we can acquire the image of the context state */ > > + vaddr = i915_gem_object_pin_map(engine->default_state, > > + I915_MAP_WB); > > + if (IS_ERR(vaddr)) { > > + err = PTR_ERR(vaddr); > > + goto err_active; > > + } > > + > > + i915_gem_object_unpin_map(engine->default_state); > > Ah this perhaps strengthens the argument to have > __intel_engines_pin_default_state? (And the unpin pair.) I hope it doesn't come to making this into an interface :) Definitely not an idea I endorse, random manipulation of the default_state. > And I guess you made this patch come before the switch to WC since it is > not FORCE_WB here? Yes, full sequence of patches in your inbox. -Chris
On 14/09/2018 10:51, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-14 10:43:12) >> >> On 14/09/2018 10:21, Chris Wilson wrote: >>> Check we can indeed acquire a WB mapping of the context image on module >>> load. Later this will give us the opportunity to validate that we can >>> switch from WC to WB as required. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>> index be9d012d851b..37353afec66e 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.c >>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>> @@ -5424,6 +5424,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) >>> >>> for_each_engine(engine, i915, id) { >>> struct i915_vma *state; >>> + void *vaddr; >>> >>> state = to_intel_context(ctx, engine)->state; >>> if (!state) >>> @@ -5446,6 +5447,16 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) >>> goto err_active; >>> >>> engine->default_state = i915_gem_object_get(state->obj); >>> + >>> + /* Check we can acquire the image of the context state */ >>> + vaddr = i915_gem_object_pin_map(engine->default_state, >>> + I915_MAP_WB); >>> + if (IS_ERR(vaddr)) { >>> + err = PTR_ERR(vaddr); >>> + goto err_active; >>> + } >>> + >>> + i915_gem_object_unpin_map(engine->default_state); >> >> Ah this perhaps strengthens the argument to have >> __intel_engines_pin_default_state? (And the unpin pair.) > > I hope it doesn't come to making this into an interface :) > Definitely not an idea I endorse, random manipulation of the > default_state. Double underscore alleviates those concerns, no? :) Would makes it only one place the following patch needed to switch to MAP_FORCE_WB. Regards, Tvrtko >> And I guess you made this patch come before the switch to WC since it is >> not FORCE_WB here? > > Yes, full sequence of patches in your inbox. > -Chris >
Quoting Tvrtko Ursulin (2018-09-14 11:03:18) > > On 14/09/2018 10:51, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-09-14 10:43:12) > >> > >> On 14/09/2018 10:21, Chris Wilson wrote: > >>> Check we can indeed acquire a WB mapping of the context image on module > >>> load. Later this will give us the opportunity to validate that we can > >>> switch from WC to WB as required. > >>> > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>> --- > >>> drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >>> index be9d012d851b..37353afec66e 100644 > >>> --- a/drivers/gpu/drm/i915/i915_gem.c > >>> +++ b/drivers/gpu/drm/i915/i915_gem.c > >>> @@ -5424,6 +5424,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) > >>> > >>> for_each_engine(engine, i915, id) { > >>> struct i915_vma *state; > >>> + void *vaddr; > >>> > >>> state = to_intel_context(ctx, engine)->state; > >>> if (!state) > >>> @@ -5446,6 +5447,16 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) > >>> goto err_active; > >>> > >>> engine->default_state = i915_gem_object_get(state->obj); > >>> + > >>> + /* Check we can acquire the image of the context state */ > >>> + vaddr = i915_gem_object_pin_map(engine->default_state, > >>> + I915_MAP_WB); > >>> + if (IS_ERR(vaddr)) { > >>> + err = PTR_ERR(vaddr); > >>> + goto err_active; > >>> + } > >>> + > >>> + i915_gem_object_unpin_map(engine->default_state); > >> > >> Ah this perhaps strengthens the argument to have > >> __intel_engines_pin_default_state? (And the unpin pair.) > > > > I hope it doesn't come to making this into an interface :) > > Definitely not an idea I endorse, random manipulation of the > > default_state. > > Double underscore alleviates those concerns, no? :) Bah, did you see the EXPORT_SYMBOL_GPL(__i915_gem_object_set_pages), nothing is safe! > Would makes it only one place the following patch needed to switch to > MAP_FORCE_WB. But that was the point of having a separate step here. This is then the only mapping of default_state that needs to do the force. I liked that. The other one we need is the initialisation of the lrc state. -Chris
On 14/09/2018 11:52, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-14 11:03:18) >> >> On 14/09/2018 10:51, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2018-09-14 10:43:12) >>>> >>>> On 14/09/2018 10:21, Chris Wilson wrote: >>>>> Check we can indeed acquire a WB mapping of the context image on module >>>>> load. Later this will give us the opportunity to validate that we can >>>>> switch from WC to WB as required. >>>>> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>>>> index be9d012d851b..37353afec66e 100644 >>>>> --- a/drivers/gpu/drm/i915/i915_gem.c >>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>>>> @@ -5424,6 +5424,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) >>>>> >>>>> for_each_engine(engine, i915, id) { >>>>> struct i915_vma *state; >>>>> + void *vaddr; >>>>> >>>>> state = to_intel_context(ctx, engine)->state; >>>>> if (!state) >>>>> @@ -5446,6 +5447,16 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) >>>>> goto err_active; >>>>> >>>>> engine->default_state = i915_gem_object_get(state->obj); >>>>> + >>>>> + /* Check we can acquire the image of the context state */ >>>>> + vaddr = i915_gem_object_pin_map(engine->default_state, >>>>> + I915_MAP_WB); >>>>> + if (IS_ERR(vaddr)) { >>>>> + err = PTR_ERR(vaddr); >>>>> + goto err_active; >>>>> + } >>>>> + >>>>> + i915_gem_object_unpin_map(engine->default_state); >>>> >>>> Ah this perhaps strengthens the argument to have >>>> __intel_engines_pin_default_state? (And the unpin pair.) >>> >>> I hope it doesn't come to making this into an interface :) >>> Definitely not an idea I endorse, random manipulation of the >>> default_state. >> >> Double underscore alleviates those concerns, no? :) > > Bah, did you see the EXPORT_SYMBOL_GPL(__i915_gem_object_set_pages), > nothing is safe! > >> Would makes it only one place the following patch needed to switch to >> MAP_FORCE_WB. > > But that was the point of having a separate step here. This is then the > only mapping of default_state that needs to do the force. I liked that. > The other one we need is the initialisation of the lrc state. Hmm true.. makes some sense as much as I like avoiding code duplication. Okay.. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index be9d012d851b..37353afec66e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5424,6 +5424,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) for_each_engine(engine, i915, id) { struct i915_vma *state; + void *vaddr; state = to_intel_context(ctx, engine)->state; if (!state) @@ -5446,6 +5447,16 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) goto err_active; engine->default_state = i915_gem_object_get(state->obj); + + /* Check we can acquire the image of the context state */ + vaddr = i915_gem_object_pin_map(engine->default_state, + I915_MAP_WB); + if (IS_ERR(vaddr)) { + err = PTR_ERR(vaddr); + goto err_active; + } + + i915_gem_object_unpin_map(engine->default_state); } if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) {
Check we can indeed acquire a WB mapping of the context image on module load. Later this will give us the opportunity to validate that we can switch from WC to WB as required. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++ 1 file changed, 11 insertions(+)