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@xxxxxxxxx>
>> Reported-by: Farblos <farblos@xxxxxxxxxxxxxxx>
>
> Fixes: 2c6d1fffa1d9 ("drm: add support for DisplayPort CEC-Tunneling-over-AUX")
Cc: <stable@xxxxxxxxxxxxxxx> # 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);
>
>
