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

[Vandana Kannan <vandana.kannan@intel.com>]

On Mar-26-2014 6:06 PM, Jani Nikula wrote:
> On Fri, 07 Mar 2014, Vandana Kannan <vandana.kannan@intel.com> wrote:
>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>
>> 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.
>>
>> v3: Incorporated Jani's review comments
>> Removed function which deducts drrs mode from panel_type. Modified some
>> print statements. Made changes to use DRRS_NOT_SUPPORTED as 0 instead of -1.
>>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h   |   13 +++++++++++++
>>  drivers/gpu/drm/i915/intel_bios.c |   29 +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
>>  3 files changed, 71 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 728b9c3..6b4d0b20 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1218,6 +1218,12 @@ struct ddi_vbt_port_info {
>>  	uint8_t supports_dp:1;
>>  };
>>  
>> +enum drrs_support_type {
>> +	DRRS_NOT_SUPPORTED = 0,
>> +	STATIC_DRRS_SUPPORT = 1,
>> +	SEAMLESS_DRRS_SUPPORT = 2
>> +};
>> +
>>  struct intel_vbt_data {
>>  	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>>  	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
>> @@ -1233,6 +1239,13 @@ struct intel_vbt_data {
>>  	int lvds_ssc_freq;
>>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>>  
>> +	/*
>> +	 * DRRS support type (Seamless OR Static DRRS OR not supported)
>> +	 * drrs_support type Val 0x2 is Seamless DRRS and 0 is Static DRRS.
>> +	 * These values correspond to the VBT values for drrs mode.
>> +	 */
> 
> The comment is wrong.
>
Could you elaborate on this?

>> +	enum drrs_support_type drrs_type;
>> +
>>  	/* 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 86b95ca..2414eca 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -218,6 +218,25 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>  
>>  	panel_type = lvds_options->panel_type;
>>  
>> +	dev_priv->vbt.drrs_type = (lvds_options->dps_panel_type_bits
>> +					>> (panel_type * 2)) & MODE_MASK;
>> +	/*
>> +	 * VBT has static DRRS = 0 and seamless DRRS = 2.
>> +	 * The below piece of code is required to adjust vbt.drrs_type
>> +	 * to match the enum drrs_support_type.
>> +	 */
>> +	switch (dev_priv->vbt.drrs_type) {
> 
> Please don't mix the vbt and the enum at all like this, it's error
> prone. Just make the switch on the value in vbt, and for each case
> assign the appropriate enum to dev_priv->vbt.drrs_type.
> 
Ok
>> +	case 0:
>> +		dev_priv->vbt.drrs_type = STATIC_DRRS_SUPPORT;
>> +		DRM_DEBUG_KMS("DRRS supported mode is static\n");
>> +		break;
>> +	case 2:
>> +		DRM_DEBUG_KMS("DRRS supported mode is seamless\n");
>> +		break;
>> +	default:
>> +		break;
> 
> And fail on the default.
> 
Ok
>> +	}
>> +
>>  	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
>>  	if (!lvds_lfp_data)
>>  		return;
>> @@ -516,6 +535,16 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>>  
>>  	if (driver->dual_frequency)
>>  		dev_priv->render_reclock_avail = true;
>> +
>> +	DRM_DEBUG_KMS("DRRS State Enabled:%d\n", driver->drrs_enabled);
>> +	/*
>> +	 * If DRRS is not supported, drrs_type has to be set to 0.
>> +	 * This is because, VBT is configured in such a way that
>> +	 * static DRRS is 0 and DRRS not supported is represented by
>> +	 * driver->drrs_enabled=false
>> +	 */
>> +	if (!driver->drrs_enabled)
>> +		dev_priv->vbt.drrs_type = driver->drrs_enabled;
> 
> Don't assign a boolean to an enum.
> 
Ok
>>  }
>>  
>>  static void
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 282de5e..5505d6c 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;
>> +	/* 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;
>> +	/* 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 */
>> @@ -478,6 +493,20 @@ struct bdb_driver_features {
>>  
>>  	u8 hdmi_termination;
>>  	u8 custom_vbt_version;
>> +	/* Driver features data block */
>> +	u16 rmpm_enabled:1;
>> +	u16 s2ddt_enabled:1;
>> +	u16 dpst_enabled:1;
>> +	u16 bltclt_enabled:1;
>> +	u16 adb_enabled:1;
>> +	u16 drrs_enabled:1;
>> +	u16 grs_enabled:1;
>> +	u16 gpmt_enabled:1;
>> +	u16 tbt_enabled:1;
>> +	u16 psr_enabled:1;
>> +	u16 ips_enabled:1;
>> +	u16 reserved3:4;
>> +	u16 pc_feature_valid:1;
>>  } __packed;
>>  
>>  #define EDP_18BPP	0
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>