Re: [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Wed, 14 Feb 2018 18:16:55 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 14/02/18 18:02, Mauro Carvalho Chehab wrote:
> > Em Wed, 14 Feb 2018 17:34:17 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> On 14/02/18 17:03, Mauro Carvalho Chehab wrote:  
> >>> Em Mon, 22 Jan 2018 13:31:18 +0100
> >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>     
> >>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>
> >>>> Convert all g/s_parm calls to g/s_frame_interval. This allows us
> >>>> to remove the g/s_parm ops since those are a duplicate of
> >>>> g/s_frame_interval.
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> ---
> >>>>  drivers/media/i2c/mt9v011.c                     | 31 +++++++-------------
> >>>>  drivers/media/i2c/ov6650.c                      | 35 +++++++++-------------
> >>>>  drivers/media/i2c/ov7670.c                      | 24 +++++++--------
> >>>>  drivers/media/i2c/ov7740.c                      | 31 +++++++-------------
> >>>>  drivers/media/i2c/tvp514x.c                     | 39 +++++++++----------------
> >>>>  drivers/media/i2c/vs6624.c                      | 29 +++++++-----------
> >>>>  drivers/media/platform/atmel/atmel-isc.c        | 10 ++-----
> >>>>  drivers/media/platform/atmel/atmel-isi.c        | 12 ++------
> >>>>  drivers/media/platform/blackfin/bfin_capture.c  | 14 +++------
> >>>>  drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++----
> >>>>  drivers/media/platform/soc_camera/soc_camera.c  | 10 ++++---
> >>>>  drivers/media/platform/via-camera.c             |  4 +--
> >>>>  drivers/media/usb/em28xx/em28xx-video.c         | 36 +++++++++++++++++++----
> >>>>  13 files changed, 122 insertions(+), 165 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
> >>>> index 5e29064fae91..3e23c5b0de1f 100644
> >>>> --- a/drivers/media/i2c/mt9v011.c
> >>>> +++ b/drivers/media/i2c/mt9v011.c
> >>>> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
> >>>>  	return 0;
> >>>>  }
> >>>>  
> >>>> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >>>> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd,
> >>>> +				    struct v4l2_subdev_frame_interval *ival)
> >>>>  {
> >>>> -	struct v4l2_captureparm *cp = &parms->parm.capture;
> >>>> -
> >>>> -	if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >>>> -		return -EINVAL;
> >>>> -
> >>>> -	memset(cp, 0, sizeof(struct v4l2_captureparm));
> >>>> -	cp->capability = V4L2_CAP_TIMEPERFRAME;
> >>>> +	memset(ival->reserved, 0, sizeof(ival->reserved));    
> >>>
> >>> Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing,
> >>> the best would be to do, instead:    
> >>
> >> g_frame_interval is called by bridge drivers through the subdev ops. So that
> >> path doesn't go through subdev_do_ioctl(). So it doesn't help putting it in
> >> v4l2-subdev.c.  
> > 
> > True, but you could also do the same for v4l2 ioctl() handling logic.
> > 
> > That would mean just two places with memset() instead of repeating the same
> > pattern everywhere.
> >   
> >> That doesn't mean it shouldn't be there as well. I believe my MC patch series
> >> actually adds the memset in subdev_do_ioctl.  
> 
> What could be done is that this patch https://patchwork.linuxtv.org/patch/46955/
> is applied first. After that these memsets can be removed since internally we
> don't need to touch them.

Works for me.

> >>  
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> index c5639817db34..b18b418c080f 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>>  		if (fi->pad >= sd->entity.num_pads)
> >>>  			return -EINVAL;
> >>>  
> >>> +		memset(fi->reserved, 0, sizeof(ival->reserved));
> >>>  		return v4l2_subdev_call(sd, video, g_frame_interval, arg);
> >>>  	}
> >>>  
> >>> (same applies to s_frame_interval).
> >>>
> >>>     
> >>>>  	calc_fps(sd,
> >>>> -		 &cp->timeperframe.numerator,
> >>>> -		 &cp->timeperframe.denominator);
> >>>> +		 &ival->interval.numerator,
> >>>> +		 &ival->interval.denominator);
> >>>>  
> >>>>  	return 0;
> >>>>  }
> >>>>  
> >>>> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >>>> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd,
> >>>> +				    struct v4l2_subdev_frame_interval *ival)
> >>>>  {
> >>>> -	struct v4l2_captureparm *cp = &parms->parm.capture;
> >>>> -	struct v4l2_fract *tpf = &cp->timeperframe;
> >>>> +	struct v4l2_fract *tpf = &ival->interval;
> >>>>  	u16 speed;
> >>>>  
> >>>> -	if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >>>> -		return -EINVAL;
> >>>> -	if (cp->extendedmode != 0)
> >>>> -		return -EINVAL;
> >>>> -    
> >>>
> >>> Hmm... why are you removing those sanity checks everywhere?
> >>> The core doesn't do it.
> >>>
> >>> All the above comments also apply to the other files modified by
> >>> this patch.    
> >>
> >> struct v4l2_subdev_frame_interval has neither type nor extendedmode.
> >>
> >> The check for type is done in the v4l2_g/s_parm_cap helpers instead.  
> > 
> > Well, the subdev handler at v4l2-subdev.c doesn't seem to be checking it.  
> 
> ????
> 
> Are you confusing struct v4l2_streamparm with struct v4l2_subdev_frame_interval?
> 
> v4l2_subdev.c deals with the latter, and struct v4l2_subdev_frame_interval has
> no type field. There is nothing to check.
> 
> 'type' makes no sense in subdev drivers anyway since it refers to a buffer type
> and subdevs do not deal with buffers.

Yeah, you're right: those checks can be removed.

> >> And extendedmode is always set to 0.
> >>  
> >>>     
> >>>> +	memset(ival->reserved, 0, sizeof(ival->reserved));
> >>>>  	speed = calc_speed(sd, tpf->numerator, tpf->denominator);
> >>>>  
> >>>>  	mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed);
> >>>> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = {
> >>>>  };
> >>>>  
> >>>>  static const struct v4l2_subdev_video_ops mt9v011_video_ops = {
> >>>> -	.g_parm = mt9v011_g_parm,
> >>>> -	.s_parm = mt9v011_s_parm,
> >>>> +	.g_frame_interval = mt9v011_g_frame_interval,
> >>>> +	.s_frame_interval = mt9v011_s_frame_interval,
> >>>>  };
> >>>>  
> >>>>  static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = {
> >>>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> >>>> index 8975d16b2b24..3f962dae7534 100644
> >>>> --- a/drivers/media/i2c/ov6650.c
> >>>> +++ b/drivers/media/i2c/ov6650.c
> >>>> @@ -201,7 +201,7 @@ struct ov6650 {
> >>>>  	struct v4l2_rect	rect;		/* sensor cropping window */
> >>>>  	unsigned long		pclk_limit;	/* from host */
> >>>>  	unsigned long		pclk_max;	/* from resolution and format */
> >>>> -	struct v4l2_fract	tpf;		/* as requested with s_parm */
> >>>> +	struct v4l2_fract	tpf;		/* as requested with s_frame_interval */
> >>>>  	u32 code;
> >>>>  	enum v4l2_colorspace	colorspace;
> >>>>  };
> >>>> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd,
> >>>>  	return 0;
> >>>>  }
> >>>>  
> >>>> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >>>> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
> >>>> +				   struct v4l2_subdev_frame_interval *ival)
> >>>>  {
> >>>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >>>>  	struct ov6650 *priv = to_ov6650(client);
> >>>> -	struct v4l2_captureparm *cp = &parms->parm.capture;
> >>>>  
> >>>> -	if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >>>> -		return -EINVAL;
> >>>> -
> >>>> -	memset(cp, 0, sizeof(*cp));
> >>>> -	cp->capability = V4L2_CAP_TIMEPERFRAME;
> >>>> -	cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
> >>>> +	memset(ival->reserved, 0, sizeof(ival->reserved));
> >>>> +	ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
> >>>>  			priv->pclk_limit, priv->pclk_max));
> >>>> -	cp->timeperframe.denominator = FRAME_RATE_MAX;
> >>>> +	ival->interval.denominator = FRAME_RATE_MAX;
> >>>>  
> >>>>  	dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
> >>>> -		cp->timeperframe.numerator, cp->timeperframe.denominator);
> >>>> +		ival->interval.numerator, ival->interval.denominator);    
> >>>
> >>> Hmm... not sure if a debug is needed here. Yet, if this is needed, 
> >>> IMHO, it would make mroe sense to move it to the core.    
> >>
> >> The core doesn't see this if this subdev op is called from a bridge driver.  
> > 
> > True, but, when calling via a bridge driver, there's already a way to
> > enable such kind debug.  
> 
> It can debug VIDIOC_G/S_PARM, not the g_frame_interval op. Also, when called
> via a v4l-subdev device node there is currently NO core logging.
> 
> For the record, I don't really care about this debug statement myself one
> way or another, but changing this one way or another doesn't belong in this
> patch series.

Ok. Yet, IMHO, it is probably safe to just remove the debug statements
on the above driver, as I fail to see that just ov6650 driver would
need this.

> 
> Regards,
> 
> 	Hans



Thanks,
Mauro