| Message ID | 20230505215257.60704-3-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generic line based metadata support, internal pads | expand |
On 06/05/2023 00:52, Sakari Ailus wrote: > Take the new INTERNAL_SOURCE pad flag into account in validating routing > IOCTL argument. Effectively this is a SINK pad in this respect. Due to the > union there's no need to check for the particular name. What union? The one you add in the next patch? As a concept the internal source pads sound good, and they work in practice in my tests. But this union is what grates me a bit. We have a flag, MEDIA_PAD_FL_INTERNAL_SOURCE, which tells which field of the union to use, and then we go and do not use the new union field. Well, and also the naming, as we normally have source and sink pads, but now we also have internal source pads, which are actually identical to sink pads... I understand the idea and reasoning, but the two points above do confuse me and I'm sure would confuse others also. I wonder if it would be less or more confusing to simplify this by just adding a MEDIA_PAD_FL_INTERNAL, which could be applied to either a source or a sink pad, and would prevent linking to it. The flag would indicate that the stream from/to that pad is generated/consumed internally. (I don't know when you would need an internal pad to consume data, but... who knows, the need might pop up =). That would mean that an "internal sink pad" would generate a data stream, i.e. it's a "source", but I think a normal sink pad is almost the same anyway: when considering entity's internal routing, the normal sink pad is a "source", and the same logic would apply with the internal pads too. An internal sink pad that generates a stream is a bit more confusing than an internal source pad, but I think that confusion is less than the rest of the confusion in the code-side that comes with the internal source pads. Tomi > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 2ec179cd1264..6312fc2bf1f0 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -906,7 +906,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > return -EINVAL; > > if (!(pads[route->sink_pad].flags & > - MEDIA_PAD_FL_SINK)) > + (MEDIA_PAD_FL_SINK | > + MEDIA_PAD_FL_INTERNAL_SOURCE))) > return -EINVAL; > > if (route->source_pad >= sd->entity.num_pads) > @@ -1787,7 +1788,8 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd, > > /* Validate the sink and source pad numbers. */ > if (route->sink_pad >= sd->entity.num_pads || > - !(sd->entity.pads[route->sink_pad].flags & MEDIA_PAD_FL_SINK)) { > + !(sd->entity.pads[route->sink_pad].flags & > + (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL_SOURCE))) { > dev_dbg(sd->dev, "route %u sink (%u) is not a sink pad\n", > i, route->sink_pad); > goto out;
Moi, On Mon, May 08, 2023 at 01:14:07PM +0300, Tomi Valkeinen wrote: > On 06/05/2023 00:52, Sakari Ailus wrote: > > Take the new INTERNAL_SOURCE pad flag into account in validating routing > > IOCTL argument. Effectively this is a SINK pad in this respect. Due to the > > union there's no need to check for the particular name. > > What union? The one you add in the next patch? Oops. I think we can reorder the patches. > > As a concept the internal source pads sound good, and they work in practice > in my tests. But this union is what grates me a bit. We have a flag, > MEDIA_PAD_FL_INTERNAL_SOURCE, which tells which field of the union to use, > and then we go and do not use the new union field. Well, and also the > naming, as we normally have source and sink pads, but now we also have > internal source pads, which are actually identical to sink pads... The union still should be used by the user space. We're testing flags here and those flags are the same independently of the INTERNAL_SOURCE flag. I'm fine by not adding that union though, but for the user space I think it's better we have it: explaining that the sink_pad has a different meaning if that flag is set is harder than making it a union member. > > I understand the idea and reasoning, but the two points above do confuse me > and I'm sure would confuse others also. > > I wonder if it would be less or more confusing to simplify this by just > adding a MEDIA_PAD_FL_INTERNAL, which could be applied to either a source or > a sink pad, and would prevent linking to it. The flag would indicate that > the stream from/to that pad is generated/consumed internally. (I don't know > when you would need an internal pad to consume data, but... who knows, the > need might pop up =). This is another option. But I envision there will be more compatibility issues. Although... generally using such hardware requires knowledge of the device, and we haven't obviously had any support for devices needing this functionality in the tree. So in the end it might not matter much. > > That would mean that an "internal sink pad" would generate a data stream, > i.e. it's a "source", but I think a normal sink pad is almost the same > anyway: when considering entity's internal routing, the normal sink pad is a > "source", and the same logic would apply with the internal pads too. > > An internal sink pad that generates a stream is a bit more confusing than an > internal source pad, but I think that confusion is less than the rest of the > confusion in the code-side that comes with the internal source pads. I prefer having the possible sources of the confusion in the framework than in the drivers. Therefore I think INTERNAL_SOURCE flag is a (slightly) better option. I wonder what Llaurent thinks.
On Mon, May 08, 2023 at 03:24:10PM +0300, Sakari Ailus wrote: > Moi, こんにちは > On Mon, May 08, 2023 at 01:14:07PM +0300, Tomi Valkeinen wrote: > > On 06/05/2023 00:52, Sakari Ailus wrote: > > > Take the new INTERNAL_SOURCE pad flag into account in validating routing > > > IOCTL argument. Effectively this is a SINK pad in this respect. Due to the > > > union there's no need to check for the particular name. > > > > What union? The one you add in the next patch? > > Oops. I think we can reorder the patches. > > > As a concept the internal source pads sound good, and they work in practice > > in my tests. But this union is what grates me a bit. We have a flag, > > MEDIA_PAD_FL_INTERNAL_SOURCE, which tells which field of the union to use, > > and then we go and do not use the new union field. Well, and also the > > naming, as we normally have source and sink pads, but now we also have > > internal source pads, which are actually identical to sink pads... > > The union still should be used by the user space. We're testing flags here > and those flags are the same independently of the INTERNAL_SOURCE flag. > > I'm fine by not adding that union though, but for the user space I think > it's better we have it: explaining that the sink_pad has a different > meaning if that flag is set is harder than making it a union member. > > > I understand the idea and reasoning, but the two points above do confuse me > > and I'm sure would confuse others also. > > > > I wonder if it would be less or more confusing to simplify this by just > > adding a MEDIA_PAD_FL_INTERNAL, which could be applied to either a source or > > a sink pad, and would prevent linking to it. The flag would indicate that > > the stream from/to that pad is generated/consumed internally. (I don't know > > when you would need an internal pad to consume data, but... who knows, the > > need might pop up =). > > This is another option. But I envision there will be more compatibility > issues. Although... generally using such hardware requires knowledge of the > device, and we haven't obviously had any support for devices needing this > functionality in the tree. So in the end it might not matter much. > > > That would mean that an "internal sink pad" would generate a data stream, > > i.e. it's a "source", but I think a normal sink pad is almost the same > > anyway: when considering entity's internal routing, the normal sink pad is a > > "source", and the same logic would apply with the internal pads too. > > > > An internal sink pad that generates a stream is a bit more confusing than an > > internal source pad, but I think that confusion is less than the rest of the > > confusion in the code-side that comes with the internal source pads. > > I prefer having the possible sources of the confusion in the framework than > in the drivers. Therefore I think INTERNAL_SOURCE flag is a (slightly) > better option. > > I wonder what Llaurent thinks. I like the idea of a MEDIA_PAD_FL_INTERNAL flag. That's actually how I read patch 1/7, I didn't notice it used MEDIA_PAD_FL_INTERNAL*_SOURCE* :-) We could define MEDIA_PAD_FL_INTERNAL_SOURCE #define MEDIA_PAD_FL_INTERNAL_SOURCE (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL) but I'm not sure it would be less confusing. Regarding isolating the sources of confusion in the framework rather than spreading them through drivers, I can't disagree, but I think that, for raw camera sensors at least, the best option would be to create a new camera sensor object with a much more tailored API than v4l2_subdev (and of course wrapping that new object in a v4l2_subdev in the framework). This won't address the other types of drivers, but I'm not sure we'll get any in the foreseable future.
On Fri, Jun 02, 2023 at 12:44:12PM +0300, Laurent Pinchart wrote: > On Mon, May 08, 2023 at 03:24:10PM +0300, Sakari Ailus wrote: > > Moi, > > こんにちは > > > On Mon, May 08, 2023 at 01:14:07PM +0300, Tomi Valkeinen wrote: > > > On 06/05/2023 00:52, Sakari Ailus wrote: > > > > Take the new INTERNAL_SOURCE pad flag into account in validating routing > > > > IOCTL argument. Effectively this is a SINK pad in this respect. Due to the > > > > union there's no need to check for the particular name. > > > > > > What union? The one you add in the next patch? > > > > Oops. I think we can reorder the patches. > > > > > As a concept the internal source pads sound good, and they work in practice > > > in my tests. But this union is what grates me a bit. We have a flag, > > > MEDIA_PAD_FL_INTERNAL_SOURCE, which tells which field of the union to use, > > > and then we go and do not use the new union field. Well, and also the > > > naming, as we normally have source and sink pads, but now we also have > > > internal source pads, which are actually identical to sink pads... > > > > The union still should be used by the user space. We're testing flags here > > and those flags are the same independently of the INTERNAL_SOURCE flag. > > > > I'm fine by not adding that union though, but for the user space I think > > it's better we have it: explaining that the sink_pad has a different > > meaning if that flag is set is harder than making it a union member. > > > > > I understand the idea and reasoning, but the two points above do confuse me > > > and I'm sure would confuse others also. > > > > > > I wonder if it would be less or more confusing to simplify this by just > > > adding a MEDIA_PAD_FL_INTERNAL, which could be applied to either a source or > > > a sink pad, and would prevent linking to it. The flag would indicate that > > > the stream from/to that pad is generated/consumed internally. (I don't know > > > when you would need an internal pad to consume data, but... who knows, the > > > need might pop up =). > > > > This is another option. But I envision there will be more compatibility > > issues. Although... generally using such hardware requires knowledge of the > > device, and we haven't obviously had any support for devices needing this > > functionality in the tree. So in the end it might not matter much. > > > > > That would mean that an "internal sink pad" would generate a data stream, > > > i.e. it's a "source", but I think a normal sink pad is almost the same > > > anyway: when considering entity's internal routing, the normal sink pad is a > > > "source", and the same logic would apply with the internal pads too. > > > > > > An internal sink pad that generates a stream is a bit more confusing than an > > > internal source pad, but I think that confusion is less than the rest of the > > > confusion in the code-side that comes with the internal source pads. > > > > I prefer having the possible sources of the confusion in the framework than > > in the drivers. Therefore I think INTERNAL_SOURCE flag is a (slightly) > > better option. > > > > I wonder what Llaurent thinks. > > I like the idea of a MEDIA_PAD_FL_INTERNAL flag. That's actually how I > read patch 1/7, I didn't notice it used MEDIA_PAD_FL_INTERNAL*_SOURCE* > :-) > > We could define MEDIA_PAD_FL_INTERNAL_SOURCE > > #define MEDIA_PAD_FL_INTERNAL_SOURCE (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL) One option would be to find terms different from sink and source in this case. It would generate less confusion to map (e.g., not a really good name) MEDIA_PAD_FL_INTERNAL_PRODUCER to MEDIA_PAD_FL_SINK and to the sink_pad field than using MEDIA_PAD_FL_INTERNAL_SOURCE. > but I'm not sure it would be less confusing. > > Regarding isolating the sources of confusion in the framework rather > than spreading them through drivers, I can't disagree, but I think that, > for raw camera sensors at least, the best option would be to create a > new camera sensor object with a much more tailored API than v4l2_subdev > (and of course wrapping that new object in a v4l2_subdev in the > framework). This won't address the other types of drivers, but I'm not > sure we'll get any in the foreseable future.
Heippa Tomi, Laurent, On Fri, Jun 02, 2023 at 12:46:50PM +0300, Laurent Pinchart wrote: > On Fri, Jun 02, 2023 at 12:44:12PM +0300, Laurent Pinchart wrote: > > On Mon, May 08, 2023 at 03:24:10PM +0300, Sakari Ailus wrote: > > > Moi, > > > > こんにちは > > > > > On Mon, May 08, 2023 at 01:14:07PM +0300, Tomi Valkeinen wrote: > > > > On 06/05/2023 00:52, Sakari Ailus wrote: > > > > > Take the new INTERNAL_SOURCE pad flag into account in validating routing > > > > > IOCTL argument. Effectively this is a SINK pad in this respect. Due to the > > > > > union there's no need to check for the particular name. > > > > > > > > What union? The one you add in the next patch? > > > > > > Oops. I think we can reorder the patches. > > > > > > > As a concept the internal source pads sound good, and they work in practice > > > > in my tests. But this union is what grates me a bit. We have a flag, > > > > MEDIA_PAD_FL_INTERNAL_SOURCE, which tells which field of the union to use, > > > > and then we go and do not use the new union field. Well, and also the > > > > naming, as we normally have source and sink pads, but now we also have > > > > internal source pads, which are actually identical to sink pads... > > > > > > The union still should be used by the user space. We're testing flags here > > > and those flags are the same independently of the INTERNAL_SOURCE flag. > > > > > > I'm fine by not adding that union though, but for the user space I think > > > it's better we have it: explaining that the sink_pad has a different > > > meaning if that flag is set is harder than making it a union member. > > > > > > > I understand the idea and reasoning, but the two points above do confuse me > > > > and I'm sure would confuse others also. > > > > > > > > I wonder if it would be less or more confusing to simplify this by just > > > > adding a MEDIA_PAD_FL_INTERNAL, which could be applied to either a source or > > > > a sink pad, and would prevent linking to it. The flag would indicate that > > > > the stream from/to that pad is generated/consumed internally. (I don't know > > > > when you would need an internal pad to consume data, but... who knows, the > > > > need might pop up =). > > > > > > This is another option. But I envision there will be more compatibility > > > issues. Although... generally using such hardware requires knowledge of the > > > device, and we haven't obviously had any support for devices needing this > > > functionality in the tree. So in the end it might not matter much. > > > > > > > That would mean that an "internal sink pad" would generate a data stream, > > > > i.e. it's a "source", but I think a normal sink pad is almost the same > > > > anyway: when considering entity's internal routing, the normal sink pad is a > > > > "source", and the same logic would apply with the internal pads too. > > > > > > > > An internal sink pad that generates a stream is a bit more confusing than an > > > > internal source pad, but I think that confusion is less than the rest of the > > > > confusion in the code-side that comes with the internal source pads. > > > > > > I prefer having the possible sources of the confusion in the framework than > > > in the drivers. Therefore I think INTERNAL_SOURCE flag is a (slightly) > > > better option. > > > > > > I wonder what Llaurent thinks. > > > > I like the idea of a MEDIA_PAD_FL_INTERNAL flag. That's actually how I > > read patch 1/7, I didn't notice it used MEDIA_PAD_FL_INTERNAL*_SOURCE* > > :-) > > > > We could define MEDIA_PAD_FL_INTERNAL_SOURCE > > > > #define MEDIA_PAD_FL_INTERNAL_SOURCE (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL) > > One option would be to find terms different from sink and source in this > case. It would generate less confusion to map (e.g., not a really good > name) MEDIA_PAD_FL_INTERNAL_PRODUCER to MEDIA_PAD_FL_SINK and to the > sink_pad field than using MEDIA_PAD_FL_INTERNAL_SOURCE. I don't think this helps as you'd still be accessing the sink pad related fields that are there for another purpose. Alternatively I'd have the (plain) INTERNAL flag and drop the union, without masking or adding compound flags. I can switch to that if you promise not to change your mind again. ;-) > > > but I'm not sure it would be less confusing. > > > > Regarding isolating the sources of confusion in the framework rather > > than spreading them through drivers, I can't disagree, but I think that, > > for raw camera sensors at least, the best option would be to create a > > new camera sensor object with a much more tailored API than v4l2_subdev > > (and of course wrapping that new object in a v4l2_subdev in the > > framework). This won't address the other types of drivers, but I'm not > > sure we'll get any in the foreseable future. >
Hi Sakari, On Fri, Jun 02, 2023 at 01:10:40PM +0000, Sakari Ailus wrote: > On Fri, Jun 02, 2023 at 12:46:50PM +0300, Laurent Pinchart wrote: > > On Fri, Jun 02, 2023 at 12:44:12PM +0300, Laurent Pinchart wrote: > > > On Mon, May 08, 2023 at 03:24:10PM +0300, Sakari Ailus wrote: > > > > On Mon, May 08, 2023 at 01:14:07PM +0300, Tomi Valkeinen wrote: > > > > > On 06/05/2023 00:52, Sakari Ailus wrote: > > > > > > Take the new INTERNAL_SOURCE pad flag into account in validating routing > > > > > > IOCTL argument. Effectively this is a SINK pad in this respect. Due to the > > > > > > union there's no need to check for the particular name. > > > > > > > > > > What union? The one you add in the next patch? > > > > > > > > Oops. I think we can reorder the patches. > > > > > > > > > As a concept the internal source pads sound good, and they work in practice > > > > > in my tests. But this union is what grates me a bit. We have a flag, > > > > > MEDIA_PAD_FL_INTERNAL_SOURCE, which tells which field of the union to use, > > > > > and then we go and do not use the new union field. Well, and also the > > > > > naming, as we normally have source and sink pads, but now we also have > > > > > internal source pads, which are actually identical to sink pads... > > > > > > > > The union still should be used by the user space. We're testing flags here > > > > and those flags are the same independently of the INTERNAL_SOURCE flag. > > > > > > > > I'm fine by not adding that union though, but for the user space I think > > > > it's better we have it: explaining that the sink_pad has a different > > > > meaning if that flag is set is harder than making it a union member. > > > > > > > > > I understand the idea and reasoning, but the two points above do confuse me > > > > > and I'm sure would confuse others also. > > > > > > > > > > I wonder if it would be less or more confusing to simplify this by just > > > > > adding a MEDIA_PAD_FL_INTERNAL, which could be applied to either a source or > > > > > a sink pad, and would prevent linking to it. The flag would indicate that > > > > > the stream from/to that pad is generated/consumed internally. (I don't know > > > > > when you would need an internal pad to consume data, but... who knows, the > > > > > need might pop up =). > > > > > > > > This is another option. But I envision there will be more compatibility > > > > issues. Although... generally using such hardware requires knowledge of the > > > > device, and we haven't obviously had any support for devices needing this > > > > functionality in the tree. So in the end it might not matter much. > > > > > > > > > That would mean that an "internal sink pad" would generate a data stream, > > > > > i.e. it's a "source", but I think a normal sink pad is almost the same > > > > > anyway: when considering entity's internal routing, the normal sink pad is a > > > > > "source", and the same logic would apply with the internal pads too. > > > > > > > > > > An internal sink pad that generates a stream is a bit more confusing than an > > > > > internal source pad, but I think that confusion is less than the rest of the > > > > > confusion in the code-side that comes with the internal source pads. > > > > > > > > I prefer having the possible sources of the confusion in the framework than > > > > in the drivers. Therefore I think INTERNAL_SOURCE flag is a (slightly) > > > > better option. > > > > > > > > I wonder what Llaurent thinks. > > > > > > I like the idea of a MEDIA_PAD_FL_INTERNAL flag. That's actually how I > > > read patch 1/7, I didn't notice it used MEDIA_PAD_FL_INTERNAL*_SOURCE* > > > :-) > > > > > > We could define MEDIA_PAD_FL_INTERNAL_SOURCE > > > > > > #define MEDIA_PAD_FL_INTERNAL_SOURCE (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL) > > > > One option would be to find terms different from sink and source in this > > case. It would generate less confusion to map (e.g., not a really good > > name) MEDIA_PAD_FL_INTERNAL_PRODUCER to MEDIA_PAD_FL_SINK and to the > > sink_pad field than using MEDIA_PAD_FL_INTERNAL_SOURCE. > > I don't think this helps as you'd still be accessing the sink pad related > fields that are there for another purpose. > > Alternatively I'd have the (plain) INTERNAL flag and drop the union, > without masking or adding compound flags. > > I can switch to that if you promise not to change your mind again. ;-) I'll do my best :-) Could you show the impact (if any) it would have on drivers when accessing the routing fields ? > > > but I'm not sure it would be less confusing. > > > > > > Regarding isolating the sources of confusion in the framework rather > > > than spreading them through drivers, I can't disagree, but I think that, > > > for raw camera sensors at least, the best option would be to create a > > > new camera sensor object with a much more tailored API than v4l2_subdev > > > (and of course wrapping that new object in a v4l2_subdev in the > > > framework). This won't address the other types of drivers, but I'm not > > > sure we'll get any in the foreseable future.
Hi Laurent, On Sun, Jun 04, 2023 at 05:26:26PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Fri, Jun 02, 2023 at 01:10:40PM +0000, Sakari Ailus wrote: > > On Fri, Jun 02, 2023 at 12:46:50PM +0300, Laurent Pinchart wrote: > > > On Fri, Jun 02, 2023 at 12:44:12PM +0300, Laurent Pinchart wrote: > > > > On Mon, May 08, 2023 at 03:24:10PM +0300, Sakari Ailus wrote: > > > > > On Mon, May 08, 2023 at 01:14:07PM +0300, Tomi Valkeinen wrote: > > > > > > On 06/05/2023 00:52, Sakari Ailus wrote: > > > > > > > Take the new INTERNAL_SOURCE pad flag into account in validating routing > > > > > > > IOCTL argument. Effectively this is a SINK pad in this respect. Due to the > > > > > > > union there's no need to check for the particular name. > > > > > > > > > > > > What union? The one you add in the next patch? > > > > > > > > > > Oops. I think we can reorder the patches. > > > > > > > > > > > As a concept the internal source pads sound good, and they work in practice > > > > > > in my tests. But this union is what grates me a bit. We have a flag, > > > > > > MEDIA_PAD_FL_INTERNAL_SOURCE, which tells which field of the union to use, > > > > > > and then we go and do not use the new union field. Well, and also the > > > > > > naming, as we normally have source and sink pads, but now we also have > > > > > > internal source pads, which are actually identical to sink pads... > > > > > > > > > > The union still should be used by the user space. We're testing flags here > > > > > and those flags are the same independently of the INTERNAL_SOURCE flag. > > > > > > > > > > I'm fine by not adding that union though, but for the user space I think > > > > > it's better we have it: explaining that the sink_pad has a different > > > > > meaning if that flag is set is harder than making it a union member. > > > > > > > > > > > I understand the idea and reasoning, but the two points above do confuse me > > > > > > and I'm sure would confuse others also. > > > > > > > > > > > > I wonder if it would be less or more confusing to simplify this by just > > > > > > adding a MEDIA_PAD_FL_INTERNAL, which could be applied to either a source or > > > > > > a sink pad, and would prevent linking to it. The flag would indicate that > > > > > > the stream from/to that pad is generated/consumed internally. (I don't know > > > > > > when you would need an internal pad to consume data, but... who knows, the > > > > > > need might pop up =). > > > > > > > > > > This is another option. But I envision there will be more compatibility > > > > > issues. Although... generally using such hardware requires knowledge of the > > > > > device, and we haven't obviously had any support for devices needing this > > > > > functionality in the tree. So in the end it might not matter much. > > > > > > > > > > > That would mean that an "internal sink pad" would generate a data stream, > > > > > > i.e. it's a "source", but I think a normal sink pad is almost the same > > > > > > anyway: when considering entity's internal routing, the normal sink pad is a > > > > > > "source", and the same logic would apply with the internal pads too. > > > > > > > > > > > > An internal sink pad that generates a stream is a bit more confusing than an > > > > > > internal source pad, but I think that confusion is less than the rest of the > > > > > > confusion in the code-side that comes with the internal source pads. > > > > > > > > > > I prefer having the possible sources of the confusion in the framework than > > > > > in the drivers. Therefore I think INTERNAL_SOURCE flag is a (slightly) > > > > > better option. > > > > > > > > > > I wonder what Llaurent thinks. > > > > > > > > I like the idea of a MEDIA_PAD_FL_INTERNAL flag. That's actually how I > > > > read patch 1/7, I didn't notice it used MEDIA_PAD_FL_INTERNAL*_SOURCE* > > > > :-) > > > > > > > > We could define MEDIA_PAD_FL_INTERNAL_SOURCE > > > > > > > > #define MEDIA_PAD_FL_INTERNAL_SOURCE (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL) > > > > > > One option would be to find terms different from sink and source in this > > > case. It would generate less confusion to map (e.g., not a really good > > > name) MEDIA_PAD_FL_INTERNAL_PRODUCER to MEDIA_PAD_FL_SINK and to the > > > sink_pad field than using MEDIA_PAD_FL_INTERNAL_SOURCE. > > > > I don't think this helps as you'd still be accessing the sink pad related > > fields that are there for another purpose. > > > > Alternatively I'd have the (plain) INTERNAL flag and drop the union, > > without masking or adding compound flags. > > > > I can switch to that if you promise not to change your mind again. ;-) > > I'll do my best :-) Could you show the impact (if any) it would have on > drivers when accessing the routing fields ? I don't think there's much of an impact for the drivers. It's mainly affecting setting up pads for the entities. Tomi?
On Mon, Jun 05, 2023 at 08:06:58AM +0000, Sakari Ailus wrote: > On Sun, Jun 04, 2023 at 05:26:26PM +0300, Laurent Pinchart wrote: > > On Fri, Jun 02, 2023 at 01:10:40PM +0000, Sakari Ailus wrote: > > > On Fri, Jun 02, 2023 at 12:46:50PM +0300, Laurent Pinchart wrote: > > > > On Fri, Jun 02, 2023 at 12:44:12PM +0300, Laurent Pinchart wrote: > > > > > On Mon, May 08, 2023 at 03:24:10PM +0300, Sakari Ailus wrote: > > > > > > On Mon, May 08, 2023 at 01:14:07PM +0300, Tomi Valkeinen wrote: > > > > > > > On 06/05/2023 00:52, Sakari Ailus wrote: > > > > > > > > Take the new INTERNAL_SOURCE pad flag into account in validating routing > > > > > > > > IOCTL argument. Effectively this is a SINK pad in this respect. Due to the > > > > > > > > union there's no need to check for the particular name. > > > > > > > > > > > > > > What union? The one you add in the next patch? > > > > > > > > > > > > Oops. I think we can reorder the patches. > > > > > > > > > > > > > As a concept the internal source pads sound good, and they work in practice > > > > > > > in my tests. But this union is what grates me a bit. We have a flag, > > > > > > > MEDIA_PAD_FL_INTERNAL_SOURCE, which tells which field of the union to use, > > > > > > > and then we go and do not use the new union field. Well, and also the > > > > > > > naming, as we normally have source and sink pads, but now we also have > > > > > > > internal source pads, which are actually identical to sink pads... > > > > > > > > > > > > The union still should be used by the user space. We're testing flags here > > > > > > and those flags are the same independently of the INTERNAL_SOURCE flag. > > > > > > > > > > > > I'm fine by not adding that union though, but for the user space I think > > > > > > it's better we have it: explaining that the sink_pad has a different > > > > > > meaning if that flag is set is harder than making it a union member. > > > > > > > > > > > > > I understand the idea and reasoning, but the two points above do confuse me > > > > > > > and I'm sure would confuse others also. > > > > > > > > > > > > > > I wonder if it would be less or more confusing to simplify this by just > > > > > > > adding a MEDIA_PAD_FL_INTERNAL, which could be applied to either a source or > > > > > > > a sink pad, and would prevent linking to it. The flag would indicate that > > > > > > > the stream from/to that pad is generated/consumed internally. (I don't know > > > > > > > when you would need an internal pad to consume data, but... who knows, the > > > > > > > need might pop up =). > > > > > > > > > > > > This is another option. But I envision there will be more compatibility > > > > > > issues. Although... generally using such hardware requires knowledge of the > > > > > > device, and we haven't obviously had any support for devices needing this > > > > > > functionality in the tree. So in the end it might not matter much. > > > > > > > > > > > > > That would mean that an "internal sink pad" would generate a data stream, > > > > > > > i.e. it's a "source", but I think a normal sink pad is almost the same > > > > > > > anyway: when considering entity's internal routing, the normal sink pad is a > > > > > > > "source", and the same logic would apply with the internal pads too. > > > > > > > > > > > > > > An internal sink pad that generates a stream is a bit more confusing than an > > > > > > > internal source pad, but I think that confusion is less than the rest of the > > > > > > > confusion in the code-side that comes with the internal source pads. > > > > > > > > > > > > I prefer having the possible sources of the confusion in the framework than > > > > > > in the drivers. Therefore I think INTERNAL_SOURCE flag is a (slightly) > > > > > > better option. > > > > > > > > > > > > I wonder what Llaurent thinks. > > > > > > > > > > I like the idea of a MEDIA_PAD_FL_INTERNAL flag. That's actually how I > > > > > read patch 1/7, I didn't notice it used MEDIA_PAD_FL_INTERNAL*_SOURCE* > > > > > :-) > > > > > > > > > > We could define MEDIA_PAD_FL_INTERNAL_SOURCE > > > > > > > > > > #define MEDIA_PAD_FL_INTERNAL_SOURCE (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL) > > > > > > > > One option would be to find terms different from sink and source in this > > > > case. It would generate less confusion to map (e.g., not a really good > > > > name) MEDIA_PAD_FL_INTERNAL_PRODUCER to MEDIA_PAD_FL_SINK and to the > > > > sink_pad field than using MEDIA_PAD_FL_INTERNAL_SOURCE. > > > > > > I don't think this helps as you'd still be accessing the sink pad related > > > fields that are there for another purpose. > > > > > > Alternatively I'd have the (plain) INTERNAL flag and drop the union, > > > without masking or adding compound flags. > > > > > > I can switch to that if you promise not to change your mind again. ;-) > > > > I'll do my best :-) Could you show the impact (if any) it would have on > > drivers when accessing the routing fields ? > > I don't think there's much of an impact for the drivers. It's mainly > affecting setting up pads for the entities. Tomi? I think it would be useful to discuss it with an actual sensor driver using the API.
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 2ec179cd1264..6312fc2bf1f0 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -906,7 +906,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, return -EINVAL; if (!(pads[route->sink_pad].flags & - MEDIA_PAD_FL_SINK)) + (MEDIA_PAD_FL_SINK | + MEDIA_PAD_FL_INTERNAL_SOURCE))) return -EINVAL; if (route->source_pad >= sd->entity.num_pads) @@ -1787,7 +1788,8 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd, /* Validate the sink and source pad numbers. */ if (route->sink_pad >= sd->entity.num_pads || - !(sd->entity.pads[route->sink_pad].flags & MEDIA_PAD_FL_SINK)) { + !(sd->entity.pads[route->sink_pad].flags & + (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL_SOURCE))) { dev_dbg(sd->dev, "route %u sink (%u) is not a sink pad\n", i, route->sink_pad); goto out;
Take the new INTERNAL_SOURCE pad flag into account in validating routing IOCTL argument. Effectively this is a SINK pad in this respect. Due to the union there's no need to check for the particular name. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)