| Message ID | 20240220130339.543749-3-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use INTEGER64 type for MEI CSI LINK_FREQ control | expand |
On 20/02/2024 14:03, Sakari Ailus wrote: > When a device passes through the video data while still having its own > receiver and transmitter, it can use the same frequency as the upstream > link does. The Intel MEI CSI device is an example of this. An integer menu > control isn't useful in conveying the actual frequency to the receiver in > this case. > > Document that the LINK_FREQ control may also be a 64-bit integer control. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > index b1c2ab2854af..7a3ccb100e1d 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > @@ -22,7 +22,7 @@ Image Process Control IDs > > .. _v4l2-cid-link-freq: > > -``V4L2_CID_LINK_FREQ (integer menu)`` > +``V4L2_CID_LINK_FREQ (integer menu or 64-bit integer)`` > The frequency of the data bus (e.g. parallel or CSI-2). I really think a new control should be created for this. As I understand it, V4L2_CID_LINK_FREQ gives a set of supported frequencies, and the application has to pick one (I think?). Is it supposed to be a read-only control? Some driver seem to set the READ_ONLY flag, and some do not. The documentation isn't helpful in that respect. In the case of the Intel MEI CSI and similar devices a new control would be better, I think. Do I understand it correctly that for these devices it would always be a read-only control? I.e. it just reports the frequency, but applications cannot change it? Regards, Hans > > .. _v4l2-cid-pixel-rate:
Hi Hans, On Fri, Apr 26, 2024 at 09:22:40AM +0200, Hans Verkuil wrote: > On 20/02/2024 14:03, Sakari Ailus wrote: > > When a device passes through the video data while still having its own > > receiver and transmitter, it can use the same frequency as the upstream > > link does. The Intel MEI CSI device is an example of this. An integer menu > > control isn't useful in conveying the actual frequency to the receiver in > > this case. > > > > Document that the LINK_FREQ control may also be a 64-bit integer control. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > > index b1c2ab2854af..7a3ccb100e1d 100644 > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > > @@ -22,7 +22,7 @@ Image Process Control IDs > > > > .. _v4l2-cid-link-freq: > > > > -``V4L2_CID_LINK_FREQ (integer menu)`` > > +``V4L2_CID_LINK_FREQ (integer menu or 64-bit integer)`` > > The frequency of the data bus (e.g. parallel or CSI-2). > > I really think a new control should be created for this. > > As I understand it, V4L2_CID_LINK_FREQ gives a set of supported frequencies, > and the application has to pick one (I think?). Is it supposed to be a > read-only control? Some driver seem to set the READ_ONLY flag, and some do not. > The documentation isn't helpful in that respect. This is read-only effectively in current IVSC implementation. > > In the case of the Intel MEI CSI and similar devices a new control would be > better, I think. Do I understand it correctly that for these devices it would > always be a read-only control? I.e. it just reports the frequency, but applications > cannot change it? How would you call the new control? V4L2_CID_LINK_FREQ_READ_ONLY? Originally the reason for changing LINK_FREQ for sensors was be part of changing sensor's configuration to achieve a given frame interval.
On 4/26/24 09:39, Sakari Ailus wrote: > Hi Hans, > > On Fri, Apr 26, 2024 at 09:22:40AM +0200, Hans Verkuil wrote: >> On 20/02/2024 14:03, Sakari Ailus wrote: >>> When a device passes through the video data while still having its own >>> receiver and transmitter, it can use the same frequency as the upstream >>> link does. The Intel MEI CSI device is an example of this. An integer menu >>> control isn't useful in conveying the actual frequency to the receiver in >>> this case. >>> >>> Document that the LINK_FREQ control may also be a 64-bit integer control. >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst >>> index b1c2ab2854af..7a3ccb100e1d 100644 >>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst >>> @@ -22,7 +22,7 @@ Image Process Control IDs >>> >>> .. _v4l2-cid-link-freq: >>> >>> -``V4L2_CID_LINK_FREQ (integer menu)`` >>> +``V4L2_CID_LINK_FREQ (integer menu or 64-bit integer)`` >>> The frequency of the data bus (e.g. parallel or CSI-2). >> >> I really think a new control should be created for this. >> >> As I understand it, V4L2_CID_LINK_FREQ gives a set of supported frequencies, >> and the application has to pick one (I think?). Is it supposed to be a >> read-only control? Some driver seem to set the READ_ONLY flag, and some do not. >> The documentation isn't helpful in that respect. > > This is read-only effectively in current IVSC implementation. > >> >> In the case of the Intel MEI CSI and similar devices a new control would be >> better, I think. Do I understand it correctly that for these devices it would >> always be a read-only control? I.e. it just reports the frequency, but applications >> cannot change it? > > How would you call the new control? > > V4L2_CID_LINK_FREQ_READ_ONLY? > > Originally the reason for changing LINK_FREQ for sensors was be part of > changing sensor's configuration to achieve a given frame interval. Will this new variant always be read-only? How about V4L2_CID_CUR_LINK_FREQ? I.e., it returns the current link frequency. That way it can also be used by drivers that implement V4L2_CID_LINK_FREQ. Better ideas are welcome :-) Regards, Hans
Hi Hans, On Fri, Apr 26, 2024 at 10:12:37AM +0200, Hans Verkuil wrote: > On 4/26/24 09:39, Sakari Ailus wrote: > > Hi Hans, > > > > On Fri, Apr 26, 2024 at 09:22:40AM +0200, Hans Verkuil wrote: > >> On 20/02/2024 14:03, Sakari Ailus wrote: > >>> When a device passes through the video data while still having its own > >>> receiver and transmitter, it can use the same frequency as the upstream > >>> link does. The Intel MEI CSI device is an example of this. An integer menu > >>> control isn't useful in conveying the actual frequency to the receiver in > >>> this case. > >>> > >>> Document that the LINK_FREQ control may also be a 64-bit integer control. > >>> > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>> --- > >>> .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > >>> index b1c2ab2854af..7a3ccb100e1d 100644 > >>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > >>> @@ -22,7 +22,7 @@ Image Process Control IDs > >>> > >>> .. _v4l2-cid-link-freq: > >>> > >>> -``V4L2_CID_LINK_FREQ (integer menu)`` > >>> +``V4L2_CID_LINK_FREQ (integer menu or 64-bit integer)`` > >>> The frequency of the data bus (e.g. parallel or CSI-2). > >> > >> I really think a new control should be created for this. > >> > >> As I understand it, V4L2_CID_LINK_FREQ gives a set of supported frequencies, > >> and the application has to pick one (I think?). Is it supposed to be a > >> read-only control? Some driver seem to set the READ_ONLY flag, and some do not. > >> The documentation isn't helpful in that respect. > > > > This is read-only effectively in current IVSC implementation. > > > >> > >> In the case of the Intel MEI CSI and similar devices a new control would be > >> better, I think. Do I understand it correctly that for these devices it would > >> always be a read-only control? I.e. it just reports the frequency, but applications > >> cannot change it? > > > > How would you call the new control? > > > > V4L2_CID_LINK_FREQ_READ_ONLY? > > > > Originally the reason for changing LINK_FREQ for sensors was be part of > > changing sensor's configuration to achieve a given frame interval. > > Will this new variant always be read-only? At least for this purpose I think so. Apart from the sensor configuration, I'm not aware of another use case for the user to change it. > > How about V4L2_CID_CUR_LINK_FREQ? > > I.e., it returns the current link frequency. That way it can also be > used by drivers that implement V4L2_CID_LINK_FREQ. It could, but the drivers already report this using V4L2_CID_LINK_FREQ. It'd be extra driver code for little if any gain. > > Better ideas are welcome :-) V4L2_CID_LINK_FREQ_VALUE? V4L2_CID_LINK_FREQ2?? :-)
On 4/26/24 10:27, Sakari Ailus wrote: > Hi Hans, > > On Fri, Apr 26, 2024 at 10:12:37AM +0200, Hans Verkuil wrote: >> On 4/26/24 09:39, Sakari Ailus wrote: >>> Hi Hans, >>> >>> On Fri, Apr 26, 2024 at 09:22:40AM +0200, Hans Verkuil wrote: >>>> On 20/02/2024 14:03, Sakari Ailus wrote: >>>>> When a device passes through the video data while still having its own >>>>> receiver and transmitter, it can use the same frequency as the upstream >>>>> link does. The Intel MEI CSI device is an example of this. An integer menu >>>>> control isn't useful in conveying the actual frequency to the receiver in >>>>> this case. >>>>> >>>>> Document that the LINK_FREQ control may also be a 64-bit integer control. >>>>> >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>>> --- >>>>> .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst >>>>> index b1c2ab2854af..7a3ccb100e1d 100644 >>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst >>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst >>>>> @@ -22,7 +22,7 @@ Image Process Control IDs >>>>> >>>>> .. _v4l2-cid-link-freq: >>>>> >>>>> -``V4L2_CID_LINK_FREQ (integer menu)`` >>>>> +``V4L2_CID_LINK_FREQ (integer menu or 64-bit integer)`` >>>>> The frequency of the data bus (e.g. parallel or CSI-2). >>>> >>>> I really think a new control should be created for this. >>>> >>>> As I understand it, V4L2_CID_LINK_FREQ gives a set of supported frequencies, >>>> and the application has to pick one (I think?). Is it supposed to be a >>>> read-only control? Some driver seem to set the READ_ONLY flag, and some do not. >>>> The documentation isn't helpful in that respect. >>> >>> This is read-only effectively in current IVSC implementation. >>> >>>> >>>> In the case of the Intel MEI CSI and similar devices a new control would be >>>> better, I think. Do I understand it correctly that for these devices it would >>>> always be a read-only control? I.e. it just reports the frequency, but applications >>>> cannot change it? >>> >>> How would you call the new control? >>> >>> V4L2_CID_LINK_FREQ_READ_ONLY? >>> >>> Originally the reason for changing LINK_FREQ for sensors was be part of >>> changing sensor's configuration to achieve a given frame interval. >> >> Will this new variant always be read-only? > > At least for this purpose I think so. Apart from the sensor configuration, > I'm not aware of another use case for the user to change it. > >> >> How about V4L2_CID_CUR_LINK_FREQ? >> >> I.e., it returns the current link frequency. That way it can also be >> used by drivers that implement V4L2_CID_LINK_FREQ. > > It could, but the drivers already report this using V4L2_CID_LINK_FREQ. > It'd be extra driver code for little if any gain. It's mainly for if we ever want to have consistent support for this control for all drivers to make life easier for applications. In other words, supporting both controls (if we'd ever want to) wouldn't cause problems API-wise. > >> >> Better ideas are welcome :-) > > V4L2_CID_LINK_FREQ_VALUE? V4L2_CID_LINK_FREQ2?? :-) > Hmm, I prefer CUR_LINK_FREQ, since that implies that it is a read-only control. Regards, Hans
Hi Hans, On Fri, Apr 26, 2024 at 10:38:26AM +0200, Hans Verkuil wrote: > On 4/26/24 10:27, Sakari Ailus wrote: > > Hi Hans, > > > > On Fri, Apr 26, 2024 at 10:12:37AM +0200, Hans Verkuil wrote: > >> On 4/26/24 09:39, Sakari Ailus wrote: > >>> Hi Hans, > >>> > >>> On Fri, Apr 26, 2024 at 09:22:40AM +0200, Hans Verkuil wrote: > >>>> On 20/02/2024 14:03, Sakari Ailus wrote: > >>>>> When a device passes through the video data while still having its own > >>>>> receiver and transmitter, it can use the same frequency as the upstream > >>>>> link does. The Intel MEI CSI device is an example of this. An integer menu > >>>>> control isn't useful in conveying the actual frequency to the receiver in > >>>>> this case. > >>>>> > >>>>> Document that the LINK_FREQ control may also be a 64-bit integer control. > >>>>> > >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>>>> --- > >>>>> .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > >>>>> index b1c2ab2854af..7a3ccb100e1d 100644 > >>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > >>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > >>>>> @@ -22,7 +22,7 @@ Image Process Control IDs > >>>>> > >>>>> .. _v4l2-cid-link-freq: > >>>>> > >>>>> -``V4L2_CID_LINK_FREQ (integer menu)`` > >>>>> +``V4L2_CID_LINK_FREQ (integer menu or 64-bit integer)`` > >>>>> The frequency of the data bus (e.g. parallel or CSI-2). > >>>> > >>>> I really think a new control should be created for this. > >>>> > >>>> As I understand it, V4L2_CID_LINK_FREQ gives a set of supported frequencies, > >>>> and the application has to pick one (I think?). Is it supposed to be a > >>>> read-only control? Some driver seem to set the READ_ONLY flag, and some do not. > >>>> The documentation isn't helpful in that respect. > >>> > >>> This is read-only effectively in current IVSC implementation. > >>> > >>>> > >>>> In the case of the Intel MEI CSI and similar devices a new control would be > >>>> better, I think. Do I understand it correctly that for these devices it would > >>>> always be a read-only control? I.e. it just reports the frequency, but applications > >>>> cannot change it? > >>> > >>> How would you call the new control? > >>> > >>> V4L2_CID_LINK_FREQ_READ_ONLY? > >>> > >>> Originally the reason for changing LINK_FREQ for sensors was be part of > >>> changing sensor's configuration to achieve a given frame interval. > >> > >> Will this new variant always be read-only? > > > > At least for this purpose I think so. Apart from the sensor configuration, > > I'm not aware of another use case for the user to change it. > > > >> > >> How about V4L2_CID_CUR_LINK_FREQ? > >> > >> I.e., it returns the current link frequency. That way it can also be > >> used by drivers that implement V4L2_CID_LINK_FREQ. > > > > It could, but the drivers already report this using V4L2_CID_LINK_FREQ. > > It'd be extra driver code for little if any gain. > > It's mainly for if we ever want to have consistent support for this > control for all drivers to make life easier for applications. > > In other words, supporting both controls (if we'd ever want to) wouldn't > cause problems API-wise. Apart from the sensor frame interval control, the users generally don't care. It's for the CSI-2 receiver drivers which use a framework function to obtain the value (it was amended in one of the three patches). > > > > >> > >> Better ideas are welcome :-) > > > > V4L2_CID_LINK_FREQ_VALUE? V4L2_CID_LINK_FREQ2?? :-) > > > > Hmm, I prefer CUR_LINK_FREQ, since that implies that it is a read-only > control. I'd be fine with that.
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst index b1c2ab2854af..7a3ccb100e1d 100644 --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst @@ -22,7 +22,7 @@ Image Process Control IDs .. _v4l2-cid-link-freq: -``V4L2_CID_LINK_FREQ (integer menu)`` +``V4L2_CID_LINK_FREQ (integer menu or 64-bit integer)`` The frequency of the data bus (e.g. parallel or CSI-2). .. _v4l2-cid-pixel-rate:
When a device passes through the video data while still having its own receiver and transmitter, it can use the same frequency as the upstream link does. The Intel MEI CSI device is an example of this. An integer menu control isn't useful in conveying the actual frequency to the receiver in this case. Document that the LINK_FREQ control may also be a 64-bit integer control. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)