diff mbox

[3/5] drm/i915/bxt: Added _DSM call to set HPD_CTL.

Message ID 1479917907-2468-4-git-send-email-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Manna, Animesh Nov. 23, 2016, 4:18 p.m. UTC 
_DSM is added to program HPD_CTL(0x1094) register
of PMC from i915 driver which will be called
based on driver feature flag. PMC hpd control register
programming will enable PMC to get hpd interrupt
during dc9.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/intel_acpi.c | 44 ++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

Comments

Ville Syrjälä Nov. 23, 2016, 6:17 p.m. UTC | #1 
On Wed, Nov 23, 2016 at 09:48:25PM +0530, Animesh Manna wrote:
> _DSM is added to program HPD_CTL(0x1094) register
> of PMC from i915 driver which will be called
> based on driver feature flag. PMC hpd control register
> programming will enable PMC to get hpd interrupt
> during dc9.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_acpi.c | 44 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
> index 8c878ab..15d3b84 100755
> --- a/drivers/gpu/drm/i915/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/intel_acpi.c
> @@ -10,6 +10,8 @@
>  
>  #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
>  #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
> +#define INTEL_DSM_SET_HPD_WAKEUP 17
> +#define HPD_WAKEUP_EN_VAL 0xFCF0
>  
>  static struct intel_dsm_priv {
>  	acpi_handle dhandle;
> @@ -118,6 +120,25 @@ static void intel_dsm_platform_mux_info(void)
>  	ACPI_FREE(pkg);
>  }
>  
> +static void intel_dsm_set_hpd_wakeup(u8 *guid)
> +{
> +	union acpi_object *obj;
> +	union acpi_object argv4 = {
> +		.integer.type = ACPI_TYPE_INTEGER,
> +		.integer.value = HPD_WAKEUP_EN_VAL,
> +	};
> +
> +	obj = acpi_evaluate_dsm_typed(intel_dsm_priv.dhandle, guid,
> +			INTEL_DSM_REVISION_ID, INTEL_DSM_SET_HPD_WAKEUP,
> +			&argv4, ACPI_TYPE_INTEGER);
> +
> +	if (!obj)
> +		DRM_DEBUG_DRIVER("failed to evaluate _DSM\n");
> +
> +	ACPI_FREE(obj);
> +}
> +
> +
>  static bool intel_dsm_pci_probe(struct pci_dev *pdev)
>  {
>  	acpi_handle dhandle;
> @@ -134,14 +155,23 @@ static bool intel_dsm_pci_probe(struct pci_dev *pdev)
>  	else
>  		guid = intel_dsm_guid;
>  
> -	if (!acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
> -			    1 << INTEL_DSM_FN_PLATFORM_MUX_INFO)) {
> -		DRM_DEBUG_KMS("no _DSM method for intel device\n");
> -		return false;
> -	}
> -
>  	intel_dsm_priv.dhandle = dhandle;
> -	intel_dsm_platform_mux_info();
> +
> +	if (acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
> +				1 << INTEL_DSM_FN_PLATFORM_MUX_INFO))
> +		intel_dsm_platform_mux_info();
> +	else
> +		DRM_DEBUG_KMS("no _DSM method for mux-info\n");
> +
> +	/* Need to ensure vbt parsing is completed. */

Eh?

> +	if (dev_priv->vbt.hpd_wakeup_enabled &&
> +		acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
> +				1 << INTEL_DSM_SET_HPD_WAKEUP))
> +		intel_dsm_set_hpd_wakeup(guid);

So we're permanently routing hpds to the pcu? Won't that mess up
stuff like short pulse detection?

I was expecting that we'd switch between the PCU and not during
runtime suspend/resume.

> +	else {
> +		dev_priv->vbt.hpd_wakeup_enabled = false;
> +		DRM_DEBUG_KMS("no _DSM method for hpd-enabling\n");
> +	}
>  
>  	return true;
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula Nov. 24, 2016, 2:22 p.m. UTC | #2 
On Wed, 23 Nov 2016, Animesh Manna <animesh.manna@intel.com> wrote:
> _DSM is added to program HPD_CTL(0x1094) register
> of PMC from i915 driver which will be called
> based on driver feature flag. PMC hpd control register
> programming will enable PMC to get hpd interrupt
> during dc9.
>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_acpi.c | 44 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
> index 8c878ab..15d3b84 100755
> --- a/drivers/gpu/drm/i915/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/intel_acpi.c
> @@ -10,6 +10,8 @@
>  
>  #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
>  #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
> +#define INTEL_DSM_SET_HPD_WAKEUP 17
> +#define HPD_WAKEUP_EN_VAL 0xFCF0
>  
>  static struct intel_dsm_priv {
>  	acpi_handle dhandle;
> @@ -118,6 +120,25 @@ static void intel_dsm_platform_mux_info(void)
>  	ACPI_FREE(pkg);
>  }
>  
> +static void intel_dsm_set_hpd_wakeup(u8 *guid)
> +{
> +	union acpi_object *obj;
> +	union acpi_object argv4 = {
> +		.integer.type = ACPI_TYPE_INTEGER,
> +		.integer.value = HPD_WAKEUP_EN_VAL,
> +	};
> +
> +	obj = acpi_evaluate_dsm_typed(intel_dsm_priv.dhandle, guid,
> +			INTEL_DSM_REVISION_ID, INTEL_DSM_SET_HPD_WAKEUP,
> +			&argv4, ACPI_TYPE_INTEGER);
> +
> +	if (!obj)
> +		DRM_DEBUG_DRIVER("failed to evaluate _DSM\n");
> +
> +	ACPI_FREE(obj);
> +}
> +
> +
>  static bool intel_dsm_pci_probe(struct pci_dev *pdev)
>  {
>  	acpi_handle dhandle;
> @@ -134,14 +155,23 @@ static bool intel_dsm_pci_probe(struct pci_dev *pdev)
>  	else
>  		guid = intel_dsm_guid;
>  
> -	if (!acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
> -			    1 << INTEL_DSM_FN_PLATFORM_MUX_INFO)) {
> -		DRM_DEBUG_KMS("no _DSM method for intel device\n");
> -		return false;
> -	}
> -
>  	intel_dsm_priv.dhandle = dhandle;
> -	intel_dsm_platform_mux_info();
> +
> +	if (acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
> +				1 << INTEL_DSM_FN_PLATFORM_MUX_INFO))
> +		intel_dsm_platform_mux_info();
> +	else
> +		DRM_DEBUG_KMS("no _DSM method for mux-info\n");
> +
> +	/* Need to ensure vbt parsing is completed. */
> +	if (dev_priv->vbt.hpd_wakeup_enabled &&
> +		acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
> +				1 << INTEL_DSM_SET_HPD_WAKEUP))
> +		intel_dsm_set_hpd_wakeup(guid);
> +	else {
> +		dev_priv->vbt.hpd_wakeup_enabled = false;

Please don't change dev_priv->vbt at runtime.

BR,
Jani.

> +		DRM_DEBUG_KMS("no _DSM method for hpd-enabling\n");
> +	}
>  
>  	return true;
>  }
Manna, Animesh Nov. 28, 2016, 4:06 p.m. UTC | #3 
On 11/23/2016 11:47 PM, Ville Syrjälä wrote:
> On Wed, Nov 23, 2016 at 09:48:25PM +0530, Animesh Manna wrote:
>> _DSM is added to program HPD_CTL(0x1094) register
>> of PMC from i915 driver which will be called
>> based on driver feature flag. PMC hpd control register
>> programming will enable PMC to get hpd interrupt
>> during dc9.
>>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_acpi.c | 44 ++++++++++++++++++++++++++++++++-------
>>   1 file changed, 37 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
>> index 8c878ab..15d3b84 100755
>> --- a/drivers/gpu/drm/i915/intel_acpi.c
>> +++ b/drivers/gpu/drm/i915/intel_acpi.c
>> @@ -10,6 +10,8 @@
>>   
>>   #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
>>   #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
>> +#define INTEL_DSM_SET_HPD_WAKEUP 17
>> +#define HPD_WAKEUP_EN_VAL 0xFCF0
>>   
>>   static struct intel_dsm_priv {
>>   	acpi_handle dhandle;
>> @@ -118,6 +120,25 @@ static void intel_dsm_platform_mux_info(void)
>>   	ACPI_FREE(pkg);
>>   }
>>   
>> +static void intel_dsm_set_hpd_wakeup(u8 *guid)
>> +{
>> +	union acpi_object *obj;
>> +	union acpi_object argv4 = {
>> +		.integer.type = ACPI_TYPE_INTEGER,
>> +		.integer.value = HPD_WAKEUP_EN_VAL,
>> +	};
>> +
>> +	obj = acpi_evaluate_dsm_typed(intel_dsm_priv.dhandle, guid,
>> +			INTEL_DSM_REVISION_ID, INTEL_DSM_SET_HPD_WAKEUP,
>> +			&argv4, ACPI_TYPE_INTEGER);
>> +
>> +	if (!obj)
>> +		DRM_DEBUG_DRIVER("failed to evaluate _DSM\n");
>> +
>> +	ACPI_FREE(obj);
>> +}
>> +
>> +
>>   static bool intel_dsm_pci_probe(struct pci_dev *pdev)
>>   {
>>   	acpi_handle dhandle;
>> @@ -134,14 +155,23 @@ static bool intel_dsm_pci_probe(struct pci_dev *pdev)
>>   	else
>>   		guid = intel_dsm_guid;
>>   
>> -	if (!acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
>> -			    1 << INTEL_DSM_FN_PLATFORM_MUX_INFO)) {
>> -		DRM_DEBUG_KMS("no _DSM method for intel device\n");
>> -		return false;
>> -	}
>> -
>>   	intel_dsm_priv.dhandle = dhandle;
>> -	intel_dsm_platform_mux_info();
>> +
>> +	if (acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
>> +				1 << INTEL_DSM_FN_PLATFORM_MUX_INFO))
>> +		intel_dsm_platform_mux_info();
>> +	else
>> +		DRM_DEBUG_KMS("no _DSM method for mux-info\n");
>> +
>> +	/* Need to ensure vbt parsing is completed. */
> Eh?
There is a feature flag for oem to enable/disable HPD as wake feature in 
vbt and on the basis of that PMC_HPD_CTL register will be programmed.
So added a comment just to have that info captured in code-comment, for 
any concern let me know, will update accordingly.
>
>> +	if (dev_priv->vbt.hpd_wakeup_enabled &&
>> +		acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
>> +				1 << INTEL_DSM_SET_HPD_WAKEUP))
>> +		intel_dsm_set_hpd_wakeup(guid);
> So we're permanently routing hpds to the pcu? Won't that mess up
> stuff like short pulse detection?
>
> I was expecting that we'd switch between the PCU and not during
> runtime suspend/resume.
Routing hpd to the pcu will only happen during S0ix. As per my 
understanding for both HDMI and DP we will be getting long pulse during 
hotplug at S0ix. So I think it will not mess up.
Correct me if I am missing something.

- Animesh
>
>> +	else {
>> +		dev_priv->vbt.hpd_wakeup_enabled = false;
>> +		DRM_DEBUG_KMS("no _DSM method for hpd-enabling\n");
>> +	}
>>   
>>   	return true;
>>   }
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Nov. 28, 2016, 4:27 p.m. UTC | #4 
On Mon, Nov 28, 2016 at 09:36:51PM +0530, Animesh Manna wrote:
> 
> 
> On 11/23/2016 11:47 PM, Ville Syrjälä wrote:
> > On Wed, Nov 23, 2016 at 09:48:25PM +0530, Animesh Manna wrote:
> >> _DSM is added to program HPD_CTL(0x1094) register
> >> of PMC from i915 driver which will be called
> >> based on driver feature flag. PMC hpd control register
> >> programming will enable PMC to get hpd interrupt
> >> during dc9.
> >>
> >> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_acpi.c | 44 ++++++++++++++++++++++++++++++++-------
> >>   1 file changed, 37 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
> >> index 8c878ab..15d3b84 100755
> >> --- a/drivers/gpu/drm/i915/intel_acpi.c
> >> +++ b/drivers/gpu/drm/i915/intel_acpi.c
> >> @@ -10,6 +10,8 @@
> >>   
> >>   #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
> >>   #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
> >> +#define INTEL_DSM_SET_HPD_WAKEUP 17
> >> +#define HPD_WAKEUP_EN_VAL 0xFCF0
> >>   
> >>   static struct intel_dsm_priv {
> >>   	acpi_handle dhandle;
> >> @@ -118,6 +120,25 @@ static void intel_dsm_platform_mux_info(void)
> >>   	ACPI_FREE(pkg);
> >>   }
> >>   
> >> +static void intel_dsm_set_hpd_wakeup(u8 *guid)
> >> +{
> >> +	union acpi_object *obj;
> >> +	union acpi_object argv4 = {
> >> +		.integer.type = ACPI_TYPE_INTEGER,
> >> +		.integer.value = HPD_WAKEUP_EN_VAL,
> >> +	};
> >> +
> >> +	obj = acpi_evaluate_dsm_typed(intel_dsm_priv.dhandle, guid,
> >> +			INTEL_DSM_REVISION_ID, INTEL_DSM_SET_HPD_WAKEUP,
> >> +			&argv4, ACPI_TYPE_INTEGER);
> >> +
> >> +	if (!obj)
> >> +		DRM_DEBUG_DRIVER("failed to evaluate _DSM\n");
> >> +
> >> +	ACPI_FREE(obj);
> >> +}
> >> +
> >> +
> >>   static bool intel_dsm_pci_probe(struct pci_dev *pdev)
> >>   {
> >>   	acpi_handle dhandle;
> >> @@ -134,14 +155,23 @@ static bool intel_dsm_pci_probe(struct pci_dev *pdev)
> >>   	else
> >>   		guid = intel_dsm_guid;
> >>   
> >> -	if (!acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
> >> -			    1 << INTEL_DSM_FN_PLATFORM_MUX_INFO)) {
> >> -		DRM_DEBUG_KMS("no _DSM method for intel device\n");
> >> -		return false;
> >> -	}
> >> -
> >>   	intel_dsm_priv.dhandle = dhandle;
> >> -	intel_dsm_platform_mux_info();
> >> +
> >> +	if (acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
> >> +				1 << INTEL_DSM_FN_PLATFORM_MUX_INFO))
> >> +		intel_dsm_platform_mux_info();
> >> +	else
> >> +		DRM_DEBUG_KMS("no _DSM method for mux-info\n");
> >> +
> >> +	/* Need to ensure vbt parsing is completed. */
> > Eh?
> There is a feature flag for oem to enable/disable HPD as wake feature in 
> vbt and on the basis of that PMC_HPD_CTL register will be programmed.
> So added a comment just to have that info captured in code-comment, for 
> any concern let me know, will update accordingly.

The comment is making me thing the VBT parsing would happen
asynchronously somehow, and this would need then to block until it's
finsiehd. Doesn't make any sense to me.

> >
> >> +	if (dev_priv->vbt.hpd_wakeup_enabled &&
> >> +		acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
> >> +				1 << INTEL_DSM_SET_HPD_WAKEUP))
> >> +		intel_dsm_set_hpd_wakeup(guid);
> > So we're permanently routing hpds to the pcu? Won't that mess up
> > stuff like short pulse detection?
> >
> > I was expecting that we'd switch between the PCU and not during
> > runtime suspend/resume.
> Routing hpd to the pcu will only happen during S0ix. As per my 
> understanding for both HDMI and DP we will be getting long pulse during 
> hotplug at S0ix. So I think it will not mess up.
> Correct me if I am missing something.

I have no idea. I don't have the hardware to verify any of that. Please
do that yourself, and add a convicing comment/commit message with your
findings documented.

> 
> - Animesh
> >
> >> +	else {
> >> +		dev_priv->vbt.hpd_wakeup_enabled = false;
> >> +		DRM_DEBUG_KMS("no _DSM method for hpd-enabling\n");
> >> +	}
> >>   
> >>   	return true;
> >>   }
> >> -- 
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
index 8c878ab..15d3b84 100755
--- a/drivers/gpu/drm/i915/intel_acpi.c
+++ b/drivers/gpu/drm/i915/intel_acpi.c
@@ -10,6 +10,8 @@ 
 
 #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
 #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
+#define INTEL_DSM_SET_HPD_WAKEUP 17
+#define HPD_WAKEUP_EN_VAL 0xFCF0
 
 static struct intel_dsm_priv {
 	acpi_handle dhandle;
@@ -118,6 +120,25 @@  static void intel_dsm_platform_mux_info(void)
 	ACPI_FREE(pkg);
 }
 
+static void intel_dsm_set_hpd_wakeup(u8 *guid)
+{
+	union acpi_object *obj;
+	union acpi_object argv4 = {
+		.integer.type = ACPI_TYPE_INTEGER,
+		.integer.value = HPD_WAKEUP_EN_VAL,
+	};
+
+	obj = acpi_evaluate_dsm_typed(intel_dsm_priv.dhandle, guid,
+			INTEL_DSM_REVISION_ID, INTEL_DSM_SET_HPD_WAKEUP,
+			&argv4, ACPI_TYPE_INTEGER);
+
+	if (!obj)
+		DRM_DEBUG_DRIVER("failed to evaluate _DSM\n");
+
+	ACPI_FREE(obj);
+}
+
+
 static bool intel_dsm_pci_probe(struct pci_dev *pdev)
 {
 	acpi_handle dhandle;
@@ -134,14 +155,23 @@  static bool intel_dsm_pci_probe(struct pci_dev *pdev)
 	else
 		guid = intel_dsm_guid;
 
-	if (!acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
-			    1 << INTEL_DSM_FN_PLATFORM_MUX_INFO)) {
-		DRM_DEBUG_KMS("no _DSM method for intel device\n");
-		return false;
-	}
-
 	intel_dsm_priv.dhandle = dhandle;
-	intel_dsm_platform_mux_info();
+
+	if (acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
+				1 << INTEL_DSM_FN_PLATFORM_MUX_INFO))
+		intel_dsm_platform_mux_info();
+	else
+		DRM_DEBUG_KMS("no _DSM method for mux-info\n");
+
+	/* Need to ensure vbt parsing is completed. */
+	if (dev_priv->vbt.hpd_wakeup_enabled &&
+		acpi_check_dsm(dhandle, guid, INTEL_DSM_REVISION_ID,
+				1 << INTEL_DSM_SET_HPD_WAKEUP))
+		intel_dsm_set_hpd_wakeup(guid);
+	else {
+		dev_priv->vbt.hpd_wakeup_enabled = false;
+		DRM_DEBUG_KMS("no _DSM method for hpd-enabling\n");
+	}
 
 	return true;
 }