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@xxxxxxxxx>
> Reported-by: Farblos <farblos@xxxxxxxxxxxxxxx>
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);
