| Message ID | 20220818144712.997477-3-m.felsch@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support | expand |
Hi Marco On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote: > Currently the driver turn off the power after probe and toggle it during > .stream by using the .s_power callback. This is problematic since other > callbacks like .set_fmt accessing the hardware as well which will fail. Ouch! > So in the end the default format is the only supported format. > > Remove the hardware register access from the callbacks and instead sync > the state once right before the stream gets enabled to fix this. Where does it happen in this patch ? > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/media/i2c/mt9m111.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > index 53c4dac4e4bd..cd74c408e110 100644 > --- a/drivers/media/i2c/mt9m111.c > +++ b/drivers/media/i2c/mt9m111.c > @@ -481,8 +481,6 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd, > width = min(mt9m111->width, rect.width); > height = min(mt9m111->height, rect.height); > > - Why in mainline I don't see these empty lines ? > - mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code); > mt9m111->rect = rect; > mt9m111->width = width; > mt9m111->height = height; > @@ -611,7 +609,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111, > if (mt9m111->pclk_sample == 0) > mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK; > > - > mt9m111_reg_mask(client, context_a.output_fmt_ctrl2, > data_outfmt2, mask_outfmt2); > mt9m111_reg_mask(client, context_b.output_fmt_ctrl2, > @@ -678,9 +675,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd, > return 0; > } > > - > - mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code); > - mt9m111_set_pixfmt(mt9m111, mf->code); Are we looking at two different versions of the driver ?? https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9m111.c#L684 > mt9m111->width = mf->width; > mt9m111->height = mf->height; > mt9m111->fmt = fmt; > @@ -743,6 +737,8 @@ mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps, > return mode; > } > > +static int mt9m111_s_power(struct v4l2_subdev *sd, int on); > + > #ifdef CONFIG_VIDEO_ADV_DEBUG > static int mt9m111_g_register(struct v4l2_subdev *sd, > struct v4l2_dbg_register *reg) > @@ -753,10 +749,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd, > if (reg->reg > 0x2ff) > return -EINVAL; > > + mt9m111_s_power(sd, 1); > + > val = mt9m111_reg_read(client, reg->reg); > reg->size = 2; > reg->val = (u64)val; > > + mt9m111_s_power(sd, 0); > + > if (reg->val > 0xffff) > return -EIO; > > @@ -771,9 +771,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd, > if (reg->reg > 0x2ff) > return -EINVAL; > > + mt9m111_s_power(sd, 1); > + > if (mt9m111_reg_write(client, reg->reg, reg->val) < 0) > return -EIO; > > + mt9m111_s_power(sd, 0); > + > return 0; > } > #endif > @@ -896,6 +900,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl) > struct mt9m111, hdl); > int ret; > > + if (!mt9m111->is_streaming) > + return 0; > + > switch (ctrl->id) { > case V4L2_CID_VFLIP: > ret = mt9m111_set_flip(mt9m111, ctrl->val, > @@ -927,7 +934,6 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl) > ret = -EINVAL; > } > > - > return ret; > } > > -- > 2.30.2 >
Hi Jacopo, On 22-08-19, Jacopo Mondi wrote: > Hi Marco > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote: > > Currently the driver turn off the power after probe and toggle it during > > .stream by using the .s_power callback. This is problematic since other > > callbacks like .set_fmt accessing the hardware as well which will fail. > > Ouch! > > > So in the end the default format is the only supported format. > > > > Remove the hardware register access from the callbacks and instead sync > > the state once right before the stream gets enabled to fix this. > > Where does it happen in this patch ? during mt9m111_s_power which gets called by s_power() or after this small series during s_stream(). > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/media/i2c/mt9m111.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > > index 53c4dac4e4bd..cd74c408e110 100644 > > --- a/drivers/media/i2c/mt9m111.c > > +++ b/drivers/media/i2c/mt9m111.c > > @@ -481,8 +481,6 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd, > > width = min(mt9m111->width, rect.width); > > height = min(mt9m111->height, rect.height); > > > > - > > Why in mainline I don't see these empty lines ? Hm.. because I introduced this during my "media: mt9m111: fix subdev API usage" patch.. Sorry. > > - mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code); > > mt9m111->rect = rect; > > mt9m111->width = width; > > mt9m111->height = height; > > @@ -611,7 +609,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111, > > if (mt9m111->pclk_sample == 0) > > mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK; > > > > - > > mt9m111_reg_mask(client, context_a.output_fmt_ctrl2, > > data_outfmt2, mask_outfmt2); > > mt9m111_reg_mask(client, context_b.output_fmt_ctrl2, > > @@ -678,9 +675,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd, > > return 0; > > } > > > > - > > - mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code); > > - mt9m111_set_pixfmt(mt9m111, mf->code); > > Are we looking at two different versions of the driver ?? > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9m111.c#L684 Same here. > > mt9m111->width = mf->width; > > mt9m111->height = mf->height; > > mt9m111->fmt = fmt; > > @@ -743,6 +737,8 @@ mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps, > > return mode; > > } > > > > +static int mt9m111_s_power(struct v4l2_subdev *sd, int on); > > + > > #ifdef CONFIG_VIDEO_ADV_DEBUG > > static int mt9m111_g_register(struct v4l2_subdev *sd, > > struct v4l2_dbg_register *reg) > > @@ -753,10 +749,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd, > > if (reg->reg > 0x2ff) > > return -EINVAL; > > > > + mt9m111_s_power(sd, 1); > > + > > val = mt9m111_reg_read(client, reg->reg); > > reg->size = 2; > > reg->val = (u64)val; > > > > + mt9m111_s_power(sd, 0); > > + > > if (reg->val > 0xffff) > > return -EIO; > > > > @@ -771,9 +771,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd, > > if (reg->reg > 0x2ff) > > return -EINVAL; > > > > + mt9m111_s_power(sd, 1); > > + > > if (mt9m111_reg_write(client, reg->reg, reg->val) < 0) > > return -EIO; > > > > + mt9m111_s_power(sd, 0); > > + > > return 0; > > } > > #endif > > @@ -896,6 +900,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl) > > struct mt9m111, hdl); > > int ret; > > > > + if (!mt9m111->is_streaming) > > + return 0; > > + > > switch (ctrl->id) { > > case V4L2_CID_VFLIP: > > ret = mt9m111_set_flip(mt9m111, ctrl->val, > > @@ -927,7 +934,6 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl) > > ret = -EINVAL; > > } > > > > - > > return ret; > > } > > > > -- > > 2.30.2 > > >
Hi Marco, On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote: > Currently the driver turn off the power after probe and toggle it during > .stream by using the .s_power callback. This is problematic since other > callbacks like .set_fmt accessing the hardware as well which will fail. > So in the end the default format is the only supported format. It'd be much better to add runtime PM support to the driver instead.
Hi Sakari, On 22-08-22, Sakari Ailus wrote: > Hi Marco, > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote: > > Currently the driver turn off the power after probe and toggle it during > > .stream by using the .s_power callback. This is problematic since other > > callbacks like .set_fmt accessing the hardware as well which will fail. > > So in the end the default format is the only supported format. > > It'd be much better to add runtime PM support to the driver instead. I got your point, but didn't have the time for it right now, I will drop the patch from my v2. Regards, Marco > > -- > Sakari Ailus >
On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote: > Hi Sakari, > > On 22-08-22, Sakari Ailus wrote: > > Hi Marco, > > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote: > > > Currently the driver turn off the power after probe and toggle it during > > > .stream by using the .s_power callback. This is problematic since other > > > callbacks like .set_fmt accessing the hardware as well which will fail. > > > So in the end the default format is the only supported format. > > > > It'd be much better to add runtime PM support to the driver instead. > > I got your point, but didn't have the time for it right now, I will drop > the patch from my v2. The API is different but generally involves doing more or less the same what this and the 4th patch do together.
Hi Marco, On Tue, Aug 23, 2022 at 04:44:50PM +0200, Marco Felsch wrote: > Hi Sakari, > > On 22-08-22, Sakari Ailus wrote: > > On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote: > > > Hi Sakari, > > > > > > On 22-08-22, Sakari Ailus wrote: > > > > Hi Marco, > > > > > > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote: > > > > > Currently the driver turn off the power after probe and toggle it during > > > > > .stream by using the .s_power callback. This is problematic since other > > > > > callbacks like .set_fmt accessing the hardware as well which will fail. > > > > > So in the end the default format is the only supported format. > > > > > > > > It'd be much better to add runtime PM support to the driver instead. > > > > > > I got your point, but didn't have the time for it right now, I will drop > > > the patch from my v2. > > > > The API is different but generally involves doing more or less the same > > what this and the 4th patch do together. > > I know :) as soon as I got feedback on my TC35 series [1] I give it a > try and change it to dev-pm. What's the status of this set? These are nice improvements but I was expecting v2. Thanks.
Hi Sakari, On 23-01-17, Sakari Ailus wrote: > Hi Marco, > > On Tue, Aug 23, 2022 at 04:44:50PM +0200, Marco Felsch wrote: > > Hi Sakari, > > > > On 22-08-22, Sakari Ailus wrote: > > > On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote: > > > > Hi Sakari, > > > > > > > > On 22-08-22, Sakari Ailus wrote: > > > > > Hi Marco, > > > > > > > > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote: > > > > > > Currently the driver turn off the power after probe and toggle it during > > > > > > .stream by using the .s_power callback. This is problematic since other > > > > > > callbacks like .set_fmt accessing the hardware as well which will fail. > > > > > > So in the end the default format is the only supported format. > > > > > > > > > > It'd be much better to add runtime PM support to the driver instead. > > > > > > > > I got your point, but didn't have the time for it right now, I will drop > > > > the patch from my v2. > > > > > > The API is different but generally involves doing more or less the same > > > what this and the 4th patch do together. > > > > I know :) as soon as I got feedback on my TC35 series [1] I give it a > > try and change it to dev-pm. > > What's the status of this set? > > These are nice improvements but I was expecting v2. Unfortunately this was just a testing vehicle I had for the TC35 bridge chip. As soon as I have access to it again I will send a new version. Regards, Marco
On Tue, Jan 17, 2023 at 12:29:59PM +0100, Marco Felsch wrote: > Hi Sakari, > > On 23-01-17, Sakari Ailus wrote: > > Hi Marco, > > > > On Tue, Aug 23, 2022 at 04:44:50PM +0200, Marco Felsch wrote: > > > Hi Sakari, > > > > > > On 22-08-22, Sakari Ailus wrote: > > > > On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote: > > > > > Hi Sakari, > > > > > > > > > > On 22-08-22, Sakari Ailus wrote: > > > > > > Hi Marco, > > > > > > > > > > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote: > > > > > > > Currently the driver turn off the power after probe and toggle it during > > > > > > > .stream by using the .s_power callback. This is problematic since other > > > > > > > callbacks like .set_fmt accessing the hardware as well which will fail. > > > > > > > So in the end the default format is the only supported format. > > > > > > > > > > > > It'd be much better to add runtime PM support to the driver instead. > > > > > > > > > > I got your point, but didn't have the time for it right now, I will drop > > > > > the patch from my v2. > > > > > > > > The API is different but generally involves doing more or less the same > > > > what this and the 4th patch do together. > > > > > > I know :) as soon as I got feedback on my TC35 series [1] I give it a > > > try and change it to dev-pm. > > > > What's the status of this set? > > > > These are nice improvements but I was expecting v2. > > Unfortunately this was just a testing vehicle I had for the TC35 bridge > chip. As soon as I have access to it again I will send a new version. Ack, thanks!
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c index 53c4dac4e4bd..cd74c408e110 100644 --- a/drivers/media/i2c/mt9m111.c +++ b/drivers/media/i2c/mt9m111.c @@ -481,8 +481,6 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd, width = min(mt9m111->width, rect.width); height = min(mt9m111->height, rect.height); - - mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code); mt9m111->rect = rect; mt9m111->width = width; mt9m111->height = height; @@ -611,7 +609,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111, if (mt9m111->pclk_sample == 0) mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK; - mt9m111_reg_mask(client, context_a.output_fmt_ctrl2, data_outfmt2, mask_outfmt2); mt9m111_reg_mask(client, context_b.output_fmt_ctrl2, @@ -678,9 +675,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd, return 0; } - - mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code); - mt9m111_set_pixfmt(mt9m111, mf->code); mt9m111->width = mf->width; mt9m111->height = mf->height; mt9m111->fmt = fmt; @@ -743,6 +737,8 @@ mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps, return mode; } +static int mt9m111_s_power(struct v4l2_subdev *sd, int on); + #ifdef CONFIG_VIDEO_ADV_DEBUG static int mt9m111_g_register(struct v4l2_subdev *sd, struct v4l2_dbg_register *reg) @@ -753,10 +749,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd, if (reg->reg > 0x2ff) return -EINVAL; + mt9m111_s_power(sd, 1); + val = mt9m111_reg_read(client, reg->reg); reg->size = 2; reg->val = (u64)val; + mt9m111_s_power(sd, 0); + if (reg->val > 0xffff) return -EIO; @@ -771,9 +771,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd, if (reg->reg > 0x2ff) return -EINVAL; + mt9m111_s_power(sd, 1); + if (mt9m111_reg_write(client, reg->reg, reg->val) < 0) return -EIO; + mt9m111_s_power(sd, 0); + return 0; } #endif @@ -896,6 +900,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl) struct mt9m111, hdl); int ret; + if (!mt9m111->is_streaming) + return 0; + switch (ctrl->id) { case V4L2_CID_VFLIP: ret = mt9m111_set_flip(mt9m111, ctrl->val, @@ -927,7 +934,6 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl) ret = -EINVAL; } - return ret; }
Currently the driver turn off the power after probe and toggle it during .stream by using the .s_power callback. This is problematic since other callbacks like .set_fmt accessing the hardware as well which will fail. So in the end the default format is the only supported format. Remove the hardware register access from the callbacks and instead sync the state once right before the stream gets enabled to fix this. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/media/i2c/mt9m111.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)