| Message ID | 1547561141-13504-2-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: bugfixes for mt9m111 driver | expand |
Hi Akinobu, sorry for the delayed response. On 19-01-15 23:05, Akinobu Mita wrote: > The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize > V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE > is specified. > > Cc: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > Cc: Michael Grzeschik <m.grzeschik@pengutronix.de> > Cc: Marco Felsch <m.felsch@pengutronix.de> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > --- > * v3 > - Set initial try format with default configuration instead of > current one. > > drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > index d639b9b..63a5253 100644 > --- a/drivers/media/i2c/mt9m111.c > +++ b/drivers/media/i2c/mt9m111.c > @@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd, > if (format->pad) > return -EINVAL; > > + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { > +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API > + mf = v4l2_subdev_get_try_format(sd, cfg, format->pad); > + format->format = *mf; > + return 0; > +#else > + return -ENOTTY; > +#endif If've checked this again and found the ov* devices do this too. IMO it's not good for other developers to check the upper layer if the '#else' path is reachable. There are also some code analyzer tools which will report this as issue/warning. As I said, I checked the v4l2_subdev_get_try_format() usage again and found the solution made by the mt9v111.c better. Why do you don't add a dependency in the Kconfig, so we can drop this ifdef? Regards, Marco > + } > + > mf->width = mt9m111->width; > mf->height = mt9m111->height; > mf->code = mt9m111->fmt->code; > @@ -1089,6 +1099,25 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable) > return 0; > } > > +static int mt9m111_init_cfg(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg) > +{ > +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API > + struct v4l2_mbus_framefmt *format = > + v4l2_subdev_get_try_format(sd, cfg, 0); > + > + format->width = MT9M111_MAX_WIDTH; > + format->height = MT9M111_MAX_HEIGHT; > + format->code = mt9m111_colour_fmts[0].code; > + format->colorspace = mt9m111_colour_fmts[0].colorspace; > + format->field = V4L2_FIELD_NONE; > + format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > + format->quantization = V4L2_QUANTIZATION_DEFAULT; > + format->xfer_func = V4L2_XFER_FUNC_DEFAULT; > +#endif > + return 0; > +} > + > static int mt9m111_g_mbus_config(struct v4l2_subdev *sd, > struct v4l2_mbus_config *cfg) > { > @@ -1114,6 +1143,7 @@ static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = { > }; > > static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = { > + .init_cfg = mt9m111_init_cfg, > .enum_mbus_code = mt9m111_enum_mbus_code, > .get_selection = mt9m111_get_selection, > .set_selection = mt9m111_set_selection, > -- > 2.7.4 > >
2019年1月24日(木) 0:17 Marco Felsch <m.felsch@pengutronix.de>: > > Hi Akinobu, > > sorry for the delayed response. > > On 19-01-15 23:05, Akinobu Mita wrote: > > The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize > > V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE > > is specified. > > > > Cc: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > > Cc: Michael Grzeschik <m.grzeschik@pengutronix.de> > > Cc: Marco Felsch <m.felsch@pengutronix.de> > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > > --- > > * v3 > > - Set initial try format with default configuration instead of > > current one. > > > > drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > > index d639b9b..63a5253 100644 > > --- a/drivers/media/i2c/mt9m111.c > > +++ b/drivers/media/i2c/mt9m111.c > > @@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd, > > if (format->pad) > > return -EINVAL; > > > > + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { > > +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API > > + mf = v4l2_subdev_get_try_format(sd, cfg, format->pad); > > + format->format = *mf; > > + return 0; > > +#else > > + return -ENOTTY; > > +#endif > > If've checked this again and found the ov* devices do this too. IMO it's > not good for other developers to check the upper layer if the '#else' > path is reachable. There are also some code analyzer tools which will > report this as issue/warning. > > As I said, I checked the v4l2_subdev_get_try_format() usage again and > found the solution made by the mt9v111.c better. Why do you don't add a > dependency in the Kconfig, so we can drop this ifdef? I'm ok with adding CONFIG_VIDEO_V4L2_SUBDEV_API dependency to this driver, because I always enable it. But it may cause an issue on some environments that don't require CONFIG_VIDEO_V4L2_SUBDEV_API. Sakari, do you have any opinion?
Hi Mita-san, Marco, On Sun, Jan 27, 2019 at 09:29:30PM +0900, Akinobu Mita wrote: > 2019年1月24日(木) 0:17 Marco Felsch <m.felsch@pengutronix.de>: > > > > Hi Akinobu, > > > > sorry for the delayed response. > > > > On 19-01-15 23:05, Akinobu Mita wrote: > > > The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize > > > V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE > > > is specified. > > > > > > Cc: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > > > Cc: Michael Grzeschik <m.grzeschik@pengutronix.de> > > > Cc: Marco Felsch <m.felsch@pengutronix.de> > > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > > > --- > > > * v3 > > > - Set initial try format with default configuration instead of > > > current one. > > > > > > drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ > > > 1 file changed, 30 insertions(+) > > > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > > > index d639b9b..63a5253 100644 > > > --- a/drivers/media/i2c/mt9m111.c > > > +++ b/drivers/media/i2c/mt9m111.c > > > @@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd, > > > if (format->pad) > > > return -EINVAL; > > > > > > + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { > > > +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API > > > + mf = v4l2_subdev_get_try_format(sd, cfg, format->pad); > > > + format->format = *mf; > > > + return 0; > > > +#else > > > + return -ENOTTY; > > > +#endif > > > > If've checked this again and found the ov* devices do this too. IMO it's > > not good for other developers to check the upper layer if the '#else' > > path is reachable. There are also some code analyzer tools which will > > report this as issue/warning. > > > > As I said, I checked the v4l2_subdev_get_try_format() usage again and > > found the solution made by the mt9v111.c better. Why do you don't add a > > dependency in the Kconfig, so we can drop this ifdef? > > I'm ok with adding CONFIG_VIDEO_V4L2_SUBDEV_API dependency to this > driver, because I always enable it. > > But it may cause an issue on some environments that don't require > CONFIG_VIDEO_V4L2_SUBDEV_API. > > Sakari, do you have any opinion? I think the dependency is just fine. There are drivers that do not support MC (and V4L2 sub-device uAPI) but if a driver does, I don't see why it couldn't depend on the related Kconfig option.
2019年1月28日(月) 17:34 Sakari Ailus <sakari.ailus@linux.intel.com>: > > Hi Mita-san, Marco, > > On Sun, Jan 27, 2019 at 09:29:30PM +0900, Akinobu Mita wrote: > > 2019年1月24日(木) 0:17 Marco Felsch <m.felsch@pengutronix.de>: > > > > > > Hi Akinobu, > > > > > > sorry for the delayed response. > > > > > > On 19-01-15 23:05, Akinobu Mita wrote: > > > > The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize > > > > V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE > > > > is specified. > > > > > > > > Cc: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > > > > Cc: Michael Grzeschik <m.grzeschik@pengutronix.de> > > > > Cc: Marco Felsch <m.felsch@pengutronix.de> > > > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > > > > --- > > > > * v3 > > > > - Set initial try format with default configuration instead of > > > > current one. > > > > > > > > drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ > > > > 1 file changed, 30 insertions(+) > > > > > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > > > > index d639b9b..63a5253 100644 > > > > --- a/drivers/media/i2c/mt9m111.c > > > > +++ b/drivers/media/i2c/mt9m111.c > > > > @@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd, > > > > if (format->pad) > > > > return -EINVAL; > > > > > > > > + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { > > > > +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API > > > > + mf = v4l2_subdev_get_try_format(sd, cfg, format->pad); > > > > + format->format = *mf; > > > > + return 0; > > > > +#else > > > > + return -ENOTTY; > > > > +#endif > > > > > > If've checked this again and found the ov* devices do this too. IMO it's > > > not good for other developers to check the upper layer if the '#else' > > > path is reachable. There are also some code analyzer tools which will > > > report this as issue/warning. > > > > > > As I said, I checked the v4l2_subdev_get_try_format() usage again and > > > found the solution made by the mt9v111.c better. Why do you don't add a > > > dependency in the Kconfig, so we can drop this ifdef? > > > > I'm ok with adding CONFIG_VIDEO_V4L2_SUBDEV_API dependency to this > > driver, because I always enable it. > > > > But it may cause an issue on some environments that don't require > > CONFIG_VIDEO_V4L2_SUBDEV_API. > > > > Sakari, do you have any opinion? > > I think the dependency is just fine. There are drivers that do not support > MC (and V4L2 sub-device uAPI) but if a driver does, I don't see why it > couldn't depend on the related Kconfig option. OK. I'll prepare a patch that adds the dependency and removes the ifdef. I made similar change for ov2640, so I'll do for mt9m111 and ov2640, respectively.
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c index d639b9b..63a5253 100644 --- a/drivers/media/i2c/mt9m111.c +++ b/drivers/media/i2c/mt9m111.c @@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd, if (format->pad) return -EINVAL; + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + mf = v4l2_subdev_get_try_format(sd, cfg, format->pad); + format->format = *mf; + return 0; +#else + return -ENOTTY; +#endif + } + mf->width = mt9m111->width; mf->height = mt9m111->height; mf->code = mt9m111->fmt->code; @@ -1089,6 +1099,25 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable) return 0; } +static int mt9m111_init_cfg(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg) +{ +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + struct v4l2_mbus_framefmt *format = + v4l2_subdev_get_try_format(sd, cfg, 0); + + format->width = MT9M111_MAX_WIDTH; + format->height = MT9M111_MAX_HEIGHT; + format->code = mt9m111_colour_fmts[0].code; + format->colorspace = mt9m111_colour_fmts[0].colorspace; + format->field = V4L2_FIELD_NONE; + format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; + format->quantization = V4L2_QUANTIZATION_DEFAULT; + format->xfer_func = V4L2_XFER_FUNC_DEFAULT; +#endif + return 0; +} + static int mt9m111_g_mbus_config(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg) { @@ -1114,6 +1143,7 @@ static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = { }; static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = { + .init_cfg = mt9m111_init_cfg, .enum_mbus_code = mt9m111_enum_mbus_code, .get_selection = mt9m111_get_selection, .set_selection = mt9m111_set_selection,
The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE is specified. Cc: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> Cc: Michael Grzeschik <m.grzeschik@pengutronix.de> Cc: Marco Felsch <m.felsch@pengutronix.de> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> --- * v3 - Set initial try format with default configuration instead of current one. drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)