[Intel-gfx] [PATCH v2 5/7] drm/i915: check LSPCON vendor OUI

[Sharma, Shashank]

Thanks for the review, Maarten.

My comments, inline.

Regards
Shashank
On 10/31/2017 2:30 PM, Maarten Lankhorst wrote:
> Hey,
>
> Op 09-08-17 om 08:46 schreef Shashank Sharma:
>> Intel LSPCON chip is provided by 2 vendors:
>> - Megachips America (MCA)
>> - Parade technologies (Parade tech)
>>
>> Its important to know the vendor of this chip, as the address to
>> write AVI infoframes is different for those two.
>>
>> This patch reads the vendor OUI signature, and marks into LSPCON
>> encoder structure for future usages.
>>
>> This patch also does a small re-arrangement of the code, by moving
>> lspcon mode change into probe function.
>>
>> V2: Use dp->desc for OUI detection, dont add a helper for this
>>      (Ville)
>>
>> Cc: Imre Deak <imre.deak at intel.com>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_drv.h    |  6 ++++
>>   drivers/gpu/drm/i915/intel_lspcon.c | 69 +++++++++++++++++++++++++++++--------
>>   2 files changed, 61 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 7eadac0..adab635 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1043,9 +1043,15 @@ struct intel_dp {
>>   	struct intel_dp_compliance compliance;
>>   };
>>   
>> +enum lspcon_vendor {
>> +	LSPCON_VENDOR_MCA,
>> +	LSPCON_VENDOR_PARADE
>> +};
>> +
>>   struct intel_lspcon {
>>   	bool active;
>>   	enum drm_lspcon_mode mode;
>> +	enum lspcon_vendor vendor;
>>   };
>>   
>>   struct intel_digital_port {
>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>> index 5abef482..93507c5 100644
>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -27,6 +27,10 @@
>>   #include <drm/drm_dp_dual_mode_helper.h>
>>   #include "intel_drv.h"
>>   
>> +/* LSPCON OUI Vendor ID(signatures) */
>> +#define LSPCON_VENDOR_PARADE_OUI 0x001CF8
>> +#define LSPCON_VENDOR_MCA_OUI 0x0060AD
>> +
>>   static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
>>   {
>>   	struct intel_digital_port *dig_port =
>> @@ -50,6 +54,40 @@ static const char *lspcon_mode_name(enum drm_lspcon_mode mode)
>>   	}
>>   }
>>   
>> +static bool lspcon_detect_vendor(struct intel_lspcon *lspcon)
>> +{
>> +	struct intel_dp *dp = lspcon_to_intel_dp(lspcon);
>> +	struct drm_dp_dpcd_ident *ident;
>> +	u32 vendor_oui;
>> +
>> +	if (drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd))) {
>> +		DRM_ERROR("Can't read description\n");
>> +		return false;
>> +	}
>> +
>> +	ident = &dp->desc.ident;
>> +	vendor_oui = (ident->oui[0] << 16) | (ident->oui[1] << 8) |
>> +		      ident->oui[2];
>> +
>> +	switch (vendor_oui) {
>> +	case LSPCON_VENDOR_MCA_OUI:
>> +		lspcon->vendor = LSPCON_VENDOR_MCA;
>> +		DRM_DEBUG_KMS("Vendor: Mega Chips\n");
>> +		break;
>> +
>> +	case LSPCON_VENDOR_PARADE_OUI:
>> +		lspcon->vendor = LSPCON_VENDOR_PARADE;
>> +		DRM_DEBUG_KMS("Vendor: Parade Tech\n");
>> +		break;
>> +
>> +	default:
>> +		DRM_ERROR("Invalid/Unknown vendor OUI\n");
> MISSING_CASE(vendor_oui) ?
Sure, seems good.
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>   static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
>>   {
>>   	enum drm_lspcon_mode current_mode;
>> @@ -151,7 +189,18 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
>>   	/* Yay ... got a LSPCON device */
>>   	DRM_DEBUG_KMS("LSPCON detected\n");
>>   	lspcon->mode = lspcon_wait_mode(lspcon, expected_mode);
>> -	lspcon->active = true;
>> +
>> +	/*
>> +	 * In the SW state machine, lets Put LSPCON in PCON mode only.
>> +	 * In this way, it will work with both HDMI 1.4 sinks as well as HDMI
>> +	 * 2.0 sinks.
>> +	 */
>> +	if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
>> +		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
>> +			DRM_ERROR("LSPCON mode change to PCON failed\n");
>> +			return false;
>> +		}
>> +	}
>>   	return true;
>>   }
>>   
>> @@ -223,25 +272,17 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>   		return false;
>>   	}
>>   
>> -	/*
>> -	* In the SW state machine, lets Put LSPCON in PCON mode only.
>> -	* In this way, it will work with both HDMI 1.4 sinks as well as HDMI
>> -	* 2.0 sinks.
>> -	*/
>> -	if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
>> -		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
>> -			DRM_ERROR("LSPCON mode change to PCON failed\n");
>> -			return false;
>> -		}
>> -	}
>> -
>>   	if (!intel_dp_read_dpcd(dp)) {
>>   		DRM_ERROR("LSPCON DPCD read failed\n");
>>   		return false;
>>   	}
>>   
>> -	drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd));
>> +	if (!lspcon_detect_vendor(lspcon)) {
>> +		DRM_ERROR("LSPCON vendor detection failed\n");
>> +		return false;
>> +	}
> Error seems double here, lspcon_detect_vendor should already succeed.
>
> Same for lspcon_probe I think, better to upgrade the DRM_DEBUG_KMS failure there to DRM_ERROR there and remove the one from lspcon_init.
Yeah, seems like a good idea, initially I thought of dumping a specific 
and a generic error, but making generic error as _KMS sounds better.
- Shashank
>>   
>> +	lspcon->active = true;
>>   	DRM_DEBUG_KMS("Success: LSPCON init\n");
>>   	return true;
>>   }
>