Re: [PATCH 1/7] drm/i915: add lspcon vbt bit parsing

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards
Shashank

On 3/22/2016 10:35 PM, Ville Syrjälä wrote:
> On Tue, Mar 22, 2016 at 10:20:33PM +0530, Sharma, Shashank wrote:
>> Thanks for the review, Jani.
>>
>> Regards
>> Shashank
>>
>> On 3/22/2016 9:24 PM, Jani Nikula wrote:
>>> On Tue, 22 Mar 2016, Shashank Sharma <shashank.sharma@intel.com> wrote:
>>>> LSPCON can be configured on a port using VBT entry.
>>>> This patch adds code to parse VBT and detect presence of
>>>> LSPCON for a ddi port.
>>>>
>>>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h       |  1 +
>>>>    drivers/gpu/drm/i915/intel_bios.c     | 42 +++++++++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_vbt_defs.h |  1 +
>>>>    3 files changed, 44 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index f330a53..cbd40de 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -3373,6 +3373,7 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv);
>>>>    bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin);
>>>>    bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port);
>>>>    bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port);
>>>> +bool intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port);
>>>>
>>>>    /* intel_opregion.c */
>>>>    #ifdef CONFIG_ACPI
>>>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>>> index 083003b..a04ab5c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>>> @@ -1588,3 +1588,45 @@ bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv,
>>>>
>>>>    	return false;
>>>>    }
>>>> +
>>>
>>> Please add kernel-doc comment like all the other intel_bios_is_*
>>> functions have.
>>>
>> Ok,
>>>> +bool
>>>> +intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port)
>>>
>>> You mean is *present*?
>>>
>> Well, this is embarrassing :) yes it was supposed to be present.
>>> Please pass struct drm_i915_private *dev_priv instead of dev.
>>>
>> Ok
>>>> +{
>>>> +	unsigned char i;
>>>> +	enum port dvo_port = 0;
>>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>> +
>>>> +	/* LSPCON is supported only for GEN 9 */
>>>> +	if (!IS_GEN9(dev))
>>>> +		return false;
>>>> +
>>>> +	/* Check if lspcon is supported in VBT */
>>>> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>>>> +		if (!dev_priv->vbt.child_dev[i].common.lspcon)
>>>
>>> I think to be defensive we should also check for bdb version >=
>>> 192. That we could check before the loop along with the gen check.
>>>
>> Yes, this can be done.
>>>> +			continue;
>>>> +
>>>> +		switch (dev_priv->vbt.child_dev[i].common.dvo_port) {
>>>> +		case DVO_PORT_DPB:
>>>> +			dvo_port = PORT_B;
>>>> +			break;
>>>> +
>>>> +		case DVO_PORT_DPC:
>>>> +			dvo_port = PORT_C;
>>>> +			break;
>>>> +
>>>> +		case DVO_PORT_DPD:
>>>> +			dvo_port = PORT_D;
>>>> +			break;
>>>> +
>>>> +		default:
>>>> +			continue;
>>>> +		}
>>>> +
>>>> +		if (dvo_port == port) {
>>>> +			DRM_DEBUG_DRIVER("LSPCON configured on port %c\n",
>>>> +			port_name(port));
>>>> +			return true;
>>>> +		}
>>>> +	}
>>>> +	return false;
>>>> +}
>>>> diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h
>>>> index 749dcea..0066b24 100644
>>>> --- a/drivers/gpu/drm/i915/intel_vbt_defs.h
>>>> +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
>>>> @@ -276,6 +276,7 @@ struct common_child_dev_config {
>>>>    	u8 flags_1;
>>>>    	u8 not_common3[13];
>>>>    	u8 iboost_level;
>>>> +	u8 lspcon:1;
>>>
>>> Huh? AFAICT from the spec, lspcon is bit 2 in flags_1. You could define
>>> flags_1 in terms of bitfields, including IBOOST_ENABLE.
>>>
>> I am not sure about this, let me go to VBT spec again, if there is a
>> recent change. Will update you on this.
>
> There was a patch [1] posted recently which added the lspcon bit and others,
> and that at least matches the spec at the time when I looked at it (or I
> made a mistake). The patch just needs a rebase and it should be good to
> go I think, though someone else double checking wouldn't hurt.
>
> [1] https://lists.freedesktop.org/archives/intel-gfx/2016-March/089759.html
>
Thanks for pointing this out.
I almost forgot about this patch, which Durga/Siva added.
Will sync up on this patch first, and if a rebase is the only thing 
required, we can proceed on this one.
>
>>>>    } __packed;
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx