Re: [PATCH 1/2] drm/i915: Re-add enable_rc6 modparam

["Summers, Stuart" <stuart.summers@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4433 bytes --]

On Fri, 2019-05-17 at 09:17 -0700, Rodrigo Vivi wrote:
> On Thu, May 16, 2019 at 03:49:19PM +0000, Summers, Stuart wrote:
> > On Thu, 2019-05-16 at 18:42 +0300, Jani Nikula wrote:
> > > On Thu, 16 May 2019, "Summers, Stuart" <stuart.summers@intel.com>
> > > wrote:
> > > > On Thu, 2019-05-16 at 12:59 +0300, Jani Nikula wrote:
> > > > > On Tue, 14 May 2019, Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > wrote:
> > > > > > One possibility that just came to my mind now is, what if
> > > > > > we
> > > > > > make
> > > > > > this only for platforms that are still protected by
> > > > > > is_alpha_support=1
> > > > > > (soon becoming require_force_probe=1)
> > > > > 
> > > > > Please don't conflate alpha_support or force_probe with
> > > > > *anything*
> > > > > else.
> > > > > 
> > > > > > But this is just one side of the coin... when product is
> > > > > > out
> > > > > > there
> > > > > > and we want the user to debug the issue to see if it is a
> > > > > > RC6
> > > > > > bug
> > > > > > we have no way to verify that. :/
> > > > > 
> > > > > The problem is, if it works with rc6 disabled, it doesn't
> > > > > prove
> > > > > it's
> > > > > an
> > > > > rc6 bug either.
> 
> Well, RC6 is the main GT power gating. The issue could be on may
> other
> individual power gating, but if by disabling RC6 the issue is gone
> it is a very good indication that it is a GT-PM bug somewhere.
> 
> > > > 
> > > > Good point. I'm not saying we should enforce a process of
> > > > disabling
> > > > RC6
> > > > for the platform if enable_rc6=0 results in success. I'm just
> > > > saying
> > > > having the option is useful from a debug perspective. We will
> > > > still
> > > > need to do the appropriate full analysis, including the normal
> > > > code
> > > > review process on a pre-case basis when debug involves this
> > > > parameter.
> > > > But the parameter itself is still useful.
> > > 
> > > The trouble starts when users figure out that enable_rc6=0 works
> > > around
> > > a particular problem they have (likely by way of disabling
> > > runtime
> > > pm,
> > > not directly related to rc6). You could argue this is a good
> > > thing,
> > > but
> > > unfortunately we generally never hear from them again, and the
> > > root
> > > cause remains unsolved, with degraded user experience wrt power
> > > management.
> 
> This is indeed bad. We should probably be clear that by disabling
> that
> they are draining their power and probably killing battery life.

I could add a DRM_ERROR or even a GEM_WARN_ON if this is set with a
description similar to what you have if that's interesting.

> 
> > 
> > So I understand the reasoning here, and agree that isn't an ideal
> > situation. I'd also like a way to debug more efficiently. What did
> > you
> > think about the suggestion from Tvrtko to only apply on
> > CONFIG_DRM_I915_DEBUG?
> 
> if debug is on and parameter is set, right? Might be a good thing to
> avoid the abuse on the parameter.
> 
> > 
> > Or we could even wrap this entirely with a CONFIG_I915_DEBUG_PM -
> > although I'd like to suggest we still use the module parameter, we
> > could just use the config option to hide the modparam under normal
> > operation.
> 
> I think this looks more like you are enabling more logs and
> not that you are disabling... unless the plan is to go with
> this flag plus logs and traces around PM. Than I think it is
> a good idea.

I'm not precluding people from adding more logs here - and there will
at least be some additional logs indicating we are bypassing the PM
flows. But at least for this patch, the intention would be to add new
"debug capability", which in this case, means the addition of the new
module parameter, enable_rc6.

Really this is just an extra deterent for users, since they'll need to
custom build the kernel to get this - not something enabled by default
in one of the distros, for instance.

Thanks,
Stuart

> 
> Same rules needs to apply to  RC6, RPM, DC states,
> display power well management, psr, etc.
> 
> Thanks,
> Rodrigo.
> 
> > 
> > Thanks,
> > Stuart
> > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> 
> 
> 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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