Re: [PATCH 01/12] drm/amdgpu: Write masked value to control register

[Alex Deucher]

On Thu, Jul 14, 2022 at 3:05 PM André Almeida <andrealmeid@xxxxxxxxxx> wrote:
>
> Hi Maíra,
>
> Thank you for your patch,
>
> Às 13:44 de 14/07/22, Maíra Canal escreveu:
> > On the dce_v6_0 and dce_v8_0 hpd tear down callback, the tmp variable
> > should be written into the control register instead of 0.
> >
>
> Why? I do see that tmp was unused before your patch, but why should we
> write it into this register? Did you manage to test this somehow?

The patch is correct.  We should only be clearing the enable bit in
this case, not the entire register.  Clearing the other fields could
cause spurious hotplug events as it affects the short and long pulse
times for the HPD pin.

Alex

>
> > Fixes: b00861b9 ("drm/amd/amdgpu: port of DCE v6 to new headers (v3)")
> > Fixes: 2285b91c ("drm/amdgpu/dce8: simplify hpd code")
>
> Checking both commits, I can see that 0 is written at `mmDC_HPD1_CONTROL
> + N` register in _hpd_fini() in them, so if you are trying to fix the
> commit that inserted that behavior, I think aren't those ones. For instance:
>
> $ git show 2285b91c
>
> [...]
>
> @@ -479,28 +372,11 @@ static void dce_v8_0_hpd_fini(struct amdgpu_device
> *adev)
>         list_for_each_entry(connector, &dev->mode_config.connector_list,
> head) {
>                 struct amdgpu_connector *amdgpu_connector =
> to_amdgpu_connector(connector);
>
> -               switch (amdgpu_connector->hpd.hpd) {
> -               case AMDGPU_HPD_1:
> -                       WREG32(mmDC_HPD1_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_2:
> -                       WREG32(mmDC_HPD2_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_3:
> -                       WREG32(mmDC_HPD3_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_4:
> -                       WREG32(mmDC_HPD4_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_5:
> -                       WREG32(mmDC_HPD5_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_6:
> -                       WREG32(mmDC_HPD6_CONTROL, 0);
> -                       break;
> -               default:
> -                       break;
> -               }
> +               if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
> +                       continue;
> +
> +               WREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], 0);
> +
>
> 0 was the valued written here before this commit. The commit basically
> replaces the switch case with a sum in this snippet.
>
> thanks,
>         andré