2013/9/11 Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>:
> On Wed, Sep 11, 2013 at 06:02:48PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> We currently use the recommended values from BSpec, but the VBT
>> specifies the correct value to use for the hardware we have, so use
>> it. We also fall back to the recommended value in case we can't find
>> the VBT.
>>
>> In addition, this code also provides some infrastructure to parse more
>> information about the DDI ports. There's a lot more information we
>> could extract and use in the future.
>>
>> v2: - Move some code to init_vbt_defaults.
>> v3: - Rebase
>> - Clarify the "DVO Port" matching code
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 6 +++
>> drivers/gpu/drm/i915/intel_bios.c | 78 +++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_bios.h | 13 +++++++
>> drivers/gpu/drm/i915/intel_ddi.c | 24 +++++++++++-
>> 4 files changed, 119 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index ca8856a..298c671 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1052,6 +1052,10 @@ enum modeset_restore {
>> MODESET_SUSPENDED,
>> };
>>
>> +struct ddi_vbt_port_info {
>> + uint8_t hdmi_level_shift;
>> +};
>> +
>> struct intel_vbt_data {
>> struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>> struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
>> @@ -1086,6 +1090,8 @@ struct intel_vbt_data {
>>
>> int child_dev_num;
>> union child_device_config *child_dev;
>> +
>> + struct ddi_vbt_port_info ddi_port_info[5];
>
> s/5/PORT_E+1/ or better #define NUM_DDI_PORTS PORT_E+1.
Looks like I can just use the already-defined I915_MAX_PORTS. It also
matches the I915_MAX_PIPES usage.
>
>> };
>>
>> enum intel_ddb_partitioning {
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 33003b9..d7ff8fc 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -583,6 +583,72 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>> dev_priv->vbt.dsi.panel_id = mipi->panel_id;
>> }
>>
>> +static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>> + struct bdb_header *bdb)
>> +{
>> + union child_device_config *it, *child = NULL;
>> + struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
>> + uint8_t hdmi_level_shift;
>> + int i, j;
>> + /* Each DDI port can have more than one value on the "DVO Port" field,
>> + * so look for all the possible values for each port and abort if more
>> + * than one is found. */
>> + int dvo_ports[][2] = {
>> + {DVO_PORT_HDMIA, DVO_PORT_DPA},
>> + {DVO_PORT_HDMIB, DVO_PORT_DPB},
>> + {DVO_PORT_HDMIC, DVO_PORT_DPC},
>> + {DVO_PORT_HDMID, DVO_PORT_DPD},
>> + {DVO_PORT_CRT, -1 /* Port E can only be DVO_PORT_CRT */ },
>> + };
>> +
>
> I don't see the point of calling this function if bdb->version < 158 ?
I built the whole function as a single patch, then I split it in 5
later, and chose this chunk to be the first since it was more
important than the others. So this patch looks weird, but things make
more sense if you see the next patches.
>
>> + /* Find the child device to use, abort if more than one found. */
>> + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> + it = dev_priv->vbt.child_dev + i;
>> +
>> + for (j = 0; j < 2; j++) {
>> + if (dvo_ports[port][j] == -1)
>> + break;
>> +
>> + if (it->common.dvo_port == dvo_ports[port][j]) {
>> + if (child) {
>> + DRM_ERROR("More than one child device for port %c in VBT.\n",
>> + port_name(port));
>> + return;
>> + }
>> + child = it;
>> + }
>> + }
>> + }
>> + if (!child)
>> + return;
>> +
>> + if (bdb->version >= 158) {
>> + /* The VBT HDMI level shift values match the table we have. */
>> + hdmi_level_shift = child->raw[7] & 0xF;
>> + if (hdmi_level_shift < 0xC) {
>> + DRM_DEBUG_KMS("VBT HDMI level shift for port %c: %d\n",
>> + port_name(port),
>> + hdmi_level_shift);
>> + info->hdmi_level_shift = hdmi_level_shift;
>> + }
>> + }
>> +}
>> +
>> +static void parse_ddi_ports(struct drm_i915_private *dev_priv,
>> + struct bdb_header *bdb)
>> +{
>> + enum port port;
>> +
>> + if (!dev_priv->vbt.child_dev_num)
>> + return;
>> +
>> + if (bdb->version < 155)
>> + return;
>> +
>> + for (port = PORT_A; port <= PORT_E; port++)
>> + parse_ddi_port(dev_priv, port, bdb);
>> +}
>> +
>> static void
>> parse_device_mapping(struct drm_i915_private *dev_priv,
>> struct bdb_header *bdb)
>> @@ -670,6 +736,16 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
>> dev_priv->vbt.lvds_use_ssc = 1;
>> dev_priv->vbt.lvds_ssc_freq = intel_bios_ssc_frequency(dev, 1);
>> DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->vbt.lvds_ssc_freq);
>> +
>> + if (HAS_DDI(dev)) {
>
> Might as well just set the defaults anyway. I only really care if the
> recommendations change between generations.
Done.
>
>> + enum port port;
>> +
>> + for (port = PORT_A; port <= PORT_E; port++) {
>> + /* Recommended BSpec default: 800mV 0dB. */
>> + dev_priv->vbt.ddi_port_info[port].hdmi_level_shift = 6;
>> + }
>> + }
>> +
>> }
>>
>> static int __init intel_no_opregion_vbt_callback(const struct dmi_system_id *id)
>> @@ -761,6 +837,8 @@ intel_parse_bios(struct drm_device *dev)
>> parse_driver_features(dev_priv, bdb);
>> parse_edp(dev_priv, bdb);
>> parse_mipi(dev_priv, bdb);
>> + if (HAS_DDI(dev))
>> + parse_ddi_ports(dev_priv, bdb);
>
> Move the check here into parse_ddi_ports() so the calling sequence is
> clear.
Done.
Thanks for the reviews!
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
