Re: [PATCH v1] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

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

Em Fri, 15 Sep 2017 01:50:29 -0400
Satendra Singh Thakur <satendra.t@samsung.com> escreveu:

> Hello  Mr Chehab,
> Thanks for reviewing the patch.
> I have modified the patch as per your comments.
> Please check if it looks fine now.

Forgot to mention, but, for the patch series to be applied, you'll need to
also patch Documentation/media/uapi/dvb.

> Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 212 +++++++++++++++++++++++++++++++++-
>  include/uapi/linux/dvb/frontend.h     |  24 ++++
>  2 files changed, 234 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index e3fff8f..bc35a86 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1914,6 +1914,188 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
>  	return r;
>  }
>  
> +/**
> + * dtv_property_short_process_set
> + * @fe: Pointer to struct dvb_frontend
> + * @file: Pointer to struct file
> + * @cmd: Property name
> + * @data: Property value
> + *
> + * helper function for dvb_frontend_ioctl_properties,
> + * which can be used to set dtv property using ioctl
> + * cmd FE_SET_PROPERTY_SHORT.
> + * It assigns property value to corresponding member of
> + * property-cache structure
> + * This func is a variant of the func dtv_property_process_set
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +static int dtv_property_short_process_set(struct dvb_frontend *fe,
> +					struct file *file,
> +					u32 cmd, u32 data)
> +{
> +	int r = 0;
> +	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> +	switch (cmd) {
> +	case DTV_CLEAR:
> +		/*
> +		 * Reset a cache of data specific to the frontend here. This
> +		 * does not effect hardware.
> +		 */
> +		dvb_frontend_clear_cache(fe);
> +		break;

No, you got me wrong. What I want is to have just *one* switch that
would work for both the normal and the short version of this ioctl.

In other words, I was expecting you to patch the existing
dtv_property_process_set() for it to take u32 cmd and u32 data as
arguments and to be used by both FE_SET_PROPERTY and FE_SET_PROPERTY_SHORT
handling.

> +	case DTV_TUNE:
> +		/* interpret the cache of data, build either a traditional
> +		 * frontend tune request so we can pass validation in the
> +		 * FE_SET_FRONTEND  ioctl.
> +		 */
> +		c->state = cmd;
> +		dev_dbg(fe->dvb->device, "%s: Finalised property cache\n",
> +				__func__);
> +
> +		r = dtv_set_frontend(fe);
> +		break;
> +	case DTV_FREQUENCY:
> +		c->frequency = data;
> +		break;
> +	case DTV_MODULATION:
> +		c->modulation = data;
> +		break;
> +	case DTV_BANDWIDTH_HZ:
> +		c->bandwidth_hz = data;
> +		break;
> +	case DTV_INVERSION:
> +		c->inversion = data;
> +		break;
> +	case DTV_SYMBOL_RATE:
> +		c->symbol_rate = data;
> +		break;
> +	case DTV_INNER_FEC:
> +		c->fec_inner = data;
> +		break;
> +	case DTV_PILOT:
> +		c->pilot = data;
> +		break;
> +	case DTV_ROLLOFF:
> +		c->rolloff = data;
> +		break;
> +	case DTV_DELIVERY_SYSTEM:
> +		r = dvbv5_set_delivery_system(fe, data);
> +		break;
> +	case DTV_VOLTAGE:
> +		c->voltage = data;
> +		r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
> +			(void *)c->voltage);
> +		break;
> +	case DTV_TONE:
> +		c->sectone = data;
> +		r = dvb_frontend_ioctl_legacy(file, FE_SET_TONE,
> +			(void *)c->sectone);
> +		break;
> +	case DTV_CODE_RATE_HP:
> +		c->code_rate_HP = data;
> +		break;
> +	case DTV_CODE_RATE_LP:
> +		c->code_rate_LP = data;
> +		break;
> +	case DTV_GUARD_INTERVAL:
> +		c->guard_interval = data;
> +		break;
> +	case DTV_TRANSMISSION_MODE:
> +		c->transmission_mode = data;
> +		break;
> +	case DTV_HIERARCHY:
> +		c->hierarchy = data;
> +		break;
> +	case DTV_INTERLEAVING:
> +		c->interleaving = data;
> +		break;
> +
> +	/* ISDB-T Support here */
> +	case DTV_ISDBT_PARTIAL_RECEPTION:
> +		c->isdbt_partial_reception = data;
> +		break;
> +	case DTV_ISDBT_SOUND_BROADCASTING:
> +		c->isdbt_sb_mode = data;
> +		break;
> +	case DTV_ISDBT_SB_SUBCHANNEL_ID:
> +		c->isdbt_sb_subchannel = data;
> +		break;
> +	case DTV_ISDBT_SB_SEGMENT_IDX:
> +		c->isdbt_sb_segment_idx = data;
> +		break;
> +	case DTV_ISDBT_SB_SEGMENT_COUNT:
> +		c->isdbt_sb_segment_count = data;
> +		break;
> +	case DTV_ISDBT_LAYER_ENABLED:
> +		c->isdbt_layer_enabled = data;
> +		break;
> +	case DTV_ISDBT_LAYERA_FEC:
> +		c->layer[0].fec = data;
> +		break;
> +	case DTV_ISDBT_LAYERA_MODULATION:
> +		c->layer[0].modulation = data;
> +		break;
> +	case DTV_ISDBT_LAYERA_SEGMENT_COUNT:
> +		c->layer[0].segment_count = data;
> +		break;
> +	case DTV_ISDBT_LAYERA_TIME_INTERLEAVING:
> +		c->layer[0].interleaving = data;
> +		break;
> +	case DTV_ISDBT_LAYERB_FEC:
> +		c->layer[1].fec = data;
> +		break;
> +	case DTV_ISDBT_LAYERB_MODULATION:
> +		c->layer[1].modulation = data;
> +		break;
> +	case DTV_ISDBT_LAYERB_SEGMENT_COUNT:
> +		c->layer[1].segment_count = data;
> +		break;
> +	case DTV_ISDBT_LAYERB_TIME_INTERLEAVING:
> +		c->layer[1].interleaving = data;
> +		break;
> +	case DTV_ISDBT_LAYERC_FEC:
> +		c->layer[2].fec = data;
> +		break;
> +	case DTV_ISDBT_LAYERC_MODULATION:
> +		c->layer[2].modulation = data;
> +		break;
> +	case DTV_ISDBT_LAYERC_SEGMENT_COUNT:
> +		c->layer[2].segment_count = data;
> +		break;
> +	case DTV_ISDBT_LAYERC_TIME_INTERLEAVING:
> +		c->layer[2].interleaving = data;
> +		break;
> +
> +	/* Multistream support */
> +	case DTV_STREAM_ID:
> +	case DTV_DVBT2_PLP_ID_LEGACY:
> +		c->stream_id = data;
> +		break;
> +
> +	/* ATSC-MH */
> +	case DTV_ATSCMH_PARADE_ID:
> +		fe->dtv_property_cache.atscmh_parade_id = data;
> +		break;
> +	case DTV_ATSCMH_RS_FRAME_ENSEMBLE:
> +		fe->dtv_property_cache.atscmh_rs_frame_ensemble = data;
> +		break;
> +
> +	case DTV_LNA:
> +		c->lna = data;
> +		if (fe->ops.set_lna)
> +			r = fe->ops.set_lna(fe);
> +		if (r < 0)
> +			c->lna = LNA_AUTO;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return r;
> +}
> +
>  static int dvb_frontend_ioctl(struct file *file,
>  			unsigned int cmd, void *parg)
>  {
> @@ -1939,7 +2121,8 @@ static int dvb_frontend_ioctl(struct file *file,
>  		return -EPERM;
>  	}
>  
> -	if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY))
> +	if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY)
> +		|| (cmd == FE_SET_PROPERTY_SHORT))
>  		err = dvb_frontend_ioctl_properties(file, cmd, parg);
>  	else {
>  		c->state = DTV_UNDEFINED;
> @@ -2026,7 +2209,32 @@ static int dvb_frontend_ioctl_properties(struct file *file,
>  			err = -EFAULT;
>  			goto out;
>  		}
> -
> +	/* New ioctl for optimizing property set
> +	 */
> +	} else if (cmd == FE_SET_PROPERTY_SHORT) {
> +		struct dtv_property_short *tvp_short = NULL;
> +		struct dtv_properties_short *tvps_short = parg;
> +
> +		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps_short->num);
> +		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps_short->props);
> +		if ((!tvps_short->num) ||
> +		(tvps_short->num > DTV_IOCTL_MAX_MSGS))
> +			return -EINVAL;
> +		tvp_short = memdup_user(tvps_short->props,
> +		tvps_short->num * sizeof(*tvp_short));
> +		if (IS_ERR(tvp_short))
> +			return PTR_ERR(tvp_short);
> +		for (i = 0; i < tvps_short->num; i++) {
> +			err = dtv_property_short_process_set(fe, file,
> +			(tvp_short + i)->cmd, (tvp_short + i)->data);
> +			if (err < 0) {
> +				kfree(tvp_short);
> +				return err;
> +			}
> +		}
> +		if (c->state == DTV_TUNE)
> +			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
> +		kfree(tvp_short);
>  	} else
>  		err = -EOPNOTSUPP;
>  
> diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
> index 00a20cd..aa82179 100644
> --- a/include/uapi/linux/dvb/frontend.h
> +++ b/include/uapi/linux/dvb/frontend.h
> @@ -476,6 +476,17 @@ struct dtv_property {
>  	int result;
>  } __attribute__ ((packed));
>  
> +/**
> + * @struct dtv_property_short
> + * A shorter version of struct dtv_property
> + * @cmd: Property type
> + * @data: Property value
> + */
> +struct dtv_property_short {
> +	__u32 cmd;
> +	__u32 data;
> +};
> +
>  /* num of properties cannot exceed DTV_IOCTL_MAX_MSGS per ioctl */
>  #define DTV_IOCTL_MAX_MSGS 64
>  
> @@ -484,6 +495,18 @@ struct dtv_properties {
>  	struct dtv_property *props;
>  };
>  
> +/**
> + * @struct dtv_properties_short
> + * A variant of struct dtv_properties
> + * to support struct dtv_property_short
> + * @num: Number of properties
> + * @props: Pointer to struct dtv_property_short
> + */
> +struct dtv_properties_short {
> +	__u32 num;
> +	struct dtv_property_short *props;
> +};
> +
>  #if defined(__DVB_CORE__) || !defined (__KERNEL__)
>  
>  /*
> @@ -565,6 +588,7 @@ struct dvb_frontend_event {
>  
>  #define FE_SET_PROPERTY		   _IOW('o', 82, struct dtv_properties)
>  #define FE_GET_PROPERTY		   _IOR('o', 83, struct dtv_properties)
> +#define FE_SET_PROPERTY_SHORT	   _IOW('o', 84, struct dtv_properties_short)
>  
>  /**
>   * When set, this flag will disable any zigzagging or other "normal" tuning



Thanks,
Mauro