LinuxTV Patchwork [1/6] media: v4l2-subdev: stub v4l2_subdev_get_try_format()

login
register
mail settings
Submitter Lubomir Rintel
Date Nov. 28, 2018, 5:19 p.m.
Message ID <20181128171918.160643-2-lkundrak@v3.sk>
Download mbox | patch
Permalink /patch/53194/
State Changes Requested
Delegated to: Hans Verkuil
Headers show

Comments

Lubomir Rintel - Nov. 28, 2018, 5:19 p.m.
Provide a dummy implementation when configured without
CONFIG_VIDEO_V4L2_SUBDEV_API to avoid ifdef dance in the drivers that can
be built either with or without the option.

Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 include/media/v4l2-subdev.h | 11 +++++++++++
 1 file changed, 11 insertions(+)
jacopo@jmondi.org - Dec. 3, 2018, 1:48 p.m.
Hi Lubomir,

  thanks for the patches

On Wed, Nov 28, 2018 at 06:19:13PM +0100, Lubomir Rintel wrote:
> Provide a dummy implementation when configured without
> CONFIG_VIDEO_V4L2_SUBDEV_API to avoid ifdef dance in the drivers that can
> be built either with or without the option.
>
> Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  include/media/v4l2-subdev.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 9102d6ca566e..906e28011bb4 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -967,6 +967,17 @@ static inline struct v4l2_rect
>  		pad = 0;
>  	return &cfg[pad].try_compose;
>  }
> +
> +#else /* !defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
> +
> +static inline struct v4l2_mbus_framefmt
> +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_pad_config *cfg,
> +			    unsigned int pad)
> +{
> +	return ERR_PTR(-ENOTTY);
> +}
> +
>  #endif

While at there, what about doing the same for get_try_crop and
get_try_compose? At lease provide stubs, I let you figure out if
you're willing to fix callers too, it seems there are quite a few of
them though

$ git grep v4l2_subdev_get_try* drivers/media/ | grep -v '_format' | wc -l
44

>
>  extern const struct v4l2_file_operations v4l2_subdev_fops;
> --
> 2.19.1
>
Lubomir Rintel - Dec. 4, 2018, 3:01 p.m.
On Mon, 2018-12-03 at 14:48 +0100, jacopo mondi wrote:
> Hi Lubomir,
> 
>   thanks for the patches
> 
> On Wed, Nov 28, 2018 at 06:19:13PM +0100, Lubomir Rintel wrote:
> > Provide a dummy implementation when configured without
> > CONFIG_VIDEO_V4L2_SUBDEV_API to avoid ifdef dance in the drivers
> > that can
> > be built either with or without the option.
> > 
> > Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > ---
> >  include/media/v4l2-subdev.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-
> > subdev.h
> > index 9102d6ca566e..906e28011bb4 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -967,6 +967,17 @@ static inline struct v4l2_rect
> >  		pad = 0;
> >  	return &cfg[pad].try_compose;
> >  }
> > +
> > +#else /* !defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
> > +
> > +static inline struct v4l2_mbus_framefmt
> > +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > +			    struct v4l2_subdev_pad_config *cfg,
> > +			    unsigned int pad)
> > +{
> > +	return ERR_PTR(-ENOTTY);
> > +}
> > +
> >  #endif
> 
> While at there, what about doing the same for get_try_crop and
> get_try_compose? At lease provide stubs, I let you figure out if
> you're willing to fix callers too, it seems there are quite a few of
> them though
> 
> $ git grep v4l2_subdev_get_try* drivers/media/ | grep -v '_format' |
> wc -l
> 44

I'd be happy to do that. However, the drivers that use those seem to
depend on CONFIG_VIDEO_V4L2_SUBDEV_API anyway. Should those
dependencies be eventually done away with?

Please pardon my ignorance -- I don't actually understand why would
anyone disable CONFIG_VIDEO_V4L2_SUBDEV_API.

I'll be following up with a v2 after I get a response from you. It will
address locking issues found with smatch: one introduced by my patch
and one that was there before.

Cheers,
Lubo

> >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> > --
> > 2.19.1
> >
jacopo@jmondi.org - Dec. 6, 2018, 8:30 a.m.
Hi Lubomir,

On Tue, Dec 04, 2018 at 04:01:43PM +0100, Lubomir Rintel wrote:
> On Mon, 2018-12-03 at 14:48 +0100, jacopo mondi wrote:
> > Hi Lubomir,
> >
> >   thanks for the patches
> >
> > On Wed, Nov 28, 2018 at 06:19:13PM +0100, Lubomir Rintel wrote:
> > > Provide a dummy implementation when configured without
> > > CONFIG_VIDEO_V4L2_SUBDEV_API to avoid ifdef dance in the drivers
> > > that can
> > > be built either with or without the option.
> > >
> > > Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > ---
> > >  include/media/v4l2-subdev.h | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-
> > > subdev.h
> > > index 9102d6ca566e..906e28011bb4 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -967,6 +967,17 @@ static inline struct v4l2_rect
> > >  		pad = 0;
> > >  	return &cfg[pad].try_compose;
> > >  }
> > > +
> > > +#else /* !defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
> > > +
> > > +static inline struct v4l2_mbus_framefmt
> > > +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > > +			    struct v4l2_subdev_pad_config *cfg,
> > > +			    unsigned int pad)
> > > +{
> > > +	return ERR_PTR(-ENOTTY);
> > > +}
> > > +
> > >  #endif
> >
> > While at there, what about doing the same for get_try_crop and
> > get_try_compose? At lease provide stubs, I let you figure out if
> > you're willing to fix callers too, it seems there are quite a few of
> > them though
> >
> > $ git grep v4l2_subdev_get_try* drivers/media/ | grep -v '_format' |
> > wc -l
> > 44
>
> I'd be happy to do that. However, the drivers that use those seem to
> depend on CONFIG_VIDEO_V4L2_SUBDEV_API anyway. Should those
> dependencies be eventually done away with?
>

I don't think it is the case to drop the dependencies. If you go down
that path you would need to be very careful. It's enough to add stubs
for those functions like you've done for v4l2_subdev_get_try_format().

Now, looking around a bit in more detail, most sensor drivers return
-ENOTTY if you require V4L2_SUBDEV_FORMAT_TRY format when
CONFIG_VIDEO_V4L2_SUBDEV_API is not defined. I would say all drivers
but mt9v111.c, which is one of the most recent ones, and that deals
with the issue as:

static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format(
					struct mt9v111_dev *mt9v111,
					struct v4l2_subdev_pad_config *cfg,
					unsigned int pad,
					enum v4l2_subdev_format_whence which)
{
	switch (which) {
	case V4L2_SUBDEV_FORMAT_TRY:
#if IS_ENABLED(CONFIG_VIDEO_V4L2_SUBDEV_API)
		return v4l2_subdev_get_try_format(&mt9v111->sd, cfg, pad);
#else
		return &cfg->try_fmt;
#endif
	case V4L2_SUBDEV_FORMAT_ACTIVE:
		return &mt9v111->fmt;
	default:
		return NULL;
	}
}

Since I wrote that part, and I recall it had been suggested to me, I
wonder which one of the two approaches it actually correct :/


> Please pardon my ignorance -- I don't actually understand why would
> anyone disable CONFIG_VIDEO_V4L2_SUBDEV_API.

The config options is described as:
Enables the V4L2 sub-device pad-level userspace API used to configure
video format, size and frame rate between hardware blocks.

Some driver simply do not expose a subdev in userspace. It might be
discussed that if selecting MEDIA_CONTROLLER should in facts be enough
and to imply CONFIG_VIDEO_V4L2_SUBDEV_API, but that's a separate
issue.
>
> I'll be following up with a v2 after I get a response from you. It will
> address locking issues found with smatch: one introduced by my patch
> and one that was there before.
>

Yep, ov2659 was b0rken already, thanks for fixing it while at there.

Thanks
   j

> Cheers,
> Lubo
>
> > >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> > > --
> > > 2.19.1
> > >
>
Hans Verkuil - Dec. 7, 2018, 8:57 a.m.
On 12/06/2018 09:30 AM, jacopo mondi wrote:
> Hi Lubomir,
> 
> On Tue, Dec 04, 2018 at 04:01:43PM +0100, Lubomir Rintel wrote:
>> On Mon, 2018-12-03 at 14:48 +0100, jacopo mondi wrote:
>>> Hi Lubomir,
>>>
>>>   thanks for the patches
>>>
>>> On Wed, Nov 28, 2018 at 06:19:13PM +0100, Lubomir Rintel wrote:
>>>> Provide a dummy implementation when configured without
>>>> CONFIG_VIDEO_V4L2_SUBDEV_API to avoid ifdef dance in the drivers
>>>> that can
>>>> be built either with or without the option.
>>>>
>>>> Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
>>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>>>> ---
>>>>  include/media/v4l2-subdev.h | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-
>>>> subdev.h
>>>> index 9102d6ca566e..906e28011bb4 100644
>>>> --- a/include/media/v4l2-subdev.h
>>>> +++ b/include/media/v4l2-subdev.h
>>>> @@ -967,6 +967,17 @@ static inline struct v4l2_rect
>>>>  		pad = 0;
>>>>  	return &cfg[pad].try_compose;
>>>>  }
>>>> +
>>>> +#else /* !defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
>>>> +
>>>> +static inline struct v4l2_mbus_framefmt
>>>> +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
>>>> +			    struct v4l2_subdev_pad_config *cfg,
>>>> +			    unsigned int pad)
>>>> +{
>>>> +	return ERR_PTR(-ENOTTY);
>>>> +}
>>>> +
>>>>  #endif
>>>
>>> While at there, what about doing the same for get_try_crop and
>>> get_try_compose? At lease provide stubs, I let you figure out if
>>> you're willing to fix callers too, it seems there are quite a few of
>>> them though
>>>
>>> $ git grep v4l2_subdev_get_try* drivers/media/ | grep -v '_format' |
>>> wc -l
>>> 44
>>
>> I'd be happy to do that. However, the drivers that use those seem to
>> depend on CONFIG_VIDEO_V4L2_SUBDEV_API anyway. Should those
>> dependencies be eventually done away with?
>>
> 
> I don't think it is the case to drop the dependencies. If you go down
> that path you would need to be very careful. It's enough to add stubs
> for those functions like you've done for v4l2_subdev_get_try_format().
> 
> Now, looking around a bit in more detail, most sensor drivers return
> -ENOTTY if you require V4L2_SUBDEV_FORMAT_TRY format when
> CONFIG_VIDEO_V4L2_SUBDEV_API is not defined. I would say all drivers
> but mt9v111.c, which is one of the most recent ones, and that deals
> with the issue as:
> 
> static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format(
> 					struct mt9v111_dev *mt9v111,
> 					struct v4l2_subdev_pad_config *cfg,
> 					unsigned int pad,
> 					enum v4l2_subdev_format_whence which)
> {
> 	switch (which) {
> 	case V4L2_SUBDEV_FORMAT_TRY:
> #if IS_ENABLED(CONFIG_VIDEO_V4L2_SUBDEV_API)
> 		return v4l2_subdev_get_try_format(&mt9v111->sd, cfg, pad);
> #else
> 		return &cfg->try_fmt;
> #endif
> 	case V4L2_SUBDEV_FORMAT_ACTIVE:
> 		return &mt9v111->fmt;
> 	default:
> 		return NULL;
> 	}
> }
> 
> Since I wrote that part, and I recall it had been suggested to me, I
> wonder which one of the two approaches it actually correct :/
> 
> 
>> Please pardon my ignorance -- I don't actually understand why would
>> anyone disable CONFIG_VIDEO_V4L2_SUBDEV_API.
> 
> The config options is described as:
> Enables the V4L2 sub-device pad-level userspace API used to configure
> video format, size and frame rate between hardware blocks.
> 
> Some driver simply do not expose a subdev in userspace. It might be
> discussed that if selecting MEDIA_CONTROLLER should in facts be enough
> and to imply CONFIG_VIDEO_V4L2_SUBDEV_API, but that's a separate
> issue.

I've thought for quite some time now that CONFIG_VIDEO_V4L2_SUBDEV_API
should be removed. I really do not see any advantage of having that config
option. You save a tiny bit of memory, at the cost of creating confusion.

Complexity is the real enemy of media drivers, and trying to keep that down
is always a good thing.

That said, removing CONFIG_VIDEO_V4L2_SUBDEV_API won't help much since in
the code above it would just replace #if IS_ENABLED(CONFIG_VIDEO_V4L2_SUBDEV_API)
by #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER).

In fact, I would favor to just always enable both MEDIA_CONTROLLER and
V4L2_SUBDEV_API for V4L2 devices. They don't have to use the media controller
if they don't want to, but it's there if needed.

It simplifies a lot of code at the expense of a bit more memory usage. But
given the huge amount of memory that the video buffers take, I really don't
think that is an issue.

Also, as more and more drivers require the media controller anyway, it is likely
to be enabled in standard distros anyway. It is for debian, in any case.

I'll make an RFC patch and post it. See what people think.

Regards,

	Hans

>>
>> I'll be following up with a v2 after I get a response from you. It will
>> address locking issues found with smatch: one introduced by my patch
>> and one that was there before.
>>
> 
> Yep, ov2659 was b0rken already, thanks for fixing it while at there.
> 
> Thanks
>    j
> 
>> Cheers,
>> Lubo
>>
>>>>  extern const struct v4l2_file_operations v4l2_subdev_fops;
>>>> --
>>>> 2.19.1
>>>>
>>
Sakari Ailus - Dec. 9, 2018, 9:17 p.m.
Hi Lubomir, Jacopo,

On Tue, Dec 04, 2018 at 04:01:43PM +0100, Lubomir Rintel wrote:
> On Mon, 2018-12-03 at 14:48 +0100, jacopo mondi wrote:
> > Hi Lubomir,
> > 
> >   thanks for the patches
> > 
> > On Wed, Nov 28, 2018 at 06:19:13PM +0100, Lubomir Rintel wrote:
> > > Provide a dummy implementation when configured without
> > > CONFIG_VIDEO_V4L2_SUBDEV_API to avoid ifdef dance in the drivers
> > > that can
> > > be built either with or without the option.
> > > 
> > > Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > ---
> > >  include/media/v4l2-subdev.h | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-
> > > subdev.h
> > > index 9102d6ca566e..906e28011bb4 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -967,6 +967,17 @@ static inline struct v4l2_rect
> > >  		pad = 0;
> > >  	return &cfg[pad].try_compose;
> > >  }
> > > +
> > > +#else /* !defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
> > > +
> > > +static inline struct v4l2_mbus_framefmt
> > > +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > > +			    struct v4l2_subdev_pad_config *cfg,
> > > +			    unsigned int pad)
> > > +{
> > > +	return ERR_PTR(-ENOTTY);
> > > +}
> > > +
> > >  #endif
> > 
> > While at there, what about doing the same for get_try_crop and
> > get_try_compose? At lease provide stubs, I let you figure out if
> > you're willing to fix callers too, it seems there are quite a few of
> > them though
> > 
> > $ git grep v4l2_subdev_get_try* drivers/media/ | grep -v '_format' |
> > wc -l
> > 44
> 
> I'd be happy to do that. However, the drivers that use those seem to
> depend on CONFIG_VIDEO_V4L2_SUBDEV_API anyway. Should those
> dependencies be eventually done away with?
> 
> Please pardon my ignorance -- I don't actually understand why would
> anyone disable CONFIG_VIDEO_V4L2_SUBDEV_API.
> 
> I'll be following up with a v2 after I get a response from you. It will
> address locking issues found with smatch: one introduced by my patch
> and one that was there before.

Hans wrote a patch that enables VIDEO_V4L2_SUBDEV_API if
MEDIA_CAMERA_SUPPORT has been selected. So much of these #ifdefs will not
be needed going forward. It's an RFC for now, but that'd require a lot of
conditional compiling in drivers.

<URL:https://patchwork.linuxtv.org/patch/53370/>

The current use of the get_fmt callback serves the regular V4L2 API use, in
which case getting the try format is not meaningful.

Patch

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 9102d6ca566e..906e28011bb4 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -967,6 +967,17 @@  static inline struct v4l2_rect
 		pad = 0;
 	return &cfg[pad].try_compose;
 }
+
+#else /* !defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
+
+static inline struct v4l2_mbus_framefmt
+*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_pad_config *cfg,
+			    unsigned int pad)
+{
+	return ERR_PTR(-ENOTTY);
+}
+
 #endif
 
 extern const struct v4l2_file_operations v4l2_subdev_fops;

Privacy Policy