Re: [Intel-gfx] Why idle_freq is set to RPn and not RPe

[Szwichtenberg, Radoslaw]

Hello Chris!

The question is: why this change in behavior was made? 

On previous platforms Gfx Turbo frequency selection range in driver was in 
between Rpe & Rp0. Since Rpe is the possible Fmax at Vmin, it was used as the 
starting frequency once driver booted and any value lower than that was not 
requested.

Thanks!
Radek

> -Original Message-
> From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> Sent: Wednesday, December 30, 2015 10:31 AM
> To: Kamble, Sagar A
> Cc: S, Deepak; Szwichtenberg, Radoslaw; Intel Graphics Development; Goel,
> Akash
> Subject: Re: Why idle_freq is set to RPn and not RPe
> 
> On Wed, Dec 30, 2015 at 02:51:27PM +0530, Kamble, Sagar A wrote:
> > Hi Chris,
> >
> > With below commit, idle frequency is made RPn (HW Min).
> > Why are we not keeping it at RPe (Efficient Frequency)?
> > My understanding was to set Rpe on idle so that when GPU is out of
> > RC6 it can start operating at efficient frequency.
> 
> The driver is *idle*. When there is work to be submitted to the GPU, then
> we go back to RPe (though we wait for it to wake up first). RPe is just an
> inflexion point on the power curve.
> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre


Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial 
Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | 
Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i 
moze zawierac informacje poufne. W razie przypadkowego otrzymania tej 
wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; 
jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole 
use of the intended recipient(s). If you are not the intended recipient, please 
contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Why idle_freq is set to RPn and not RPe

[Szwichtenberg, Radoslaw]

Hello Chris,

Happy New Year!
Thanks for answers so far. I have some additional questions.

You wrote that this change was made to take a defensive position to ensure 
minimum power consumption - did we do any power measurements to confirm the 
benefit? How does this change affect user experience and overall performance? 
Did we do any performance impact measurements? Is same change done in Windows 
implementation? Was this change in the algorithm discussed with HW team?

Thanks!
Radek

> -Original Message-
> From: Kamble, Sagar A
> Sent: Thursday, December 31, 2015 6:38 PM
> To: Chris Wilson; Szwichtenberg, Radoslaw; S, Deepak; Intel Graphics
> Development; Goel, Akash
> Subject: Re: Why idle_freq is set to RPn and not RPe
> 
> 
> 
> On 12/30/2015 4:20 PM, Chris Wilson wrote:
> > On Wed, Dec 30, 2015 at 04:09:46PM +0530, Kamble, Sagar A wrote:
> >> Turbo frequency range is Rpe to Rp0 when GPU is active as, on
> workload
> >> submission frequency is taken to Rpe.
> >>
> >> Does the HW require us to drop to RPn before entering RC6?
> >> If we can enter RC6 even with other frequencies I think we can keep
> >> running at Rpe on Idle.
> > Remember that we quite frequently prevent the hardware going into RC6,
> I assume this is threshold times in TO/EI mode for which GT is idle but not
> power gated.
> > and that it has been known for the hardware to fail to enter RC6
> > itself (through driver error or whatnot).
> And assume this is because of forcewake/rc6 setup errors in driver paths
> which should not happen in best case :) Agree that running at Rpn makes
> sense.
> >   Going to the extreme, why wouldn't
> > you set Rp0 on idle, since that will give the best restart latency?
> True. We can have different logic that starts from Rp0 and comes down if
> perf is met.
> > -Chris
> >
> Thanks for the inputs Chris.


Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial 
Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | 
Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i 
moze zawierac informacje poufne. W razie przypadkowego otrzymania tej 
wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; 
jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole 
use of the intended recipient(s). If you are not the intended recipient, please 
contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [igt PATCH] igt/pm_rps: Remove remaining assert on CUR <= MAX

[Szwichtenberg, Radoslaw]

On Wed, 2017-06-14 at 12:44 -0700, jeff.mc...@intel.com wrote:
> From: Jeff McGee 
> 
> This completes the change started by:
> 
> commit 39cccab83b7c515a2b57abe679a8cb304c8933ef
> Author: Chris Wilson 
> Date:   Fri May 19 09:41:40 2017 +0100
> 
> igt/pm_rps: Allow CUR to be greater than MAX (overclocking)
> 
> Cc: Chris Wilson 
> Signed-off-by: Jeff McGee 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC i-g-t 1/4] igt: Remove default from the engine list

[Szwichtenberg, Radoslaw]

On Fri, 2017-06-23 at 12:31 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> Default is not an engine but an ABI alias for RCS. Remove it
> from the engine list to eliminate redundant subtests and test
> passes.
Does it mean that we will have an ABI part that we don't test?
-Radek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC i-g-t 1/4] igt: Remove default from the engine list

[Szwichtenberg, Radoslaw]

On Fri, 2017-06-23 at 15:35 +0100, Tvrtko Ursulin wrote:
> On 23/06/2017 15:17, Szwichtenberg, Radoslaw wrote:
> > On Fri, 2017-06-23 at 12:31 +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin 
> > > 
> > > Default is not an engine but an ABI alias for RCS. Remove it
> > > from the engine list to eliminate redundant subtests and test
> > > passes.
> > 
> > Does it mean that we will have an ABI part that we don't test?
> 
> Second patch adds the ABI testing to gem_exec_basic. Plus there is and 
> odd test here and there which explicitly sends batches to 
> I915_EXEC_DEFAULT. But to me just basic verification that the default 
> works sounds enough.
Looks good to me.
-Radek
> Regards,
> 
> Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] lib/ioctl_wrappers: Fix some comments

[Szwichtenberg, Radoslaw]

On Wed, 2017-06-28 at 13:40 +0300, Arkadiusz Hiler wrote:
> "This is a wraps" -> "This wraps"
> "hw/hardware context" -> "context"
> 
> gem_context_create does not use igt_require() but igt_skip_on() so make
> the similarity note more vague and in result true.
> 
> Cc: Daniel Vetter 
> Cc: Radoslaw Szwichtenberg 
> Signed-off-by: Arkadiusz Hiler 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 1/2] igt: Move read_rc6_residency function to lib

[Szwichtenberg, Radoslaw]

On Tue, 2017-07-25 at 17:26 +0200, Ewelina Musial wrote:
> Gem_mocs_settings and pm_rc6_residency tests are defining
> the same functionality to read residency from sysfs.
> Moving that function to lib/igt_aux and updating tests.
> 
> Signed-off-by: Ewelina Musial 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/glk: RGB565 planes now allow 90/270 rotation

[Szwichtenberg, Radoslaw]

On Wed, 2017-06-07 at 10:45 -0700, clinton.a.tay...@intel.com wrote:
> From: Clint Taylor 
> 
> RGB565 Pixel format planes can now be rotated at 90 and 270 degrees
> 
> Signed-off-by: Clint Taylor 
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
I think we should also update corresponding IGT tests.
-Radek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 1/2] tests: Clean up igt_skip_on_simulation() uses

[Szwichtenberg, Radoslaw]

On Fri, 2017-10-13 at 14:59 +0300, Arkadiusz Hiler wrote:
> General update to reflect current state of things.
> 
> Signed-off-by: Arkadiusz Hiler 
Acked-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] tests/pm_rps: Move some test logic out of boost function

[Szwichtenberg, Radoslaw]

On Wed, 2017-10-18 at 11:10 +0530, Sagar Arun Kamble wrote:
> 
> On 10/17/2017 6:13 PM, Radoslaw Szwichtenberg wrote:
> > Moving code out of the boost function will allow its usage
> > in other/new test scenarios.
> > 
> > Signed-off-by: Radoslaw Szwichtenberg 
> > Cc: Sagar Arun Kamble 
> > ---
> >   tests/pm_rps.c | 19 ++-
> >   1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> > index 89f3e31c..dad87646 100644
> > --- a/tests/pm_rps.c
> > +++ b/tests/pm_rps.c
> > @@ -583,11 +583,6 @@ static void boost_freq(int fd, int *boost_freqs)
> >     int64_t timeout = 1;
> >     igt_spin_t *load;
> >     unsigned int engine;
> > -   int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
> > -
> > -   fmid = get_hw_rounded_freq(fmid);
> > -   /* Set max freq to less then boost freq */
> > -   writeval(sysfs_files[MAX].filp, fmid);
> >   
> >     /* Put boost on the same engine as low load */
> >     engine = I915_EXEC_RENDER;
> > @@ -604,9 +599,6 @@ static void boost_freq(int fd, int *boost_freqs)
> >     igt_spin_batch_end(load);
> >     gem_sync(fd, load->handle);
> >     igt_spin_batch_free(fd, load);
> > -
> > -   /* Set max freq to original softmax */
> > -   writeval(sysfs_files[MAX].filp, origfreqs[MAX]);
> >   }
> >   
> >   static void waitboost(int fd, bool reset)
> > @@ -615,6 +607,9 @@ static void waitboost(int fd, bool reset)
> >     int boost_freqs[NUMFREQ];
> >     int post_freqs[NUMFREQ];
> >   
> > +   int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
> > +   fmid = get_hw_rounded_freq(fmid);
> > +
> 
> How about function for getting mid? We can reuse in min_max_config too.
You think about something like:

static int get_fmid(void)
{
int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
return get_hw_rounded_freq(fmid);
}

Even though it wasn't scope of this change I think I can add it - do you think
it will be beneficial? We just use it two places so at this moment the benefit
will be minimal.
> >     load_helper_run(LOW);
> >   
> >     igt_debug("Apply low load...\n");
> > @@ -627,11 +622,17 @@ static void waitboost(int fd, bool reset)
> >     sleep(1);
> >     }
> >   
> > +   /* Set max freq to less than boost freq */
> > +   writeval(sysfs_files[MAX].filp, fmid);
> > +
> >     /* When we wait upon the GPU, we want to temporarily boost it
> >      * to maximum.
> >      */
> >     boost_freq(fd, boost_freqs);
> >   
> > +   /* Set max freq to original softmax */
> > +   writeval(sysfs_files[MAX].filp, origfreqs[MAX]);
> > +
> >     igt_debug("Apply low load again...\n");
> >     sleep(1);
> >     stabilize_check(post_freqs);
> > @@ -643,7 +644,6 @@ static void waitboost(int fd, bool reset)
> >     igt_assert_lt(pre_freqs[CUR], pre_freqs[MAX]);
> >     igt_assert_eq(boost_freqs[CUR], boost_freqs[BOOST]);
> >     igt_assert_lt(post_freqs[CUR], post_freqs[MAX]);
> > -
> >   }
> >   
> >   static void pm_rps_exit_handler(int sig)
> > @@ -656,6 +656,7 @@ static void pm_rps_exit_handler(int sig)
> >     writeval(sysfs_files[MAX].filp, origfreqs[MAX]);
> >     }
> >   
> > +   writeval(sysfs_files[BOOST].filp, origfreqs[BOOST]);
> 
> We are not changing boost_freq in current patch. Is this planned in new 
> testcase?
> Please defer this change until then.
Will remove!

Thanks for review!
Radek
> >     load_helper_deinit();
> >     close(drm_fd);
> >   }
> 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH igt 5/5] igt/gem_flink_race: Limit name subtest to 5s

[Szwichtenberg, Radoslaw]

On Mon, 2017-09-11 at 09:56 +0100, Chris Wilson wrote:
> At present, we try to do 1,000,000 cycles, which may be a reasonable
> estimate for detecting the race, takes 6 minutes in practice on bxt on a
> good day (as it spends more time doing rpm suspend/resume than actual work,
> and that accounts for more of the relative difference in performance
> between bxt and big core than the difference in clocks+ipc).
> 
> An ideal solution would be to have a data-race detector in the kernel
> combined with a short test to exercise the different paths. Lacking the
> DRD, use a shorter test anyway. 5s is chosen simply on the basis that
> the other race subtest is also run over a 5s interval.
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 00/20] Add support for GuC-based SLPC

[Szwichtenberg, Radoslaw]

On Fri, 2017-09-01 at 12:55 +0530, Sagar Arun Kamble wrote:
> SLPC (Single Loop Power Controller) is a replacement for some host-based
> power management features. The SLPC implementation runs in GuC firmware.
> This series has been tested with SKL/APL/KBL GuC firmware v9 and v10
> which are yet to be released on 01.org.
> 
> The graphics power management features in SLPC are called GTPERF,
> BALANCER, and DCC.
> 1. GTPERF is a combination of DFPS (Dynamic FPS) and Turbo. DFPS adjusts
> requested graphics frequency to maintain target framerate. Turbo adjusts
> requested graphics frequency to maintain target GT busyness.
> 2. BALANCER adjusts balance between power budgets for IA and GT in power
> limited scenarios.
> 3. DCC (Duty Cycle Control) adjusts requested graphics frequency and stalls
> guc-scheduler to maintain actual graphics frequency in efficient range.
> 
> This series activates GTPERF Turbo and BALANCER in GuC SLPC.
> Patch to enable SLPC by default on platforms having support is removed
> from this series as there are following new changes to be added in future
> before we enable GuC/SLPC by default:
> 1. Link waitboost with SLPC.
> 2. Handle CPG as part of SLPC.
> 3. IA p-state logic update with GuC submission.
> 
> In order to enable CI/PnP testing of SLPC and to avoid frequent
> rebase, this series should be safe for merge with feature in disabled
> state.
> 
> v2: Addressed review comments on v1. Removed patch to enable SLPC by
> default.
> 
> v3: Addressed WARNING in igt@drv_module_reload_basic flagged by trybot BAT.
> Added change for sanitizing GT PM during reset. Added separate patch
> for sysfs interface to know HW requested frequency. Also, earlier
> patches did not go as series hence were not correctly picked up by BAT.
> 
> v4: Changes to multiple patches. CI BAT is passing. Performance run on SKL
> GT2 done and shows perf at parity with Host Turbo. For BXT, SLPC
> improves performance when GuC is enabled compared to Host Turbo.
> This series keeps only support of 9.18 firmware for better readability.
> If needed, other SLPC interfaces for different GuC version will be
> added later.
> 
> v5: This series incorporates feedback from code reviews on earlier series
> and adds following new changes:
>   1. More changes for separation of RPS and RC6 handling for Gen9.
>   2. Tied up SLPC enabling with GuC load/GuC submission sequence.
>   3. SLPC structures are defined explicitly for event input/output.
>   4. Definition of SLPC parameter control and task control functions
>      agnostic to the underlying param definitions as they might
>      change with GuC versions and prepared helpers for common tasks.
>   5. Transition of i915 overrides done through host to guc events
>      to shared data and single reset event.
>   6. Handling SLPC status post reset through shared memory.
>   7. Derived helpers for setting frequency limits.
>   8. Removed sysfs interface to know RPNSWREQ as it is available in
>      debugfs interface i915_frequency_info.
>   9. Simple igt test to verify SLPC configuration by i915 in various
>      driver scenarios is prepared.
> 
> v6: This series adds following new changes:
>   1. Updated intel_guc_send for SLPC to receive output data from GuC.
>   2. Added task overrides and min frequency overrides in intel_slpc_init.
>      min frequency is set to Rpe.
>   3. New debugfs interface added to set/unset/read SLPC parameters
>      other than tasks and frequencies. SLPC reset post parameter update
>      added.
>   4. SLPC parameters persist as part of i915-GuC shared data hence not
>      overriding frequency limits while re-enabling SLPC.
>   5. Other minor fixes to clear pm_rps_events, clflush the shared data.
> 
> v7: This series adds following new changes:
>   1. Reordered patches. SLPC communication interfaces (structures and
>      functions) are pulled into patches earlier in the series.
>   2. Eliminated dependency on i915.enable_slpc at various functions where
>      rps_enabled is available.
>   3. s/i915_ggtt_offset/guc_ggtt_offset and sanitization of parameter
>      in intel_uc_sanitize_options.
> 
> v8: Activated Balancer. Changed prototype of SLPC functions to accept
> struct intel_slpc as parameter instead of drm_i915_private.
> 
> VIZ-6889, VIZ-6890
> 
> Cc: Chris Wilson 
> Cc: Daniel Vetter 
> Cc: Beuchat, Marc 
> Cc: Radoslaw Szwichtenberg 
> Cc: Jeff McGee 
> Cc: Arkadiusz Hiler 
> Cc: Oscar Mateo 
> Cc: Michał Winiarski 

I did enable SLPC on my machine and looks like everything is working fine. I
will be spending more time reviewing whole series and also running some tests on
my KBL to see if there are no functional problems.

-Radek
Acked-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.fre

Re: [Intel-gfx] [PATCH i-g-t v3] tests/gem_flink_basic: Add documentation for subtests

[Szwichtenberg, Radoslaw]

On Thu, 2017-09-14 at 11:09 -0700, Vinay Belgaumkar wrote:
> Added the missing IGT_TEST_DESCRIPTION and some subtest
> descriptions.
> 
> v2: Removed duplication, addressed comments, cc'd test author
> 
> v3: Only comment abstract code, change some igt_info to igt_debug.
> Changed description to reflect this is a patch, not an RFC.
> 
> Cc: Michał Winiarski 
> Cc: Eric Anholt 
> Cc: Arkadiusz Hiler 
> Cc: Daniel Vetter 
> 
> Signed-off-by: Vinay Belgaumkar 

LGTM
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v3] tests/gem_flink_basic: Add documentation for subtests

[Szwichtenberg, Radoslaw]

On Fri, 2017-09-15 at 07:34 +, Szwichtenberg, Radoslaw wrote:
> On Thu, 2017-09-14 at 11:09 -0700, Vinay Belgaumkar wrote:
> > Added the missing IGT_TEST_DESCRIPTION and some subtest
> > descriptions.
> > 
> > v2: Removed duplication, addressed comments, cc'd test author
> > 
> > v3: Only comment abstract code, change some igt_info to igt_debug.
> > Changed description to reflect this is a patch, not an RFC.
> > 
> > Cc: Michał Winiarski 
> > Cc: Eric Anholt 
> > Cc: Arkadiusz Hiler 
> > Cc: Daniel Vetter 
> > 
> > Signed-off-by: Vinay Belgaumkar 
> 
> LGTM
> Reviewed-by: Radoslaw Szwichtenberg 
> 
Maybe just one minor with the comment style - it would be good to make it
consistent across whole file. Let's start comment with a capital letter and
finish it with a dot.

Still - my r-b is yours :)
-Radek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/31] drm/i915: Separate RPS and RC6 handling for BDW

[Szwichtenberg, Radoslaw]

On Tue, 2017-09-19 at 23:11 +0530, Sagar Arun Kamble wrote:
> This patch separates RC6 and RPS enabling for BDW.
> RC6/RPS Disabling are handled through gen6 functions.
> 
> Cc: Imre Deak 
> Cc: Chris Wilson 
> Signed-off-by: Sagar Arun Kamble 
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 27 +++
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f78a1e8..6de69ae 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6613,7 +6613,7 @@ static void gen9_enable_rc6(struct drm_i915_private
> *dev_priv)
>   intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }
>  
> -static void gen8_enable_rps(struct drm_i915_private *dev_priv)
> +static void gen8_enable_rc6(struct drm_i915_private *dev_priv)
>  {
>   struct intel_engine_cs *engine;
>   enum intel_engine_id id;
> @@ -6645,16 +6645,18 @@ static void gen8_enable_rps(struct drm_i915_private
> *dev_priv)
>   if (intel_enable_rc6() & INTEL_RC6_ENABLE)
>   rc6_mask = GEN6_RC_CTL_RC6_ENABLE;
>   intel_print_rc6_info(dev_priv, rc6_mask);
> - if (IS_BROADWELL(dev_priv))
> - I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> - GEN7_RC_CTL_TO_MODE |
> - rc6_mask);
> - else
> - I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> - GEN6_RC_CTL_EI_MODE(1) |
> - rc6_mask);
> + I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> + GEN7_RC_CTL_TO_MODE |
> + rc6_mask);
>  
> - /* 4 Program defaults and thresholds for RPS*/
> + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +}
> +
> +static void gen8_enable_rps(struct drm_i915_private *dev_priv)
> +{
> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> +
> + /* 1 Program defaults and thresholds for RPS*/
>   I915_WRITE(GEN6_RPNSWREQ,
>      HSW_FREQUENCY(dev_priv->rps.rp1_freq));
>   I915_WRITE(GEN6_RC_VIDEO_FREQ,
> @@ -6674,7 +6676,7 @@ static void gen8_enable_rps(struct drm_i915_private
> *dev_priv)
>  
>   I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
>  
> - /* 5: Enable RPS */
> + /* 2: Enable RPS */
>   I915_WRITE(GEN6_RP_CONTROL,
>      GEN6_RP_MEDIA_TURBO |
>      GEN6_RP_MEDIA_HW_NORMAL_MODE |
> @@ -6683,7 +6685,7 @@ static void gen8_enable_rps(struct drm_i915_private
> *dev_priv)
>      GEN6_RP_UP_BUSY_AVG |
>      GEN6_RP_DOWN_IDLE_AVG);
>  
> - /* 6: Ring frequency + overclocking (our driver does this later */
> + /* 3: Ring frequency + overclocking (our driver does this later */
This comment looks invalid (no overclocking done here). Also closing bracket
missing and maybe one white line to be removed :)

-Radek
>  
>   reset_rps(dev_priv, gen6_set_rps);
>  
> @@ -7976,6 +7978,7 @@ void intel_enable_gt_powersave(struct drm_i915_private
> *dev_priv)
>   if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv))
>   gen6_update_ring_freq(dev_priv);
>   } else if (IS_BROADWELL(dev_priv)) {
> + gen8_enable_rc6(dev_priv);
>   gen8_enable_rps(dev_priv);
>   gen6_update_ring_freq(dev_priv);
>   } else if (INTEL_GEN(dev_priv) >= 6) {
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 02/31] drm/i915: Separate RPS and RC6 handling for gen6+

[Szwichtenberg, Radoslaw]

On Tue, 2017-09-19 at 23:11 +0530, Sagar Arun Kamble wrote:
> This patch separates enable/disable of RC6 and RPS for gen6+
> platforms prior to VLV.
> 
> Cc: Imre Deak 
> Cc: Chris Wilson 
> Signed-off-by: Sagar Arun Kamble 

Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/31] drm/i915: Separate RPS and RC6 handling for VLV

[Szwichtenberg, Radoslaw]

On Tue, 2017-09-19 at 23:11 +0530, Sagar Arun Kamble wrote:
> This patch separates enable/disable of RC6 and RPS for VLV.
> 
> Cc: Imre Deak 
> Cc: Chris Wilson 
> Signed-off-by: Sagar Arun Kamble 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/31] drm/i915: Separate RPS and RC6 handling for BDW

[Szwichtenberg, Radoslaw]

On Wed, 2017-09-20 at 11:14 +, Szwichtenberg, Radoslaw wrote:
> On Tue, 2017-09-19 at 23:11 +0530, Sagar Arun Kamble wrote:
> > This patch separates RC6 and RPS enabling for BDW.
> > RC6/RPS Disabling are handled through gen6 functions.
> > 
> > Cc: Imre Deak 
> > Cc: Chris Wilson 
> > Signed-off-by: Sagar Arun Kamble 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 27 +++
> >  1 file changed, 15 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index f78a1e8..6de69ae 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -6613,7 +6613,7 @@ static void gen9_enable_rc6(struct drm_i915_private
> > *dev_priv)
> >     intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> >  
> > -static void gen8_enable_rps(struct drm_i915_private *dev_priv)
> > +static void gen8_enable_rc6(struct drm_i915_private *dev_priv)
> >  {
> >     struct intel_engine_cs *engine;
> >     enum intel_engine_id id;
> > @@ -6645,16 +6645,18 @@ static void gen8_enable_rps(struct drm_i915_private
> > *dev_priv)
> >     if (intel_enable_rc6() & INTEL_RC6_ENABLE)
> >     rc6_mask = GEN6_RC_CTL_RC6_ENABLE;
> >     intel_print_rc6_info(dev_priv, rc6_mask);
> > -   if (IS_BROADWELL(dev_priv))
> > -   I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> > -   GEN7_RC_CTL_TO_MODE |
> > -   rc6_mask);
> > -   else
> > -   I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> > -   GEN6_RC_CTL_EI_MODE(1) |
> > -   rc6_mask);
> > +   I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> > +   GEN7_RC_CTL_TO_MODE |
> > +   rc6_mask);
> >  
> > -   /* 4 Program defaults and thresholds for RPS*/
> > +   intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> > +}
> > +
> > +static void gen8_enable_rps(struct drm_i915_private *dev_priv)
> > +{
> > +   intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> > +
> > +   /* 1 Program defaults and thresholds for RPS*/
> >     I915_WRITE(GEN6_RPNSWREQ,
> >        HSW_FREQUENCY(dev_priv->rps.rp1_freq));
> >     I915_WRITE(GEN6_RC_VIDEO_FREQ,
> > @@ -6674,7 +6676,7 @@ static void gen8_enable_rps(struct drm_i915_private
> > *dev_priv)
> >  
> >     I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
> >  
> > -   /* 5: Enable RPS */
> > +   /* 2: Enable RPS */
> >     I915_WRITE(GEN6_RP_CONTROL,
> >        GEN6_RP_MEDIA_TURBO |
> >        GEN6_RP_MEDIA_HW_NORMAL_MODE |
> > @@ -6683,7 +6685,7 @@ static void gen8_enable_rps(struct drm_i915_private
> > *dev_priv)
> >        GEN6_RP_UP_BUSY_AVG |
> >        GEN6_RP_DOWN_IDLE_AVG);
> >  
> > -   /* 6: Ring frequency + overclocking (our driver does this later */
> > +   /* 3: Ring frequency + overclocking (our driver does this later */
> 
> This comment looks invalid (no overclocking done here). Also closing bracket
> missing and maybe one white line to be removed :)
> 
> -Radek
Forgot to mention - beside this comment all changes look good to me (you can
have my r-b after this is fixed).
> >  
> >     reset_rps(dev_priv, gen6_set_rps);
> >  
> > @@ -7976,6 +7978,7 @@ void intel_enable_gt_powersave(struct drm_i915_private
> > *dev_priv)
> >     if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv))
> >     gen6_update_ring_freq(dev_priv);
> >     } else if (IS_BROADWELL(dev_priv)) {
> > +   gen8_enable_rc6(dev_priv);
> >     gen8_enable_rps(dev_priv);
> >     gen6_update_ring_freq(dev_priv);
> >     } else if (INTEL_GEN(dev_priv) >= 6) {
> 
> ___
> 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] [PATCH 05/31] drm/i915: Separate RPS and RC6 handling for CHV

[Szwichtenberg, Radoslaw]

On Tue, 2017-09-19 at 23:11 +0530, Sagar Arun Kamble wrote:
> This patch separates enable/disable of RC6 and RPS for CHV.
> 
> Cc: Imre Deak 
> Cc: Chris Wilson 
> Signed-off-by: Sagar Arun Kamble 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 06/31] drm/i915: Name i915_runtime_pm structure in dev_priv as "rpm"

[Szwichtenberg, Radoslaw]

On Tue, 2017-09-19 at 23:11 +0530, Sagar Arun Kamble wrote:
> Will be using pm for state containing RPS/RC6 state in the next patch.
> 
> Cc: Imre Deak 
> Cc: Chris Wilson 
> Signed-off-by: Sagar Arun Kamble 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/31] drm/i915: Name structure in dev_priv that contains RPS/RC6 state as "pm"

[Szwichtenberg, Radoslaw]

On Tue, 2017-09-19 at 23:11 +0530, Sagar Arun Kamble wrote:
> Prepared substructure rps for RPS related state. autoenable_work and
> pcu_lock are used for RC6 hence they are defined outside rps structure.
> Renamed the RPS lock as pcu_lock.
> 
> Cc: Chris Wilson 
> Cc: Imre Deak 
> Signed-off-by: Sagar Arun Kamble 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 08/31] drm/i915: Rename intel_enable_rc6 to intel_rc6_enabled

[Szwichtenberg, Radoslaw]

On Tue, 2017-09-19 at 23:11 +0530, Sagar Arun Kamble wrote:
> This function gives the status of RC6, whether disabled or if
> enabled then which state. intel_enable_rc6 will be used for
> enabling RC6 in the next patch.
> 
> Cc: Chris Wilson 
> Cc: Imre Deak 
> Signed-off-by: Sagar Arun Kamble 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Don't accidentally increase the frequency in handling DOWN rps

[Szwichtenberg, Radoslaw]

On Fri, 2017-02-10 at 15:03 +, Chris Wilson wrote:
> If we receive a DOWN_TIMEOUT rps interrupt, we respond by reducing the
> GPU clocks significantly. Before we do, double check that the frequency
> we pick is actually a decrease.
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Enable fine-tuned RPS for cherryview

[Szwichtenberg, Radoslaw]

On Fri, 2017-02-10 at 15:03 +, Chris Wilson wrote:
> When the RPS tuning was applied to Baytrail, in commit 8fb55197e64d
> ("drm/i915: Agressive downclocking on Baytrail"), concern was given that
> it might cause Cherryview excess wakeups of the common power well.
> However, the static thresholds perform poorly for Kodi, and the GPU is
> unable to deliver the video frames on time. Enabling the dynamic, finer
> thresholds used on all other platforms (including Skylake and Broxton
> that also have the same multiple powerwell concerns) allows the GPU to
> pick a more appropriate frequency and not drop frames.
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Only apply the jump to the "efficient RPS" frequency on startup

[Szwichtenberg, Radoslaw]

On Fri, 2017-02-10 at 15:03 +, Chris Wilson wrote:
> Currently we apply the jump to rpe if we are below it and the GPU needs
> more power. For some GPUs, the rpe is 75% of the maximum range causing
> us to dramatically overshoot low power applications *and* unable to
> reach the low frequency that can most efficiently deliver their
> workload.
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Move the common RPS warnings to intel_set_rps()

[Szwichtenberg, Radoslaw]

On Fri, 2017-02-17 at 08:37 +, Chris Wilson wrote:
> Instead of having each back-end provide identical guards, just have a
> singular set in intel_set_rps() to verify that the caller is obeying the
> rules.
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Remove unrequired POSTING_READ from gen6_set_rps()

[Szwichtenberg, Radoslaw]

On Fri, 2017-02-17 at 08:31 +, Chris Wilson wrote:
> The uncached mmio is sufficient to queue the mmio writes without raising
> forcewake. The forced flush along with acquiring forcewake from the
> posting read is not required for adjusting the RPS frequency.
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Remove unneeded struct_mutex around rpm

[Szwichtenberg, Radoslaw]

On Sat, 2017-02-18 at 15:00 +, Chris Wilson wrote:
> We don't need struct_mutex for acquiring an rpm wakeref, and do not need
> to serialise those register read (it's the wrong mutex for those
> registers in any case). Begone!
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Prevent divide-by-zero in debugfs/i915_rps_boost_info

[Szwichtenberg, Radoslaw]

On Sat, 2017-02-18 at 11:27 +, Chris Wilson wrote:
> Either by chance, or by misread, the current evaluation interval may be
> zero. If that is the case, don't divide by it!
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Remove unrequired POSTING_READ from gen6_set_rps()

[Szwichtenberg, Radoslaw]

On Mon, 2017-02-20 at 09:47 +, Chris Wilson wrote:
> The uncached mmio is sufficient to queue the mmio writes without raising
> forcewake. The forced flush along with acquiring forcewake from the
> posting read is not required for adjusting the RPS frequency.
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Stop RPS as we adjust thresholds

[Szwichtenberg, Radoslaw]

On Mon, 2017-02-20 at 09:47 +, Chris Wilson wrote:
> Disable RPS by setting RP_CONTROL to 0, remembering its earlier value.
> Then adjust the thresholds before re-enabling RP_CONTROL.
> 
> Signed-off-by: Chris Wilson 
> Cc: Mika Kuoppala 
> Cc: sta...@vger.kernel.org
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Store the requested frequency whilst RPS is disabled

[Szwichtenberg, Radoslaw]

On Mon, 2017-02-20 at 09:47 +, Chris Wilson wrote:
> If intel_set_rps() is called whilst the hw is disabled, just store the
> requested frequency (from the user) for application when we wake the hw
> up.
> 
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/7] drm/i915: Restart RPS using the same RP_CONTROL as from initialisation

[Szwichtenberg, Radoslaw]

On Mon, 2017-02-20 at 09:47 +, Chris Wilson wrote:
> During initialisation, we set different flags for different
> architectures - these should be preserved when we reload the RPS
> thresholds. If we use a mmio read, it will first ensure that the
> threshold registers are written before we apply the latch in RP_CONTROL.
> 
> Signed-off-by: Chris Wilson 
> Cc: Mika Kuoppala 
> Cc: sta...@vger.kernel.org
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH igt] lib: Check and report if a subtest triggers a new kernel taint

[Szwichtenberg, Radoslaw]

On Wed, 2017-11-29 at 12:40 +, Chris Wilson wrote:
> Quoting Chris Wilson (2017-11-29 12:30:23)
> > Checking for a tainted kernel is a convenient way to see if the test
> > generated a critical error such as a oops, or machine check.
> > 
> > Signed-off-by: Chris Wilson 
> > Cc: Daniel Vetter 
> > Cc: Radoslaw Szwichtenberg 
> > ---
> > diff --git a/lib/igt_kernel_taint.c b/lib/igt_kernel_taint.c
> > new file mode 100644
> > index ..86d9cd20
> > --- /dev/null
> > +++ b/lib/igt_kernel_taint.c
> > @@ -0,0 +1,95 @@
> > +/*
> > + * Copyright 2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > next
> > + * paragraph) shall be included in all copies or substantial portions of
> > the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include 
> > +#include 
> > +
> > +#include "igt.h"
> > +#include "igt_kernel_taint.h"
> > +#include "igt_sysfs.h"
> > +
> > +#define BIT(x) (1ul << (x))
> > +
> > +static const struct kernel_taint {
> > +   const char *msg;
> > +   unsigned int flags;
> > +} taints[] = {
> > +   { "Non-GPL module loaded" },
> > +   { "Forced module load" },
> > +   { "Unsafe SMP processor" },
> > +   { "Forced module unload" },
> > +   { "Machine Check Exception", TAINT_WARN },
> > +   { "Bad page detected", TAINT_ERROR },
> > +   { "Tainted by user request", TAINT_WARN },
> 
> Since unsafe modparams generate these and we are still using them
> extensively, we should probably ignore this one.
> 
> > +   { "System is on fire", TAINT_ERROR },
> > +   { "ACPI DSDT has been overridden by user" },
> > +   { "OOPS", TAINT_ERROR },
> > +   { "Staging driver loaded; are you mad?" },
> > +   { "Severe firmware bug workaround active", TAINT_WARN },
> > +   { "Out-of-tree module loaded" },
> > +   { "Unsigned module loaded" },
> > +   { "Soft-lockup detected", TAINT_WARN },
> > +   { "Kernel has been live patched" },
> > +};
> > +
> > +unsigned long igt_read_kernel_taint(void)
> 
> One thing I haven't checked is whether we can clear the kernel taints.
> At the moment, once we see an oops, we never report a second test
> generating another oops.
> -Chris

I guess that clearing kernel taints is not needed when you hit oops - you
probably should stop executing tests and reboot the machine, right?

Radek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 1/2] igt: Remove Android support

[Szwichtenberg, Radoslaw]

On Fri, 2017-11-24 at 17:17 +0200, Arkadiusz Hiler wrote:
> This patch gets rid of the Android support, deleting all the hacks and
> moving code around to the places it belongs.
> 
> Android build is not really maintained properly and rots rather fast.
> With recent push for Meson here and Android going for Soong it will only
> accelerate.
> 
> It's a good time to drop the illusion of providing any support.
> 
> Cc: Daniel Vetter 
> Cc: Kalyan Kondapally 
> Cc: Petri Latvala 
> Cc: Radoslaw Szwichtenberg 
> Signed-off-by: Arkadiusz Hiler 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC i-g-t] tests/gem_exec_basic: Documentation for subtests

[Szwichtenberg, Radoslaw]

On Tue, 2017-08-08 at 15:09 -0700, Vinay Belgaumkar wrote:
> This is an RFC for adding documentation to IGT subtests. Each subtest can have
> something similar to a WHAT - explaining what the subtest actually does,
> and a WHY - which explains a use case, if applicable. Additionally,
> include comments for anything in the subtest code which can help
> explain HOW the test has been implemented. We don't actually need the WHAT
> and WHY tags in the documentation.
> 
> These comments will not be linked to gtkdoc as of now, since we do not have a
>  mechanism to link it to every subtest name.
> 
> Signed-off-by: Vinay Belgaumkar 
> Cc: Daniel Vetter 
> Cc: Petri Latvala 
> Cc: Chris Wilson 
> ---
>  tests/gem_exec_basic.c | 21 +
>  1 file changed, 21 insertions(+)
> 
> diff --git a/tests/gem_exec_basic.c b/tests/gem_exec_basic.c
> index 2f057ef..b1491cd 100644
> --- a/tests/gem_exec_basic.c
> +++ b/tests/gem_exec_basic.c
> @@ -25,6 +25,11 @@
>  
>  IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl rings.");
>  
> +/*
> +(WHAT) This subtest submits an empty batch to each ring and verifies
> +that it is executed successfully
> +(WHY) It validates that GT buffer submission mechanism is functional
> +*/
>  static void noop(int fd, unsigned ring)
>  {
>   uint32_t bbe = MI_BATCH_BUFFER_END;
> @@ -45,6 +50,11 @@ static void noop(int fd, unsigned ring)
>   gem_close(fd, exec.handle);
>  }
>  
> +/*
> +(WHAT) This subtest memory maps a buffer, marks it as read only, 
> +and submits it to each ring for execution.
> +(WHY) It helps us validate that the GT can execute read-only buffers
> +*/
>  static void readonly(int fd, unsigned ring)
>  {
>   uint32_t bbe = MI_BATCH_BUFFER_END;
> @@ -57,12 +67,15 @@ static void readonly(int fd, unsigned ring)
>   exec.handle = gem_create(fd, 4096);
>   gem_write(fd, exec.handle, 0, &bbe, sizeof(bbe));
>  
> + /* Memory map a buffer and use it as the execbuf to be submitted */
I am not convinced that comment describing what mmap does is needed. I think we
should not document what system calls do.
>   execbuf = mmap(NULL, 4096, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1,
> 0);
>   igt_assert(execbuf != NULL);
>  
>   execbuf->buffers_ptr = to_user_pointer(&exec);
>   execbuf->buffer_count = 1;
>   execbuf->flags = ring;
> +
> + /* Now mark the buffer as read-only */
>   igt_assert(mprotect(execbuf, 4096, PROT_READ) == 0);
Same as before - mprotect is a system call, this does not need any documentation
in my opinion. If there is a reason of making buffer read only that is not
obvious or not described in test description then it is worth stating why you
are marking it this way. 
>  
>   gem_execbuf(fd, execbuf);
> @@ -70,6 +83,10 @@ static void readonly(int fd, unsigned ring)
>   gem_close(fd, exec.handle);
>  }
>  
> +/*
> +(WHAT) Create a gtt mapped buffer and submit to the GPU.
> +(WHY) It ensures GPU can properly map and access GTT mapped buffers
> +*/
>  static void gtt(int fd, unsigned ring)
>  {
>   uint32_t bbe = MI_BATCH_BUFFER_END;
> @@ -82,6 +99,8 @@ static void gtt(int fd, unsigned ring)
>   handle = gem_create(fd, 4096);
>  
>   gem_set_domain(fd, handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +
> + /* Create a memory mapping through GTT */
>   execbuf = gem_mmap__gtt(fd, handle, 4096, PROT_WRITE);
>   exec = (struct drm_i915_gem_exec_object2 *)(execbuf + 1);
>   gem_close(fd, handle);
> @@ -108,6 +127,8 @@ igt_main
>   fd = drm_open_driver(DRIVER_INTEL);
>   igt_require_gem(fd);
>  
> + /* Start the hang detector process. Test will fail
> + if a GPU hang is detected during any subtest */
>   igt_fork_hang_detector(fd);
>   }
>  
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 08/11] tests/perf: load gt_boost_freq_mhz as max gt frequency

[Szwichtenberg, Radoslaw]

On Fri, 2017-08-04 at 12:20 +0100, Lionel Landwerlin wrote:
> We want the absolute max the hardware can do, not the max value
> set by a previous application/user.
> 
> Signed-off-by: Lionel Landwerlin 
> ---
>  tests/perf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/perf.c b/tests/perf.c
> index 6c0af32d..6be9f552 100644
> --- a/tests/perf.c
> +++ b/tests/perf.c
> @@ -1141,7 +1141,7 @@ static void
>  gt_frequency_range_save(void)
>  {
>   gt_min_freq_mhz_saved = sysfs_read("gt_min_freq_mhz");
> - gt_max_freq_mhz_saved = sysfs_read("gt_max_freq_mhz");
> + gt_max_freq_mhz_saved = sysfs_read("gt_boost_freq_mhz");
gt_boost_freq_mhz if I remember correctly also can be set by the user - it has
nothing to do with max hardware capabilities.

Radek
>  
>   gt_min_freq_mhz = gt_min_freq_mhz_saved;
>   gt_max_freq_mhz = gt_max_freq_mhz_saved;
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Boost GPU clocks if we miss the pageflip's vblank

[Szwichtenberg, Radoslaw]

On Fri, 2017-08-18 at 08:54 +0100, Chris Wilson wrote:
> Quoting Chris Wilson (2017-08-17 13:37:06)
> > If we miss the current vblank because the gpu was busy, that may cause a
> > jitter as the frame rate temporarily drops. We try to limit the impact
> > of this by then boosting the GPU clock to deliver the frame as quickly
> > as possible. Originally done in commit 6ad790c0f5ac ("drm/i915: Boost GPU
> > frequency if we detect outstanding pageflips") but was never forward
> > ported to atomic and finally dropped in commit fd3a40242e87 ("drm/i915:
> > Rip out legacy page_flip completion/irq handling").
> 
>  
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=102199
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102199
> > Signed-off-by: Chris Wilson 
> > Cc: Maarten Lankhorst 
> > Cc: Ville Syrjälä 
> > Cc: Daniel Vetter 
> 
> Tested-by: Lyude Paul 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] pm_rps: Extended testcases with checking PMINTRMSK register value

[Szwichtenberg, Radoslaw]

On Tue, 2017-08-22 at 01:31 +0300, Arkadiusz Hiler wrote:
> On Mon, Aug 21, 2017 at 09:39:24PM +0200, Daniel Vetter wrote:
> > On Mon, Aug 21, 2017 at 11:21:49AM +0100, Chris Wilson wrote:
> > > Quoting Chris Wilson (2017-08-21 10:53:36)
> > > > Quoting Arkadiusz Hiler (2017-08-21 10:42:25)
> > > > > On Mon, Aug 21, 2017 at 08:05:58AM +, Dec, Katarzyna wrote:
> > > > > > I understand we do not want to check registers in IGT tests. What
> > > > > > about reading interrupt masks from debugfs (i915_frequency_info).
> > > > > 
> > > > > Hey Kasia
> > > > > 
> > > > > It would be pretty much the same thing, but instead of us reading the
> > > > > PMINTRMASK directly we would ask the kernel to do that on our behalf.
> > > > > 
> > > > > That would just hide register read, not get rid of it.
> > > > > 
> > > > > 
> > > > > I think you are missing the point. The idea is that we do not want to
> > > > > test details of in-kernel implementation, not ban the register reads
> > > > > completely.
> > > > > 
> > > > > Reading register directly, especially just to make sure that the
> > > > > kernel
> > > > > set something correctly is a good indicator that we are trying to do
> > > > > just that - test the internal details.
> > > > > 
> > > > > > Would that be better approach? You guys suggested to get interested
> > > > > > in
> > > > > > kselftests for having such checks, but I am afraid that it could be
> > > > > > too much job and we have too few hands to work.
> > > > > 
> > > > > How much of an effort would the kselftest be, since it seems that you
> > > > > did some
> > > > > investigation already?
> > > > 
> > > > It doesn't even require a whole selftest, just something like
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 448e71af4772..e83b67fe0354 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -7733,7 +7733,8 @@ void intel_suspend_gt_powersave(struct
> > > > drm_i915_private *dev_priv)
> > > > if (cancel_delayed_work_sync(&dev_priv->rps.autoenable_work))
> > > > intel_runtime_pm_put(dev_priv);
> > > >  
> > > > -   /* gen6_rps_idle() will be called later to disable interrupts */
> > > > +   WARN_ON(I915_READ(GEN6_PMINTRMSK) !=
> > > > +   gen6_sanitize_rps_pm_mask(dev_priv, ~0));
> > > >  }
> > > 
> > > Wrong spot. We actually need a call from
> > > intel_runtime_pm_disable_interrupts.
> > 
> > Yeah, for consistency checks which are very closely tied to the
> > implementation we tend to sprinkle WARN_ON all over the place. In some
> > cases those checks are too expensive for production, then we add a
> > compile-time-option to hide them (e.g. GEM_BUG_ON).
> > 
> > I chatted with Radek, and if I understand things correctly, the main value
> > you derive from these is making sure a frankenstein port to an older
> > kernel doesn't miss or miss-apply any critical patches. In-kernel
> > consistency checks unfortunately don't really help with that, but we
> > heavily rely on these for validation.
> 
> Having that stated on the mailing list from the beginning (e.g. in the
> commit message or as one of the first replies) would help directing the
> whole discussion on the right track and make us understand your needs
> better.
> 
> I agree with Daniel's earlier statement that we should be very
> (over)verbose about the changes we are making and purpose they are
> serving.
> 
> > There's also examples of this (and again, they're very important) outside
> > of i915, like kasan, lockdep (and maybe we'll even get kmemleak somehow
> > integrated into CI/igt eventually).
> > 
> > So still no idea what would be the best suggestion here for your team.
> 
> Kasia and Radek, can you elaborate a little more on the "frankenstein
> port" and your use cases for such tests?
> 
> How is that comparable to backports to stable/LTS kernel branches?
> 
This test proposed by Kasia not only was used to find various regressions
(including performance ones) that were later fixed on upstream (and example
would be patch from Sagar: https://patchwork.kernel.org/patch/9527335/).

Additionally we do sometimes use older releases of kernel for which we do
backport some of the latest changes (on need basis). As this test verifies
whether masking is done as we expect it to be done it allows us to ensure that
during forklift/cherrypicking or any other process any of the required patches
were not missed.

I believe that proposed changes are not introducing any overhead or information
that is not really usefull for developers/test runners. Also providing support
for all previous and future platforms should not be an issue in this case.
Information about masking proved multiple times to be usefull when looking for
root cause of issues with rps we were facing. Fixes for all these issues (if
were still applicable to upstream code) were sent upstream (I think mostly by
Sagar). I th

Re: [Intel-gfx] [PATCH i-g-t] pm_rps: Extended testcases with checking PMINTRMSK register value

[Szwichtenberg, Radoslaw]

On Tue, 2017-08-22 at 13:33 +0100, Chris Wilson wrote:
> Quoting Szwichtenberg, Radoslaw (2017-08-22 12:56:00)
> > On Tue, 2017-08-22 at 01:31 +0300, Arkadiusz Hiler wrote:
> > > On Mon, Aug 21, 2017 at 09:39:24PM +0200, Daniel Vetter wrote:
> > > > On Mon, Aug 21, 2017 at 11:21:49AM +0100, Chris Wilson wrote:
> > > > > Quoting Chris Wilson (2017-08-21 10:53:36)
> > > > > > Quoting Arkadiusz Hiler (2017-08-21 10:42:25)
> > > > > > > On Mon, Aug 21, 2017 at 08:05:58AM +, Dec, Katarzyna wrote:
> > > > > > > > I understand we do not want to check registers in IGT tests.
> > > > > > > > What
> > > > > > > > about reading interrupt masks from debugfs
> > > > > > > > (i915_frequency_info).
> > > > > > > 
> > > > > > > Hey Kasia
> > > > > > > 
> > > > > > > It would be pretty much the same thing, but instead of us reading
> > > > > > > the
> > > > > > > PMINTRMASK directly we would ask the kernel to do that on our
> > > > > > > behalf.
> > > > > > > 
> > > > > > > That would just hide register read, not get rid of it.
> > > > > > > 
> > > > > > > 
> > > > > > > I think you are missing the point. The idea is that we do not want
> > > > > > > to
> > > > > > > test details of in-kernel implementation, not ban the register
> > > > > > > reads
> > > > > > > completely.
> > > > > > > 
> > > > > > > Reading register directly, especially just to make sure that the
> > > > > > > kernel
> > > > > > > set something correctly is a good indicator that we are trying to
> > > > > > > do
> > > > > > > just that - test the internal details.
> > > > > > > 
> > > > > > > > Would that be better approach? You guys suggested to get
> > > > > > > > interested
> > > > > > > > in
> > > > > > > > kselftests for having such checks, but I am afraid that it could
> > > > > > > > be
> > > > > > > > too much job and we have too few hands to work.
> > > > > > > 
> > > > > > > How much of an effort would the kselftest be, since it seems that
> > > > > > > you
> > > > > > > did some
> > > > > > > investigation already?
> > > > > > 
> > > > > > It doesn't even require a whole selftest, just something like
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index 448e71af4772..e83b67fe0354 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -7733,7 +7733,8 @@ void intel_suspend_gt_powersave(struct
> > > > > > drm_i915_private *dev_priv)
> > > > > > if (cancel_delayed_work_sync(&dev_priv-
> > > > > > >rps.autoenable_work))
> > > > > > intel_runtime_pm_put(dev_priv);
> > > > > >  
> > > > > > -   /* gen6_rps_idle() will be called later to disable
> > > > > > interrupts */
> > > > > > +   WARN_ON(I915_READ(GEN6_PMINTRMSK) !=
> > > > > > +   gen6_sanitize_rps_pm_mask(dev_priv, ~0));
> > > > > >  }
> > > > > 
> > > > > Wrong spot. We actually need a call from
> > > > > intel_runtime_pm_disable_interrupts.
> > > > 
> > > > Yeah, for consistency checks which are very closely tied to the
> > > > implementation we tend to sprinkle WARN_ON all over the place. In some
> > > > cases those checks are too expensive for production, then we add a
> > > > compile-time-option to hide them (e.g. GEM_BUG_ON).
> > > > 
> > > > I chatted with Radek, and if I understand things correctly, the main
> > > > value
> > > > you derive from these is making sure a frankenstein port to an older
> > > > kernel doesn't miss or miss-apply any critical patches. In-kernel
> &

Re: [Intel-gfx] [PATCH i-g-t v4] pm_rps: Changes in waitboost scenario

[Szwichtenberg, Radoslaw]

On Mon, 2017-08-28 at 10:50 +0200, Katarzyna Dec wrote:
> CI is observing sporadical failures in pm_rps subtests.
> There are a couple of reasons. One of them is the fact that
> on gen6, gen7 and gen7.5, max frequency (as in the HW limit)
> is not set to RP0, but the value obtaind from PCODE (which
> may be different from RP0). Thus the test is operating under
> wrong assumptions (SOFTMAX == RP0 == BOOST which is simply
> not the case). Let's compare current frequency with BOOST
> frequency rather than SOFTMAX to get the test behaviour under control.
> In boost_freq function I set MAX freq to midium freqency, which ensures
> that we for sure reach BOOST frequency. This could help with failures
> with boost frequency failing to drop down.
> GPU reset needs to be modified so we are not dependent on kernel's low
> priority retire worker. Reset method was replaced by igt_force_gpu_reset()
> and in reset testcase we make sure that we can recover from hang.
> 
> v2: Commit message, simplified waiting for boost to finish, drop
> noisy whitespace cleanup.
> 
> v3: Removed reading from i915_rps_boost_info debugfs because it not
> the same on every kernel. Removed function waiting for boost.
> Instead of that I made sure we will reach in boost by setting MAX freq to
> fmid.
> 
> v4: Moved proposal with making test drm master to other patch
> 
> v5: Used igt_force_gpu_reset() to reset GPU. Modified "reset" testcase.
> 
> Cc: Chris Wilson 
> Cc: Jeff Mcgee 
> Cc: Petri Latvala 
> Cc: Jani Saarinen 
> Cc: Radoslaw Szwichtenberg 
> Signed-off-by: Katarzyna Dec 
> ---
>  tests/pm_rps.c | 63 +++
> ---
>  1 file changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> index f0455e78..a6c6f1eb 100644
> --- a/tests/pm_rps.c
> +++ b/tests/pm_rps.c
> @@ -50,6 +50,7 @@ enum {
>   RP0,
>   RP1,
>   RPn,
> + BOOST,
>   NUMFREQ
>  };
>  
> @@ -60,7 +61,7 @@ struct junk {
>   const char *mode;
>   FILE *filp;
>  } stuff[] = {
> - { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL },
> { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { NULL,
> NULL, NULL }
> + { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL },
> { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { "boost",
> "rb+", NULL }, { NULL, NULL, NULL }
>  };
>  
>  static int readval(FILE *filp)
> @@ -167,7 +168,7 @@ static void dump(const int *freqs)
>  }
>  
>  enum load {
> - LOW,
> + LOW = 0,
>   HIGH
>  };
>  
> @@ -185,9 +186,10 @@ static struct load_helper {
>  
>  static void load_helper_signal_handler(int sig)
>  {
> - if (sig == SIGUSR2)
> - lh.load = lh.load == LOW ? HIGH : LOW;
> - else
> + if (sig == SIGUSR2) {
> + lh.load = !lh.load;
> + igt_debug("Switching background load to %s\n", lh.load ?
> "high" : "low");
> + } else
>   lh.exit = true;
>  }
>  
> @@ -238,6 +240,7 @@ static void load_helper_run(enum load load)
>   return;
>   }
>  
> + lh.exit = false;
>   lh.load = load;
>  
>   igt_fork_helper(&lh.igt_proc) {
> @@ -263,6 +266,8 @@ static void load_helper_run(enum load load)
>   if (intel_gen(lh.devid) >= 6)
>   execbuf.flags = I915_EXEC_BLT;
>  
> + igt_debug("Applying %s load...\n", lh.load ? "high" : "low");
> +
>   while (!lh.exit) {
>   memset(&object, 0, sizeof(object));
>   object.handle = fences[val%3];
> @@ -296,6 +301,8 @@ static void load_helper_run(enum load load)
>   gem_close(drm_fd, fences[0]);
>   gem_close(drm_fd, fences[1]);
>   gem_close(drm_fd, fences[2]);
> +
> + igt_drop_caches_set(drm_fd, DROP_RETIRE);
>   }
>  }
>  
> @@ -553,38 +560,43 @@ static void stabilize_check(int *out)
>   igt_debug("Waited %d msec to stabilize cur\n", wait);
>  }
>  
> -static void reset_gpu(void)
> -{
> - int fd = drm_open_driver(DRIVER_INTEL);
> - igt_post_hang_ring(fd, igt_hang_ring(fd, I915_EXEC_DEFAULT));
> - close(fd);
> -}
> -
>  static void boost_freq(int fd, int *boost_freqs)
>  {
>   int64_t timeout = 1;
> - int ring = -1;
>   igt_spin_t *load;
> + unsigned int engine;
> + int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
>  
> - load = igt_spin_batch_new(fd, ring, 0);
> + fmid = get_hw_rounded_freq(fmid);
> + //set max freq to less then boost freq
Looks good to me.
Just one very minor comment - we do use two different comment styles, should we
instead use one? Do you think we should add any simple test descriptions above
the tests or these tests are easy to understand?
> + writeval(stuff[MAX].filp, fmid);
>  
> + /* put boost on the same engine as low load */
> + engine = I915_EXEC_RENDER;

Otherwise
Reviewed-by: Radoslaw Szwichtenberg https://lists

Re: [Intel-gfx] [PATCH i-g-t v2 2/3] docs: Add user and developer documentation about Chamelium support

[Szwichtenberg, Radoslaw]

On Mon, 2017-08-28 at 11:12 +0300, Paul Kocialkowski wrote:
> This introduces plain-text documentation about the Chamelium aimed at
> users who wish to deploy the platform, as well as developers who wish
> to work on improving IGT support for it.
> 
> Given the contents of this documentation, it felt more relevant to make
> it part of the tree instead of the API reference.
> 
> Signed-off-by: Paul Kocialkowski 
Read it twice and is pretty clear to me. Also no spelling mistakes.
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 1/5] Add support for subtest-specific documentation

[Szwichtenberg, Radoslaw]

On Fri, 2017-08-11 at 10:20 +0200, Daniel Vetter wrote:
> On Thu, Aug 10, 2017 at 01:26:47PM +0300, Petri Latvala wrote:
> > The current documentation for tests is limited to a single string per
> > test binary. This patch adds support for documenting individual
> > subtests.
> > 
> > The syntax for subtest documentation is:
> > 
> >    igt_document_subtest("Frob knobs to see if one of the "
> > "crossbeams will go out of skew on the "
> > "treadle.\n");
> >    igt_subtest("knob-frobbing-askew")
> >  test_frob();
> > 
> > or with a format string:
> > 
> >   for_example_loop(e) {
> > igt_document_subtest_f("Frob %s to see if one of the "
> >    "crossbeams will go out of skew on the "
> >    "treadle.\n", e->readable_name);
> > igt_subtest_f("%s-frob-askew", e->name)
> >   test_frob(e);
> >   }
> 
> I'm not sold on this layout at all. Everywhere else where we do in-line
> code documentation it is through specially formatted comments. That gives
> you a lot of freedom for plain-text layout, in combination with some mild
> markup (gtk-doc for igt and rst for kernel) that result in docs which both
> look good in the code and look good rendered.
> 
> This here essentially restricts you to one-liners, and a one-liner can't
> really explain what a more complext test does.
I second that. For many cases one-liner will do - but these more complicated
cases are really worth the effort when documenting.
> 
> If we imagine what e.g. Paulo's test documentation in
> kms_frontbuffer_tracking.c looks like, it'll be bad.
> 
> I thought the test documentation that Thomas Wood worked on (no idea about
> status) would give us the full power and expressiveness of gtkdoc, but for
> binaries. I think that's what we want.
> 
> Then for testcases I think we again want to follow the inline
> documentation style, perhaps with something like the below:
> 
> 
>   /**
>* IGT-Subtest: tests some stuff
>*
>* Longer explanation of test approach and result evalution.
>*
>* Maybe over multiple paragraphs with headlines like CAVEATS, or
>* explaining hw bugs and stuff
>*/
>   igt_subtest("bla-bla-subtest")
> 
> 
> There's also the question of how to associate a given doc entry with more
> the multiple subtest names that igt_subtest_f can generate, but I guess
> that should be solveable.
I don't think its even worth having same text with just single words changed
generated for every subtest if test name describes the difference (and I guess
in many cases that is what we have now). It would be good to document such tests
in groups.

Thanks,
Radek
> 
> Any, in my opinion documentation has to look pleasing, both in code and
> rendered, otherwise people will not look at it, not write it and not
> update it. Or worse, they stick to writing full comments, because that's
> the only thing that allows them to express what they want to document.
> 
> We need something much better imo than this patch series from that pov.
> 
> Thanks, Daniel
> 
> > The documentation cannot be extracted from just comments, because
> > associating them with the correct subtest name will then require doing
> > pattern matching in the documentation generator, for subtests where
> > the name is generated at runtime using igt_subtest_f.
> > 
> > v2: Rebase, change function name in commit message to match code
> > 
> > v3: Fix current documentation string tracking, add missing va_end (Vinay)
> > 
> > v4: Fix brainfart in __igt_run_subtest
> > 
> > Signed-off-by: Petri Latvala 
> > Acked-by: Leo Li 
> > Acked-by: Arkadiusz Hiler 
> > ---
> >  lib/igt_aux.c  |   8 ++--
> >  lib/igt_core.c | 149 +-
> > ---
> >  lib/igt_core.h |   6 ++-
> >  3 files changed, 128 insertions(+), 35 deletions(-)
> > 
> > diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> > index f428f15..d56f41f 100644
> > --- a/lib/igt_aux.c
> > +++ b/lib/igt_aux.c
> > @@ -311,7 +311,7 @@ static void sig_handler(int i)
> >   */
> >  void igt_fork_signal_helper(void)
> >  {
> > -   if (igt_only_list_subtests())
> > +   if (igt_only_collect_data())
> >     return;
> >  
> >     /* We pick SIGCONT as it is a "safe" signal - if we send SIGCONT to
> > @@ -344,7 +344,7 @@ void igt_fork_signal_helper(void)
> >   */
> >  void igt_stop_signal_helper(void)
> >  {
> > -   if (igt_only_list_subtests())
> > +   if (igt_only_collect_data())
> >     return;
> >  
> >     igt_stop_helper(&signal_helper);
> > @@ -375,7 +375,7 @@ static void __attribute__((noreturn))
> > shrink_helper_process(int fd, pid_t pid)
> >   */
> >  void igt_fork_shrink_helper(int drm_fd)
> >  {
> > -   assert(!igt_only_list_subtests());
> > +   assert(!igt_only_collect_data());
> >     igt_require(igt_drop_caches_has(drm_fd, DROP_SHRINK_ALL));
> >     igt_fork_helper(&shrink_helper)
> >     shrink_helper_process(drm_

Re: [Intel-gfx] [PATCH i-g-t 02/12] build: Nuke #ifdef HAVE_CONFIG_H cargo-cult

[Szwichtenberg, Radoslaw]

On Mon, 2017-09-04 at 12:27 +0300, Arkadiusz Hiler wrote:
> On Mon, Sep 04, 2017 at 10:45:19AM +0200, Daniel Vetter wrote:
> > On Mon, Sep 04, 2017 at 11:28:09AM +0300, Arkadiusz Hiler wrote:
> > > On Sat, Sep 02, 2017 at 07:03:56PM +0200, Daniel Vetter wrote:
> > > > We have it. Daniel Stone said it comes from the X11 transition to the
> > > > modular build.
> > > > 
> > > > Signed-off-by: Daniel Vetter 
> > > 
> > > It's also used for Android builds, which lack the generated config.h.
> > > 
> > > But is this even used nowadays by anyone?
We do use IGTs to test kernels used in Android. We do not use the latest version
of IGT though as we are using one of older kernel versions.
> > > 
> > > How we are going to deal with the Android.mk with the transition to
> > > meson? Can we just drop them?
> > > 
> > > Android, in recent releases, is shifting from Makefiles to Soong.
> > > anyway. 
> > 
> > Looking at recent igt patches, we've stuffed all the fancy stuff (e.g.
> > chamelium) into Makefile.am, not Makefile.sources. And no one updated the
> > android build.
We dont use/run chamelium tests - I guess we don't use any fancy stuff ;)
> > 
> > Judging from that, I think the android build is already firmly in bitrot
> > territory :-/
I do have automated build for latest IGTs on android N and so far all builds are
still passing.
> > 
> > So yeah the plan would be to just leave it there, until someone cares
> > more. From a few googles it looks like meson might be able to
> > cross-compile into an android environment, but it won't be able to get
> > integrated into the android image build.
> 
> I am actually leaning towards getting rid of it completely from the
> tree, instead of making the illusion that Android is supported in any
> way.
> 
> If someone will care and pick up the maintenance, this should be easy
> enough to dig up from the history.
> 
Is there no plan at all to support Android long term?

Thanks,
Radek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] pm_rps: [RFC] RPS tests documentation update

[Szwichtenberg, Radoslaw]

On Thu, 2017-09-07 at 14:15 +0200, Katarzyna Dec wrote:
> Added comments in tricky places for better feature understanding.
> Added IGT_TEST_DESCRIPTION and short description for non-obvious
> subtests.
> Changed name of 'magic' checkit() function to something meaningfull.
> Changed junk struct and stuff array names.
> Made some minor coding style changes.
> 
> Cc: Vinay Belgaumkar 
> Cc: Petri Latvala 
> Cc: Arkadiusz Hiler 
> Cc: Daniel Vetter 
> 
> Signed-off: Katarzyna Dec 
Acked-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] pm_rps: [RFC] RPS tests documentation update

[Szwichtenberg, Radoslaw]

On Thu, 2017-09-07 at 11:28 -0700, Belgaumkar, Vinay wrote:
> 
> On 9/7/2017 5:15 AM, Katarzyna Dec wrote:
> > Added comments in tricky places for better feature understanding.
> > Added IGT_TEST_DESCRIPTION and short description for non-obvious
> > subtests.
> > Changed name of 'magic' checkit() function to something meaningfull.
> > Changed junk struct and stuff array names.
> > Made some minor coding style changes.
> > 
> > Cc: Vinay Belgaumkar 
> > Cc: Petri Latvala 
> > Cc: Arkadiusz Hiler 
> > Cc: Daniel Vetter 
> > 
> > Signed-off: Katarzyna Dec 
> > ---
> >  tests/pm_rps.c | 108 ++--
> > -
> >  1 file changed, 64 insertions(+), 44 deletions(-)
> > 
> > diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> > index e79f0ea7..1eeb6c6a 100644
> > --- a/tests/pm_rps.c
> > +++ b/tests/pm_rps.c
> > @@ -40,6 +40,8 @@
> > 
> >  #include "intel_bufmgr.h"
> > 
> > +IGT_TEST_DESCRIPTION("Render P-States tests - verify GPU frequency
> > changes");
> > +
> >  static int drm_fd;
> > 
> >  static const char sysfs_base_path[] =
> > "/sys/class/drm/card%d/gt_%s_freq_mhz";
> > @@ -56,12 +58,19 @@ enum {
> > 
> >  static int origfreqs[NUMFREQ];
> > 
> > -struct junk {
> > +struct sysfs_file {
> >     const char *name;
> >     const char *mode;
> >     FILE *filp;
> > -} stuff[] = {
> > -   { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL
> > }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, {
> > "boost", "rb+", NULL }, { NULL, NULL, NULL }
> > +} sysfs_files[] = {
> > +   { "cur", "r", NULL },
> > +   { "min", "rb+", NULL },
> > +   { "max", "rb+", NULL },
> > +   { "RP0", "r", NULL },
> > +   { "RP1", "r", NULL },
> > +   { "RPn", "r", NULL },
> > +   { "boost", "rb+", NULL },
> > +   { NULL, NULL, NULL }
> >  };
> > 
> >  static int readval(FILE *filp)
> > @@ -81,7 +90,7 @@ static void read_freqs(int *freqs)
> >     int i;
> > 
> >     for (i = 0; i < NUMFREQ; i++)
> > -   freqs[i] = readval(stuff[i].filp);
> > +   freqs[i] = readval(sysfs_files[i].filp);
> >  }
> > 
> >  static void nsleep(unsigned long ns)
> > @@ -143,7 +152,7 @@ static int do_writeval(FILE *filp, int val, int lerrno,
> > bool readback_check)
> >  #define writeval_inval(filp, val) do_writeval(filp, val, EINVAL, true)
> >  #define writeval_nocheck(filp, val) do_writeval(filp, val, 0, false)
> > 
> > -static void checkit(const int *freqs)
> > +static void check_freq_constraints(const int *freqs)
> >  {
> >     igt_assert_lte(freqs[MIN], freqs[MAX]);
> >     igt_assert_lte(freqs[CUR], freqs[MAX]);
> > @@ -162,7 +171,7 @@ static void dump(const int *freqs)
> > 
> >     igt_debug("gt freq (MHz):");
> >     for (i = 0; i < NUMFREQ; i++)
> > -   igt_debug("  %s=%d", stuff[i].name, freqs[i]);
> > +   igt_debug("  %s=%d", sysfs_files[i].name, freqs[i]);
> > 
> >     igt_debug("\n");
> >  }
> > @@ -387,14 +396,18 @@ static int get_hw_rounded_freq(int target)
> >     idx = MAX;
> > 
> >     old_freq = freqs[idx];
> > -   writeval_nocheck(stuff[idx].filp, target);
> > +   writeval_nocheck(sysfs_files[idx].filp, target);
> >     read_freqs(freqs);
> >     ret = freqs[idx];
> > -   writeval_nocheck(stuff[idx].filp, old_freq);
> > +   writeval_nocheck(sysfs_files[idx].filp, old_freq);
> > 
> >     return ret;
> >  }
> > 
> > +/*
> > + * Modify softlimit MIN and MAX freqs to valid and invalid levels.
> > Depending
> > + * on subtest run different check after each modification.
> > + */
> >  static void min_max_config(void (*check)(void), bool load_gpu)
> >  {
> >     int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
> > @@ -411,78 +424,78 @@ static void min_max_config(void (*check)(void), bool
> > load_gpu)
> >     check();
> > 
> >     igt_debug("\nSet min=RPn and max=RP0...\n");
> > -   writeval(stuff[MIN].filp, origfreqs[RPn]);
> > -   writeval(stuff[MAX].filp, origfreqs[RP0]);
> > +   writeval(sysfs_files[MIN].filp, origfreqs[RPn]);
> > +   writeval(sysfs_files[MAX].filp, origfreqs[RP0]);
> >     if (load_gpu)
> >     do_load_gpu();
> >     check();
> > 
> >     igt_debug("\nIncrease min to midpoint...\n");
> > -   writeval(stuff[MIN].filp, fmid);
> > +   writeval(sysfs_files[MIN].filp, fmid);
> >     if (load_gpu)
> >     do_load_gpu();
> >     check();
> > 
> >     igt_debug("\nIncrease min to RP0...\n");
> > -   writeval(stuff[MIN].filp, origfreqs[RP0]);
> > +   writeval(sysfs_files[MIN].filp, origfreqs[RP0]);
> >     if (load_gpu)
> >     do_load_gpu();
> >     check();
> > 
> >     igt_debug("\nIncrease min above RP0 (invalid)...\n");
> > -   writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000);
> > +   writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0] + 1000);
> >     check();
> > 
> >     igt_debug("\nDecrease max to RPn (invalid)...\n");
> > -   writeval_inval(stuff[MAX].filp, origfreqs[RPn]);
> > +   writeval_inval(sysfs_files[MAX].filp, origfreqs[RPn]);
> >     check();
> > 
> >     igt_debug("\nDecr

Re: [Intel-gfx] [PATCH i-g-t 00/22] RFC: meson build system support

[Szwichtenberg, Radoslaw]

On Tue, 2017-09-05 at 14:36 +0200, Daniel Vetter wrote:
> Assuming we can get some consensus around this I'd like to merge it and
> polish the meson support in-tree, it's kinda growing into a bigger series
> already. And of course we need to keep autohell working for probably a
> fairly long time, at least for the tools that distro install.

As discussed on IRC, we may have to re-visit Android support in the future
(maybe even near future), but for now it is not really a concern.

Acked-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 01/20] drm/i915/debugfs: Create generic string tokenize function and update CRC control parsing

[Szwichtenberg, Radoslaw]

On Fri, 2017-09-01 at 12:55 +0530, Sagar Arun Kamble wrote:
> Input string parsing used in CRC control parameter parsing is generic
> and can be reused for other debugfs interfaces. Hence name it as
> buffer_tokenize instead of tieing to display_crc. Also fix the function
s/tieing/tying ?
> desciption for CRC control parsing that was misplaced at tokenize function.
s/desciption/description
> 
> Cc: Tomeu Vizoso 
> Signed-off-by: Sagar Arun Kamble 
Don't you think we could try to review and merge this change as separate commit
and not part of this series?

Some spelling mistakes, otherwise LGTM
Acked-by: Radoslaw Szwichtenberg 

-Radek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 02/20] drm/i915/gen9+: Separate RPS and RC6 handling

[Szwichtenberg, Radoslaw]

On Fri, 2017-09-01 at 12:55 +0530, Sagar Arun Kamble wrote:
> With GuC based SLPC, frequency control will be moved to GuC and Host will
> continue to control RC6 and Ring frequency setup. SLPC can be enabled in
> the GuC setup path and can happen in parallel in GuC with other i915 setup.
> Hence we can do away with deferred RPS enabling. This needs separate
> handling of RPS, RC6 and ring frequencies in driver flows. We can still use
> the *gt_powersave routines with separate status variables of RPS, RC6 and
> SLPC. With this patch, RC6 and ring frequencies setup(if applicable) can be
> tracked through rps.rc6_enabled and RPS through rps.rps_enabled.
> Also, Active RPS check in suspend flow is needed for platforms with RC6
> and RPS enabling/disabling coupled together. RPM suspend depends only on
> RC6 though. Hence Active RPS check is done only for non-Gen9 platforms.
> 
> v2: Changing parameter to dev_priv for IS_GEN9 and HAS_RUNTIME_PM and line
> spacing changes. (David)
> and commit message update for checkpatch issues.
> 
> v3: Rebase.
> 
> v4: Commit message update.
> 
> v5: Updated intel_enable_gt_powersave and intel_disable_gt_powersave
> routines with separated RPS and RC6 handling and rebase. Commit message
> update.(Sagar)
> 
> v6: Added comments at the definition of rc6_enabled.
> 
> v7: s/rps.enabled/rps.rps_enabled. With gen9 preproduction RPS disabling
> changes removed, updating rps_enabled in enable/disable_gt_powersave.
> Added checks for rc6_enabled and rps_enabled for gen9+ platforms.
> 
> Signed-off-by: Sagar Arun Kamble 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] tests/pm_rc6_residency: Add subtest to check RC6 suspend handling

[Szwichtenberg, Radoslaw]

On Mon, 2017-04-24 at 14:55 +0200, Ewelina Musial wrote:
> In some cases we observed that forcewake isn't kept after
> resume and then RC6 residency is not constant.
> 
> References: HSD#1804921797
> Cc: Arkadiusz Hiler 
> Cc: Michal Winiarski 
> Cc: Lukasz Fiedorowicz 
> Signed-off-by: Ewelina Musial 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH igt v2] igt/pm_rps: Always allocate spin[0]

[Szwichtenberg, Radoslaw]

On Tue, 2017-12-19 at 12:34 +, Chris Wilson wrote:
> Avoid having to test for spin[0] existing by starting the load-loop with
> it allocated.
> 
> v2: Preallocate the spin[1] as well for high load.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=104060
> Signed-off-by: Chris Wilson 
Reviewed-by: Radoslaw Szwichtenberg 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] XDC 2021: Registration & Call for Proposals now open!

[Szwichtenberg, Radoslaw]

Hello!

Registration & Call for Proposals are now open for XDC 2021, which will
take place on September 15-17, 2021. This year we will repeat as
virtual event.

https://indico.freedesktop.org/event/1/

As usual, the conference is free of charge and open to the general
public. If you plan on attending, please make sure to register as early
as possible!

In order to register as attendee, you will therefore need to register
via the XDC website. As XDC moved to a new Indico infrastructure, if
you previously registered on the XDC website, you need to create a new
account again.

https://indico.freedesktop.org/event/1/registrations/1/

In addition to registration, the CfP is now open for talks, workshops
and demos at XDC 2021. While any serious proposal will be gratefully
considered, topics of interest to X.Org and freedesktop.org developers
are encouraged. The program focus is on new development, ongoing
challenges and anything else that will spark discussions among
attendees in the hallway track.

We are open to talks across all layers of the graphics stack, from the
kernel to desktop environments / graphical applications and about how
to make things better for the developers who build them. Head to the
CfP page to learn more:

https://indico.freedesktop.org/event/1/abstracts/

The deadline for submissions is Sunday, 4 July 2021.

Last year we modified our Reimbursement Policy to accept speaker
expenses for X.Org virtual events like XDC 2021. Check it out here:

https://www.x.org/wiki/XorgFoundation/Policies/Reimbursement/

If you have any questions, please send me an email to
radoslaw.szwichtenb...@intel.com,  
adding on CC the X.org board (board
at foundation.x.org).

And don't forget, you can follow us on Twitter for all the latest
updates and to stay connected:


https://twitter.com/XOrgDevConf

Best,

Radek

P.S: a DNS redirection (xdc2021.x.org) is work in progress. Please use
the mentioned links for the moment.


Radosław Szwichtenberg
-
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173, 80-298 Gdansk
KRS 101882 - NIP 957-07-52-316

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] XDC 2020: Registration & Call for Proposals now open!

[Szwichtenberg, Radoslaw]

Hello!

Registration & Call for Proposals are now open for XDC 2020, which will
take place at the Gdańsk University of Technology in Gdańsk, Poland on 
September 16-18, 2020.

Thanks to LWN.net for hosting the website again this year!

https://xdc2020.x.org

As usual, the conference is free of charge and open to the general public. If 
you plan on attending, please make sure to register as early as possible! 
However, don't book any travel or hotel until the organization decides if we 
keep the conference as it is or there is any change. Please read this message 
on the website for more information:

https://xdc2020.x.org/event/9/page/78-covid-19

In order to register as attendee, you will therefore need to register via the 
XDC
website. However, as XDC is sharing the same Indico infrastructure as
LPC, if you previously registered on the LPC website
(linuxplumbersconference.org) or on the XDC 2019 website (xdc2019.x.org), then 
you already have an active account
and can use the same username & password to login!

https://xdc2020.x.org/event/9/registrations/7/

In addition to registration, the CfP is now open for talks, workshops
and demos at XDC 2020. While any serious proposal will be gratefully
considered, topics of interest to X.Org and freedesktop.org developers
are encouraged. The program focus is on new development, ongoing
challenges and anything else that will spark discussions among
attendees in the hallway track.

We are open to talks across all layers of the graphics stack, from the
kernel to desktop environments / graphical applications and about how
to make things better for the developers who build them. Head to the
CfP page to learn more: 

https://xdc2020.x.org/event/9/abstracts/

The deadline for submissions is Sunday, 5 July 2020.

Notice that the event may end up being postponed, converted to a fully online 
conference, or even a hybrid one (physical event + some remote talks). It 
depends on how COVID-19 situation evolves in the different countries and the 
restrictions we will have at that time.
Also, some people may decide to skip the physical conference to avoid any risk 
of infection. Because of that, please indicate in your talk submission if you 
prefer to give a remote talk in the case that XDC keeps being a physical event 
this year. Similarly, if you think that your talk makes no sense if XDC ends up 
being a fully-virtual conference, please indicate that in the notes of the talk 
submission.

If COVID-19 situation allows it, we are looking forward to seeing you in 
Gdańsk! If you have any questions, please send me an email to 
radoslaw.szwichtenb...@intel.com,  adding on CC the X.org board (board at 
foundation.x.org).

And don't forget, you can follow us on Twitter for all the latest
updates and to stay connected:

https://twitter.com/xdc2020

Best,

Radek


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx