| Message ID | 1416220913-5047-1-git-send-email-prabhakar.csengg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <prabhakar.csengg@gmail.com> > --- > 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. > #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; > >
Hi Sakari, On Mon, Nov 17, 2014 at 10:53 AM, Sakari Ailus <sakari.ailus@linux.intel.com> 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 <prabhakar.csengg@gmail.com> >> --- >> 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 > sakari.ailus@linux.intel.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <prabhakar.csengg@gmail.com> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Tue, Nov 18, 2014 at 9:39 AM, Hans Verkuil <hverkuil@xs4all.nl> 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 <prabhakar.csengg@gmail.com> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <prabhakar.csengg@gmail.com> > > --- > > 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.
Hi Laurent, On Tue, Nov 18, 2014 at 6:07 PM, Sakari Ailus <sakari.ailus@iki.fi> 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 <prabhakar.csengg@gmail.com> >> > --- >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Prabhakar, On Saturday 29 November 2014 18:30:09 Prabhakar Lad wrote: > On Tue, Nov 18, 2014 at 6:07 PM, Sakari Ailus <sakari.ailus@iki.fi> 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 <prabhakar.csengg@gmail.com> > >> > --- > >> > > >> > 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 ?
Hi Laurent, On Sat, Nov 29, 2014 at 7:12 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ;-)
Hi Laurent, On Sun, Nov 30, 2014 at 9:16 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Mon, Dec 1, 2014 at 11:26 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :-)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Tuesday 02 December 2014 20:34:22 Hans Verkuil wrote: > 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? You can't do that, because format and selection rectangles on a pad can depend on the formats and selection rectangles on other pads of the same subdev. For instance setting the format on the source pad of a scaler will ignore the media bus code and return the code configured on the sink pad. A full configuration must thus be passed to the pad operations. > 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. Exactly :-) > 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. That's done all over the place, look at the OMAP3 ISP subdev for instance. > Let me know what you think,
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;
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 <prabhakar.csengg@gmail.com> --- include/media/v4l2-subdev.h | 2 -- 1 file changed, 2 deletions(-)