[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-media
Subject:    Re: [PATCH 1/1] media: uvcvideo: Support devices that report an OT as an entity source
From:       Hans de Goede <hdegoede () redhat ! com>
Date:       2021-03-31 11:37:26
Message-ID: 7000176e-4819-b8f2-2152-ec1af4d4164d () redhat ! com
[Download RAW message or body]

Hi,

On 3/31/21 1:03 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Mar 31, 2021 at 12:38:07PM +0200, Hans de Goede wrote:
>> On 3/8/21 11:31 AM, Hans de Goede wrote:
>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>> Some devices reference an output terminal as the source of extension
>>> units. This is incorrect, as output terminals only have an input pin,
>>> and thus can't be connected to any entity in the forward direction. The
>>> resulting topology would cause issues when registering the media
>>> controller graph. To avoid this problem, connect the extension unit to
>>> the source of the output terminal instead.
>>>
>>> While at it, and while no device has been reported to be affected by
>>> this issue, also handle forward scans where two output terminals would
>>> be connected together, and skip the terminals found through such an
>>> invalid connection.
>>>
>>> Reported-and-tested-by: John Nealy <jnealy3@yahoo.com>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Ping? This is a bug-fix which fixes a WARN triggering, causing many
>> users to see a backtrace in their kernel-logs and reporting bugs about this:
>>
>> https://bugzilla.redhat.com/buglist.cgi?quicksearch=mc-entity.c
>>
>> Currently shows 12 open bugs for this and this is not counting all the
>> ones which have already been triaged and matched as dups.
>>
>> As such it would be really good if we can get this merged and on
>> its way to 5.12-rc# as a fix for 5.12 (and to be backported to the
>> stable series).
> 
> The patch is included in "[GIT PULL FOR v5.13] Miscellaneous changes".

Ah I missed that.

> I have no personal issue with it being merged in v5.12-rc, but technically
> it's not a regression fix, is it ? I'll let Mauro decide what works best
> for him.

It is true that this is not a regression-fix but it is a bug-fix and for
a bug which users are actively hitting.

Regards,

Hans


> 
>>> ---
>>>  drivers/media/usb/uvc/uvc_driver.c | 32 ++++++++++++++++++++++++++++++
>>>  1 file changed, 32 insertions(+)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>> index 30ef2a3110f7..8df58f04dac6 100644
>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>> @@ -1716,6 +1716,31 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
>>>  				return -EINVAL;
>>>  			}
>>>  
>>> +			/*
>>> +			 * Some devices reference an output terminal as the
>>> +			 * source of extension units. This is incorrect, as
>>> +			 * output terminals only have an input pin, and thus
>>> +			 * can't be connected to any entity in the forward
>>> +			 * direction. The resulting topology would cause issues
>>> +			 * when registering the media controller graph. To
>>> +			 * avoid this problem, connect the extension unit to
>>> +			 * the source of the output terminal instead.
>>> +			 */
>>> +			if (UVC_ENTITY_IS_OTERM(entity)) {
>>> +				struct uvc_entity *source;
>>> +
>>> +				source = uvc_entity_by_id(chain->dev,
>>> +							  entity->baSourceID[0]);
>>> +				if (!source) {
>>> +					uvc_dbg(chain->dev, DESCR,
>>> +						"Can't connect extension unit %u in chain\n",
>>> +						forward->id);
>>> +					break;
>>> +				}
>>> +
>>> +				forward->baSourceID[0] = source->id;
>>> +			}
>>> +
>>>  			list_add_tail(&forward->chain, &chain->entities);
>>>  			if (!found)
>>>  				uvc_dbg_cont(PROBE, " (->");
>>> @@ -1735,6 +1760,13 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
>>>  				return -EINVAL;
>>>  			}
>>>  
>>> +			if (UVC_ENTITY_IS_OTERM(entity)) {
>>> +				uvc_dbg(chain->dev, DESCR,
>>> +					"Unsupported connection between output terminals %u and %u\n",
>>> +					entity->id, forward->id);
>>> +				break;
>>> +			}
>>> +
>>>  			list_add_tail(&forward->chain, &chain->entities);
>>>  			if (!found)
>>>  				uvc_dbg_cont(PROBE, " (->");
> 

[prev in list] [next in list] [prev in thread] [next in thread]