* RE: [PATCH] drm/amdgpu: Refactor 'amdgpu_connector_dvi_detect' in amdgpu_connectors.c
2023-08-02 2:19 [PATCH] drm/amdgpu: Refactor 'amdgpu_connector_dvi_detect' in amdgpu_connectors.c Srinivasan Shanmugam
@ 2023-08-16 8:05 ` Chen, Guchun
2023-11-07 14:43 ` Aurabindo Pillai
1 sibling, 0 replies; 3+ messages in thread
From: Chen, Guchun @ 2023-08-16 8:05 UTC (permalink / raw)
To: SHANMUGAM, SRINIVASAN, Koenig, Christian, Deucher, Alexander,
Pan, Xinhui
Cc: Pillai, Aurabindo, Siqueira, Rodrigo,
amd-gfx@lists.freedesktop.org
[Public]
Acked-by: Guchun Chen <guchun.chen@amd.com>
And possibly Rodrigo/Jay needs to review as well before pushing.
Regards,
Guchun
> -----Original Message-----
> From: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>
> Sent: Wednesday, August 2, 2023 10:19 AM
> To: Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>;
> Pan, Xinhui <Xinhui.Pan@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM@amd.com>; Siqueira, Rodrigo
> <Rodrigo.Siqueira@amd.com>; Pillai, Aurabindo
> <Aurabindo.Pillai@amd.com>
> Subject: [PATCH] drm/amdgpu: Refactor 'amdgpu_connector_dvi_detect' in
> amdgpu_connectors.c
>
> 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) {
> + 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);
> }
> }
> --
> 2.25.1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] drm/amdgpu: Refactor 'amdgpu_connector_dvi_detect' in amdgpu_connectors.c
2023-08-02 2:19 [PATCH] drm/amdgpu: Refactor 'amdgpu_connector_dvi_detect' in amdgpu_connectors.c Srinivasan Shanmugam
2023-08-16 8:05 ` Chen, Guchun
@ 2023-11-07 14:43 ` Aurabindo Pillai
1 sibling, 0 replies; 3+ messages in thread
From: Aurabindo Pillai @ 2023-11-07 14:43 UTC (permalink / raw)
To: Srinivasan Shanmugam, Christian König, Alex Deucher,
Guchun Chen, Pan Xinhui
Cc: Rodrigo Siqueira, amd-gfx
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>
^ permalink raw reply [flat|nested] 3+ messages in thread