Re: [PATCH 01/12] drm/amdgpu: Write masked value to control register
Applied. Thanks!
On Thu, Jul 14, 2022 at 12:45 PM Maíra Canal wrote:
>
> 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.
>
> Fixes: b00861b9 ("drm/amd/amdgpu: port of DCE v6 to new headers (v3)")
> Fixes: 2285b91c ("drm/amdgpu/dce8: simplify hpd code")
> Signed-off-by: Maíra Canal
> ---
> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index f5a29526684d..0a7b1c002822 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -339,7 +339,7 @@ static void dce_v6_0_hpd_fini(struct amdgpu_device *adev)
>
> tmp = RREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd]);
> tmp &= ~DC_HPD1_CONTROL__DC_HPD1_EN_MASK;
> - WREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], 0);
> + WREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], tmp);
>
> amdgpu_irq_put(adev, >hpd_irq,
> amdgpu_connector->hpd.hpd);
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> index 780a8aa972fe..f57f4a25cf5a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -333,7 +333,7 @@ static void dce_v8_0_hpd_fini(struct amdgpu_device *adev)
>
> tmp = RREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd]);
> tmp &= ~DC_HPD1_CONTROL__DC_HPD1_EN_MASK;
> - WREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], 0);
> + WREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], tmp);
>
> amdgpu_irq_put(adev, >hpd_irq,
> amdgpu_connector->hpd.hpd);
> }
> --
> 2.36.1
>
Re: [PATCH 01/12] drm/amdgpu: Write masked value to control register
À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.
>
> Fixes: b00861b9 ("drm/amd/amdgpu: port of DCE v6 to new headers (v3)")
> Fixes: 2285b91c ("drm/amdgpu/dce8: simplify hpd code")
> Signed-off-by: Maíra Canal
Series is Reviewed-by: André Almeida
Re: [PATCH 01/12] drm/amdgpu: Write masked value to control register
Às 16:14 de 14/07/22, Alex Deucher escreveu: > On Thu, Jul 14, 2022 at 3:05 PM André Almeida 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. > Got it, nice catch Maíra :) Next time, please add this kind of information in the commit message.
Re: [PATCH 01/12] drm/amdgpu: Write masked value to control register
On Thu, Jul 14, 2022 at 3:05 PM André Almeida 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, >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é
Re: [PATCH 01/12] drm/amdgpu: Write masked value to control register
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?
> 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, >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é
[PATCH 01/12] drm/amdgpu: Write masked value to control register
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.
Fixes: b00861b9 ("drm/amd/amdgpu: port of DCE v6 to new headers (v3)")
Fixes: 2285b91c ("drm/amdgpu/dce8: simplify hpd code")
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index f5a29526684d..0a7b1c002822 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -339,7 +339,7 @@ static void dce_v6_0_hpd_fini(struct amdgpu_device *adev)
tmp = RREG32(mmDC_HPD1_CONTROL +
hpd_offsets[amdgpu_connector->hpd.hpd]);
tmp &= ~DC_HPD1_CONTROL__DC_HPD1_EN_MASK;
- WREG32(mmDC_HPD1_CONTROL +
hpd_offsets[amdgpu_connector->hpd.hpd], 0);
+ WREG32(mmDC_HPD1_CONTROL +
hpd_offsets[amdgpu_connector->hpd.hpd], tmp);
amdgpu_irq_put(adev, >hpd_irq, amdgpu_connector->hpd.hpd);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 780a8aa972fe..f57f4a25cf5a 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -333,7 +333,7 @@ static void dce_v8_0_hpd_fini(struct amdgpu_device *adev)
tmp = RREG32(mmDC_HPD1_CONTROL +
hpd_offsets[amdgpu_connector->hpd.hpd]);
tmp &= ~DC_HPD1_CONTROL__DC_HPD1_EN_MASK;
- WREG32(mmDC_HPD1_CONTROL +
hpd_offsets[amdgpu_connector->hpd.hpd], 0);
+ WREG32(mmDC_HPD1_CONTROL +
hpd_offsets[amdgpu_connector->hpd.hpd], tmp);
amdgpu_irq_put(adev, >hpd_irq, amdgpu_connector->hpd.hpd);
}
--
2.36.1
