Re: [Intel-gfx] [PATCH 15/19] drm/i915: i915.quirks_set/quirks_mask overrides dmi match

From: Kamal Mostafa <kamal@canonical.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>,
	intel-gfx@lists.freedesktop.org, stable@vger.kernel.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [Intel-gfx] [PATCH 15/19] drm/i915: i915.quirks_set/quirks_mask overrides dmi match
Date: Tue, 17 Sep 2013 09:42:08 -0700	[thread overview]
Message-ID: <1379436128.3371.198.camel@fourier> (raw)
In-Reply-To: <87k3inydie.fsf@intel.com>

[-- Attachment #1: Type: text/plain, Size: 4216 bytes --]

On Wed, 2013-09-11 at 11:21 +0300, Jani Nikula wrote:
> On Wed, 11 Sep 2013, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> > From: Kamal Mostafa <kamal@canonical.com>
> >
> > Boot params quirks_set and quirks_mask allow the user to enable or
> > inhibit any dmi-matched quirks, overriding the dmi match table.  Examples:
> >
> >   i915.quirks_set=0x2   - enables QUIRK_LVDS_SSC_DISABLE
> >   i915.quirks_set=0x8   - enables QUIRK_NO_PCH_PWM_ENABLE
> >   i915.quirks_mask=0x8  - disables QUIRK_NO_PCH_PWM_ENABLE
> >   i915.quirks_mask=0xFF - disables all quirks

Thanks for reviewing this, Jani.  I hope you're open to a little more
debate on the topic...

> While I admit this can be used to workaround issues with certain
> machines, this is a hack that exposes an implementation detail to the
> userspace, and carves it into the stone. Any user of it would have to
> look at the kernel source to make a smart choice of parameters; more
> likely forums would start filling with tips what to set.

All of the above is true of all boot params.  Why is that a problem?

Consider that if this quirks_set/mask feature were available:

 - Users and developers could trivially check whether any future machine
needs to be added to any of the existing dmi-match quirk tables.
Currently we have to build a test kernel and get the user to install it,
but this would allow us to just ask "Does i915.quirks_set=0x2 fix it?".
That feature alone makes me want quirks_set/mask.

 - We would unblock some black-screen-on-boot users who currently have
no other solution.  And we would unblock future users who face future
dmi/quirk-mismatch issues (of which I am sure there will be more).

 - The existing i915.invert_brightness override param would not have
been needed.  (It is worth nothing also that my previously proposed
i915.disable_pch_pwm override patch was rejected, even though its
implementation is identical to invert_brightness.)

> So NAK from me.
> 
> That said, I think we still have a few stones unturned wrt backlight and
> what BIOS does in UEFI mode.

I would be thrilled to see the UEFI/BIOS/backlight issue get fixed, but
in my opinion i915.quirks_set/mask would be very useful (a) anyway, and
(b) immediately.

 -Kamal

> 
> Cheers,
> Jani.
> 
> >
> > Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1eabca3..12607f2 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10043,8 +10043,19 @@ static struct intel_quirk intel_quirks[] = {
> >  	{ 0x0166, 0x1028, 0x058b, quirk_no_pcm_pwm_enable },
> >  };
> >  
> > +unsigned long i915_quirks_set __read_mostly = 0;
> > +module_param_named(quirks_set, i915_quirks_set, ulong, 0600);
> > +MODULE_PARM_DESC(quirks_set,
> > +		"Enable specified quirks bits.");
> > +
> > +unsigned long i915_quirks_mask __read_mostly = 0;
> > +module_param_named(quirks_mask, i915_quirks_mask, ulong, 0600);
> > +MODULE_PARM_DESC(quirks_mask,
> > +		"Disable specified quirks bits (override dmi match).");
> > +
> >  static void intel_init_quirks(struct drm_device *dev)
> >  {
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct pci_dev *d = dev->pdev;
> >  	int i;
> >  
> > @@ -10062,6 +10073,10 @@ static void intel_init_quirks(struct drm_device *dev)
> >  		if (dmi_check_system(*intel_dmi_quirks[i].dmi_id_list) != 0)
> >  			intel_dmi_quirks[i].hook(dev);
> >  	}
> > +
> > +	/* handle user-specified quirks overrides */
> > +	dev_priv->quirks |= i915_quirks_set;
> > +	dev_priv->quirks &= ~i915_quirks_mask;
> >  }
> >  
> >  /* Disable the VGA plane that we never use */
> > -- 
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]