| Message ID | 20250625155308.2325438-1-dan.scally@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
On Wed, Jun 25, 2025 at 04:53:06PM +0100, Daniel Scally wrote: > There are a couple of places in libcamera that fetch a MediaEntity > from a MediaDevice using the entity's name. This has raised an issue > lately on the RZ/V2H platforms, as the drivers for some of the > hardware give the entities that they create names that incorporate > things like the memory address for the hardware's registers. If we > simply use those names as-is then the pipeline handler would only > work for the instance of the hardware that's at the hard-coded > address and not any of the others. This raises a few questions: - Should drivers do that ? - If not, can we fix them ? Entities exist in the context of a media graph. Including addresses in entity names seems pointless if the address is already conveyed by the media device, and it makes userspace more complex (as shown by this series). If a media graph contains multiple entities of the same type, they need to be differentiated. Some drivers use an index (e.g. indexing video devices at the output of a CSI-2 receiver). Other use addresses, which may be the best option when there is no intrinsic index (e.g. two identical CSI-2 RX connected to the same ISP). Still brainstorming, I wonder if that case should be handled by exposing a bus info for entities, in addition to their names. That would be a more complex API extension though, and would require updates in some userspace tools. In any case, I think we need to document the entity naming best practices in the kernel. > To work around the issue, this series updates libcamera to use regex > matching when searching for a MediaEntity by name. Existing call > sites which just pass a normal string should work unaffected, but new > call sites could use a string that builds a valid regular expression > instead. > > Daniel Scally (2): > libcamera: device_enumerator: Use regex to match entity names > libcamera: media_device: Use a regex to match entity name > > .../libcamera/internal/device_enumerator.h | 3 ++- > src/libcamera/device_enumerator.cpp | 27 ++++++++++--------- > src/libcamera/media_device.cpp | 12 ++++++--- > src/libcamera/v4l2_subdevice.cpp | 4 +-- > 4 files changed, 26 insertions(+), 20 deletions(-)
Hi Laurent On 25/06/2025 23:18, Laurent Pinchart wrote: > On Wed, Jun 25, 2025 at 04:53:06PM +0100, Daniel Scally wrote: >> There are a couple of places in libcamera that fetch a MediaEntity >> from a MediaDevice using the entity's name. This has raised an issue >> lately on the RZ/V2H platforms, as the drivers for some of the >> hardware give the entities that they create names that incorporate >> things like the memory address for the hardware's registers. If we >> simply use those names as-is then the pipeline handler would only >> work for the instance of the hardware that's at the hard-coded >> address and not any of the others. > This raises a few questions: > > - Should drivers do that ? Ideally not > - If not, can we fix them ? We can take the addresses out sure, but is it still worth making some allowance for poorly behaved drivers? > Entities exist in the context of a media graph. Including addresses in > entity names seems pointless if the address is already conveyed by the > media device, and it makes userspace more complex (as shown by this > series). > > If a media graph contains multiple entities of the same type, they need > to be differentiated. Some drivers use an index (e.g. indexing video > devices at the output of a CSI-2 receiver). Other use addresses, which > may be the best option when there is no intrinsic index (e.g. two > identical CSI-2 RX connected to the same ISP). Separate graphs in this case, which makes it difficult to index them given there'll be no interaction between the different instances of the driver - I guess that's why they chose address. > > Still brainstorming, I wonder if that case should be handled by exposing > a bus info for entities, in addition to their names. That would be a > more complex API extension though, and would require updates in some > userspace tools. In any case, I think we need to document the entity > naming best practices in the kernel. The media device does expose a bus_info field already which might be sufficient? At least in this case it is; it contains the address of the CRU instance but since there's a 1:1 relationship between the CRU and CSI it's enough to identify which of the four physical devices the media_device relates to. Thanks Dan >> To work around the issue, this series updates libcamera to use regex >> matching when searching for a MediaEntity by name. Existing call >> sites which just pass a normal string should work unaffected, but new >> call sites could use a string that builds a valid regular expression >> instead. >> >> Daniel Scally (2): >> libcamera: device_enumerator: Use regex to match entity names >> libcamera: media_device: Use a regex to match entity name >> >> .../libcamera/internal/device_enumerator.h | 3 ++- >> src/libcamera/device_enumerator.cpp | 27 ++++++++++--------- >> src/libcamera/media_device.cpp | 12 ++++++--- >> src/libcamera/v4l2_subdevice.cpp | 4 +-- >> 4 files changed, 26 insertions(+), 20 deletions(-)
Hi Laurent On 26/06/2025 10:33, Dan Scally wrote: > Hi Laurent > > On 25/06/2025 23:18, Laurent Pinchart wrote: >> On Wed, Jun 25, 2025 at 04:53:06PM +0100, Daniel Scally wrote: >>> There are a couple of places in libcamera that fetch a MediaEntity >>> from a MediaDevice using the entity's name. This has raised an issue >>> lately on the RZ/V2H platforms, as the drivers for some of the >>> hardware give the entities that they create names that incorporate >>> things like the memory address for the hardware's registers. If we >>> simply use those names as-is then the pipeline handler would only >>> work for the instance of the hardware that's at the hard-coded >>> address and not any of the others. >> This raises a few questions: >> >> - Should drivers do that ? > Ideally not >> - If not, can we fix them ? > > > We can take the addresses out sure, but is it still worth making some allowance for poorly behaved > drivers? ...or thinking about it, _can_ we change them? Would they not count as part of the Kernel's public APIs? > >> Entities exist in the context of a media graph. Including addresses in >> entity names seems pointless if the address is already conveyed by the >> media device, and it makes userspace more complex (as shown by this >> series). >> >> If a media graph contains multiple entities of the same type, they need >> to be differentiated. Some drivers use an index (e.g. indexing video >> devices at the output of a CSI-2 receiver). Other use addresses, which >> may be the best option when there is no intrinsic index (e.g. two >> identical CSI-2 RX connected to the same ISP). > Separate graphs in this case, which makes it difficult to index them given there'll be no > interaction between the different instances of the driver - I guess that's why they chose address. >> >> Still brainstorming, I wonder if that case should be handled by exposing >> a bus info for entities, in addition to their names. That would be a >> more complex API extension though, and would require updates in some >> userspace tools. In any case, I think we need to document the entity >> naming best practices in the kernel. > > The media device does expose a bus_info field already which might be sufficient? At least in this > case it is; it contains the address of the CRU instance but since there's a 1:1 relationship > between the CRU and CSI it's enough to identify which of the four physical devices the > media_device relates to. > > > Thanks > > Dan > >>> To work around the issue, this series updates libcamera to use regex >>> matching when searching for a MediaEntity by name. Existing call >>> sites which just pass a normal string should work unaffected, but new >>> call sites could use a string that builds a valid regular expression >>> instead. >>> >>> Daniel Scally (2): >>> libcamera: device_enumerator: Use regex to match entity names >>> libcamera: media_device: Use a regex to match entity name >>> >>> .../libcamera/internal/device_enumerator.h | 3 ++- >>> src/libcamera/device_enumerator.cpp | 27 ++++++++++--------- >>> src/libcamera/media_device.cpp | 12 ++++++--- >>> src/libcamera/v4l2_subdevice.cpp | 4 +-- >>> 4 files changed, 26 insertions(+), 20 deletions(-)
On Thu, Jun 26, 2025 at 10:33:17AM +0100, Daniel Scally wrote: > On 25/06/2025 23:18, Laurent Pinchart wrote: > > On Wed, Jun 25, 2025 at 04:53:06PM +0100, Daniel Scally wrote: > >> There are a couple of places in libcamera that fetch a MediaEntity > >> from a MediaDevice using the entity's name. This has raised an issue > >> lately on the RZ/V2H platforms, as the drivers for some of the > >> hardware give the entities that they create names that incorporate > >> things like the memory address for the hardware's registers. If we > >> simply use those names as-is then the pipeline handler would only > >> work for the instance of the hardware that's at the hard-coded > >> address and not any of the others. > > > > This raises a few questions: > > > > - Should drivers do that ? > > Ideally not > > > - If not, can we fix them ? > > We can take the addresses out sure, but is it still worth making some > allowance for poorly behaved drivers? In my experience, by being more strict in libcamera we can ensure that drivers stop behaving poorly. Of course we may need to support existing drivers that we can't change without breaking userspace, but for new drivers we should do things right in the kernel. > > Entities exist in the context of a media graph. Including addresses in > > entity names seems pointless if the address is already conveyed by the > > media device, and it makes userspace more complex (as shown by this > > series). > > > > If a media graph contains multiple entities of the same type, they need > > to be differentiated. Some drivers use an index (e.g. indexing video > > devices at the output of a CSI-2 receiver). Other use addresses, which > > may be the best option when there is no intrinsic index (e.g. two > > identical CSI-2 RX connected to the same ISP). > > Separate graphs in this case, which makes it difficult to index them > given there'll be no interaction between the different instances of > the driver - I guess that's why they chose address. A separate media graph is not always possible. Think of the i.MX8MP for instance, we have two CSI-2 receivers connected to the ISI, in a single media graph. We can't use indices for the CSI-2 receiver entity names as there's no intrinsic index. > > Still brainstorming, I wonder if that case should be handled by exposing > > a bus info for entities, in addition to their names. That would be a > > more complex API extension though, and would require updates in some > > userspace tools. In any case, I think we need to document the entity > > naming best practices in the kernel. > > The media device does expose a bus_info field already which might be > sufficient? At least in this case it is; it contains the address of > the CRU instance but since there's a 1:1 relationship between the CRU > and CSI it's enough to identify which of the four physical devices the > media_device relates to. The media device bus_info is for the whole media device. In the above i.MX8MP ISI example, it won't solve our problem. > >> To work around the issue, this series updates libcamera to use regex > >> matching when searching for a MediaEntity by name. Existing call > >> sites which just pass a normal string should work unaffected, but new > >> call sites could use a string that builds a valid regular expression > >> instead. > >> > >> Daniel Scally (2): > >> libcamera: device_enumerator: Use regex to match entity names > >> libcamera: media_device: Use a regex to match entity name > >> > >> .../libcamera/internal/device_enumerator.h | 3 ++- > >> src/libcamera/device_enumerator.cpp | 27 ++++++++++--------- > >> src/libcamera/media_device.cpp | 12 ++++++--- > >> src/libcamera/v4l2_subdevice.cpp | 4 +-- > >> 4 files changed, 26 insertions(+), 20 deletions(-)