| Message ID | 361bb03d-1691-4e23-84da-0861ead5dbdc@xs4all.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | gpu: drm_dp_cec: fix broken CEC adapter properties check | expand |
On Wed, Jan 29, 2025 at 10:51:48AM +0100, Hans Verkuil wrote: > If the hotplug detect of a display is low for longer than one second > (configurable through drm_dp_cec_unregister_delay), then the CEC adapter > is unregistered since we assume the display was disconnected. If the > HPD went low for less than one second, then we check if the properties > of the CEC adapter have changed, since that indicates that we actually > switch to new hardware and we have to unregister the old CEC device and > register a new one. > > Unfortunately, the test for changed properties was written poorly, and > after a new CEC capability was added to the CEC core code the test always > returned true (i.e. the properties had changed). > > As a result the CEC device was unregistered and re-registered for every > HPD toggle. If the CEC remote controller integration was also enabled > (CONFIG_MEDIA_CEC_RC was set), then the corresponding input device was > also unregistered and re-registered. As a result the input device in > /sys would keep incrementing its number, e.g.: > > /sys/devices/pci0000:00/0000:00:08.1/0000:e7:00.0/rc/rc0/input20 > > Since short HPD toggles are common, the number could over time get into > the thousands. > > While not a serious issue (i.e. nothing crashes), it is not intended > to work that way. > > This patch changes the test so that it only checks for the single CEC > capability that can actually change, and it ignores any other > capabilities, so this is now safe as well if new caps are added in > the future. > > With the changed test the bit under #ifndef CONFIG_MEDIA_CEC_RC can be > dropped as well, so that's a nice cleanup. > > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> > Reported-by: Farblos <farblos@vodafonemail.de> > --- > Jens (aka Farblos), can you test this patch? > --- > drivers/gpu/drm/display/drm_dp_cec.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On 29/01/2025 10:51, Hans Verkuil wrote: > If the hotplug detect of a display is low for longer than one second > (configurable through drm_dp_cec_unregister_delay), then the CEC adapter > is unregistered since we assume the display was disconnected. If the > HPD went low for less than one second, then we check if the properties > of the CEC adapter have changed, since that indicates that we actually > switch to new hardware and we have to unregister the old CEC device and > register a new one. > > Unfortunately, the test for changed properties was written poorly, and > after a new CEC capability was added to the CEC core code the test always > returned true (i.e. the properties had changed). > > As a result the CEC device was unregistered and re-registered for every > HPD toggle. If the CEC remote controller integration was also enabled > (CONFIG_MEDIA_CEC_RC was set), then the corresponding input device was > also unregistered and re-registered. As a result the input device in > /sys would keep incrementing its number, e.g.: > > /sys/devices/pci0000:00/0000:00:08.1/0000:e7:00.0/rc/rc0/input20 > > Since short HPD toggles are common, the number could over time get into > the thousands. > > While not a serious issue (i.e. nothing crashes), it is not intended > to work that way. > > This patch changes the test so that it only checks for the single CEC > capability that can actually change, and it ignores any other > capabilities, so this is now safe as well if new caps are added in > the future. > > With the changed test the bit under #ifndef CONFIG_MEDIA_CEC_RC can be > dropped as well, so that's a nice cleanup. > > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> > Reported-by: Farblos <farblos@vodafonemail.de> Fixes: 2c6d1fffa1d9 ("drm: add support for DisplayPort CEC-Tunneling-over-AUX") Regards, Hans > --- > Jens (aka Farblos), can you test this patch? > --- > drivers/gpu/drm/display/drm_dp_cec.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/display/drm_dp_cec.c b/drivers/gpu/drm/display/drm_dp_cec.c > index 007ceb281d00..56a4965e518c 100644 > --- a/drivers/gpu/drm/display/drm_dp_cec.c > +++ b/drivers/gpu/drm/display/drm_dp_cec.c > @@ -311,16 +311,6 @@ void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address) > if (!aux->transfer) > return; > > -#ifndef CONFIG_MEDIA_CEC_RC > - /* > - * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by > - * cec_allocate_adapter() if CONFIG_MEDIA_CEC_RC is undefined. > - * > - * Do this here as well to ensure the tests against cec_caps are > - * correct. > - */ > - cec_caps &= ~CEC_CAP_RC; > -#endif > cancel_delayed_work_sync(&aux->cec.unregister_work); > > mutex_lock(&aux->cec.lock); > @@ -337,7 +327,9 @@ void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address) > num_las = CEC_MAX_LOG_ADDRS; > > if (aux->cec.adap) { > - if (aux->cec.adap->capabilities == cec_caps && > + /* Check if the adapter properties have changed */ > + if ((aux->cec.adap->capabilities & CEC_CAP_MONITOR_ALL) == > + (cec_caps & CEC_CAP_MONITOR_ALL) && > aux->cec.adap->available_log_addrs == num_las) { > /* Unchanged, so just set the phys addr */ > cec_s_phys_addr(aux->cec.adap, source_physical_address, false);
On Wed, Jan 29, 2025 at 11:56:39PM +0100, Jens Schmidt wrote: > On 2025-01-29 10:51, Hans Verkuil wrote: > > > Jens (aka Farblos), can you test this patch? > > TL;DR: Your patch fixes the issue on my system, thanks. Could you please respond with the 'Tested-by: Your Name <email>' tag on a separate line? > > Details: > > ### build #13 - stock 6.12.10 kernel > > [~]$ uname -a > Linux host01 6.12.10 #13 SMP PREEMPT_DYNAMIC Wed Jan 29 22:10:03 CET 2025 x86_64 GNU/Linux > > [~]$ ls -ald /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input* > drwxr-xr-x 6 root root 0 Jan 29 22:17 /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input30 > > # you also get quite a lot of these without your patch ... > [~]$ journalctl --boot | grep -c 'kernel: Registered IR keymap rc-cec' > 7 > > ### build #14 - 6.12.10 with your patch > > [~]$ uname -a > Linux host01 6.12.10 #14 SMP PREEMPT_DYNAMIC Wed Jan 29 22:24:47 CET 2025 x86_64 GNU/Linux > > [~]$ ls -ald /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input* > drwxr-xr-x 6 root root 0 Jan 29 22:27 /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input11 > > [~]$ journalctl --boot | grep -c 'kernel: Registered IR keymap rc-cec' > 1 > > ... wait ... let screen saver kick in ... wait ... > > [~]$ ls -ald /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input* > drwxr-xr-x 6 root root 0 Jan 29 22:27 /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input11 > > [~]$ journalctl --boot | grep -c 'kernel: Registered IR keymap rc-cec' > 1 >
[Resend to make the automation happy ... thanks for the hint.] On 2025-01-29 10:51, Hans Verkuil wrote: > Jens (aka Farblos), can you test this patch? TL;DR: Your patch fixes the issue on my system, thanks. Tested-by: Farblos <farblos@vodafonemail.de> Details: ### build #13 - stock 6.12.10 kernel [~]$ uname -a Linux host01 6.12.10 #13 SMP PREEMPT_DYNAMIC Wed Jan 29 22:10:03 CET 2025 x86_64 GNU/Linux [~]$ ls -ald /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input* drwxr-xr-x 6 root root 0 Jan 29 22:17 /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input30 # you also get quite a lot of these without your patch ... [~]$ journalctl --boot | grep -c 'kernel: Registered IR keymap rc-cec' 7 ### build #14 - 6.12.10 with your patch [~]$ uname -a Linux host01 6.12.10 #14 SMP PREEMPT_DYNAMIC Wed Jan 29 22:24:47 CET 2025 x86_64 GNU/Linux [~]$ ls -ald /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input* drwxr-xr-x 6 root root 0 Jan 29 22:27 /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input11 [~]$ journalctl --boot | grep -c 'kernel: Registered IR keymap rc-cec' 1 ... wait ... let screen saver kick in ... wait ... [~]$ ls -ald /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input* drwxr-xr-x 6 root root 0 Jan 29 22:27 /sys/devices/pci0000:00/0000:00:02.0/rc/rc0/input11 [~]$ journalctl --boot | grep -c 'kernel: Registered IR keymap rc-cec' 1
On 29/01/2025 13:21, Hans Verkuil wrote: > On 29/01/2025 10:51, Hans Verkuil wrote: >> If the hotplug detect of a display is low for longer than one second >> (configurable through drm_dp_cec_unregister_delay), then the CEC adapter >> is unregistered since we assume the display was disconnected. If the >> HPD went low for less than one second, then we check if the properties >> of the CEC adapter have changed, since that indicates that we actually >> switch to new hardware and we have to unregister the old CEC device and >> register a new one. >> >> Unfortunately, the test for changed properties was written poorly, and >> after a new CEC capability was added to the CEC core code the test always >> returned true (i.e. the properties had changed). >> >> As a result the CEC device was unregistered and re-registered for every >> HPD toggle. If the CEC remote controller integration was also enabled >> (CONFIG_MEDIA_CEC_RC was set), then the corresponding input device was >> also unregistered and re-registered. As a result the input device in >> /sys would keep incrementing its number, e.g.: >> >> /sys/devices/pci0000:00/0000:00:08.1/0000:e7:00.0/rc/rc0/input20 >> >> Since short HPD toggles are common, the number could over time get into >> the thousands. >> >> While not a serious issue (i.e. nothing crashes), it is not intended >> to work that way. >> >> This patch changes the test so that it only checks for the single CEC >> capability that can actually change, and it ignores any other >> capabilities, so this is now safe as well if new caps are added in >> the future. >> >> With the changed test the bit under #ifndef CONFIG_MEDIA_CEC_RC can be >> dropped as well, so that's a nice cleanup. >> >> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> >> Reported-by: Farblos <farblos@vodafonemail.de> > > Fixes: 2c6d1fffa1d9 ("drm: add support for DisplayPort CEC-Tunneling-over-AUX") Cc: <stable@vger.kernel.org> # 6.12 While the bug has been present since the introduction of drm_dp_cec.c, it lay dormant until a new CEC capability was introduced in 6.12. So this fix doesn't need to be backported all the way, just from 6.12 onwards. Dmitry, do you want to pick this up? I can do it as well, but it is quite some time ago since I last worked with drm. Regards, Hans > > Regards, > > Hans > >> --- >> Jens (aka Farblos), can you test this patch? >> --- >> drivers/gpu/drm/display/drm_dp_cec.c | 14 +++----------- >> 1 file changed, 3 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/display/drm_dp_cec.c b/drivers/gpu/drm/display/drm_dp_cec.c >> index 007ceb281d00..56a4965e518c 100644 >> --- a/drivers/gpu/drm/display/drm_dp_cec.c >> +++ b/drivers/gpu/drm/display/drm_dp_cec.c >> @@ -311,16 +311,6 @@ void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address) >> if (!aux->transfer) >> return; >> >> -#ifndef CONFIG_MEDIA_CEC_RC >> - /* >> - * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by >> - * cec_allocate_adapter() if CONFIG_MEDIA_CEC_RC is undefined. >> - * >> - * Do this here as well to ensure the tests against cec_caps are >> - * correct. >> - */ >> - cec_caps &= ~CEC_CAP_RC; >> -#endif >> cancel_delayed_work_sync(&aux->cec.unregister_work); >> >> mutex_lock(&aux->cec.lock); >> @@ -337,7 +327,9 @@ void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address) >> num_las = CEC_MAX_LOG_ADDRS; >> >> if (aux->cec.adap) { >> - if (aux->cec.adap->capabilities == cec_caps && >> + /* Check if the adapter properties have changed */ >> + if ((aux->cec.adap->capabilities & CEC_CAP_MONITOR_ALL) == >> + (cec_caps & CEC_CAP_MONITOR_ALL) && >> aux->cec.adap->available_log_addrs == num_las) { >> /* Unchanged, so just set the phys addr */ >> cec_s_phys_addr(aux->cec.adap, source_physical_address, false); > >
On Wed, 29 Jan 2025 10:51:48 +0100, Hans Verkuil wrote: > If the hotplug detect of a display is low for longer than one second > (configurable through drm_dp_cec_unregister_delay), then the CEC adapter > is unregistered since we assume the display was disconnected. If the > HPD went low for less than one second, then we check if the properties > of the CEC adapter have changed, since that indicates that we actually > switch to new hardware and we have to unregister the old CEC device and > register a new one. > > [...] Applied to drm-misc-next-fixes, thanks! [1/1] gpu: drm_dp_cec: fix broken CEC adapter properties check commit: 6daaae5ff7f3b23a2dacc9c387ff3d4f95b67cad Best regards,
diff --git a/drivers/gpu/drm/display/drm_dp_cec.c b/drivers/gpu/drm/display/drm_dp_cec.c index 007ceb281d00..56a4965e518c 100644 --- a/drivers/gpu/drm/display/drm_dp_cec.c +++ b/drivers/gpu/drm/display/drm_dp_cec.c @@ -311,16 +311,6 @@ void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address) if (!aux->transfer) return; -#ifndef CONFIG_MEDIA_CEC_RC - /* - * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by - * cec_allocate_adapter() if CONFIG_MEDIA_CEC_RC is undefined. - * - * Do this here as well to ensure the tests against cec_caps are - * correct. - */ - cec_caps &= ~CEC_CAP_RC; -#endif cancel_delayed_work_sync(&aux->cec.unregister_work); mutex_lock(&aux->cec.lock); @@ -337,7 +327,9 @@ void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address) num_las = CEC_MAX_LOG_ADDRS; if (aux->cec.adap) { - if (aux->cec.adap->capabilities == cec_caps && + /* Check if the adapter properties have changed */ + if ((aux->cec.adap->capabilities & CEC_CAP_MONITOR_ALL) == + (cec_caps & CEC_CAP_MONITOR_ALL) && aux->cec.adap->available_log_addrs == num_las) { /* Unchanged, so just set the phys addr */ cec_s_phys_addr(aux->cec.adap, source_physical_address, false);
If the hotplug detect of a display is low for longer than one second (configurable through drm_dp_cec_unregister_delay), then the CEC adapter is unregistered since we assume the display was disconnected. If the HPD went low for less than one second, then we check if the properties of the CEC adapter have changed, since that indicates that we actually switch to new hardware and we have to unregister the old CEC device and register a new one. Unfortunately, the test for changed properties was written poorly, and after a new CEC capability was added to the CEC core code the test always returned true (i.e. the properties had changed). As a result the CEC device was unregistered and re-registered for every HPD toggle. If the CEC remote controller integration was also enabled (CONFIG_MEDIA_CEC_RC was set), then the corresponding input device was also unregistered and re-registered. As a result the input device in /sys would keep incrementing its number, e.g.: /sys/devices/pci0000:00/0000:00:08.1/0000:e7:00.0/rc/rc0/input20 Since short HPD toggles are common, the number could over time get into the thousands. While not a serious issue (i.e. nothing crashes), it is not intended to work that way. This patch changes the test so that it only checks for the single CEC capability that can actually change, and it ignores any other capabilities, so this is now safe as well if new caps are added in the future. With the changed test the bit under #ifndef CONFIG_MEDIA_CEC_RC can be dropped as well, so that's a nice cleanup. Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> Reported-by: Farblos <farblos@vodafonemail.de> --- Jens (aka Farblos), can you test this patch? --- drivers/gpu/drm/display/drm_dp_cec.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)