[PATCH v1 09/19] staging: media: tegra-video: vi: add flip controls only if no source controls are provided

Svyatoslav Ryhel posted 19 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v1 09/19] staging: media: tegra-video: vi: add flip controls only if no source controls are provided
Posted by Svyatoslav Ryhel 1 month, 2 weeks ago
Add HFLIP and VFLIP from SoC only if camera sensor does not provide those
controls.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/staging/media/tegra-video/vi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 4f67adc395ac..61b65a2c1436 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -961,6 +961,7 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
 	}
 #else
 	struct v4l2_subdev *subdev;
+	struct v4l2_ctrl *hflip, *vflip;
 
 	/* custom control */
 	v4l2_ctrl_new_custom(&chan->ctrl_handler, &syncpt_timeout_ctrl, NULL);
@@ -986,11 +987,13 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
 		return ret;
 	}
 
-	if (chan->vi->soc->has_h_v_flip) {
+	hflip = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_HFLIP);
+	if (chan->vi->soc->has_h_v_flip && !hflip)
 		v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
-		v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
-	}
 
+	vflip = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_VFLIP);
+	if (chan->vi->soc->has_h_v_flip && !vflip)
+		v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
 #endif
 
 	/* setup the controls */
-- 
2.48.1
Re: [PATCH v1 09/19] staging: media: tegra-video: vi: add flip controls only if no source controls are provided
Posted by Luca Ceresoli 4 weeks ago
Hello Svyatoslav,

On Tue, 19 Aug 2025 15:16:21 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> Add HFLIP and VFLIP from SoC only if camera sensor does not provide those
> controls.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/staging/media/tegra-video/vi.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
> index 4f67adc395ac..61b65a2c1436 100644
> --- a/drivers/staging/media/tegra-video/vi.c
> +++ b/drivers/staging/media/tegra-video/vi.c
> @@ -961,6 +961,7 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
>  	}
>  #else
>  	struct v4l2_subdev *subdev;
> +	struct v4l2_ctrl *hflip, *vflip;
>  
>  	/* custom control */
>  	v4l2_ctrl_new_custom(&chan->ctrl_handler, &syncpt_timeout_ctrl, NULL);
> @@ -986,11 +987,13 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
>  		return ret;
>  	}
>  
> -	if (chan->vi->soc->has_h_v_flip) {
> +	hflip = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_HFLIP);
> +	if (chan->vi->soc->has_h_v_flip && !hflip)
>  		v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> -		v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> -	}
>  
> +	vflip = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_VFLIP);
> +	if (chan->vi->soc->has_h_v_flip && !vflip)
> +		v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);

Based on my understanding of V4L2, this should not be done.
AFAIK subdevs should expose what the hardware block can do,
independently from other subdevs. It is up to userspace (e.g.
libcamera) to use the most appropriate control when there are redundant
ones.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v1 09/19] staging: media: tegra-video: vi: add flip controls only if no source controls are provided
Posted by Svyatoslav Ryhel 4 weeks ago
пт, 5 вер. 2025 р. о 18:59 Luca Ceresoli <luca.ceresoli@bootlin.com> пише:
>
> Hello Svyatoslav,
>
> On Tue, 19 Aug 2025 15:16:21 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > Add HFLIP and VFLIP from SoC only if camera sensor does not provide those
> > controls.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  drivers/staging/media/tegra-video/vi.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
> > index 4f67adc395ac..61b65a2c1436 100644
> > --- a/drivers/staging/media/tegra-video/vi.c
> > +++ b/drivers/staging/media/tegra-video/vi.c
> > @@ -961,6 +961,7 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
> >       }
> >  #else
> >       struct v4l2_subdev *subdev;
> > +     struct v4l2_ctrl *hflip, *vflip;
> >
> >       /* custom control */
> >       v4l2_ctrl_new_custom(&chan->ctrl_handler, &syncpt_timeout_ctrl, NULL);
> > @@ -986,11 +987,13 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
> >               return ret;
> >       }
> >
> > -     if (chan->vi->soc->has_h_v_flip) {
> > +     hflip = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_HFLIP);
> > +     if (chan->vi->soc->has_h_v_flip && !hflip)
> >               v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> > -             v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> > -     }
> >
> > +     vflip = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_VFLIP);
> > +     if (chan->vi->soc->has_h_v_flip && !vflip)
> > +             v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
>
> Based on my understanding of V4L2, this should not be done.
> AFAIK subdevs should expose what the hardware block can do,
> independently from other subdevs. It is up to userspace (e.g.
> libcamera) to use the most appropriate control when there are redundant
> ones.
>

This driver is video-centric, interactions are done via /dev/videoX
not subdevices like media-centric derivers do. Conversion is possible
but it is not scope of this patchset and in case such conversion takes
place, one who will do that, will definitely know what to do.
Video-centric drivers expose all controls within single video device
and it cannot hold duplicates of controls, this causes error. So this
solution exposes camera flip controls and if camera has none, SoC
controls are exposed.

> Luca
>
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Re: [PATCH v1 09/19] staging: media: tegra-video: vi: add flip controls only if no source controls are provided
Posted by Luca Ceresoli 3 weeks, 3 days ago
Hello Svyatoslav,

On Fri, 5 Sep 2025 19:05:16 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> пт, 5 вер. 2025 р. о 18:59 Luca Ceresoli <luca.ceresoli@bootlin.com> пише:
> >
> > Hello Svyatoslav,
> >
> > On Tue, 19 Aug 2025 15:16:21 +0300
> > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >  
> > > Add HFLIP and VFLIP from SoC only if camera sensor does not provide those
> > > controls.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  drivers/staging/media/tegra-video/vi.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
> > > index 4f67adc395ac..61b65a2c1436 100644
> > > --- a/drivers/staging/media/tegra-video/vi.c
> > > +++ b/drivers/staging/media/tegra-video/vi.c
> > > @@ -961,6 +961,7 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
> > >       }
> > >  #else
> > >       struct v4l2_subdev *subdev;
> > > +     struct v4l2_ctrl *hflip, *vflip;
> > >
> > >       /* custom control */
> > >       v4l2_ctrl_new_custom(&chan->ctrl_handler, &syncpt_timeout_ctrl, NULL);
> > > @@ -986,11 +987,13 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
> > >               return ret;
> > >       }
> > >
> > > -     if (chan->vi->soc->has_h_v_flip) {
> > > +     hflip = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_HFLIP);
> > > +     if (chan->vi->soc->has_h_v_flip && !hflip)
> > >               v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > -             v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > -     }
> > >
> > > +     vflip = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_VFLIP);
> > > +     if (chan->vi->soc->has_h_v_flip && !vflip)
> > > +             v4l2_ctrl_new_std(&chan->ctrl_handler, &vi_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);  
> >
> > Based on my understanding of V4L2, this should not be done.
> > AFAIK subdevs should expose what the hardware block can do,
> > independently from other subdevs. It is up to userspace (e.g.
> > libcamera) to use the most appropriate control when there are redundant
> > ones.
> >  
> 
> This driver is video-centric, interactions are done via /dev/videoX
> not subdevices like media-centric derivers do. Conversion is possible
> but it is not scope of this patchset and in case such conversion takes
> place, one who will do that, will definitely know what to do.
> Video-centric drivers expose all controls within single video device
> and it cannot hold duplicates of controls, this causes error. So this
> solution exposes camera flip controls and if camera has none, SoC
> controls are exposed.

Sorry, forgot to think in the past. :-)
Thanks for the clarification, it makes sense now!

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com