Re: [PATCH 1/5] drm/i915: Adding VBT fields to support eDP DRRS feature

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 30 Jan 2014, Vandana Kannan <vandana.kannan@xxxxxxxxx> wrote:
> On Jan-22-2014 6:39 PM, Jani Nikula wrote:
>> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@xxxxxxxxx> wrote:
>>> From: Pradeep Bhat <pradeep.bhat@xxxxxxxxx>
>>>
>>> This patch reads the DRRS support and Mode type from VBT fields.
>>> The read information will be stored in VBT struct during BIOS
>>> parsing. The above functionality is needed for decision making
>>> whether DRRS feature is supported in i915 driver for eDP panels.
>>> This information helps us decide if seamless DRRS can be done
>>> at runtime to support certain power saving features. This patch
>>> was tested by setting necessary bit in VBT struct and merging
>>> the new VBT with system BIOS so that we can read the value.
>>>
>>> v2: Incorporated review comments from Chris Wilson
>>> Removed "intel_" as a prefix for DRRS specific declarations.
>>>
>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@xxxxxxxxx>
>>> Signed-off-by: Vandana Kannan <vandana.kannan@xxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h   |    9 +++++++++
>>>  drivers/gpu/drm/i915/intel_bios.c |   23 +++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
>>>  3 files changed, 61 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index ae2c80c..f8fd045 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1174,6 +1174,15 @@ struct intel_vbt_data {
>>>  	int lvds_ssc_freq;
>>>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>>>  
>>> +	/**
>> 
>> This is not a kerneldoc comment, so drop the extra *.
>> 
> Ok.
>>> +	 * DRRS mode type (Seamless OR Static DRRS)
>>> +	 * drrs_mode Val 0x2 is Seamless DRRS and 0 is Static DRRS.
>>> +	 * These values correspond to the VBT values for drrs mode.
>>> +	 */
>>> +	int drrs_mode;
>>> +	/* DRRS enabled or disabled in VBT */
>>> +	bool drrs_enabled;
>> 
>> Both the drrs_mode and drrs_enabled should be replaced by one enum
>> drrs_support_type which you introduce later. It's all self-explanatory
>> that way, and you don't need to explain so much.
>> 
> There are 2 options in the VBT. drrs_enabled to check if DRRS is
> supported, drrs_mode to show which type. It has been added here as it is
> for readability.

I can understand the argument for anything defined in intel_bios.[ch],
but for the rest, including struct intel_vbt_data, readability comes
from other reasons than being equivalent with the VBT specs.

>>> +
>>>  	/* eDP */
>>>  	int edp_rate;
>>>  	int edp_lanes;
>>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>> index f88e507..5b04a69 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> @@ -195,6 +195,21 @@ get_lvds_fp_timing(const struct bdb_header *bdb,
>>>  	return (const struct lvds_fp_timing *)((const u8 *)bdb + ofs);
>>>  }
>>>  
>>> +/**
>>> + * This function returns the 2 bit information pertaining to a panel type
>>> + * present in a 32 bit field in VBT blocks. There are 16 panel types in VBT
>>> + * each occupying 2 bits of information in some 32 bit fields of VBT blocks.
>>> + */
>>> +static int
>>> +get_mode_by_paneltype(unsigned int word)
>>> +{
>>> +	/**
>>> +	 * The caller of this API should interpret the 2 bits
>>> +	 * based on VBT description for that field.
>>> +	 */
>>> +	return (word >> ((panel_type - 1) * 2)) & MODE_MASK;
>> 
>> Everywhere else in intel_bios.c panel_type is used 0-based.
>> 
> VBT indexes panel type as 1,2,3. Therefore, we have to make the change
> to match kernel's 0-based usage.

Again, everywhere else in intel_bios.c we use panel_type, directly as it
is in VBT, 0-based. Are you saying it's all wrong? panel_type can't be
1-based in this one instance, and 0-based in all other instances, right?

This is actually the first priority to check.

>>> +}
>> 
>> You use the above function only once. IMHO with all the explaining above
>> it's just too much of a burden to the reader. Just do this in the code.
>> 
> Ok.
>>> +
>>>  /* Try to find integrated panel data */
>>>  static void
>>>  parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>> @@ -218,6 +233,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>>  
>>>  	panel_type = lvds_options->panel_type;
>>>  
>>> +	dev_priv->vbt.drrs_mode =
>>> +		get_mode_by_paneltype(lvds_options->dps_panel_type_bits);
>>> +	DRM_DEBUG_KMS("DRRS supported mode is : %s\n",
>>                                              ^ drop the space here
>> 
> Ok
>>> +			(dev_priv->vbt.drrs_mode == 0) ? "STATIC" : "SEAMLESS");
>> 
>> Why shouting?
>> 
>>> +
>>>  	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
>>>  	if (!lvds_lfp_data)
>>>  		return;
>>> @@ -488,6 +508,9 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>>>  
>>>  	if (driver->dual_frequency)
>>>  		dev_priv->render_reclock_avail = true;
>>> +
>>> +	dev_priv->vbt.drrs_enabled = driver->drrs_state;
>>> +	DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->drrs_state);
>>                                          ^ and here and everywhere
>> 
> Ok
>>>  }
>>>  
>>>  static void
>>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>>> index 81ed58c..0a3a751 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.h
>>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>>> @@ -281,6 +281,9 @@ struct bdb_general_definitions {
>>>  	union child_device_config devices[0];
>>>  } __packed;
>>>  
>>> +/* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */
>>> +#define MODE_MASK		0x3
>>> +
>>>  struct bdb_lvds_options {
>>>  	u8 panel_type;
>>>  	u8 rsvd1;
>>> @@ -293,6 +296,18 @@ struct bdb_lvds_options {
>>>  	u8 lvds_edid:1;
>>>  	u8 rsvd2:1;
>>>  	u8 rsvd4;
>>> +	/* LVDS Panel channel bits stored here */
>>> +	u32 lvds_panel_channel_bits;
>> 
>> Why does this have "lvds" but the rest not? Why do some fields end in
>> "bits" but some others not? Some consistency please.
>> 
> This is how the vbt block definition doc mentions each of these fields.
> This is the reason why it has been added in this manner.

I don't have my vbt spec handy right now, but when I was last checking
these it didn't look consistent with the spec.

Same for the ones below.

>
>>> +	/* LVDS SSC (Spread Spectrum Clock) bits stored here. */
>>> +	u16 ssc_bits;
>>> +	u16 ssc_freq;
>>> +	u16 ssc_ddt;
>>> +	/* Panel color depth defined here */
>>> +	u16 panel_color_depth;
>> 
>> I really *really* wish I knew why some stuff is specified in the lvds
>> bios blob and some other in edp blob and some stuff specified here is
>> used in the edp side. Ugh. I wonder if we should check this
>> panel_color_depth for edp too.
>>
> This is how the vbt block definition doc mentions each of these fields.
> This is the reason why it has been added in this manner.
>
>>> +	/* LVDS panel type bits stored here */
>>> +	u32 dps_panel_type_bits;
>>> +	/* LVDS backlight control type bits stored here */
>>> +	u32 blt_control_type_bits;
>>>  } __packed;
>>>  
>>>  /* LFP pointer table contains entries to the struct below */
>>> @@ -462,6 +477,20 @@ struct bdb_driver_features {
>>>  
>>>  	u8 hdmi_termination;
>>>  	u8 custom_vbt_version;
>>> +	/* Driver features data block */
>>> +	u16 rmpm_state:1;
>>> +	u16 s2ddt_state:1;
>>> +	u16 dpst_state:1;
>>> +	u16 bltclt_state:1;
>>> +	u16 adb_state:1;
>>> +	u16 drrs_state:1;
>>> +	u16 grs_state:1;
>>> +	u16 gpmt_state:1;
>>> +	u16 tbt_state:1;
>>> +	u16 psr_state:1;
>>> +	u16 ips_state:1;
>> 
>> All of the above should be foo_enabled to make them self-explanatory.
>> 
>>> +	u16 reserved3:4;
>>> +	u16 pc_feature_validity:1;
>> 
>> Similarly for this one, should be pc_feature_valid.
>> 
> This is how the vbt block definition doc mentions this field.
> This is the reason why it has been added in this manner.
>>>  } __packed;
>>>  
>>>  #define EDP_18BPP	0
>>> -- 
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux