Re: [PATCH] drm/amdgpu: Refactor 'amdgpu_connector_dvi_detect' in amdgpu_connectors.c

[Aurabindo Pillai <aurabindo.pillai@amd.com>]

On 8/1/2023 10:19 PM, Srinivasan Shanmugam wrote:
> Fixes the below:
> 
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> WARNING: Missing a blank line after declarations
> WARNING: Too many leading tabs - consider code refactoring
> +                                               if (list_connector->connector_type != DRM_MODE_CONNECTOR_VGA) {
> WARNING: Too many leading tabs - consider code refactoring
> +                                                       if (!amdgpu_display_hpd_sense(adev, amdgpu_connector->hpd.hpd)) {
> 
> Cc: Guchun Chen <guchun.chen@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    | 69 +++++++++++--------
>   1 file changed, 42 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> index d34037b85cf8..173e836b00fd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> @@ -103,7 +103,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector)
>   	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
>   	struct amdgpu_connector_atom_dig *dig_connector;
>   	int bpc = 8;
> -	unsigned mode_clock, max_tmds_clock;
> +	unsigned int mode_clock, max_tmds_clock;
>   
>   	switch (connector->connector_type) {
>   	case DRM_MODE_CONNECTOR_DVII:
> @@ -255,6 +255,7 @@ struct edid *amdgpu_connector_edid(struct drm_connector *connector)
>   		return amdgpu_connector->edid;
>   	} else if (edid_blob) {
>   		struct edid *edid = kmemdup(edid_blob->data, edid_blob->length, GFP_KERNEL);
> +
>   		if (edid)
>   			amdgpu_connector->edid = edid;
>   	}
> @@ -588,6 +589,7 @@ static int amdgpu_connector_set_property(struct drm_connector *connector,
>   			amdgpu_encoder = to_amdgpu_encoder(connector->encoder);
>   		} else {
>   			const struct drm_connector_helper_funcs *connector_funcs = connector->helper_private;
> +
>   			amdgpu_encoder = to_amdgpu_encoder(connector_funcs->best_encoder(connector));
>   		}
>   
> @@ -804,6 +806,7 @@ static int amdgpu_connector_set_lcd_property(struct drm_connector *connector,
>   		amdgpu_encoder = to_amdgpu_encoder(connector->encoder);
>   	else {
>   		const struct drm_connector_helper_funcs *connector_funcs = connector->helper_private;
> +
>   		amdgpu_encoder = to_amdgpu_encoder(connector_funcs->best_encoder(connector));
>   	}
>   
> @@ -986,6 +989,41 @@ amdgpu_connector_check_hpd_status_unchanged(struct drm_connector *connector)
>   	return false;
>   }
>   
> +static void amdgpu_connector_shared_ddc(enum drm_connector_status *status,
> +					struct drm_connector *connector,
> +					struct amdgpu_connector *amdgpu_connector)
> +{
> +	struct drm_connector *list_connector;
> +	struct drm_connector_list_iter iter;
> +	struct amdgpu_connector *list_amdgpu_connector;
> +	struct drm_device *dev = connector->dev;
> +	struct amdgpu_device *adev = drm_to_adev(dev);
> +
> +	if (amdgpu_connector->shared_ddc && *status == connector_status_connected) {
> +		drm_connector_list_iter_begin(dev, &iter);
> +		drm_for_each_connector_iter(list_connector,
> +					    &iter) {

You could probably bring this part to the previous line, since the 80 
character limit is not enforced anymore.
> +			if (connector == list_connector)
> +				continue > +			list_amdgpu_connector = to_amdgpu_connector(list_connector);
> +			if (list_amdgpu_connector->shared_ddc &&
> +			    list_amdgpu_connector->ddc_bus->rec.i2c_id ==
> +			     amdgpu_connector->ddc_bus->rec.i2c_id) {
> +				/* cases where both connectors are digital */
> +				if (list_connector->connector_type != DRM_MODE_CONNECTOR_VGA) {
> +					/* hpd is our only option in this case */
> +					if (!amdgpu_display_hpd_sense(adev,
> +								      amdgpu_connector->hpd.hpd)) {
> +						amdgpu_connector_free_edid(connector);
> +						*status = connector_status_disconnected;
> +					}
> +				}
> +			}
> +		}
> +		drm_connector_list_iter_end(&iter);
> +	}
> +}
> +
>   /*
>    * DVI is complicated
>    * Do a DDC probe, if DDC probe passes, get the full EDID so
> @@ -1072,32 +1110,7 @@ amdgpu_connector_dvi_detect(struct drm_connector *connector, bool force)
>   			 * DDC line.  The latter is more complex because with DVI<->HDMI adapters
>   			 * you don't really know what's connected to which port as both are digital.
>   			 */
> -			if (amdgpu_connector->shared_ddc && (ret == connector_status_connected)) {
> -				struct drm_connector *list_connector;
> -				struct drm_connector_list_iter iter;
> -				struct amdgpu_connector *list_amdgpu_connector;
> -
> -				drm_connector_list_iter_begin(dev, &iter);
> -				drm_for_each_connector_iter(list_connector,
> -							    &iter) {
> -					if (connector == list_connector)
> -						continue;
> -					list_amdgpu_connector = to_amdgpu_connector(list_connector);
> -					if (list_amdgpu_connector->shared_ddc &&
> -					    (list_amdgpu_connector->ddc_bus->rec.i2c_id ==
> -					     amdgpu_connector->ddc_bus->rec.i2c_id)) {
> -						/* cases where both connectors are digital */
> -						if (list_connector->connector_type != DRM_MODE_CONNECTOR_VGA) {
> -							/* hpd is our only option in this case */
> -							if (!amdgpu_display_hpd_sense(adev, amdgpu_connector->hpd.hpd)) {
> -								amdgpu_connector_free_edid(connector);
> -								ret = connector_status_disconnected;
> -							}
> -						}
> -					}
> -				}
> -				drm_connector_list_iter_end(&iter);
> -			}
> +			 amdgpu_connector_shared_ddc(&ret, connector, amdgpu_connector);
>   		}
>   	}
>   
> @@ -1199,6 +1212,7 @@ amdgpu_connector_dvi_encoder(struct drm_connector *connector)
>   static void amdgpu_connector_dvi_force(struct drm_connector *connector)
>   {
>   	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
> +
>   	if (connector->force == DRM_FORCE_ON)
>   		amdgpu_connector->use_digital = false;
>   	if (connector->force == DRM_FORCE_ON_DIGITAL)
> @@ -1433,6 +1447,7 @@ amdgpu_connector_dp_detect(struct drm_connector *connector, bool force)
>   				ret = connector_status_connected;
>   			else if (amdgpu_connector->dac_load_detect) { /* try load detection */
>   				const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
> +
>   				ret = encoder_funcs->detect(encoder, connector);
>   			}
>   		}

With or without the suggested change, the patch is

Reviewed-by: Aurabindo Pillai <aurabindo.pillai@amd.com>