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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 17 Sep 2013, Kamal Mostafa <kamal@canonical.com> wrote:
> 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...

I'm just offering my opinions; at the end of the day it's Daniel's call.

>> 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?

Personally I think we are accumulating too many parameters to begin
with. This is a "meta parameter" that makes all of our quirks an
interface to our driver. Something that would be non-trivial to change
or remove.

It opens quirks intended to some specific machines to *all* machines.

> 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.

>From a developer perspective, sure, I'd like that.

The flip side is that if setting a module parameter fixes things for the
advanced user, some of the bugs might go unreported. We wouldn't get the
feedback we need to make the driver work with the default settings. I
wouldn't like that.

I've already seen this with the i915.invert_brightness option; if
setting that fixes the problem, it can be hard to convince the reporter
to even send 'lspci -vmmnn' output!

>  - 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).

Again, how to convince those users to interact with us to fix the issues
for the rest?

>  - 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.)

FWIW, I don't much like that parameter either.

>> 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.

If this was debugfs, with zero guarantees on the API or forward
compatibility, I'd be all in. Perhaps that would be useful for some of
the cases, but not all.


Cheers,
Jani.


PS. One code comment inline below.

>
>  -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).");

There's no point in making the perm writable, since it's only read on
driver init time.

>> > +
>> >  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
>> 
>

-- 
Jani Nikula, Intel Open Source Technology Center