| Message ID | 1471669765-5935-27-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 20, 2016 at 10:39:25AM +0530, Sagar Arun Kamble wrote: > + obj = dev_priv->guc.slpc.vma->obj; > + if (obj) { OOPS. > + intel_slpc_query_task_state(dev_priv); > + > + page = i915_gem_object_get_page(obj, 0); > + if (page) > + pv = kmap_atomic(page); > + } > + > + if (pv) { > + data = *(struct slpc_shared_data *) pv; > + kunmap_atomic(pv); Can kmap_atomic return zero? > + > + /* > + * TODO: Define separate variables for slice and unslice > + * frequencies for driver state variable. > + */ > + dev_priv->rps.max_freq_softlimit = > + data.task_state_data.freq_unslice_max; > + dev_priv->rps.min_freq_softlimit = > + data.task_state_data.freq_unslice_min; These are user values, you do not get to arbitrarily rewrite them. You control dev_priv->rps.[min|max]_freq. -Chris
On 8/20/2016 1:32 PM, Chris Wilson wrote: > On Sat, Aug 20, 2016 at 10:39:25AM +0530, Sagar Arun Kamble wrote: >> + obj = dev_priv->guc.slpc.vma->obj; >> + if (obj) { > OOPS. Fixed in next series. > >> + intel_slpc_query_task_state(dev_priv); >> + >> + page = i915_gem_object_get_page(obj, 0); >> + if (page) >> + pv = kmap_atomic(page); >> + } >> + >> + if (pv) { >> + data = *(struct slpc_shared_data *) pv; >> + kunmap_atomic(pv); > Can kmap_atomic return zero? Fixed in next series. > >> + >> + /* >> + * TODO: Define separate variables for slice and unslice >> + * frequencies for driver state variable. >> + */ >> + dev_priv->rps.max_freq_softlimit = >> + data.task_state_data.freq_unslice_max; >> + dev_priv->rps.min_freq_softlimit = >> + data.task_state_data.freq_unslice_min; > These are user values, you do not get to arbitrarily rewrite them. > > You control dev_priv->rps.[min|max]_freq. With SLPC, GuC firmware SLPC S/W requested frequency be operated in the softlimits analogous to Host softlimits. Limits might be different with SLPC and can be controlled through regular interfaces. dev_priv->rps.[min|max]_freq are HW Min/Max. > -Chris >
On Sun, Aug 21, 2016 at 11:39:22AM +0530, Kamble, Sagar A wrote: > > > On 8/20/2016 1:32 PM, Chris Wilson wrote: > >On Sat, Aug 20, 2016 at 10:39:25AM +0530, Sagar Arun Kamble wrote: > >>+ obj = dev_priv->guc.slpc.vma->obj; > >>+ if (obj) { > >OOPS. > Fixed in next series. > > > >>+ intel_slpc_query_task_state(dev_priv); > >>+ > >>+ page = i915_gem_object_get_page(obj, 0); > >>+ if (page) > >>+ pv = kmap_atomic(page); > >>+ } > >>+ > >>+ if (pv) { > >>+ data = *(struct slpc_shared_data *) pv; > >>+ kunmap_atomic(pv); > >Can kmap_atomic return zero? > Fixed in next series. > > > >>+ > >>+ /* > >>+ * TODO: Define separate variables for slice and unslice > >>+ * frequencies for driver state variable. > >>+ */ > >>+ dev_priv->rps.max_freq_softlimit = > >>+ data.task_state_data.freq_unslice_max; > >>+ dev_priv->rps.min_freq_softlimit = > >>+ data.task_state_data.freq_unslice_min; > >These are user values, you do not get to arbitrarily rewrite them. > > > >You control dev_priv->rps.[min|max]_freq. > With SLPC, GuC firmware SLPC S/W requested frequency be operated in > the softlimits analogous to > Host softlimits. Limits might be different with SLPC and can be > controlled through regular interfaces. > dev_priv->rps.[min|max]_freq are HW Min/Max. Exactly. The soft limits are *only* set by the user. They are not to modified by the driver. (The caveat would be a dynamic update of the hw range, but that too should never be required.) -Chris
On 8/21/2016 2:09 PM, Chris Wilson wrote: > On Sun, Aug 21, 2016 at 11:39:22AM +0530, Kamble, Sagar A wrote: >> >> On 8/20/2016 1:32 PM, Chris Wilson wrote: >>> On Sat, Aug 20, 2016 at 10:39:25AM +0530, Sagar Arun Kamble wrote: >>>> + obj = dev_priv->guc.slpc.vma->obj; >>>> + if (obj) { >>> OOPS. >> Fixed in next series. >>>> + intel_slpc_query_task_state(dev_priv); >>>> + >>>> + page = i915_gem_object_get_page(obj, 0); >>>> + if (page) >>>> + pv = kmap_atomic(page); >>>> + } >>>> + >>>> + if (pv) { >>>> + data = *(struct slpc_shared_data *) pv; >>>> + kunmap_atomic(pv); >>> Can kmap_atomic return zero? >> Fixed in next series. >>>> + >>>> + /* >>>> + * TODO: Define separate variables for slice and unslice >>>> + * frequencies for driver state variable. >>>> + */ >>>> + dev_priv->rps.max_freq_softlimit = >>>> + data.task_state_data.freq_unslice_max; >>>> + dev_priv->rps.min_freq_softlimit = >>>> + data.task_state_data.freq_unslice_min; >>> These are user values, you do not get to arbitrarily rewrite them. >>> >>> You control dev_priv->rps.[min|max]_freq. >> With SLPC, GuC firmware SLPC S/W requested frequency be operated in >> the softlimits analogous to >> Host softlimits. Limits might be different with SLPC and can be >> controlled through regular interfaces. >> dev_priv->rps.[min|max]_freq are HW Min/Max. > Exactly. The soft limits are *only* set by the user. They are not to > modified by the driver. (The caveat would be a dynamic update of the hw > range, but that too should never be required.) > -Chris This initialization is similar to following from intel_init_gt_powersave dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq; dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq; I assume min_freq is hw min(RPn). With SLPC, min_freq(RPn) will not be requested. SLPC operating range today is (>Rpe, Rp0) so I wanted user to know the min_softlimit being initialized by SLPC by default. >
On Sun, Aug 21, 2016 at 09:39:17PM +0530, Kamble, Sagar A wrote: > > > On 8/21/2016 2:09 PM, Chris Wilson wrote: > >On Sun, Aug 21, 2016 at 11:39:22AM +0530, Kamble, Sagar A wrote: > >> > >>On 8/20/2016 1:32 PM, Chris Wilson wrote: > >>>On Sat, Aug 20, 2016 at 10:39:25AM +0530, Sagar Arun Kamble wrote: > >>>>+ obj = dev_priv->guc.slpc.vma->obj; > >>>>+ if (obj) { > >>>OOPS. > >>Fixed in next series. > >>>>+ intel_slpc_query_task_state(dev_priv); > >>>>+ > >>>>+ page = i915_gem_object_get_page(obj, 0); > >>>>+ if (page) > >>>>+ pv = kmap_atomic(page); > >>>>+ } > >>>>+ > >>>>+ if (pv) { > >>>>+ data = *(struct slpc_shared_data *) pv; > >>>>+ kunmap_atomic(pv); > >>>Can kmap_atomic return zero? > >>Fixed in next series. > >>>>+ > >>>>+ /* > >>>>+ * TODO: Define separate variables for slice and unslice > >>>>+ * frequencies for driver state variable. > >>>>+ */ > >>>>+ dev_priv->rps.max_freq_softlimit = > >>>>+ data.task_state_data.freq_unslice_max; > >>>>+ dev_priv->rps.min_freq_softlimit = > >>>>+ data.task_state_data.freq_unslice_min; > >>>These are user values, you do not get to arbitrarily rewrite them. > >>> > >>>You control dev_priv->rps.[min|max]_freq. > >>With SLPC, GuC firmware SLPC S/W requested frequency be operated in > >>the softlimits analogous to > >>Host softlimits. Limits might be different with SLPC and can be > >>controlled through regular interfaces. > >>dev_priv->rps.[min|max]_freq are HW Min/Max. > >Exactly. The soft limits are *only* set by the user. They are not to > >modified by the driver. (The caveat would be a dynamic update of the hw > >range, but that too should never be required.) > >-Chris > This initialization is similar to following from intel_init_gt_powersave > dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq; > dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq; > I assume min_freq is hw min(RPn). With SLPC, min_freq(RPn) will not > be requested. > SLPC operating range today is (>Rpe, Rp0) so I wanted user to know > the min_softlimit being initialized by SLPC by default. Hmm, my mistake here was thinking this was more than a one off. Setting the initial soft (user) range on startup is fine. Continually changing them after userspace registration is not. (The value the user writes into the limit is what should be read back - without very good reason, such as the hard limits changing). -Chris
On 8/24/2016 2:07 PM, Chris Wilson wrote: > On Sun, Aug 21, 2016 at 09:39:17PM +0530, Kamble, Sagar A wrote: >> >> On 8/21/2016 2:09 PM, Chris Wilson wrote: >>> On Sun, Aug 21, 2016 at 11:39:22AM +0530, Kamble, Sagar A wrote: >>>> On 8/20/2016 1:32 PM, Chris Wilson wrote: >>>>> On Sat, Aug 20, 2016 at 10:39:25AM +0530, Sagar Arun Kamble wrote: >>>>>> + obj = dev_priv->guc.slpc.vma->obj; >>>>>> + if (obj) { >>>>> OOPS. >>>> Fixed in next series. >>>>>> + intel_slpc_query_task_state(dev_priv); >>>>>> + >>>>>> + page = i915_gem_object_get_page(obj, 0); >>>>>> + if (page) >>>>>> + pv = kmap_atomic(page); >>>>>> + } >>>>>> + >>>>>> + if (pv) { >>>>>> + data = *(struct slpc_shared_data *) pv; >>>>>> + kunmap_atomic(pv); >>>>> Can kmap_atomic return zero? >>>> Fixed in next series. >>>>>> + >>>>>> + /* >>>>>> + * TODO: Define separate variables for slice and unslice >>>>>> + * frequencies for driver state variable. >>>>>> + */ >>>>>> + dev_priv->rps.max_freq_softlimit = >>>>>> + data.task_state_data.freq_unslice_max; >>>>>> + dev_priv->rps.min_freq_softlimit = >>>>>> + data.task_state_data.freq_unslice_min; >>>>> These are user values, you do not get to arbitrarily rewrite them. >>>>> >>>>> You control dev_priv->rps.[min|max]_freq. >>>> With SLPC, GuC firmware SLPC S/W requested frequency be operated in >>>> the softlimits analogous to >>>> Host softlimits. Limits might be different with SLPC and can be >>>> controlled through regular interfaces. >>>> dev_priv->rps.[min|max]_freq are HW Min/Max. >>> Exactly. The soft limits are *only* set by the user. They are not to >>> modified by the driver. (The caveat would be a dynamic update of the hw >>> range, but that too should never be required.) >>> -Chris >> This initialization is similar to following from intel_init_gt_powersave >> dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq; >> dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq; >> I assume min_freq is hw min(RPn). With SLPC, min_freq(RPn) will not >> be requested. >> SLPC operating range today is (>Rpe, Rp0) so I wanted user to know >> the min_softlimit being initialized by SLPC by default. > Hmm, my mistake here was thinking this was more than a one off. Setting > the initial soft (user) range on startup is fine. Continually changing > them after userspace registration is not. (The value the user writes > into the limit is what should be read back - without very good reason, > such as the hard limits changing). > -Chris I interpreted the rewrite differently, thinking SLPC should not write it post init_gt_powersave. As discussed, I will change this to make sure SLPC init does not touch these values once it is initialized in the load path. >
diff --git a/drivers/gpu/drm/i915/intel_slpc.c b/drivers/gpu/drm/i915/intel_slpc.c index 26cea21..2bfb30f 100644 --- a/drivers/gpu/drm/i915/intel_slpc.c +++ b/drivers/gpu/drm/i915/intel_slpc.c @@ -286,6 +286,10 @@ void intel_slpc_disable(struct drm_i915_private *dev_priv) void intel_slpc_enable(struct drm_i915_private *dev_priv) { + struct drm_i915_gem_object *obj; + struct page *page; + void *pv = NULL; + struct slpc_shared_data data; u64 val; host2guc_slpc_reset(dev_priv); @@ -330,6 +334,32 @@ void intel_slpc_enable(struct drm_i915_private *dev_priv) SLPC_PARAM_GLOBAL_ENABLE_BALANCER_IN_NON_GAMING_MODE, 0); + obj = dev_priv->guc.slpc.vma->obj; + if (obj) { + intel_slpc_query_task_state(dev_priv); + + page = i915_gem_object_get_page(obj, 0); + if (page) + pv = kmap_atomic(page); + } + + if (pv) { + data = *(struct slpc_shared_data *) pv; + kunmap_atomic(pv); + + /* + * TODO: Define separate variables for slice and unslice + * frequencies for driver state variable. + */ + dev_priv->rps.max_freq_softlimit = + data.task_state_data.freq_unslice_max; + dev_priv->rps.min_freq_softlimit = + data.task_state_data.freq_unslice_min; + + dev_priv->rps.max_freq_softlimit *= GEN9_FREQ_SCALER; + dev_priv->rps.min_freq_softlimit *= GEN9_FREQ_SCALER; + } + return; }
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/intel_slpc.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)