[PATCH 0/6] Avoid v4l2_subdev_get_try_ ifdef dance

[PATCH 0/6] Avoid v4l2_subdev_get_try_ ifdef dance

[Marco Felsch]

Hi,

during my work on [1] I prepared a patch to avoid driver interal ifdef
dances for:
 - v4l2_subdev_get_try_format
 - v4l2_subdev_get_try_crop
 - v4l2_subdev_get_try_compose
helper functions. Jacopo did some comments on it so I picked Lubomir's
series [2] as base and prepared a new one since this series didn't got
merged.

During discussion on [2] Sakari mentioned Hans RFC Patch [3] which didn't
got merged too due to Mauro's concerns.

The driver changes are only compile tested due to the lack of missing
hardware. It would be cool if someone can verify my changes.

[1] https://patchwork.kernel.org/cover/10786553/
[2] https://patchwork.kernel.org/patch/10703029/
[3] https://patchwork.linuxtv.org/patch/53370/

Marco Felsch (6):
  media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
  media: ov2659: get rid of extra ifdefs
  media: ov2680: get rid of extra ifdefs
  media: ov5695: get rid of extra ifdefs
  media: ov7670: get rid of extra ifdefs
  media: ov7740: get rid of extra ifdefs

 drivers/media/i2c/ov2659.c  | 15 +++++++--------
 drivers/media/i2c/ov2680.c  | 20 +++++++++-----------
 drivers/media/i2c/ov5695.c  | 26 ++++++++++++++------------
 drivers/media/i2c/ov7670.c  | 19 +++++++------------
 drivers/media/i2c/ov7740.c  | 27 +++++++++++++--------------
 include/media/v4l2-subdev.h | 29 ++++++++++++++++++++++++++++-
 6 files changed, 78 insertions(+), 58 deletions(-)

-- 
2.20.1

[PATCH 1/6] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*

[Marco Felsch]

In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
available. So each driver have to add ifdefs around those helpers or
add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.

Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
isn't set to avoid ifdefs. This approach is less error prone too.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
The patch was previously part of series [1]. Since I want to get
series [1] merged in 5.2 I split this possible stopper out of the
serie and prepared a own series for it. I applied Jacopos comments and
switched to Lubomir's approach [2]. During discussion on series [2]
Sakari pointed out Hans approach [3] which didn't got into the kernel
due to Mauro's concerns. So I think this would be the smalles common
dennominator.

[1] https://patchwork.kernel.org/cover/10786553/
[2] https://patchwork.kernel.org/patch/10703029/
[3] https://patchwork.linuxtv.org/patch/53370/
---
 include/media/v4l2-subdev.h | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a7fa5b80915a..eea792757426 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -984,7 +984,34 @@ static inline struct v4l2_rect
 		pad = 0;
 	return &cfg[pad].try_compose;
 }
-#endif
+
+#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);
+}
+
+static inline struct v4l2_rect
+*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_pad_config *cfg,
+			  unsigned int pad)
+{
+	return ERR_PTR(-ENOTTY);
+}
+
+static inline struct v4l2_rect
+*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_pad_config *cfg,
+			     unsigned int pad)
+{
+	return ERR_PTR(-ENOTTY);
+}
+
+#endif /* defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
 
 extern const struct v4l2_file_operations v4l2_subdev_fops;
 
-- 
2.20.1

Re: [PATCH 1/6] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*

[Sakari Ailus]

Hi Marco,

On Thu, Apr 04, 2019 at 09:39:57AM +0200, Marco Felsch wrote:
> In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
> available. So each driver have to add ifdefs around those helpers or
> add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.
> 
> Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
> isn't set to avoid ifdefs. This approach is less error prone too.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> The patch was previously part of series [1]. Since I want to get
> series [1] merged in 5.2 I split this possible stopper out of the
> serie and prepared a own series for it. I applied Jacopos comments and
> switched to Lubomir's approach [2]. During discussion on series [2]
> Sakari pointed out Hans approach [3] which didn't got into the kernel
> due to Mauro's concerns. So I think this would be the smalles common
> dennominator.
> 
> [1] https://patchwork.kernel.org/cover/10786553/
> [2] https://patchwork.kernel.org/patch/10703029/
> [3] https://patchwork.linuxtv.org/patch/53370/
> ---
>  include/media/v4l2-subdev.h | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a7fa5b80915a..eea792757426 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -984,7 +984,34 @@ static inline struct v4l2_rect
>  		pad = 0;
>  	return &cfg[pad].try_compose;
>  }
> -#endif
> +
> +#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);
> +}

Almost all users of these functions directly use the struct pointer they
receive from the said functions, without checks for the pointer value.

These drivers now depend on CONFIG_VIDEO_V4L2_SUBDEV_API, but for new
drivers it could be possible to miss that, leading to dereferencing
ERR_PTR(-ENOTTY) as a result if the sub-device API isn't enabled.

I have to say I'm not a huge fan of such a change; it's one more such thing
to remember that results in a kernel crash if you get it wrong.

I wonder if v4l2_subdev_call() could become a bit smarter about this;
checking whether the try format is attempted to be obtained without the
sub-device API being enabled, and returning an error in that case without
passing the IOCTL to the driver at all.

Just an idea. It doesn't have to be in the same patchset.

I'd still prefer enabling subdev API always to decrease the number of
possible different kernel configurations.

I wonder what others think.

> +
> +static inline struct v4l2_rect
> +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_pad_config *cfg,
> +			  unsigned int pad)
> +{
> +	return ERR_PTR(-ENOTTY);
> +}
> +
> +static inline struct v4l2_rect
> +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> +			     struct v4l2_subdev_pad_config *cfg,
> +			     unsigned int pad)
> +{
> +	return ERR_PTR(-ENOTTY);
> +}
> +
> +#endif /* defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
>  
>  extern const struct v4l2_file_operations v4l2_subdev_fops;
>  

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/6] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 4747 bytes --]

Hi Marco, Sakari,

On Thu, Apr 04, 2019 at 11:39:34AM +0300, Sakari Ailus wrote:
> Hi Marco,
>
> On Thu, Apr 04, 2019 at 09:39:57AM +0200, Marco Felsch wrote:
> > In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
> > available. So each driver have to add ifdefs around those helpers or
> > add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.
> >
> > Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
> > isn't set to avoid ifdefs. This approach is less error prone too.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > The patch was previously part of series [1]. Since I want to get
> > series [1] merged in 5.2 I split this possible stopper out of the
> > serie and prepared a own series for it. I applied Jacopos comments and
> > switched to Lubomir's approach [2]. During discussion on series [2]
> > Sakari pointed out Hans approach [3] which didn't got into the kernel
> > due to Mauro's concerns. So I think this would be the smalles common
> > dennominator.
> >
> > [1] https://patchwork.kernel.org/cover/10786553/
> > [2] https://patchwork.kernel.org/patch/10703029/
> > [3] https://patchwork.linuxtv.org/patch/53370/
> > ---
> >  include/media/v4l2-subdev.h | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index a7fa5b80915a..eea792757426 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -984,7 +984,34 @@ static inline struct v4l2_rect
> >  		pad = 0;
> >  	return &cfg[pad].try_compose;
> >  }
> > -#endif
> > +
> > +#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);
> > +}
>
> Almost all users of these functions directly use the struct pointer they
> receive from the said functions, without checks for the pointer value.
>
> These drivers now depend on CONFIG_VIDEO_V4L2_SUBDEV_API, but for new
> drivers it could be possible to miss that, leading to dereferencing
> ERR_PTR(-ENOTTY) as a result if the sub-device API isn't enabled.
>
> I have to say I'm not a huge fan of such a change; it's one more such thing
> to remember that results in a kernel crash if you get it wrong.
>

Unfortunately, I have to agree with Sakari here

> I wonder if v4l2_subdev_call() could become a bit smarter about this;
> checking whether the try format is attempted to be obtained without the
> sub-device API being enabled, and returning an error in that case without
> passing the IOCTL to the driver at all.

I'm not much excited by the idea of specializing v4l2_subdev_call(), I
had a try and I would be ashemed of sharing that code. If someone
could come up with something nice we could consider this too.

For the record, it is not enough to check for V4L2_SUBDEV_API and try
format, as a few (1?) bridge drivers calls the subdev operation providing a
'v4l2_subdev_pad_config' argument, that could be used by the sensor
drivers in place of the one returned from v4l2_subdev_get_try_*, if
V4L2_SUBDEV_API is not available

Caller:
https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/renesas-ceu.c#L858
Sensor:
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9v111.c#L792

As I wrote both, they could be very well be wrong, but if they're not,
one other option would be to refuse NULL v4l2_subdev_pad_config arguments
(changing most of the bridge drivers in mainline) and move the code
linked above here from the mt9v111 driver to the v4l2_subdev_get_try_*
functions.

>
> Just an idea. It doesn't have to be in the same patchset.
>
> I'd still prefer enabling subdev API always to decrease the number of
> possible different kernel configurations.

That would be best imho, but Hans' attempt didn't go very far because
of the issue Marco reported in his cover letter.

Thanks
  j

>
> I wonder what others think.
>
> > +
> > +static inline struct v4l2_rect
> > +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> > +			  struct v4l2_subdev_pad_config *cfg,
> > +			  unsigned int pad)
> > +{
> > +	return ERR_PTR(-ENOTTY);
> > +}
> > +
> > +static inline struct v4l2_rect
> > +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> > +			     struct v4l2_subdev_pad_config *cfg,
> > +			     unsigned int pad)
> > +{
> > +	return ERR_PTR(-ENOTTY);
> > +}
> > +
> > +#endif /* defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
> >
> >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> >
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*

[Marco Felsch]

Hi Sakari, Jacopo,

On 19-04-04 16:39, Jacopo Mondi wrote:
> Hi Marco, Sakari,
> 
> On Thu, Apr 04, 2019 at 11:39:34AM +0300, Sakari Ailus wrote:
> > Hi Marco,
> >
> > On Thu, Apr 04, 2019 at 09:39:57AM +0200, Marco Felsch wrote:
> > > In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
> > > available. So each driver have to add ifdefs around those helpers or
> > > add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.
> > >
> > > Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
> > > isn't set to avoid ifdefs. This approach is less error prone too.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > > The patch was previously part of series [1]. Since I want to get
> > > series [1] merged in 5.2 I split this possible stopper out of the
> > > serie and prepared a own series for it. I applied Jacopos comments and
> > > switched to Lubomir's approach [2]. During discussion on series [2]
> > > Sakari pointed out Hans approach [3] which didn't got into the kernel
> > > due to Mauro's concerns. So I think this would be the smalles common
> > > dennominator.
> > >
> > > [1] https://patchwork.kernel.org/cover/10786553/
> > > [2] https://patchwork.kernel.org/patch/10703029/
> > > [3] https://patchwork.linuxtv.org/patch/53370/
> > > ---
> > >  include/media/v4l2-subdev.h | 29 ++++++++++++++++++++++++++++-
> > >  1 file changed, 28 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index a7fa5b80915a..eea792757426 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -984,7 +984,34 @@ static inline struct v4l2_rect
> > >  		pad = 0;
> > >  	return &cfg[pad].try_compose;
> > >  }
> > > -#endif
> > > +
> > > +#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);
> > > +}
> >
> > Almost all users of these functions directly use the struct pointer they
> > receive from the said functions, without checks for the pointer value.
> >
> > These drivers now depend on CONFIG_VIDEO_V4L2_SUBDEV_API, but for new
> > drivers it could be possible to miss that, leading to dereferencing
> > ERR_PTR(-ENOTTY) as a result if the sub-device API isn't enabled.
> >
> > I have to say I'm not a huge fan of such a change; it's one more such thing
> > to remember that results in a kernel crash if you get it wrong.
> >
> 
> Unfortunately, I have to agree with Sakari here
>

Are those drivers depending on CONFIG_VIDEO_V4L2_SUBDEV_API used by
usb-bridge devices? I Remember that was Mauro's concern on Hans RFC patch.
I'm with you the "depending way" is quite better.

> > I wonder if v4l2_subdev_call() could become a bit smarter about this;
> > checking whether the try format is attempted to be obtained without the
> > sub-device API being enabled, and returning an error in that case without
> > passing the IOCTL to the driver at all.
> 
> I'm not much excited by the idea of specializing v4l2_subdev_call(), I
> had a try and I would be ashemed of sharing that code. If someone
> could come up with something nice we could consider this too.
> 
> For the record, it is not enough to check for V4L2_SUBDEV_API and try
> format, as a few (1?) bridge drivers calls the subdev operation providing a
> 'v4l2_subdev_pad_config' argument, that could be used by the sensor
> drivers in place of the one returned from v4l2_subdev_get_try_*, if
> V4L2_SUBDEV_API is not available

Thats true. One thing to think about your caller. This code won't work
if you are using the existing ov* implementations too since the ceu
don't depend on the subdev-api too.

So including your record increases the complexity.

> 
> Caller:
> https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/renesas-ceu.c#L858
> Sensor:
> https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9v111.c#L792
> 
> As I wrote both, they could be very well be wrong, but if they're not,
> one other option would be to refuse NULL v4l2_subdev_pad_config arguments
> (changing most of the bridge drivers in mainline) and move the code
> linked above here from the mt9v111 driver to the v4l2_subdev_get_try_*
> functions.
> 
> >
> > Just an idea. It doesn't have to be in the same patchset.
> >
> > I'd still prefer enabling subdev API always to decrease the number of
> > possible different kernel configurations.
> 
> That would be best imho, but Hans' attempt didn't go very far because
> of the issue Marco reported in his cover letter.

Yes, that's also my favourite.

@Mauro
Can you point out your concerns 'again'? Maybe we can apply Hans
approach and will fix the 'maybe' broken usb-bridge devices.

Regards,
	Marco


> 
> Thanks
>   j
> 
> >
> > I wonder what others think.
> >
> > > +
> > > +static inline struct v4l2_rect
> > > +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> > > +			  struct v4l2_subdev_pad_config *cfg,
> > > +			  unsigned int pad)
> > > +{
> > > +	return ERR_PTR(-ENOTTY);
> > > +}
> > > +
> > > +static inline struct v4l2_rect
> > > +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> > > +			     struct v4l2_subdev_pad_config *cfg,
> > > +			     unsigned int pad)
> > > +{
> > > +	return ERR_PTR(-ENOTTY);
> > > +}
> > > +
> > > +#endif /* defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
> > >
> > >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> > >
> >
> > --
> > Regards,
> >
> > Sakari Ailus
> > sakari.ailus@linux.intel.com



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*

[Mauro Carvalho Chehab]

Em Fri, 5 Apr 2019 09:26:04 +0200
Marco Felsch <m.felsch@pengutronix.de> escreveu:

> Hi Sakari, Jacopo,
> 
> On 19-04-04 16:39, Jacopo Mondi wrote:
> > Hi Marco, Sakari,
> > 
> > On Thu, Apr 04, 2019 at 11:39:34AM +0300, Sakari Ailus wrote:  
> > > Hi Marco,
> > >
> > > On Thu, Apr 04, 2019 at 09:39:57AM +0200, Marco Felsch wrote:  
> > > > In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
> > > > available. So each driver have to add ifdefs around those helpers or
> > > > add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.
> > > >
> > > > Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
> > > > isn't set to avoid ifdefs. This approach is less error prone too.
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > > The patch was previously part of series [1]. Since I want to get
> > > > series [1] merged in 5.2 I split this possible stopper out of the
> > > > serie and prepared a own series for it. I applied Jacopos comments and
> > > > switched to Lubomir's approach [2]. During discussion on series [2]
> > > > Sakari pointed out Hans approach [3] which didn't got into the kernel
> > > > due to Mauro's concerns. So I think this would be the smalles common
> > > > dennominator.
> > > >
> > > > [1] https://patchwork.kernel.org/cover/10786553/
> > > > [2] https://patchwork.kernel.org/patch/10703029/
> > > > [3] https://patchwork.linuxtv.org/patch/53370/
> > > > ---
> > > >  include/media/v4l2-subdev.h | 29 ++++++++++++++++++++++++++++-
> > > >  1 file changed, 28 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index a7fa5b80915a..eea792757426 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -984,7 +984,34 @@ static inline struct v4l2_rect
> > > >  		pad = 0;
> > > >  	return &cfg[pad].try_compose;
> > > >  }
> > > > -#endif
> > > > +
> > > > +#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);
> > > > +}  
> > >
> > > Almost all users of these functions directly use the struct pointer they
> > > receive from the said functions, without checks for the pointer value.

It they depends on VIDEO_V4L2_SUBDEV_API, there's no problem with that.

> > >
> > > These drivers now depend on CONFIG_VIDEO_V4L2_SUBDEV_API, but for new
> > > drivers it could be possible to miss that, leading to dereferencing
> > > ERR_PTR(-ENOTTY) as a result if the sub-device API isn't enabled.

If they require subdev API, but doesn't don't depend on such config, 
then they're already broken: they won't work, and probably will crash
the Kernel somewhere.

> > >
> > > I have to say I'm not a huge fan of such a change; it's one more such thing
> > > to remember that results in a kernel crash if you get it wrong.
> > >  
> > 
> > Unfortunately, I have to agree with Sakari here
> >  
> 
> Are those drivers depending on CONFIG_VIDEO_V4L2_SUBDEV_API used by
> usb-bridge devices? I Remember that was Mauro's concern on Hans RFC patch.
> I'm with you the "depending way" is quite better.

Neither USB or normal PCI drivers should depend on VIDEO_V4L2_SUBDEV_API.

There are, currently, 3 PCI drivers that depends on it:

1) drivers/media/pci/cobalt/Kconfig

   This is for a development board that not publicly available. So, I don't 
   care about what it depends on.

   Yet, I suspect that it depends on subdev just because of those drivers,
   with also depends on it:

	drivers/media/pci/cobalt/cobalt-driver.c:#include <media/i2c/adv7604.h>
	drivers/media/pci/cobalt/cobalt-driver.c:#include <media/i2c/adv7842.h>
	drivers/media/pci/cobalt/cobalt-driver.c:#include <media/i2c/adv7511.h>

2) drivers/media/pci/intel/ipu3/Kconfig

   This one seems to be the only valid case for the dependency: the IPU3
   hardware is actually part of an Intel SoC that depends on subdevs 
   and behave like any other platform driver: it will only work after
   setting a MC pipeline and command the sub-devices directly via the
   subdev API.

3) drivers/media/pci/sta2x11/Kconfig

   This one looks weird for me. It sounds that it uses just the
   adv7180 i2c driver. I suspect that it depends on VIDEO_V4L2_SUBDEV_API
   just because the I2C driver assumes that the subdev API is always
   there.

   IMO, adv7180 should be fixed to not depend on it anymore, and
   let sta2x11 to also build without such dependency.

Perhaps with the addition of this patch, (1) and (3) can be build and
run without depending at the subdev API.

> 
> > > I wonder if v4l2_subdev_call() could become a bit smarter about this;
> > > checking whether the try format is attempted to be obtained without the
> > > sub-device API being enabled, and returning an error in that case without
> > > passing the IOCTL to the driver at all.  
> > 
> > I'm not much excited by the idea of specializing v4l2_subdev_call(), I
> > had a try and I would be ashemed of sharing that code. If someone
> > could come up with something nice we could consider this too.
> > 
> > For the record, it is not enough to check for V4L2_SUBDEV_API and try
> > format, as a few (1?) bridge drivers calls the subdev operation providing a
> > 'v4l2_subdev_pad_config' argument, that could be used by the sensor
> > drivers in place of the one returned from v4l2_subdev_get_try_*, if
> > V4L2_SUBDEV_API is not available  
> 
> Thats true. One thing to think about your caller. This code won't work
> if you are using the existing ov* implementations too since the ceu
> don't depend on the subdev-api too.
> 
> So including your record increases the complexity.
> 
> > 
> > Caller:
> > https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/renesas-ceu.c#L858
> > Sensor:
> > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9v111.c#L792
> > 
> > As I wrote both, they could be very well be wrong, but if they're not,
> > one other option would be to refuse NULL v4l2_subdev_pad_config arguments
> > (changing most of the bridge drivers in mainline) and move the code
> > linked above here from the mt9v111 driver to the v4l2_subdev_get_try_*
> > functions.
> >   
> > >
> > > Just an idea. It doesn't have to be in the same patchset.
> > >
> > > I'd still prefer enabling subdev API always to decrease the number of
> > > possible different kernel configurations.  
> > 
> > That would be best imho, but Hans' attempt didn't go very far because
> > of the issue Marco reported in his cover letter.  
> 
> Yes, that's also my favourite.
> 
> @Mauro
> Can you point out your concerns 'again'? Maybe we can apply Hans
> approach and will fix the 'maybe' broken usb-bridge devices.
> 

The point is: only platform drivers require the subdev API; for
USB/PCI drivers, this is not needed, and the core can be simplified
in order to give a smaller footprint. Of course, drivers that depend
on subdevs won't be compiled, but sensors can still work. The em28xx
cameras, for example, use a few sensor drivers that can optionally
export the subdev API, when the bridge driver supports it (with is not
the case of the em28xx bridge itself).

Looking at this patch, I don't see why it would cause any problems
that aren't already there.

Thanks,
Mauro

[PATCH 2/6] media: ov2659: get rid of extra ifdefs

[Marco Felsch]

We can drop the ifdef dance since the v4l2_subdev_get_try_format
return the correct value in both cases with or without
CONFIG_VIDEO_V4L2_SUBDEV_API is enabled.

The patch is based on Lubomir's series [1].

[1] https://patchwork.kernel.org/cover/10703017/

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/ov2659.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index 799acce803fe..1c9cbd43f903 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -1046,17 +1046,16 @@ static int ov2659_get_fmt(struct v4l2_subdev *sd,
 	dev_dbg(&client->dev, "ov2659_get_fmt\n");
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 		struct v4l2_mbus_framefmt *mf;
 
 		mf = v4l2_subdev_get_try_format(sd, cfg, 0);
+		if (IS_ERR(mf))
+			return PTR_ERR(mf);
+
 		mutex_lock(&ov2659->lock);
 		fmt->format = *mf;
 		mutex_unlock(&ov2659->lock);
 		return 0;
-#else
-	return -ENOTTY;
-#endif
 	}
 
 	mutex_lock(&ov2659->lock);
@@ -1126,12 +1125,12 @@ static int ov2659_set_fmt(struct v4l2_subdev *sd,
 	mutex_lock(&ov2659->lock);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 		mf = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+		if (IS_ERR(mf)) {
+			mutex_unlock(&ov2659->lock);
+			return PTR_ERR(mf);
+		}
 		*mf = fmt->format;
-#else
-		return -ENOTTY;
-#endif
 	} else {
 		s64 val;
 
-- 
2.20.1

[PATCH 3/6] media: ov2680: get rid of extra ifdefs

[Marco Felsch]

We can drop the ifdef dance since the v4l2_subdev_get_try_format
return the correct value in both cases with or without
CONFIG_VIDEO_V4L2_SUBDEV_API is enabled.

The patch is based on Lubomir's series [1].

[1] https://patchwork.kernel.org/cover/10703017/

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/ov2680.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index b10bcfabaeeb..ea750e8304fe 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -672,11 +672,11 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 	mutex_lock(&sensor->lock);
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 		fmt = v4l2_subdev_get_try_format(&sensor->sd, cfg, format->pad);
-#else
-		ret = -ENOTTY;
-#endif
+		if (IS_ERR(fmt)) {
+			ret = PTR_ERR(fmt);
+			goto unlock;
+		}
 	} else {
 		fmt = &sensor->fmt;
 	}
@@ -684,6 +684,7 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 	if (fmt)
 		format->format = *fmt;
 
+unlock:
 	mutex_unlock(&sensor->lock);
 
 	return ret;
@@ -695,9 +696,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 {
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 	struct v4l2_mbus_framefmt *fmt = &format->format;
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 	struct v4l2_mbus_framefmt *try_fmt;
-#endif
 	const struct ov2680_mode_info *mode;
 	int ret = 0;
 
@@ -720,13 +719,12 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 		try_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
+		if (IS_ERR(try_fmt)) {
+			ret = PTR_ERR(try_fmt);
+			goto unlock;
+		}
 		format->format = *try_fmt;
-#else
-		ret = -ENOTTY;
-#endif
-
 		goto unlock;
 	}
 
-- 
2.20.1

[PATCH 4/6] media: ov5695: get rid of extra ifdefs

[Marco Felsch]

We can drop the ifdef dance since the v4l2_subdev_get_try_format
return the correct value in both cases with or without
CONFIG_VIDEO_V4L2_SUBDEV_API is enabled.

The patch is based on Lubomir's series [1].

[1] https://patchwork.kernel.org/cover/10703017/

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/ov5695.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
index 5d107c53364d..83fea416c75e 100644
--- a/drivers/media/i2c/ov5695.c
+++ b/drivers/media/i2c/ov5695.c
@@ -810,6 +810,7 @@ static int ov5695_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *fmt)
 {
 	struct ov5695 *ov5695 = to_ov5695(sd);
+	struct v4l2_mbus_framefmt *try_fmt;
 	const struct ov5695_mode *mode;
 	s64 h_blank, vblank_def;
 
@@ -821,12 +822,12 @@ static int ov5695_set_fmt(struct v4l2_subdev *sd,
 	fmt->format.height = mode->height;
 	fmt->format.field = V4L2_FIELD_NONE;
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
-		*v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
-#else
-		mutex_unlock(&ov5695->mutex);
-		return -ENOTTY;
-#endif
+		try_fmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+		if (IS_ERR(try_fmt)) {
+			mutex_unlock(&ov5695->mutex);
+			return PTR_ERR(try_fmt);
+		}
+		*try_fmt = fmt->format;
 	} else {
 		ov5695->cur_mode = mode;
 		h_blank = mode->hts_def - mode->width;
@@ -848,16 +849,17 @@ static int ov5695_get_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *fmt)
 {
 	struct ov5695 *ov5695 = to_ov5695(sd);
+	struct v4l2_mbus_framefmt *try_fmt;
 	const struct ov5695_mode *mode = ov5695->cur_mode;
 
 	mutex_lock(&ov5695->mutex);
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
-		fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
-#else
-		mutex_unlock(&ov5695->mutex);
-		return -ENOTTY;
-#endif
+		try_fmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+		if (IS_ERR(try_fmt)) {
+			mutex_unlock(&ov5695->mutex);
+			return PTR_ERR(try_fmt);
+		}
+		fmt->format = *try_fmt;
 	} else {
 		fmt->format.width = mode->width;
 		fmt->format.height = mode->height;
-- 
2.20.1

[PATCH 5/6] media: ov7670: get rid of extra ifdefs

[Marco Felsch]

We can drop the ifdef dance since the v4l2_subdev_get_try_format
return the correct value in both cases with or without
CONFIG_VIDEO_V4L2_SUBDEV_API is enabled.

The patch is based on Lubomir's series [1].

[1] https://patchwork.kernel.org/cover/10703017/

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/ov7670.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 44c3eed8a858..7d6fd9842a75 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -1097,9 +1097,7 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
 		struct v4l2_subdev_format *format)
 {
 	struct ov7670_info *info = to_state(sd);
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 	struct v4l2_mbus_framefmt *mbus_fmt;
-#endif
 	int ret;
 
 	if (format->pad)
@@ -1109,13 +1107,13 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
 		ret = ov7670_try_fmt_internal(sd, &format->format, NULL, NULL);
 		if (ret)
 			return ret;
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+
 		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
+		if (IS_ERR(mbus_fmt))
+			return PTR_ERR(mbus_fmt);
+
 		*mbus_fmt = format->format;
 		return 0;
-#else
-		return -ENOTTY;
-#endif
 	}
 
 	ret = ov7670_try_fmt_internal(sd, &format->format, &info->fmt, &info->wsize);
@@ -1138,18 +1136,15 @@ static int ov7670_get_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *format)
 {
 	struct ov7670_info *info = to_state(sd);
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 	struct v4l2_mbus_framefmt *mbus_fmt;
-#endif
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
+		if (IS_ERR(mbus_fmt))
+			return PTR_ERR(mbus_fmt);
+
 		format->format = *mbus_fmt;
 		return 0;
-#else
-		return -ENOTTY;
-#endif
 	} else {
 		format->format = info->format;
 	}
-- 
2.20.1

[PATCH 6/6] media: ov7740: get rid of extra ifdefs

[Marco Felsch]

We can drop the ifdef dance since the v4l2_subdev_get_try_format
return the correct value in both cases with or without
CONFIG_VIDEO_V4L2_SUBDEV_API is enabled.

The patch is based on Lubomir's series [1].

[1] https://patchwork.kernel.org/cover/10703017/

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/ov7740.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c
index 54e80a60aa57..58171f21d315 100644
--- a/drivers/media/i2c/ov7740.c
+++ b/drivers/media/i2c/ov7740.c
@@ -804,9 +804,7 @@ static int ov7740_set_fmt(struct v4l2_subdev *sd,
 	struct ov7740 *ov7740 = container_of(sd, struct ov7740, subdev);
 	const struct ov7740_pixfmt *ovfmt;
 	const struct ov7740_framesize *fsize;
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 	struct v4l2_mbus_framefmt *mbus_fmt;
-#endif
 	int ret;
 
 	mutex_lock(&ov7740->mutex);
@@ -819,16 +817,17 @@ static int ov7740_set_fmt(struct v4l2_subdev *sd,
 		ret = ov7740_try_fmt_internal(sd, &format->format, NULL, NULL);
 		if (ret)
 			goto error;
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+
 		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
+		if (IS_ERR(mbus_fmt)) {
+			ret = PTR_ERR(mbus_fmt);
+			goto error;
+		}
+
 		*mbus_fmt = format->format;
 
 		mutex_unlock(&ov7740->mutex);
 		return 0;
-#else
-		ret = -ENOTTY;
-		goto error;
-#endif
 	}
 
 	ret = ov7740_try_fmt_internal(sd, &format->format, &ovfmt, &fsize);
@@ -851,25 +850,25 @@ static int ov7740_get_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *format)
 {
 	struct ov7740 *ov7740 = container_of(sd, struct ov7740, subdev);
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 	struct v4l2_mbus_framefmt *mbus_fmt;
-#endif
 	int ret = 0;
 
 	mutex_lock(&ov7740->mutex);
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
+		if (IS_ERR(mbus_fmt)) {
+			ret = PTR_ERR(mbus_fmt);
+			goto error;
+		}
+
 		format->format = *mbus_fmt;
 		ret = 0;
-#else
-		ret = -ENOTTY;
-#endif
 	} else {
 		format->format = ov7740->format;
 	}
-	mutex_unlock(&ov7740->mutex);
 
+error:
+	mutex_unlock(&ov7740->mutex);
 	return ret;
 }
 
-- 
2.20.1

Re: [PATCH 0/6] Avoid v4l2_subdev_get_try_ ifdef dance

[Sakari Ailus]

Hi Marco,

On Thu, Apr 04, 2019 at 09:39:56AM +0200, Marco Felsch wrote:
> Hi,
> 
> during my work on [1] I prepared a patch to avoid driver interal ifdef
> dances for:
>  - v4l2_subdev_get_try_format
>  - v4l2_subdev_get_try_crop
>  - v4l2_subdev_get_try_compose
> helper functions. Jacopo did some comments on it so I picked Lubomir's
> series [2] as base and prepared a new one since this series didn't got
> merged.
> 
> During discussion on [2] Sakari mentioned Hans RFC Patch [3] which didn't
> got merged too due to Mauro's concerns.
> 
> The driver changes are only compile tested due to the lack of missing
> hardware. It would be cool if someone can verify my changes.
> 
> [1] https://patchwork.kernel.org/cover/10786553/
> [2] https://patchwork.kernel.org/patch/10703029/
> [3] https://patchwork.linuxtv.org/patch/53370/
> 
> Marco Felsch (6):
>   media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
>   media: ov2659: get rid of extra ifdefs
>   media: ov2680: get rid of extra ifdefs
>   media: ov5695: get rid of extra ifdefs
>   media: ov7670: get rid of extra ifdefs
>   media: ov7740: get rid of extra ifdefs

Assuming that this patch gets merged:

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

With the above patch, calling these functions when subdev API is disabled
becomes clearly a driver bug. Instead of returning an error from the
functions, I'd suggest adding dummy format and selection configurations
(which are memset to zero by the call to obtain the said configuration)
that would be returned by these functions. Calling these functions should
also emit a warning.

That would be much safer and also would make it easier to catch API misuse.

-- 
Kind regards,

Sakari Ailus