[0/3] Use INTEGER64 type for MEI CSI LINK_FREQ control

Message

Sakari Ailus Feb. 20, 2024, 1:03 p.m. UTC

Hi folks,

An integer menu isn't a useful control type for conveying the frequency
depending on an external device to the receiver. Instead, in the MEI CSI
driver, just obtain the link frequency from the upsteam sub-device and
pass it on to the receiver.

The v4l2_get_link_freq() is changed to add support for this and it's
documented as well.

Sakari Ailus (3):
  media: v4l: Support obtaining link frequency from INTEGER64 controls
  media: Documentation: v4l: LINK_FREQ can be an INTEGER64 control
  media: ivsc: csi: Fix link frequency control behaviour

 .../media/v4l/ext-ctrls-image-process.rst     |  2 +-
 drivers/media/pci/intel/ivsc/mei_csi.c        | 30 +++++++++----------
 drivers/media/v4l2-core/v4l2-common.c         |  3 ++
 3 files changed, 18 insertions(+), 17 deletions(-)

Comments

Hans Verkuil April 26, 2024, 7:12 a.m. UTC | #1

Hi Sakari,

On 20/02/2024 14:03, Sakari Ailus wrote:
> Hi folks,
> 
> An integer menu isn't a useful control type for conveying the frequency
> depending on an external device to the receiver. Instead, in the MEI CSI
> driver, just obtain the link frequency from the upsteam sub-device and
> pass it on to the receiver.
> 
> The v4l2_get_link_freq() is changed to add support for this and it's
> documented as well.
> 
> Sakari Ailus (3):
>   media: v4l: Support obtaining link frequency from INTEGER64 controls
>   media: Documentation: v4l: LINK_FREQ can be an INTEGER64 control

I'm not sure if you can do this. This means userspace has to support two
different types as well and query what to use. That's an ABI change.

>   media: ivsc: csi: Fix link frequency control behaviour

And this third patch marks the control as VOLATILE, which is wrong. It is a
normal read-only control and you can just update the control's value in
the driver whenever it needs changing.

I would comment in the patch itself, but for some reason I never got it.

I am dropping these three patches from the PR you sent.

Regards,

	Hans

> 
>  .../media/v4l/ext-ctrls-image-process.rst     |  2 +-
>  drivers/media/pci/intel/ivsc/mei_csi.c        | 30 +++++++++----------
>  drivers/media/v4l2-core/v4l2-common.c         |  3 ++
>  3 files changed, 18 insertions(+), 17 deletions(-)
>

Sakari Ailus April 26, 2024, 7:18 a.m. UTC | #2

Hi Hans,

On Fri, Apr 26, 2024 at 09:12:32AM +0200, Hans Verkuil wrote:
> Hi Sakari,
> 
> On 20/02/2024 14:03, Sakari Ailus wrote:
> > Hi folks,
> > 
> > An integer menu isn't a useful control type for conveying the frequency
> > depending on an external device to the receiver. Instead, in the MEI CSI
> > driver, just obtain the link frequency from the upsteam sub-device and
> > pass it on to the receiver.
> > 
> > The v4l2_get_link_freq() is changed to add support for this and it's
> > documented as well.
> > 
> > Sakari Ailus (3):
> >   media: v4l: Support obtaining link frequency from INTEGER64 controls
> >   media: Documentation: v4l: LINK_FREQ can be an INTEGER64 control
> 
> I'm not sure if you can do this. This means userspace has to support two
> different types as well and query what to use. That's an ABI change.
> 
> >   media: ivsc: csi: Fix link frequency control behaviour
> 
> And this third patch marks the control as VOLATILE, which is wrong. It is a
> normal read-only control and you can just update the control's value in
> the driver whenever it needs changing.
> 
> I would comment in the patch itself, but for some reason I never got it.
> 
> I am dropping these three patches from the PR you sent.

Fair enough. The IVSC takes the frequency from the upstream sub-device so
it doesn't have a list of frequencies on its own. One alternative might be
to use always the highest frequency, assuming this can be found from the
upstream sub-device but I don't know whether this would actually work.

Do you know, Wentong?

The current state of using a static value is not an option.