[Intel-gfx] [PATCH 1/6] drm/i915: Adding VBT fields to support eDP DRRS feature
Jani Nikula
jani.nikula at linux.intel.com
Thu Mar 27 11:39:32 CET 2014
On Thu, 27 Mar 2014, Vandana Kannan <vandana.kannan at intel.com> wrote:
> On Mar-26-2014 6:06 PM, Jani Nikula wrote:
>> On Fri, 07 Mar 2014, Vandana Kannan <vandana.kannan at intel.com> wrote:
>>> From: Pradeep Bhat <pradeep.bhat at 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 at intel.com>
>>> Signed-off-by: Vandana Kannan <vandana.kannan at intel.com>
>>> Acked-by: Jesse Barnes <jbarnes at 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?
The values are defined in enum drrs_support_type now and don't
correspond to VBT, e.g. 0 is not static drrs.
Please avoid having values like this in comments altogether, because
experience says they will not be updated if the enum is updated.
BR,
Jani.
>
>>> + 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 at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list