diff mbox

drm/radeon/dp: check for errors in dpcd reads

Message ID 1398864435-16246-1-git-send-email-alexander.deucher@amd.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Deucher April 30, 2014, 1:27 p.m. UTC
Check to make sure the transaction succeeded before
using the register value.  Fixes occasional link training
problems.

Noticed-by: Sergei Antonov <saproj@gmail.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/radeon/atombios_dp.c | 44 ++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 19 deletions(-)

Comments

Christian König April 30, 2014, 1:58 p.m. UTC | #1
Am 30.04.2014 15:27, schrieb Alex Deucher:
> Check to make sure the transaction succeeded before
> using the register value.  Fixes occasional link training
> problems.
>
> Noticed-by: Sergei Antonov <saproj@gmail.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

Applied to my 3.15 queue.

Christian.

> ---
>   drivers/gpu/drm/radeon/atombios_dp.c | 44 ++++++++++++++++++++----------------
>   1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c
> index bc0119f..54e4f52 100644
> --- a/drivers/gpu/drm/radeon/atombios_dp.c
> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
> @@ -366,11 +366,11 @@ static void radeon_dp_probe_oui(struct radeon_connector *radeon_connector)
>   	if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
>   		return;
>   
> -	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_SINK_OUI, buf, 3))
> +	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_SINK_OUI, buf, 3) == 3)
>   		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>   			      buf[0], buf[1], buf[2]);
>   
> -	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_BRANCH_OUI, buf, 3))
> +	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_BRANCH_OUI, buf, 3) == 3)
>   		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>   			      buf[0], buf[1], buf[2]);
>   }
> @@ -419,21 +419,23 @@ int radeon_dp_get_panel_mode(struct drm_encoder *encoder,
>   
>   	if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>   		/* DP bridge chips */
> -		drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> -				  DP_EDP_CONFIGURATION_CAP, &tmp);
> -		if (tmp & 1)
> -			panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> -		else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
> -			 (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
> -			panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
> -		else
> -			panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
> +		if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> +				      DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> +			if (tmp & 1)
> +				panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> +			else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
> +				 (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
> +				panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
> +			else
> +				panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
> +		}
>   	} else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>   		/* eDP */
> -		drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> -				  DP_EDP_CONFIGURATION_CAP, &tmp);
> -		if (tmp & 1)
> -			panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> +		if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> +				      DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> +			if (tmp & 1)
> +				panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> +		}
>   	}
>   
>   	return panel_mode;
> @@ -809,11 +811,15 @@ void radeon_dp_link_train(struct drm_encoder *encoder,
>   	else
>   		dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
>   
> -	drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_MAX_LANE_COUNT, &tmp);
> -	if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
> -		dp_info.tp3_supported = true;
> -	else
> +	if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_MAX_LANE_COUNT, &tmp)
> +	    == 1) {
> +		if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
> +			dp_info.tp3_supported = true;
> +		else
> +			dp_info.tp3_supported = false;
> +	} else {
>   		dp_info.tp3_supported = false;
> +	}
>   
>   	memcpy(dp_info.dpcd, dig_connector->dpcd, DP_RECEIVER_CAP_SIZE);
>   	dp_info.rdev = rdev;
Sergei Antonov May 5, 2014, 8:55 p.m. UTC | #2
> Am 30.04.2014 um 15:58 schrieb Christian König <deathsimple@vodafone.de>:
> 
> Am 30.04.2014 15:27, schrieb Alex Deucher:
>> Check to make sure the transaction succeeded before
>> using the register value.  Fixes occasional link training
>> problems.
>> 
>> Noticed-by: Sergei Antonov <saproj@gmail.com>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> 
> Applied to my 3.15 queue.
> 
> Christian.

Why is not this patch still in Linus' tree? I looked forward to seeing it in 3.15-rc4 but it is not there.

> 
>> ---
>>  drivers/gpu/drm/radeon/atombios_dp.c | 44 ++++++++++++++++++++----------------
>>  1 file changed, 25 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c
>> index bc0119f..54e4f52 100644
>> --- a/drivers/gpu/drm/radeon/atombios_dp.c
>> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
>> @@ -366,11 +366,11 @@ static void radeon_dp_probe_oui(struct radeon_connector *radeon_connector)
>>      if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
>>          return;
>>  -    if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_SINK_OUI, buf, 3))
>> +    if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_SINK_OUI, buf, 3) == 3)
>>          DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>>                    buf[0], buf[1], buf[2]);
>>  -    if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_BRANCH_OUI, buf, 3))
>> +    if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_BRANCH_OUI, buf, 3) == 3)
>>          DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>>                    buf[0], buf[1], buf[2]);
>>  }
>> @@ -419,21 +419,23 @@ int radeon_dp_get_panel_mode(struct drm_encoder *encoder,
>>        if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>          /* DP bridge chips */
>> -        drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> -                  DP_EDP_CONFIGURATION_CAP, &tmp);
>> -        if (tmp & 1)
>> -            panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>> -        else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>> -             (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>> -            panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>> -        else
>> -            panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>> +        if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> +                      DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>> +            if (tmp & 1)
>> +                panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>> +            else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>> +                 (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>> +                panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>> +            else
>> +                panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>> +        }
>>      } else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>>          /* eDP */
>> -        drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> -                  DP_EDP_CONFIGURATION_CAP, &tmp);
>> -        if (tmp & 1)
>> -            panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>> +        if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> +                      DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>> +            if (tmp & 1)
>> +                panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>> +        }
>>      }
>>        return panel_mode;
>> @@ -809,11 +811,15 @@ void radeon_dp_link_train(struct drm_encoder *encoder,
>>      else
>>          dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
>>  -    drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_MAX_LANE_COUNT, &tmp);
>> -    if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
>> -        dp_info.tp3_supported = true;
>> -    else
>> +    if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_MAX_LANE_COUNT, &tmp)
>> +        == 1) {
>> +        if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
>> +            dp_info.tp3_supported = true;
>> +        else
>> +            dp_info.tp3_supported = false;
>> +    } else {
>>          dp_info.tp3_supported = false;
>> +    }
>>        memcpy(dp_info.dpcd, dig_connector->dpcd, DP_RECEIVER_CAP_SIZE);
>>      dp_info.rdev = rdev;
>
Deucher, Alexander May 5, 2014, 8:56 p.m. UTC | #3
> -----Original Message-----

> From: Sergei Antonov [mailto:saproj@gmail.com]

> Sent: Monday, May 05, 2014 4:55 PM

> To: Christian König

> Cc: Alex Deucher; dri-devel@lists.freedesktop.org; Deucher, Alexander

> Subject: Re: [PATCH] drm/radeon/dp: check for errors in dpcd reads

> 

> 

> 

> > Am 30.04.2014 um 15:58 schrieb Christian König

> <deathsimple@vodafone.de>:

> >

> > Am 30.04.2014 15:27, schrieb Alex Deucher:

> >> Check to make sure the transaction succeeded before

> >> using the register value.  Fixes occasional link training

> >> problems.

> >>

> >> Noticed-by: Sergei Antonov <saproj@gmail.com>

> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

> >

> > Applied to my 3.15 queue.

> >

> > Christian.

> 

> Why is not this patch still in Linus' tree? I looked forward to seeing it in 3.15-

> rc4 but it is not there.


It hasn't been pulled into Dave's tree yet.  It should show up in his next pull request to Linus this week.

Alex

> 

> >

> >> ---

> >>  drivers/gpu/drm/radeon/atombios_dp.c | 44 ++++++++++++++++++++-

> ---------------

> >>  1 file changed, 25 insertions(+), 19 deletions(-)

> >>

> >> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c

> b/drivers/gpu/drm/radeon/atombios_dp.c

> >> index bc0119f..54e4f52 100644

> >> --- a/drivers/gpu/drm/radeon/atombios_dp.c

> >> +++ b/drivers/gpu/drm/radeon/atombios_dp.c

> >> @@ -366,11 +366,11 @@ static void radeon_dp_probe_oui(struct

> radeon_connector *radeon_connector)

> >>      if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] &

> DP_OUI_SUPPORT))

> >>          return;

> >>  -    if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,

> DP_SINK_OUI, buf, 3))

> >> +    if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,

> DP_SINK_OUI, buf, 3) == 3)

> >>          DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",

> >>                    buf[0], buf[1], buf[2]);

> >>  -    if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,

> DP_BRANCH_OUI, buf, 3))

> >> +    if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,

> DP_BRANCH_OUI, buf, 3) == 3)

> >>          DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",

> >>                    buf[0], buf[1], buf[2]);

> >>  }

> >> @@ -419,21 +419,23 @@ int radeon_dp_get_panel_mode(struct

> drm_encoder *encoder,

> >>        if (dp_bridge != ENCODER_OBJECT_ID_NONE) {

> >>          /* DP bridge chips */

> >> -        drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,

> >> -                  DP_EDP_CONFIGURATION_CAP, &tmp);

> >> -        if (tmp & 1)

> >> -            panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;

> >> -        else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||

> >> -             (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))

> >> -            panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;

> >> -        else

> >> -            panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;

> >> +        if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,

> >> +                      DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {

> >> +            if (tmp & 1)

> >> +                panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;

> >> +            else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||

> >> +                 (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))

> >> +                panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;

> >> +            else

> >> +                panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;

> >> +        }

> >>      } else if (connector->connector_type ==

> DRM_MODE_CONNECTOR_eDP) {

> >>          /* eDP */

> >> -        drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,

> >> -                  DP_EDP_CONFIGURATION_CAP, &tmp);

> >> -        if (tmp & 1)

> >> -            panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;

> >> +        if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,

> >> +                      DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {

> >> +            if (tmp & 1)

> >> +                panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;

> >> +        }

> >>      }

> >>        return panel_mode;

> >> @@ -809,11 +811,15 @@ void radeon_dp_link_train(struct drm_encoder

> *encoder,

> >>      else

> >>          dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;

> >>  -    drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,

> DP_MAX_LANE_COUNT, &tmp);

> >> -    if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))

> >> -        dp_info.tp3_supported = true;

> >> -    else

> >> +    if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,

> DP_MAX_LANE_COUNT, &tmp)

> >> +        == 1) {

> >> +        if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))

> >> +            dp_info.tp3_supported = true;

> >> +        else

> >> +            dp_info.tp3_supported = false;

> >> +    } else {

> >>          dp_info.tp3_supported = false;

> >> +    }

> >>        memcpy(dp_info.dpcd, dig_connector->dpcd,

> DP_RECEIVER_CAP_SIZE);

> >>      dp_info.rdev = rdev;

> >
diff mbox

Patch

diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c
index bc0119f..54e4f52 100644
--- a/drivers/gpu/drm/radeon/atombios_dp.c
+++ b/drivers/gpu/drm/radeon/atombios_dp.c
@@ -366,11 +366,11 @@  static void radeon_dp_probe_oui(struct radeon_connector *radeon_connector)
 	if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
 		return;
 
-	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_SINK_OUI, buf, 3))
+	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_SINK_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 
-	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_BRANCH_OUI, buf, 3))
+	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_BRANCH_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 }
@@ -419,21 +419,23 @@  int radeon_dp_get_panel_mode(struct drm_encoder *encoder,
 
 	if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
 		/* DP bridge chips */
-		drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
-				  DP_EDP_CONFIGURATION_CAP, &tmp);
-		if (tmp & 1)
-			panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
-		else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
-			 (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
-			panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
-		else
-			panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
+		if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
+				      DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
+			if (tmp & 1)
+				panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
+			else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
+				 (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
+				panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
+			else
+				panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
+		}
 	} else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
 		/* eDP */
-		drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
-				  DP_EDP_CONFIGURATION_CAP, &tmp);
-		if (tmp & 1)
-			panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
+		if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
+				      DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
+			if (tmp & 1)
+				panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
+		}
 	}
 
 	return panel_mode;
@@ -809,11 +811,15 @@  void radeon_dp_link_train(struct drm_encoder *encoder,
 	else
 		dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
 
-	drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_MAX_LANE_COUNT, &tmp);
-	if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
-		dp_info.tp3_supported = true;
-	else
+	if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_MAX_LANE_COUNT, &tmp)
+	    == 1) {
+		if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
+			dp_info.tp3_supported = true;
+		else
+			dp_info.tp3_supported = false;
+	} else {
 		dp_info.tp3_supported = false;
+	}
 
 	memcpy(dp_info.dpcd, dig_connector->dpcd, DP_RECEIVER_CAP_SIZE);
 	dp_info.rdev = rdev;