LinuxLists.cc - [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Sakari,

On Mon, Nov 17, 2014 at 10:53 AM, Sakari Ailus
<[email protected]> wrote:
> Hi Prabhakar,
>
> Thank you for the patch.
>
> Lad, Prabhakar wrote:
>>
>> this patch removes the guard CONFIG_VIDEO_V4L2_SUBDEV_API
>> for v4l2_subdev_get_try_*() functions.
>> In cases where a subdev using v4l2_subdev_get_try_*() calls
>> internally and the bridge using subdev pad ops which is
>> not MC aware forces to select MEDIA_CONTROLLER, as
>> VIDEO_V4L2_SUBDEV_API is dependent on it.
>>
>> Signed-off-by: Lad, Prabhakar <[email protected]>
>> ---
>> include/media/v4l2-subdev.h | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 5860292..076ca11 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -642,7 +642,6 @@ struct v4l2_subdev_fh {
>> #define to_v4l2_subdev_fh(fh) \
>> container_of(fh, struct v4l2_subdev_fh, vfh)
>>
>> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>
>
> Wouldn't you need to drop these from struct v4l2_subdev_fh as well? The code
> won't compile if the fields aren't there.
>
Ah missed it, thanks for the catch!

Thanks,
--Prabhakar Lad

>> #define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name) \
>> static inline struct rtype * \
>> v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh, \
>> @@ -656,7 +655,6 @@ struct v4l2_subdev_fh {
>> __V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, format, try_fmt)
>> __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, crop, try_crop)
>> __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, compose, try_compose)
>> -#endif
>>
>> extern const struct v4l2_file_operations v4l2_subdev_fops;
>>
>>
>
> --
> Kind regards,
>
> Sakari Ailus
> [email protected]

2014-11-18 09:40:09

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

On 11/17/14 11:41, Lad, Prabhakar wrote:
> this patch removes the guard CONFIG_VIDEO_V4L2_SUBDEV_API
> for v4l2_subdev_get_try_*() functions.
> In cases where a subdev using v4l2_subdev_get_try_*() calls
> internally and the bridge using subdev pad ops which is
> not MC aware forces to select MEDIA_CONTROLLER, as
> VIDEO_V4L2_SUBDEV_API is dependent on it.
>
> Signed-off-by: Lad, Prabhakar <[email protected]>
> ---
> include/media/v4l2-subdev.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 5860292..076ca11 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -642,7 +642,6 @@ struct v4l2_subdev_fh {
> #define to_v4l2_subdev_fh(fh) \
> container_of(fh, struct v4l2_subdev_fh, vfh)
>
> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> #define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name) \
> static inline struct rtype * \
> v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh, \
> @@ -656,7 +655,6 @@ struct v4l2_subdev_fh {
> __V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, format, try_fmt)
> __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, crop, try_crop)
> __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, compose, try_compose)
> -#endif
>
> extern const struct v4l2_file_operations v4l2_subdev_fops;
>
>

The problem is that v4l2_subdev_get_try_*() needs a v4l2_subdev_fh which
you don't have if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined. So I don't
see how removing the guards help with that.

What can be done is that if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined,
then these functions return NULL.

BTW, one patch I will very happily accept is one where the __V4L2_SUBDEV_MK_GET_TRY
is removed and these three try functions are just written as proper
static inlines. I find it very obfuscated code.

In addition, because it is a macro you won't find the function definitions
if you grep on the function name.

But any functional changes here need to be Acked by Laurent first.

Regards,

Hans

2014-11-18 13:36:20

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Hans,

On Tue, Nov 18, 2014 at 9:39 AM, Hans Verkuil <[email protected]> wrote:
> On 11/17/14 11:41, Lad, Prabhakar wrote:
>> this patch removes the guard CONFIG_VIDEO_V4L2_SUBDEV_API
>> for v4l2_subdev_get_try_*() functions.
>> In cases where a subdev using v4l2_subdev_get_try_*() calls
>> internally and the bridge using subdev pad ops which is
>> not MC aware forces to select MEDIA_CONTROLLER, as
>> VIDEO_V4L2_SUBDEV_API is dependent on it.
>>
>> Signed-off-by: Lad, Prabhakar <[email protected]>
>> ---
>> include/media/v4l2-subdev.h | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 5860292..076ca11 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -642,7 +642,6 @@ struct v4l2_subdev_fh {
>> #define to_v4l2_subdev_fh(fh) \
>> container_of(fh, struct v4l2_subdev_fh, vfh)
>>
>> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>> #define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name) \
>> static inline struct rtype * \
>> v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh, \
>> @@ -656,7 +655,6 @@ struct v4l2_subdev_fh {
>> __V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, format, try_fmt)
>> __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, crop, try_crop)
>> __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, compose, try_compose)
>> -#endif
>>
>> extern const struct v4l2_file_operations v4l2_subdev_fops;
>>
>>
>
> The problem is that v4l2_subdev_get_try_*() needs a v4l2_subdev_fh which
> you don't have if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined. So I don't
> see how removing the guards help with that.
>
Yes

> What can be done is that if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined,
> then these functions return NULL.
>
exactly.

> BTW, one patch I will very happily accept is one where the __V4L2_SUBDEV_MK_GET_TRY
> is removed and these three try functions are just written as proper
> static inlines. I find it very obfuscated code.
>
the functions were initially inline itself which were changes into
macro's later.

> In addition, because it is a macro you won't find the function definitions
> if you grep on the function name.
>
> But any functional changes here need to be Acked by Laurent first.
>
Yes, ill probably wait until Laurent is back from his holidays.

Thanks,
--Prabhakar Lad

2014-11-18 18:08:36

by Sakari Ailus

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Hans and Prabhakar,

On Tue, Nov 18, 2014 at 10:39:24AM +0100, Hans Verkuil wrote:
> On 11/17/14 11:41, Lad, Prabhakar wrote:
> > this patch removes the guard CONFIG_VIDEO_V4L2_SUBDEV_API
> > for v4l2_subdev_get_try_*() functions.
> > In cases where a subdev using v4l2_subdev_get_try_*() calls
> > internally and the bridge using subdev pad ops which is
> > not MC aware forces to select MEDIA_CONTROLLER, as
> > VIDEO_V4L2_SUBDEV_API is dependent on it.
> >
> > Signed-off-by: Lad, Prabhakar <[email protected]>
> > ---
> > include/media/v4l2-subdev.h | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 5860292..076ca11 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -642,7 +642,6 @@ struct v4l2_subdev_fh {
> > #define to_v4l2_subdev_fh(fh) \
> > container_of(fh, struct v4l2_subdev_fh, vfh)
> >
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > #define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name) \
> > static inline struct rtype * \
> > v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh, \
> > @@ -656,7 +655,6 @@ struct v4l2_subdev_fh {
> > __V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, format, try_fmt)
> > __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, crop, try_crop)
> > __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, compose, try_compose)
> > -#endif
> >
> > extern const struct v4l2_file_operations v4l2_subdev_fops;
> >
> >
>
> The problem is that v4l2_subdev_get_try_*() needs a v4l2_subdev_fh which
> you don't have if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined. So I don't
> see how removing the guards help with that.
>
> What can be done is that if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined,
> then these functions return NULL.

Sure. That's a better choice than removing the config option dependency of
the fields struct v4l2_subdev.

> BTW, one patch I will very happily accept is one where the __V4L2_SUBDEV_MK_GET_TRY
> is removed and these three try functions are just written as proper
> static inlines. I find it very obfuscated code.

I originally wrote them like that in order to avoid writing essentially the
same code three times over. If there will be more targets, the same repeats
further, should one write those functions open for all different macro
arguments. That's why it was a macro to begin with.

> In addition, because it is a macro you won't find the function definitions
> if you grep on the function name.

True as well. You could simply change the macro to include the full function
name. This was not suggested in review back then AFAIR.

> But any functional changes here need to be Acked by Laurent first.

--
Kind regards,

Sakari Ailus
e-mail: [email protected] XMPP: [email protected]

2014-11-29 18:30:45

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Laurent,

On Tue, Nov 18, 2014 at 6:07 PM, Sakari Ailus <[email protected]> wrote:
> Hi Hans and Prabhakar,
>
> On Tue, Nov 18, 2014 at 10:39:24AM +0100, Hans Verkuil wrote:
>> On 11/17/14 11:41, Lad, Prabhakar wrote:
>> > this patch removes the guard CONFIG_VIDEO_V4L2_SUBDEV_API
>> > for v4l2_subdev_get_try_*() functions.
>> > In cases where a subdev using v4l2_subdev_get_try_*() calls
>> > internally and the bridge using subdev pad ops which is
>> > not MC aware forces to select MEDIA_CONTROLLER, as
>> > VIDEO_V4L2_SUBDEV_API is dependent on it.
>> >
>> > Signed-off-by: Lad, Prabhakar <[email protected]>
>> > ---
>> > include/media/v4l2-subdev.h | 2 --
>> > 1 file changed, 2 deletions(-)
>> >
>> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> > index 5860292..076ca11 100644
>> > --- a/include/media/v4l2-subdev.h
>> > +++ b/include/media/v4l2-subdev.h
>> > @@ -642,7 +642,6 @@ struct v4l2_subdev_fh {
>> > #define to_v4l2_subdev_fh(fh) \
>> > container_of(fh, struct v4l2_subdev_fh, vfh)
>> >
>> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>> > #define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name) \
>> > static inline struct rtype * \
>> > v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh, \
>> > @@ -656,7 +655,6 @@ struct v4l2_subdev_fh {
>> > __V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, format, try_fmt)
>> > __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, crop, try_crop)
>> > __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, compose, try_compose)
>> > -#endif
>> >
>> > extern const struct v4l2_file_operations v4l2_subdev_fops;
>> >
>> >
>>
>> The problem is that v4l2_subdev_get_try_*() needs a v4l2_subdev_fh which
>> you don't have if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined. So I don't
>> see how removing the guards help with that.
>>
>> What can be done is that if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined,
>> then these functions return NULL.
>
> Sure. That's a better choice than removing the config option dependency of
> the fields struct v4l2_subdev.
>
>> BTW, one patch I will very happily accept is one where the __V4L2_SUBDEV_MK_GET_TRY
>> is removed and these three try functions are just written as proper
>> static inlines. I find it very obfuscated code.
>
> I originally wrote them like that in order to avoid writing essentially the
> same code three times over. If there will be more targets, the same repeats
> further, should one write those functions open for all different macro
> arguments. That's why it was a macro to begin with.
>
>> In addition, because it is a macro you won't find the function definitions
>> if you grep on the function name.
>
> True as well. You could simply change the macro to include the full function
> name. This was not suggested in review back then AFAIR.
>
>> But any functional changes here need to be Acked by Laurent first.
>

How do you want me to proceed on this ?

Thanks,
--Prabhakar Lad

2014-11-29 19:11:59

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Prabhakar,

On Saturday 29 November 2014 18:30:09 Prabhakar Lad wrote:
> On Tue, Nov 18, 2014 at 6:07 PM, Sakari Ailus <[email protected]> wrote:
> > On Tue, Nov 18, 2014 at 10:39:24AM +0100, Hans Verkuil wrote:
> >> On 11/17/14 11:41, Lad, Prabhakar wrote:
> >> > this patch removes the guard CONFIG_VIDEO_V4L2_SUBDEV_API
> >> > for v4l2_subdev_get_try_*() functions.
> >> > In cases where a subdev using v4l2_subdev_get_try_*() calls
> >> > internally and the bridge using subdev pad ops which is
> >> > not MC aware forces to select MEDIA_CONTROLLER, as
> >> > VIDEO_V4L2_SUBDEV_API is dependent on it.
> >> >
> >> > Signed-off-by: Lad, Prabhakar <[email protected]>
> >> > ---
> >> >
> >> > include/media/v4l2-subdev.h | 2 --
> >> > 1 file changed, 2 deletions(-)
> >> >
> >> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> > index 5860292..076ca11 100644
> >> > --- a/include/media/v4l2-subdev.h
> >> > +++ b/include/media/v4l2-subdev.h
> >> > @@ -642,7 +642,6 @@ struct v4l2_subdev_fh {
> >> > #define to_v4l2_subdev_fh(fh) \
> >> > container_of(fh, struct v4l2_subdev_fh, vfh)
> >> >
> >> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> >> > #define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)
> >> > \
> >> > static inline struct rtype * \
> >> > v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh, \
> >> >
> >> > @@ -656,7 +655,6 @@ struct v4l2_subdev_fh {
> >> > __V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, format, try_fmt)
> >> > __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, crop, try_crop)
> >> > __V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, compose, try_compose)
> >> > -#endif
> >> >
> >> > extern const struct v4l2_file_operations v4l2_subdev_fops;
> >>
> >> The problem is that v4l2_subdev_get_try_*() needs a v4l2_subdev_fh which
> >> you don't have if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined. So I don't
> >> see how removing the guards help with that.
> >>
> >> What can be done is that if CONFIG_VIDEO_V4L2_SUBDEV_API is not defined,
> >> then these functions return NULL.
> >
> > Sure. That's a better choice than removing the config option dependency of
> > the fields struct v4l2_subdev.

Decoupling CONFIG_VIDEO_V4L2_SUBDEV_API from the availability of the in-kernel
pad format and selection rectangles helpers is definitely a good idea. I was
thinking about decoupling the try format and rectangles from v4l2_subdev_fh by
creating a kind of configuration store structure to store them, and embedding
that structure in v4l2_subdev_fh. The pad-level operations would then take a
pointer to the configuration store instead of the v4l2_subdev_fh. Bridge
drivers that want to implement TRY_FMT based on pad-level operations would
create a configuration store, use the pad-level operations, and destroy the
configuration store. The userspace subdev API would use the configuration
store from the file handle.

> >> BTW, one patch I will very happily accept is one where the
> >> __V4L2_SUBDEV_MK_GET_TRY is removed and these three try functions are
> >> just written as proper static inlines. I find it very obfuscated code.
> >
> > I originally wrote them like that in order to avoid writing essentially
> > the same code three times over. If there will be more targets, the same
> > repeats further, should one write those functions open for all different
> > macro arguments. That's why it was a macro to begin with.
> >
> >> In addition, because it is a macro you won't find the function
> >> definitions if you grep on the function name.
> >
> > True as well. You could simply change the macro to include the full
> > function name. This was not suggested in review back then AFAIR.
> >
> >> But any functional changes here need to be Acked by Laurent first.
>
> How do you want me to proceed on this ?

--
Regards,

Laurent Pinchart

2014-11-30 21:06:25

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Laurent,

On Sat, Nov 29, 2014 at 7:12 PM, Laurent Pinchart
<[email protected]> wrote:
> Hi Prabhakar,
>
[Snip]
>> > Sure. That's a better choice than removing the config option dependency of
>> > the fields struct v4l2_subdev.
>
> Decoupling CONFIG_VIDEO_V4L2_SUBDEV_API from the availability of the in-kernel
> pad format and selection rectangles helpers is definitely a good idea. I was
> thinking about decoupling the try format and rectangles from v4l2_subdev_fh by
> creating a kind of configuration store structure to store them, and embedding
> that structure in v4l2_subdev_fh. The pad-level operations would then take a
> pointer to the configuration store instead of the v4l2_subdev_fh. Bridge
> drivers that want to implement TRY_FMT based on pad-level operations would
> create a configuration store, use the pad-level operations, and destroy the
> configuration store. The userspace subdev API would use the configuration
> store from the file handle.
>
are planning to work/post any time soon ? Or are you OK with suggestion from
Hans ?

Thanks,
--Prabhakar Lad

2014-11-30 21:16:19

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

On Sunday 30 November 2014 21:05:50 Prabhakar Lad wrote:
> On Sat, Nov 29, 2014 at 7:12 PM, Laurent Pinchart wrote:
> > Hi Prabhakar,
>
> [Snip]
>
> >> > Sure. That's a better choice than removing the config option dependency
> >> > of the fields struct v4l2_subdev.
> >
> > Decoupling CONFIG_VIDEO_V4L2_SUBDEV_API from the availability of the
> > in-kernel pad format and selection rectangles helpers is definitely a
> > good idea. I was thinking about decoupling the try format and rectangles
> > from v4l2_subdev_fh by creating a kind of configuration store structure
> > to store them, and embedding that structure in v4l2_subdev_fh. The
> > pad-level operations would then take a pointer to the configuration store
> > instead of the v4l2_subdev_fh. Bridge drivers that want to implement
> > TRY_FMT based on pad-level operations would create a configuration store,
> > use the pad-level operations, and destroy the configuration store. The
> > userspace subdev API would use the configuration store from the file
> > handle.
>
> are planning to work/post any time soon ? Or are you OK with suggestion from
> Hans ?

I have no plan to work on that myself now, I was hoping you could implement it
;-)

--
Regards,

Laurent Pinchart

2014-11-30 21:31:09

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Laurent,

On Sun, Nov 30, 2014 at 9:16 PM, Laurent Pinchart
<[email protected]> wrote:
> On Sunday 30 November 2014 21:05:50 Prabhakar Lad wrote:
>> On Sat, Nov 29, 2014 at 7:12 PM, Laurent Pinchart wrote:
>> > Hi Prabhakar,
>>
>> [Snip]
>>
>> >> > Sure. That's a better choice than removing the config option dependency
>> >> > of the fields struct v4l2_subdev.
>> >
>> > Decoupling CONFIG_VIDEO_V4L2_SUBDEV_API from the availability of the
>> > in-kernel pad format and selection rectangles helpers is definitely a
>> > good idea. I was thinking about decoupling the try format and rectangles
>> > from v4l2_subdev_fh by creating a kind of configuration store structure
>> > to store them, and embedding that structure in v4l2_subdev_fh. The
>> > pad-level operations would then take a pointer to the configuration store
>> > instead of the v4l2_subdev_fh. Bridge drivers that want to implement
>> > TRY_FMT based on pad-level operations would create a configuration store,
>> > use the pad-level operations, and destroy the configuration store. The
>> > userspace subdev API would use the configuration store from the file
>> > handle.
>>
>> are planning to work/post any time soon ? Or are you OK with suggestion from
>> Hans ?
>
> I have no plan to work on that myself now, I was hoping you could implement it
> ;-)
>
OK will implement it.

Can you please elaborate a more on this "The userspace subdev API would use
the configuration store from the file handle."

Thanks,
--Prabhakar Lad

2014-12-01 23:26:04

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Prabhakar,

On Sunday 30 November 2014 21:30:35 Prabhakar Lad wrote:
> On Sun, Nov 30, 2014 at 9:16 PM, Laurent Pinchart wrote:
> > On Sunday 30 November 2014 21:05:50 Prabhakar Lad wrote:
> >> On Sat, Nov 29, 2014 at 7:12 PM, Laurent Pinchart wrote:
> >> > Hi Prabhakar,
> >>
> >> [Snip]
> >>
> >>>>> Sure. That's a better choice than removing the config option
> >>>>> dependency of the fields struct v4l2_subdev.
> >>>
> >>> Decoupling CONFIG_VIDEO_V4L2_SUBDEV_API from the availability of the
> >>> in-kernel pad format and selection rectangles helpers is definitely a
> >>> good idea. I was thinking about decoupling the try format and
> >>> rectangles from v4l2_subdev_fh by creating a kind of configuration store
> >>> structure to store them, and embedding that structure in v4l2_subdev_fh.
> >>> The pad-level operations would then take a pointer to the configuration
> >>> store instead of the v4l2_subdev_fh. Bridge drivers that want to
> >>> implement TRY_FMT based on pad-level operations would create a
> >>> configuration store, use the pad-level operations, and destroy the
> >>> configuration store. The userspace subdev API would use the
> >>> configuration store from the file handle.
> >>
> >> are planning to work/post any time soon ? Or are you OK with suggestion
> >> from Hans ?
> >
> > I have no plan to work on that myself now, I was hoping you could
> > implement it ;-)
>
> OK will implement it.
>
> Can you please elaborate a more on this "The userspace subdev API would use
> the configuration store from the file handle."

Basically,

1. Create a subdev pad configuration store structure to store the formats and
selection rectangles for each pad.

2. Embed an instance of that structure in v4l2_subdev_fh.

3. Modify the subdev pad ops to take a configuration store pointer instead of
a file handle pointer.

The userspace API implementation (v4l2-subdev.c) would then pass &fh->store to
the pad operations instead of fh.

Bridge drivers that need to implement TRY_FMT on top of pad ops would create a
temporary store (or temporary stores when multiple subsdevs are involved),
call the pad ops with a pointer to the temporary store to propagate TRY
formats, destroy the store(s) and return the resulting format.

Is that clear ?

--
Regards,

Laurent Pinchart

2014-12-02 07:45:55

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

On 12/02/2014 12:26 AM, Laurent Pinchart wrote:
> Hi Prabhakar,
>
> On Sunday 30 November 2014 21:30:35 Prabhakar Lad wrote:
>> On Sun, Nov 30, 2014 at 9:16 PM, Laurent Pinchart wrote:
>>> On Sunday 30 November 2014 21:05:50 Prabhakar Lad wrote:
>>>> On Sat, Nov 29, 2014 at 7:12 PM, Laurent Pinchart wrote:
>>>>> Hi Prabhakar,
>>>>
>>>> [Snip]
>>>>
>>>>>>> Sure. That's a better choice than removing the config option
>>>>>>> dependency of the fields struct v4l2_subdev.
>>>>>
>>>>> Decoupling CONFIG_VIDEO_V4L2_SUBDEV_API from the availability of the
>>>>> in-kernel pad format and selection rectangles helpers is definitely a
>>>>> good idea. I was thinking about decoupling the try format and
>>>>> rectangles from v4l2_subdev_fh by creating a kind of configuration store
>>>>> structure to store them, and embedding that structure in v4l2_subdev_fh.
>>>>> The pad-level operations would then take a pointer to the configuration
>>>>> store instead of the v4l2_subdev_fh. Bridge drivers that want to
>>>>> implement TRY_FMT based on pad-level operations would create a
>>>>> configuration store, use the pad-level operations, and destroy the
>>>>> configuration store. The userspace subdev API would use the
>>>>> configuration store from the file handle.
>>>>
>>>> are planning to work/post any time soon ? Or are you OK with suggestion
>>>> from Hans ?
>>>
>>> I have no plan to work on that myself now, I was hoping you could
>>> implement it ;-)
>>
>> OK will implement it.
>>
>> Can you please elaborate a more on this "The userspace subdev API would use
>> the configuration store from the file handle."
>
> Basically,
>
> 1. Create a subdev pad configuration store structure to store the formats and
> selection rectangles for each pad.

I wouldn't call it a 'store'. Just call it fmt_config or pad_config something like
that.

>
> 2. Embed an instance of that structure in v4l2_subdev_fh.
>
> 3. Modify the subdev pad ops to take a configuration store pointer instead of
> a file handle pointer.
>
> The userspace API implementation (v4l2-subdev.c) would then pass &fh->store to
> the pad operations instead of fh.
>
> Bridge drivers that need to implement TRY_FMT on top of pad ops would create a
> temporary store (or temporary stores when multiple subsdevs are involved),
> call the pad ops with a pointer to the temporary store to propagate TRY
> formats, destroy the store(s) and return the resulting format.

That will work. I think this is a good approach and it shouldn't be too difficult.

Regards,

Hans

2014-12-02 07:52:01

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Hans,

On Mon, Dec 1, 2014 at 11:26 PM, Laurent Pinchart
<[email protected]> wrote:
> Hi Prabhakar,
>
> On Sunday 30 November 2014 21:30:35 Prabhakar Lad wrote:
>> On Sun, Nov 30, 2014 at 9:16 PM, Laurent Pinchart wrote:
>> > On Sunday 30 November 2014 21:05:50 Prabhakar Lad wrote:
>> >> On Sat, Nov 29, 2014 at 7:12 PM, Laurent Pinchart wrote:
>> >> > Hi Prabhakar,
>> >>
>> >> [Snip]
>> >>
>> >>>>> Sure. That's a better choice than removing the config option
>> >>>>> dependency of the fields struct v4l2_subdev.
>> >>>
>> >>> Decoupling CONFIG_VIDEO_V4L2_SUBDEV_API from the availability of the
>> >>> in-kernel pad format and selection rectangles helpers is definitely a
>> >>> good idea. I was thinking about decoupling the try format and
>> >>> rectangles from v4l2_subdev_fh by creating a kind of configuration store
>> >>> structure to store them, and embedding that structure in v4l2_subdev_fh.
>> >>> The pad-level operations would then take a pointer to the configuration
>> >>> store instead of the v4l2_subdev_fh. Bridge drivers that want to
>> >>> implement TRY_FMT based on pad-level operations would create a
>> >>> configuration store, use the pad-level operations, and destroy the
>> >>> configuration store. The userspace subdev API would use the
>> >>> configuration store from the file handle.
>> >>
>> >> are planning to work/post any time soon ? Or are you OK with suggestion
>> >> from Hans ?
>> >
>> > I have no plan to work on that myself now, I was hoping you could
>> > implement it ;-)
>>
>> OK will implement it.
>>
>> Can you please elaborate a more on this "The userspace subdev API would use
>> the configuration store from the file handle."
>
> Basically,
>
> 1. Create a subdev pad configuration store structure to store the formats and
> selection rectangles for each pad.
>
> 2. Embed an instance of that structure in v4l2_subdev_fh.
>
> 3. Modify the subdev pad ops to take a configuration store pointer instead of
> a file handle pointer.
>
> The userspace API implementation (v4l2-subdev.c) would then pass &fh->store to
> the pad operations instead of fh.
>
> Bridge drivers that need to implement TRY_FMT on top of pad ops would create a
> temporary store (or temporary stores when multiple subsdevs are involved),
> call the pad ops with a pointer to the temporary store to propagate TRY
> formats, destroy the store(s) and return the resulting format.
>
> Is that clear ?
>
Yes thank you, I'll soon shoot a RFC version.

Regards,
--Prabhakar Lad

2014-12-02 12:54:11

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

On 12/02/14 08:45, Hans Verkuil wrote:
> On 12/02/2014 12:26 AM, Laurent Pinchart wrote:
>> Hi Prabhakar,
>>
>> On Sunday 30 November 2014 21:30:35 Prabhakar Lad wrote:
>>> On Sun, Nov 30, 2014 at 9:16 PM, Laurent Pinchart wrote:
>>>> On Sunday 30 November 2014 21:05:50 Prabhakar Lad wrote:
>>>>> On Sat, Nov 29, 2014 at 7:12 PM, Laurent Pinchart wrote:
>>>>>> Hi Prabhakar,
>>>>>
>>>>> [Snip]
>>>>>
>>>>>>>> Sure. That's a better choice than removing the config option
>>>>>>>> dependency of the fields struct v4l2_subdev.
>>>>>>
>>>>>> Decoupling CONFIG_VIDEO_V4L2_SUBDEV_API from the availability of the
>>>>>> in-kernel pad format and selection rectangles helpers is definitely a
>>>>>> good idea. I was thinking about decoupling the try format and
>>>>>> rectangles from v4l2_subdev_fh by creating a kind of configuration store
>>>>>> structure to store them, and embedding that structure in v4l2_subdev_fh.
>>>>>> The pad-level operations would then take a pointer to the configuration
>>>>>> store instead of the v4l2_subdev_fh. Bridge drivers that want to
>>>>>> implement TRY_FMT based on pad-level operations would create a
>>>>>> configuration store, use the pad-level operations, and destroy the
>>>>>> configuration store. The userspace subdev API would use the
>>>>>> configuration store from the file handle.
>>>>>
>>>>> are planning to work/post any time soon ? Or are you OK with suggestion
>>>>> from Hans ?
>>>>
>>>> I have no plan to work on that myself now, I was hoping you could
>>>> implement it ;-)
>>>
>>> OK will implement it.
>>>
>>> Can you please elaborate a more on this "The userspace subdev API would use
>>> the configuration store from the file handle."
>>
>> Basically,
>>
>> 1. Create a subdev pad configuration store structure to store the formats and
>> selection rectangles for each pad.
>
> I wouldn't call it a 'store'. Just call it fmt_config or pad_config something like
> that.
>
>>
>> 2. Embed an instance of that structure in v4l2_subdev_fh.
>>
>> 3. Modify the subdev pad ops to take a configuration store pointer instead of
>> a file handle pointer.
>>
>> The userspace API implementation (v4l2-subdev.c) would then pass &fh->store to
>> the pad operations instead of fh.
>>
>> Bridge drivers that need to implement TRY_FMT on top of pad ops would create a
>> temporary store (or temporary stores when multiple subsdevs are involved),
>> call the pad ops with a pointer to the temporary store to propagate TRY
>> formats, destroy the store(s) and return the resulting format.
>
> That will work. I think this is a good approach and it shouldn't be too difficult.

Laurent, just so I understand this correctly: does this mean that all occurrences
of 'struct v4l2_subdev_fh *fh' will be replaced by 'struct v4l2_subdev_pad_config *cfg'?

Is there any reason why the 'fh' should still be passed on?

Personally I am in favor of this since the 'fh' always made it hard for bridge
drivers to use these pad ops. So if we can replace it by something that can
be used by bridge drivers as well, then that will make it easier to move all
drivers over to the pad ops.

Regards,

Hans

2014-12-02 12:58:56

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Hans,

On Tuesday 02 December 2014 13:53:05 Hans Verkuil wrote:
> On 12/02/14 08:45, Hans Verkuil wrote:
> > On 12/02/2014 12:26 AM, Laurent Pinchart wrote:
> >> On Sunday 30 November 2014 21:30:35 Prabhakar Lad wrote:
> >>> On Sun, Nov 30, 2014 at 9:16 PM, Laurent Pinchart wrote:
> >>>> On Sunday 30 November 2014 21:05:50 Prabhakar Lad wrote:
> >>>>> On Sat, Nov 29, 2014 at 7:12 PM, Laurent Pinchart wrote:
> >>>>>> Hi Prabhakar,
> >>>>>
> >>>>> [Snip]
> >>>>>
> >>>>>>>> Sure. That's a better choice than removing the config option
> >>>>>>>> dependency of the fields struct v4l2_subdev.
> >>>>>>
> >>>>>> Decoupling CONFIG_VIDEO_V4L2_SUBDEV_API from the availability of the
> >>>>>> in-kernel pad format and selection rectangles helpers is definitely a
> >>>>>> good idea. I was thinking about decoupling the try format and
> >>>>>> rectangles from v4l2_subdev_fh by creating a kind of configuration
> >>>>>> store structure to store them, and embedding that structure in
> >>>>>> v4l2_subdev_fh. The pad-level operations would then take a pointer to
> >>>>>> the configuration store instead of the v4l2_subdev_fh. Bridge drivers
> >>>>>> that want to implement TRY_FMT based on pad-level operations would
> >>>>>> create a configuration store, use the pad-level operations, and
> >>>>>> destroy the configuration store. The userspace subdev API would use
> >>>>>> the configuration store from the file handle.
> >>>>>
> >>>>> are planning to work/post any time soon ? Or are you OK with
> >>>>> suggestion from Hans ?
> >>>>
> >>>> I have no plan to work on that myself now, I was hoping you could
> >>>> implement it ;-)
> >>>
> >>> OK will implement it.
> >>>
> >>> Can you please elaborate a more on this "The userspace subdev API would
> >>> use the configuration store from the file handle."
> >>
> >> Basically,
> >>
> >> 1. Create a subdev pad configuration store structure to store the formats
> >> and selection rectangles for each pad.
> >
> > I wouldn't call it a 'store'. Just call it fmt_config or pad_config
> > something like that.

Sure, the name doesn't matter too much.

> >> 2. Embed an instance of that structure in v4l2_subdev_fh.
> >>
> >> 3. Modify the subdev pad ops to take a configuration store pointer
> >> instead of a file handle pointer.
> >>
> >> The userspace API implementation (v4l2-subdev.c) would then pass
> >> &fh->store to the pad operations instead of fh.
> >>
> >> Bridge drivers that need to implement TRY_FMT on top of pad ops would
> >> create a temporary store (or temporary stores when multiple subsdevs are
> >> involved), call the pad ops with a pointer to the temporary store to
> >> propagate TRY formats, destroy the store(s) and return the resulting
> >> format.
> >
> > That will work. I think this is a good approach and it shouldn't be too
> > difficult.
>
> Laurent, just so I understand this correctly: does this mean that all
> occurrences of 'struct v4l2_subdev_fh *fh' will be replaced by 'struct
> v4l2_subdev_pad_config *cfg'?

That's the plan, yes.

> Is there any reason why the 'fh' should still be passed on?

We might find out reasons to still pass the fh, but in that case I think they
should be addressed and the fh just dropped from the pad ops arguments.

> Personally I am in favor of this since the 'fh' always made it hard for
> bridge drivers to use these pad ops. So if we can replace it by something
> that can be used by bridge drivers as well, then that will make it easier
> to move all drivers over to the pad ops.

Good, looks like we have a plan for world domination :-)

--
Regards,

Laurent Pinchart

2014-12-02 19:34:48

[permalink] [raw]

Subject: Re: [PATCH] media: v4l2-subdev.h: drop the guard CONFIG_VIDEO_V4L2_SUBDEV_API for v4l2_subdev_get_try_*()

Hi Laurent,

On 12/02/2014 01:59 PM, Laurent Pinchart wrote:
>>>> Basically,
>>>>
>>>> 1. Create a subdev pad configuration store structure to store the formats
>>>> and selection rectangles for each pad.
>>>
>>> I wouldn't call it a 'store'. Just call it fmt_config or pad_config
>>> something like that.
>
> Sure, the name doesn't matter too much.
>
>>>> 2. Embed an instance of that structure in v4l2_subdev_fh.
>>>>
>>>> 3. Modify the subdev pad ops to take a configuration store pointer
>>>> instead of a file handle pointer.
>>>>
>>>> The userspace API implementation (v4l2-subdev.c) would then pass
>>>> &fh->store to the pad operations instead of fh.
>>>>
>>>> Bridge drivers that need to implement TRY_FMT on top of pad ops would
>>>> create a temporary store (or temporary stores when multiple subsdevs are
>>>> involved), call the pad ops with a pointer to the temporary store to
>>>> propagate TRY formats, destroy the store(s) and return the resulting
>>>> format.
>>>
>>> That will work. I think this is a good approach and it shouldn't be too
>>> difficult.
>>
>> Laurent, just so I understand this correctly: does this mean that all
>> occurrences of 'struct v4l2_subdev_fh *fh' will be replaced by 'struct
>> v4l2_subdev_pad_config *cfg'?
>
> That's the plan, yes.
>
>> Is there any reason why the 'fh' should still be passed on?
>
> We might find out reasons to still pass the fh, but in that case I think they
> should be addressed and the fh just dropped from the pad ops arguments.
>
>> Personally I am in favor of this since the 'fh' always made it hard for
>> bridge drivers to use these pad ops. So if we can replace it by something
>> that can be used by bridge drivers as well, then that will make it easier
>> to move all drivers over to the pad ops.
>
> Good, looks like we have a plan for world domination :-)
>

OK, so I couldn't help myself and I did this conversion. The code is here:

http://git.linuxtv.org/cgit.cgi/hverkuil/media_tree.git/log/?h=remcrop

And it introduces a new struct:

struct v4l2_subdev_pad_config {
struct v4l2_mbus_framefmt try_fmt;
struct v4l2_rect try_crop;
struct v4l2_rect try_compose;
};

And subdev_fh now looks like this:

struct v4l2_subdev_fh {
struct v4l2_fh vfh;
struct v4l2_subdev_pad_config *pad;
};

But I realized that it could be simplified: right now we pass a pointer to
the pad config array to the pad ops and the v4l2_subdev_get_try_* functions
pick the right entry based on the pad index.

Wouldn't it be easier to pass the right pad config directly?

So instead of passing fh->pad in v4l2-subdev.c you pass e.g. 'fh->pad + sel->pad'.
That way the pad_config pointer points straight to the configuration of the
requested pad.

This also simplifies things in a bridge driver: it doesn't need to allocate
a pad config array, it only has to create pad config structs for those pads that
it actually uses.

And the v4l2_subdev_get_try_* functions can effectively be removed, since you
would just access cfg->try_fmt directly. I never liked those helper functions,
so I'm not sorry to see them go.

The only reason why this would fail is if drivers need to access the pad_config
of other pads than the one currently requested. I have not seen any code that
does that, though. And I don't think that is something we want anyway since
that would make things extremely complex.

Let me know what you think,

Hans

2014-12-02 20:55:45