Re: [PATCH 8/8] drm/i915: Add drrs_interval module parameter

From: Daniel Vetter <daniel@ffwll.ch>
To: "Kannan, Vandana" <vandana.kannan@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 8/8] drm/i915: Add drrs_interval module parameter
Date: Mon, 15 Dec 2014 15:16:51 +0100	[thread overview]
Message-ID: <20141215141651.GZ27182@phenom.ffwll.local> (raw)
In-Reply-To: <548EBE24.3050600@intel.com>

On Mon, Dec 15, 2014 at 04:25:32PM +0530, Kannan, Vandana wrote:
> 
> 
> On 15-Dec-14 3:17 PM, Daniel Vetter wrote:
> >On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote:
> >>Adding i915 module parameter for setting drrs_interval. If this param is
> >>set to 0, then drrs is disabled. If changed in runtime, then the new interval
> >>value will be considered for scheduling the next drrs work.
> >>drrs_interval is set to 0 by default, i.e. DRRS is disabled by default.
> >
> >Nope, please don't hide power saving features behind module options by
> >default. New stuff must be enabled by default, otherwise it'll bitrot and
> >merging to upstream is fairly useless since we still have the rebase pain
> >(just spread out over more people) with none of the testing.
> ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second
> or let user set this delay at runtime through the same module param
> (excluding the disable feature if interval is 0 part) ?

Imo we should just set an optimal value (does vbt have any hints?).

> >Also such debug options need to be marked using the _debug variants to
> >make sure that people report issues and don't keep using them.
> >
> >Now talking about validation gets me to the next point: Do we have a basic
> >igt to make sure DRRS doesn't break right after merging? I don't think we
> >need the full-blown test setup like for psr/fbc, but a basic test to make
> >sure it enters/exit RR mode should be there.
> libdrm has vbltest which prints refresh rate..
> Apart from this, I'm adding downclock mode (if supported) in kms_flip.
> Shall I add a test which involves low RR trigger after idle time and then
> forces some activity on screen and switches back to high RR ?

kms_flip is already huge, probably better to start a new kms_drrs
testcase, similar to kms_fbc_crc (but without crc checks since that's not
ineteresting here). And yeah you should trigger RR entry and then check in
debugfs that it has indeed happened, then pageflip and check that we're
out of RR again. That should be enough to have a decent smoketest for
DRRS. We can add anything missing later on.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx