[Intel-gfx] [PATCH 1/5] drm/i915: Adding VBT fields to support eDP DRRS feature
Vandana Kannan
vandana.kannan at intel.com
Mon Feb 3 04:43:17 CET 2014
On Jan-30-2014 11:50 AM, Jani Nikula wrote:
> On Thu, 30 Jan 2014, Vandana Kannan <vandana.kannan at intel.com> wrote:
>> On Jan-22-2014 6:39 PM, Jani Nikula wrote:
>>> On Mon, 23 Dec 2013, 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.
>>>>
>>>> Signed-off-by: Pradeep Bhat <pradeep.bhat at intel.com>
>>>> Signed-off-by: Vandana Kannan <vandana.kannan at intel.com>
>>>> ---
>>>> 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.
>
I have discussed with the author of the patch and i will modify this to
make panel_type 0-based.
>>>> +}
>>>
>>> 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.
>
VBT-driver interface document has been referred for this.
>>
>>>> + /* 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 at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>
>
More information about the Intel-gfx
mailing list