[Intel-gfx] [PATCH v6 3/5] drm/i915: Parse VBT data for lspcon

[Sharma, Shashank]

Regards

Shashank


On 10/17/2016 6:04 PM, Jani Nikula wrote:
> On Fri, 14 Oct 2016, "Sharma, Shashank" <shashank.sharma at intel.com> wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 10/14/2016 8:02 PM, Jani Nikula wrote:
>>> On Fri, 14 Oct 2016, Shashank Sharma <shashank.sharma at intel.com> wrote:
>>>> Many GEN9 boards come with on-board lspcon cards.
>>>> Fot these boards, VBT configuration should properly point out
>>>> if a particular port contains lspcon device, so that driver can
>>>> initialize it properly.
>>>>
>>>> This patch adds a utility function, which checks the VBT flag
>>>> for lspcon bit, and tells us if a port is configured to have a
>>>> lspcon device or not.
>>>>
>>>> V2: Fixed review comments from Ville
>>>> - Do not forget PORT_D while checking lspcon for GEN9
>>>>
>>>> V3: Addressed review comments from Rodrigo
>>>> - Create a HAS_LSPCON() macro for better use case handling.
>>>> - Do not dump warnings for non-gen-9 platforms, it will be noise.
>>>>
>>>> V4: Rebase
>>>> V5: Rebase
>>>> V6: Pass dev_priv to HAS_LSPCON() macro
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> I was hoping you'd use the version I rebased and sent, put it first in
>>> the series, and rebase the rest on that. The point is, this series has
>>> taken so long that lspcon devices have proliferated all over the place,
>>> and we'll be getting more and more bugs about them. If this patch was
>>> first, with the debug logging, we could at least get that to 4.9, maybe
>>> stable kernels, and we'd immediately know the reason. I think it'll be a
>>> hard sell to get the whole series to 4.9 kernel.
>>>
>>> BR,
>>> Jani.
>> Jani,
>> The patch got its first r-b since a long time.
>> After that, it was waiting to be merged, for long time.
>>
>> Recently, when Imre was asked to test the patches, and he found one
>> issue specific to APL.
>> We were trying to fix a suspend-resume issue, which was fixed with the
>> last patch.
>> Now this patch is ready to be merged, just waiting for Imre's r-b.
>>
>> Third patch just gives information about if LSPCON is available or not,
>> which is not a big help for anything as such.
>> So instead of changing the sequence, and confusing the reviewers, I
>> thought it would be better to send the whole series and
>> get this merged as-it-is.
> Fine, let's merge this as-is... after patch 1/5 has been posted to
> dri-devel and/or has received an ack from Dave Airlie that it's fine to
> merge through our tree.
>
> In the bigger scheme of things, if this patch 3/5 had been first in the
> series all along, we could have merged this *months* ago. This is how
> series should be organized. Simple things first.
Please note that this was in continuation with Ville's dp_dual_mode 
series, so I had to start series with lspcon support in DP helper layer, 
and then come to I915 layer.
Also, IMHO, It was in the simplest form :).
>
> Having the debug information *is* valuable. You'd see that if you had to
> go through our incoming bugs. We've had plenty of LSPCON bugs since the
> day Skylake was launched. Yes, quite a long time now. If 3/5 was first
> in the series, we could backport that to v4.9 and older and reduce our
> debugging time.
I was already debugging the LSPCON issues on SKL for various devices 
(Like Skullcandy), with Paul's team, and they had proper information 
about this series, along with how to detect LSPCON, since the whole time.
They were also waiting for the merge itself, and the progress was 
recorded in this Jira VIZ-2800. Please see the comments from me since 
25th march, and from Manasi on 17th May.
This patch series had r-b since few months, I am not sure what else I 
could have done better.

Regards
Shashank
>
> BR,
> Jani.
>
>
>
>
>
>
>> Regards
>> Shashank
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h   |  5 ++++
>>>>    drivers/gpu/drm/i915/intel_bios.c | 49 +++++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 54 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index fe875b2..7bab2f1 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2864,6 +2864,8 @@ struct drm_i915_cmd_table {
>>>>    
>>>>    #define HAS_GMCH_DISPLAY(dev_priv) ((dev_priv)->info.has_gmch_display)
>>>>    
>>>> +#define HAS_LSPCON(dev_priv) (IS_GEN9(dev_priv))
>>>> +
>>>>    /* DPF == dynamic parity feature */
>>>>    #define HAS_L3_DPF(dev_priv) ((dev_priv)->info.has_l3_dpf)
>>>>    #define NUM_L3_SLICES(dev_priv) (IS_HSW_GT3(dev_priv) ? \
>>>> @@ -3631,6 +3633,9 @@ bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum por
>>>>    bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port);
>>>>    bool intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv,
>>>>    				     enum port port);
>>>> +bool intel_bios_is_lspcon_present(struct drm_i915_private *dev_priv,
>>>> +				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 83667e8..32e1def 100644
>>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>>> @@ -1763,3 +1763,52 @@ intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv,
>>>>    
>>>>    	return false;
>>>>    }
>>>> +
>>>> +/**
>>>> + * intel_bios_is_lspcon_present - if LSPCON is attached on %port
>>>> + * @dev_priv:	i915 device instance
>>>> + * @port:	port to check
>>>> + *
>>>> + * Return true if LSPCON is present on this port
>>>> + */
>>>> +bool
>>>> +intel_bios_is_lspcon_present(struct drm_i915_private *dev_priv,
>>>> +				enum port port)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	if (!HAS_LSPCON(dev_priv))
>>>> +		return false;
>>>> +
>>>> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>>>> +		if (!dev_priv->vbt.child_dev[i].common.lspcon)
>>>> +			continue;
>>>> +
>>>> +		switch (dev_priv->vbt.child_dev[i].common.dvo_port) {
>>>> +		case DVO_PORT_DPA:
>>>> +		case DVO_PORT_HDMIA:
>>>> +			if (port == PORT_A)
>>>> +				return true;
>>>> +			break;
>>>> +		case DVO_PORT_DPB:
>>>> +		case DVO_PORT_HDMIB:
>>>> +			if (port == PORT_B)
>>>> +				return true;
>>>> +			break;
>>>> +		case DVO_PORT_DPC:
>>>> +		case DVO_PORT_HDMIC:
>>>> +			if (port == PORT_C)
>>>> +				return true;
>>>> +			break;
>>>> +		case DVO_PORT_DPD:
>>>> +		case DVO_PORT_HDMID:
>>>> +			if (port == PORT_D)
>>>> +				return true;
>>>> +			break;
>>>> +		default:
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}