Re: [PATCH 3/3] drm/i915: Get panel_type from OpRegion panel details

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

On Fri, 08 Apr 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Apr 08, 2016 at 05:50:06PM +0300, Jani Nikula wrote:
>> On Fri, 08 Apr 2016, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > We've had problems on several occasions with using the panel type
>> > from the VBT block 40. Usually it seems to be 2, which often
>> > doesn't give us the correct timings for the panel. After some
>> > more digging I found a way to get a panel type via the OpRegion
>> > SWSCI GBDA "Get Panel Details" method. Let's try to use it.
>> >
>> > The spec has this to say about the output:
>> > "Bits [15:8] - Panel Type
>> >  Bits contain the panel type user setting from CMOS
>> >  00h = Not Valid, use default Panel Type & Timings from VBT
>> >  01h - 0Fh = Panel Number"
>> >
>> > The other bits in the output don't look relevant for the problem at
>> > hand.
>> >
>> > The input is specified as:
>> > "Bits [31:4] - Reserved
>> >  Reserved (must be zero)
>> >  Bits [3:0] - Panel Number
>> >  These bits contain the sequential index of Panel, starting at 0 and
>> >  counting upwards from the first integrated Internal Flat-Panel Display
>> >  Encoder present, and then from the first external Display Encoder
>> >  (e.g., S/DVO-B then S/DVO-C) which supports Internal Flat-Panels.
>> >  0h - 0Fh = Panel number"
>> >
>> > For now I've just hardcoded the input panel number as 0. That would seem
>> > like a decent choise for LVDS. Not so sure about eDP when port != A.
>> 
>> Frankly, I didn't understand the point of the input parameter for
>> figuring out the output. The spec is written for someone who already
>> knows what they're doing...
>> 
>> >
>> > Cc: Rob Kramer <rob@solution-space.com>
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94825
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.h       |  5 +++++
>> >  drivers/gpu/drm/i915/intel_bios.c     | 13 ++++++++++---
>> >  drivers/gpu/drm/i915/intel_opregion.c | 13 +++++++++++++
>> >  3 files changed, 28 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> > index 53ace572b292..533263a7a12d 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -3421,6 +3421,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>> >  					 bool enable);
>> >  extern int intel_opregion_notify_adapter(struct drm_device *dev,
>> >  					 pci_power_t state);
>> > +extern int intel_opregion_get_panel_type(struct drm_device *dev);
>> >  #else
>> >  static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
>> >  static inline void intel_opregion_init(struct drm_device *dev) { return; }
>> > @@ -3436,6 +3437,10 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
>> >  {
>> >  	return 0;
>> >  }
>> > +static inline int intel_opregion_get_panel_type(struct drm_device *dev)
>> > +{
>> > +	return 0;
>> > +}
>> >  #endif
>> >  
>> >  /* intel_acpi.c */
>> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> > index d9568be4b33b..718e49931b5f 100644
>> > --- a/drivers/gpu/drm/i915/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/intel_bios.c
>> > @@ -205,16 +205,23 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>> >  	struct drm_display_mode *panel_fixed_mode;
>> >  	int panel_type;
>> >  	int drrs_mode;
>> > +	int ret;
>> >  
>> >  	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
>> >  	if (!lvds_options)
>> >  		return;
>> >  
>> >  	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
>> > -	if (lvds_options->panel_type > 0xf)
>> > -		return;
>> >  
>> > -	panel_type = lvds_options->panel_type;
>> > +	ret = intel_opregion_get_panel_type(dev_priv->dev);
>> > +	if (ret >= 0x1 && ret <= 0xf) {
>> 
>> Undecided whether I'd like to have this check within
>> intel_opregion_get_panel_type(). Just make that return 0 for "use vbt"
>> (and CONFIG_ACPI=n), and valid stuff otherwise? *shrug*
>
> We don't compile intel_opregion.c when ACPI==n.

Yeah I meant the static inline stub in i915_drv.h for ACPI=n. Which does
return 0.

BR,
Jani.


>
>> 
>> > +		panel_type = ret;
>> > +	} else {
>> > +		if (lvds_options->panel_type > 0xf)
>> > +			return;
>> > +		panel_type = lvds_options->panel_type;
>> > +	}
>> > +
>> 
>> If this actually works, I'd like to have some debug logging for pretty
>> much all the code paths here.
>
> Yeah that would be nice for figuring out where the information came 
> from or why it was rejected.
>
>> 
>> Other than that, seems fine.
>> 
>> BR,
>> Jani.
>> 
>> >  	dev_priv->vbt.panel_type = panel_type;
>> >  
>> >  	drrs_mode = (lvds_options->dps_panel_type_bits
>> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> > index c15718b4862a..5e17fa5dc869 100644
>> > --- a/drivers/gpu/drm/i915/intel_opregion.c
>> > +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> > @@ -1024,3 +1024,16 @@ err_out:
>> >  	memunmap(base);
>> >  	return err;
>> >  }
>> > +
>> > +int
>> > +intel_opregion_get_panel_type(struct drm_device *dev)
>> > +{
>> > +	u32 panel_details;
>> > +	int ret;
>> > +
>> > +	ret = swsci(dev, SWSCI_GBDA_PANEL_DETAILS, 0x0, &panel_details);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	return (panel_details >> 8) & 0xff;
>> > +}
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx