2017-09-14 09:59:46

by Satendra Singh Thakur

[permalink] [raw]
Subject: [RFC] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

-For setting one frontend property , one FE_SET_PROPERTY ioctl is called
-Since, size of struct dtv_property is 72 bytes, this ioctl requires
---allocating 72 bytes of memory in user space
---allocating 72 bytes of memory in kernel space
---copying 72 bytes of data from user space to kernel space
-However, for all the properties, only 8 out of 72 bytes are used
for setting the property
-Four bytes are needed for specifying property type and another 4 for
property value
-Moreover, there are 2 properties DTV_CLEAR and DTV_TUNE which use
only 4 bytes for property name
---They don't use property value
-Therefore, we have defined new short variant/forms/version of currently
used structures for such 8 byte properties.
-This results in 89% (8*100/72) of memory saving in user and kernel space
each.
-This also results in faster copy (8 bytes as compared to 72 bytes) from
user to kernel space
-We have added new ioctl FE_SET_PROPERTY_SHORT which utilizes above
mentioned new property structures
-This ioctl can co-exist with present ioctl FE_SET_PROPERTY
-If the apps wish to use shorter forms they can use
proposed FE_SET_PROPERTY_SHORT, rest of them can continue to use
current versions FE_SET_PROPERTY
-We are currently not validating incoming properties in
function dtv_property_short_process_set because most of
the frontend drivers in linux source are not using the
method ops.set_property. Just two drivers are using it
drivers/media/dvb-frontends/stv0288.c
driver/media/usb/dvb-usb/friio-fe.c
-Moreover, stv0288 driver implemments blank function
for set_property.
-If needed in future, we can define a new
ops.set_property_short method to support
struct dtv_property_short.
Signed-off-by: Satendra Singh Thakur <[email protected]>
---
drivers/media/dvb-core/dvb_frontend.c | 228 +++++++++++++++++++++++++++++++++-
include/uapi/linux/dvb/frontend.h | 24 ++++
2 files changed, 248 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index e3fff8f..e183025 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1914,6 +1914,192 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
return r;
}

+/**
+ * dtv_property_short_process_set
+ * @fe: Pointer to struct dvb_frontend
+ * @tvp: Pointer to struct dtv_property_short
+ * @file: Pointer to struct file
+ *
+ * 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 dtv_property_short *tvp,
+ struct file *file)
+{
+ int r = 0;
+ struct dtv_frontend_properties *c = &fe->dtv_property_cache;
+ /* Currently, We do not allow the frontend to validate incoming
+ * properties, currently, just 2 drivers are using
+ * ops.set_property method , If required, we can define new
+ * ops.set_property_short method for this purpose
+ */
+ switch (tvp->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;
+ 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 = tvp->cmd;
+ dev_dbg(fe->dvb->device, "%s: Finalised property cache\n",
+ __func__);
+
+ r = dtv_set_frontend(fe);
+ break;
+ case DTV_FREQUENCY:
+ c->frequency = tvp->data;
+ break;
+ case DTV_MODULATION:
+ c->modulation = tvp->data;
+ break;
+ case DTV_BANDWIDTH_HZ:
+ c->bandwidth_hz = tvp->data;
+ break;
+ case DTV_INVERSION:
+ c->inversion = tvp->data;
+ break;
+ case DTV_SYMBOL_RATE:
+ c->symbol_rate = tvp->data;
+ break;
+ case DTV_INNER_FEC:
+ c->fec_inner = tvp->data;
+ break;
+ case DTV_PILOT:
+ c->pilot = tvp->data;
+ break;
+ case DTV_ROLLOFF:
+ c->rolloff = tvp->data;
+ break;
+ case DTV_DELIVERY_SYSTEM:
+ r = dvbv5_set_delivery_system(fe, tvp->data);
+ break;
+ case DTV_VOLTAGE:
+ c->voltage = tvp->data;
+ r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
+ (void *)c->voltage);
+ break;
+ case DTV_TONE:
+ c->sectone = tvp->data;
+ r = dvb_frontend_ioctl_legacy(file, FE_SET_TONE,
+ (void *)c->sectone);
+ break;
+ case DTV_CODE_RATE_HP:
+ c->code_rate_HP = tvp->data;
+ break;
+ case DTV_CODE_RATE_LP:
+ c->code_rate_LP = tvp->data;
+ break;
+ case DTV_GUARD_INTERVAL:
+ c->guard_interval = tvp->data;
+ break;
+ case DTV_TRANSMISSION_MODE:
+ c->transmission_mode = tvp->data;
+ break;
+ case DTV_HIERARCHY:
+ c->hierarchy = tvp->data;
+ break;
+ case DTV_INTERLEAVING:
+ c->interleaving = tvp->data;
+ break;
+
+ /* ISDB-T Support here */
+ case DTV_ISDBT_PARTIAL_RECEPTION:
+ c->isdbt_partial_reception = tvp->data;
+ break;
+ case DTV_ISDBT_SOUND_BROADCASTING:
+ c->isdbt_sb_mode = tvp->data;
+ break;
+ case DTV_ISDBT_SB_SUBCHANNEL_ID:
+ c->isdbt_sb_subchannel = tvp->data;
+ break;
+ case DTV_ISDBT_SB_SEGMENT_IDX:
+ c->isdbt_sb_segment_idx = tvp->data;
+ break;
+ case DTV_ISDBT_SB_SEGMENT_COUNT:
+ c->isdbt_sb_segment_count = tvp->data;
+ break;
+ case DTV_ISDBT_LAYER_ENABLED:
+ c->isdbt_layer_enabled = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERA_FEC:
+ c->layer[0].fec = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERA_MODULATION:
+ c->layer[0].modulation = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERA_SEGMENT_COUNT:
+ c->layer[0].segment_count = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERA_TIME_INTERLEAVING:
+ c->layer[0].interleaving = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERB_FEC:
+ c->layer[1].fec = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERB_MODULATION:
+ c->layer[1].modulation = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERB_SEGMENT_COUNT:
+ c->layer[1].segment_count = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERB_TIME_INTERLEAVING:
+ c->layer[1].interleaving = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERC_FEC:
+ c->layer[2].fec = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERC_MODULATION:
+ c->layer[2].modulation = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERC_SEGMENT_COUNT:
+ c->layer[2].segment_count = tvp->data;
+ break;
+ case DTV_ISDBT_LAYERC_TIME_INTERLEAVING:
+ c->layer[2].interleaving = tvp->data;
+ break;
+
+ /* Multistream support */
+ case DTV_STREAM_ID:
+ case DTV_DVBT2_PLP_ID_LEGACY:
+ c->stream_id = tvp->data;
+ break;
+
+ /* ATSC-MH */
+ case DTV_ATSCMH_PARADE_ID:
+ fe->dtv_property_cache.atscmh_parade_id = tvp->data;
+ break;
+ case DTV_ATSCMH_RS_FRAME_ENSEMBLE:
+ fe->dtv_property_cache.atscmh_rs_frame_ensemble = tvp->data;
+ break;
+
+ case DTV_LNA:
+ c->lna = tvp->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 +2125,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,9 +2213,42 @@ static int dvb_frontend_ioctl_properties(struct file *file,
err = -EFAULT;
goto out;
}
-
- } else
- err = -EOPNOTSUPP;
+ /* 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, tvp_short + i,\
+ file);
+ if (err < 0) {
+ kfree(tvp_short);
+ return err;
+ }
+ /* Since we are returning when error occurs
+ * There is no need to store the result as it
+ * would have been >=0 in case we didn't return
+ * (tvp + i)->result = 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;

out:
kfree(tvp);
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
--
2.7.4


2017-09-14 20:51:15

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [RFC] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

Hi Satendra,

Em Thu, 14 Sep 2017 05:59:27 -0400
Satendra Singh Thakur <[email protected]> escreveu:

> -For setting one frontend property , one FE_SET_PROPERTY ioctl is called
> -Since, size of struct dtv_property is 72 bytes, this ioctl requires
> ---allocating 72 bytes of memory in user space
> ---allocating 72 bytes of memory in kernel space
> ---copying 72 bytes of data from user space to kernel space
> -However, for all the properties, only 8 out of 72 bytes are used
> for setting the property

That's true. Yet, for get, the size can be bigger, as ISDB-T can
return statistics per layer, plus a global one.

> -Four bytes are needed for specifying property type and another 4 for
> property value
> -Moreover, there are 2 properties DTV_CLEAR and DTV_TUNE which use
> only 4 bytes for property name
> ---They don't use property value
> -Therefore, we have defined new short variant/forms/version of currently
> used structures for such 8 byte properties.
> -This results in 89% (8*100/72) of memory saving in user and kernel space
> each.
> -This also results in faster copy (8 bytes as compared to 72 bytes) from
> user to kernel space
> -We have added new ioctl FE_SET_PROPERTY_SHORT which utilizes above
> mentioned new property structures
> -This ioctl can co-exist with present ioctl FE_SET_PROPERTY
> -If the apps wish to use shorter forms they can use
> proposed FE_SET_PROPERTY_SHORT, rest of them can continue to use
> current versions FE_SET_PROPERTY

> -We are currently not validating incoming properties in
> function dtv_property_short_process_set because most of
> the frontend drivers in linux source are not using the
> method ops.set_property. Just two drivers are using it
> drivers/media/dvb-frontends/stv0288.c
> driver/media/usb/dvb-usb/friio-fe.c
> -Moreover, stv0288 driver implemments blank function
> for set_property.
> -If needed in future, we can define a new
> ops.set_property_short method to support
> struct dtv_property_short.

Nah. Better to just get rid of get_property()/set_froperty() for good.

Just sent a RFC patch series doing that.

The only thing is that stv6110 seems to have a dirty hack that may
depend on that. Someone need to double-check if the patch series
I just sent doesn't break anything. If it breaks, then we'll need
to add an extra parameter to stv6110 attach for it to know what
behavior is needed there.


> Signed-off-by: Satendra Singh Thakur <[email protected]>
> ---
> drivers/media/dvb-core/dvb_frontend.c | 228 +++++++++++++++++++++++++++++++++-
> include/uapi/linux/dvb/frontend.h | 24 ++++
> 2 files changed, 248 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index e3fff8f..e183025 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1914,6 +1914,192 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
> return r;
> }
>
> +/**
> + * dtv_property_short_process_set
> + * @fe: Pointer to struct dvb_frontend
> + * @tvp: Pointer to struct dtv_property_short
> + * @file: Pointer to struct file
> + *
> + * 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 dtv_property_short *tvp,
> + struct file *file)
> +{
> + int r = 0;
> + struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> + /* Currently, We do not allow the frontend to validate incoming
> + * properties, currently, just 2 drivers are using
> + * ops.set_property method , If required, we can define new
> + * ops.set_property_short method for this purpose
> + */
> + switch (tvp->cmd) {
> + case DTV_CLEAR:

Nah. Let's not have multiple validation routines for each variant.

It would be better to change the parameters for dtv_property_process_set
to something like:

static int dtv_property_process_set(struct dvb_frontend *fe,
struct file *file,
u32 cmd, u32 data)

And have just one validation routine that would work for both.

If we end by adding some DTV properties that would require more than
4 bytes, only such properties would be implemented on a different
function.

> static int dvb_frontend_ioctl(struct file *file,
> unsigned int cmd, void *parg)
> {
> @@ -1939,7 +2125,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,9 +2213,42 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> err = -EFAULT;
> goto out;
> }
> -
> - } else
> - err = -EOPNOTSUPP;
> + /* 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, tvp_short + i,\
> + file);
> + if (err < 0) {
> + kfree(tvp_short);
> + return err;
> + }
> + /* Since we are returning when error occurs
> + * There is no need to store the result as it
> + * would have been >=0 in case we didn't return
> + * (tvp + i)->result = err;
> + */
> + }
> + if (c->state == DTV_TUNE)
> + dev_dbg(fe->dvb->device, "%s: Property cache\
> + is full, tuning\n", __func__);

Don't break strings on two lines.

> + kfree(tvp_short);

> + } else
> + err = -EOPNOTSUPP;

Indentation here is wrong.

>
> out:
> kfree(tvp);
> 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




2017-09-15 05:51:47

by Satendra Singh Thakur

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

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

Signed-off-by: Satendra Singh Thakur <[email protected]>
---
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;
+ 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
--
2.7.4

2017-09-15 08:28:12

by Honza P

[permalink] [raw]
Subject: Re: [RFC] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

2017-09-14 22:50 GMT+02:00 Mauro Carvalho Chehab <[email protected]>:
> Hi Satendra,
>
> Em Thu, 14 Sep 2017 05:59:27 -0400
> Satendra Singh Thakur <[email protected]> escreveu:
>
>> -For setting one frontend property , one FE_SET_PROPERTY ioctl is called
>> -Since, size of struct dtv_property is 72 bytes, this ioctl requires
>> ---allocating 72 bytes of memory in user space
>> ---allocating 72 bytes of memory in kernel space
>> ---copying 72 bytes of data from user space to kernel space
>> -However, for all the properties, only 8 out of 72 bytes are used
>> for setting the property
>
> That's true. Yet, for get, the size can be bigger, as ISDB-T can
> return statistics per layer, plus a global one.
>
>> -Four bytes are needed for specifying property type and another 4 for
>> property value
>> -Moreover, there are 2 properties DTV_CLEAR and DTV_TUNE which use
>> only 4 bytes for property name
>> ---They don't use property value
>> -Therefore, we have defined new short variant/forms/version of currently
>> used structures for such 8 byte properties.
>> -This results in 89% (8*100/72) of memory saving in user and kernel space
>> each.
>> -This also results in faster copy (8 bytes as compared to 72 bytes) from
>> user to kernel space
>> -We have added new ioctl FE_SET_PROPERTY_SHORT which utilizes above
>> mentioned new property structures
>> -This ioctl can co-exist with present ioctl FE_SET_PROPERTY
>> -If the apps wish to use shorter forms they can use
>> proposed FE_SET_PROPERTY_SHORT, rest of them can continue to use
>> current versions FE_SET_PROPERTY
>
>> -We are currently not validating incoming properties in
>> function dtv_property_short_process_set because most of
>> the frontend drivers in linux source are not using the
>> method ops.set_property. Just two drivers are using it
>> drivers/media/dvb-frontends/stv0288.c
>> driver/media/usb/dvb-usb/friio-fe.c
>> -Moreover, stv0288 driver implemments blank function
>> for set_property.
>> -If needed in future, we can define a new
>> ops.set_property_short method to support
>> struct dtv_property_short.
>
> Nah. Better to just get rid of get_property()/set_froperty() for good.
>
> Just sent a RFC patch series doing that.
>
> The only thing is that stv6110 seems to have a dirty hack that may
> depend on that. Someone need to double-check if the patch series
> I just sent doesn't break anything. If it breaks, then we'll need
> to add an extra parameter to stv6110 attach for it to know what
> behavior is needed there.

Do you mean in stv6110_set_frequency()?

I must say I was shocked by the beginning of it.
Can somebody explain me the reason for such strange
srate computation?

See the head of function:

static int stv6110_set_frequency(struct dvb_frontend *fe, u32 frequency)
{
struct stv6110_priv *priv = fe->tuner_priv;
struct dtv_frontend_properties *c = &fe->dtv_property_cache;
u8 ret = 0x04;
u32 divider, ref, p, presc, i, result_freq, vco_freq;
s32 p_calc, p_calc_opt = 1000, r_div, r_div_opt = 0, p_val;
s32 srate;

dprintk("%s, freq=%d kHz, mclk=%d Hz\n", __func__,
frequency, priv->mclk);

/* K = (Reference / 1000000) - 16 */
priv->regs[RSTV6110_CTRL1] &= ~(0x1f << 3);
priv->regs[RSTV6110_CTRL1] |=
((((priv->mclk / 1000000) - 16) & 0x1f) << 3);

/* BB_GAIN = db/2 */
if (fe->ops.set_property && fe->ops.get_property) {
srate = c->symbol_rate;
dprintk("%s: Get Frontend parameters: srate=%d\n",
__func__, srate);
} else
srate = 15000000;

^^^^ here I would like to note, there there is NO MORE
anything dependant on srate. It looks like some dead code for me.

And the condition sentence looks even more funny - is it
for real to check of retrieval of srate only in case
if some other function pointers are not null?

/Honza

PS: Don't forget that we have duplicated drivers for STV6110,
stv6110 by Igor and stv6110x by Manu.

2017-09-15 08:49:36

by Mauro Carvalho Chehab

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

Em Fri, 15 Sep 2017 01:50:29 -0400
Satendra Singh Thakur <[email protected]> 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 <[email protected]>
> ---
> 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

2017-09-15 10:19:13

by Satendra Singh Thakur

[permalink] [raw]
Subject: [PATCH v2] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

Hello Mr Chehab,
Thanks for the comments.
I have modified dtv_property_process_set and
also added documentation.
Please let me know if any further modifications required.

Signed-off-by: Satendra Singh Thakur <[email protected]>
---
.../media/uapi/dvb/fe-set-property-short.rst | 60 +++++++++
Documentation/media/uapi/dvb/frontend_fcalls.rst | 1 +
drivers/media/dvb-core/dvb_frontend.c | 143 +++++++++++++--------
include/uapi/linux/dvb/frontend.h | 24 ++++
4 files changed, 173 insertions(+), 55 deletions(-)
create mode 100644 Documentation/media/uapi/dvb/fe-set-property-short.rst

diff --git a/Documentation/media/uapi/dvb/fe-set-property-short.rst b/Documentation/media/uapi/dvb/fe-set-property-short.rst
new file mode 100644
index 0000000..7da1e8d
--- /dev/null
+++ b/Documentation/media/uapi/dvb/fe-set-property-short.rst
@@ -0,0 +1,60 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _FE_SET_PROPERTY_SHORT:
+
+**************************************
+ioctl FE_SET_PROPERTY_SHORT
+**************************************
+
+Name
+====
+
+FE_SET_PROPERTY_SHORT sets one or more frontend properties.
+
+
+Synopsis
+========
+
+
+.. c:function:: int ioctl( int fd, FE_SET_PROPERTY_SHORT, struct dtv_properties_short *argp )
+ :name: FE_SET_PROPERTY_SHORT
+
+
+Arguments
+=========
+
+``fd``
+ File descriptor returned by :ref:`open() <frontend_f_open>`.
+
+``argp``
+ pointer to struct :c:type:`dtv_properties_short`,
+ which is a shorter variant of struct dtv_properties.
+
+
+Description
+===========
+
+All DVB frontend devices support the ``FE_SET_PROPERTY_SHORT`` ioctl.
+This ioctl is a shorter variant of ioctl FE_SET_PROPERTY.
+The supported properties and statistics depend on the delivery system
+and on the device:
+
+- ``FE_SET_PROPERTY_SHORT:``
+
+ - This ioctl is used to set one or more frontend properties.
+
+ - This is the basic command to request the frontend to tune into
+ some frequency and to start decoding the digital TV signal.
+
+ - This call requires read/write access to the device.
+
+ - At return, the values are updated to reflect the actual parameters
+ used.
+
+
+Return Value
+============
+
+On success 0 is returned, on error -1 and the ``errno`` variable is set
+appropriately. The generic error codes are described at the
+:ref:`Generic Error Codes <gen-errors>` chapter.
diff --git a/Documentation/media/uapi/dvb/frontend_fcalls.rst b/Documentation/media/uapi/dvb/frontend_fcalls.rst
index b03f9ca..a1246c6 100644
--- a/Documentation/media/uapi/dvb/frontend_fcalls.rst
+++ b/Documentation/media/uapi/dvb/frontend_fcalls.rst
@@ -14,6 +14,7 @@ Frontend Function Calls
fe-get-info
fe-read-status
fe-get-property
+ fe-set-property-short
fe-diseqc-reset-overload
fe-diseqc-send-master-cmd
fe-diseqc-recv-slave-reply
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index e3fff8f..6616474 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1738,23 +1738,28 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
return emulate_delivery_system(fe, delsys);
}

+/**
+ * dtv_property_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 a single dtv property
+ * It assigns property value to corresponding member of
+ * property-cache structure
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
static int dtv_property_process_set(struct dvb_frontend *fe,
- struct dtv_property *tvp,
- struct file *file)
+ struct file *file,
+ u32 cmd, u32 data)
{
int r = 0;
struct dtv_frontend_properties *c = &fe->dtv_property_cache;

- /* Allow the frontend to validate incoming properties */
- if (fe->ops.set_property) {
- r = fe->ops.set_property(fe, tvp);
- if (r < 0)
- return r;
- }
-
- dtv_property_dump(fe, true, tvp);
-
- switch(tvp->cmd) {
+ switch (cmd) {
case DTV_CLEAR:
/*
* Reset a cache of data specific to the frontend here. This does
@@ -1767,140 +1772,140 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
* tunerequest so we can pass validation in the FE_SET_FRONTEND
* ioctl.
*/
- c->state = tvp->cmd;
+ 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 = tvp->u.data;
+ c->frequency = data;
break;
case DTV_MODULATION:
- c->modulation = tvp->u.data;
+ c->modulation = data;
break;
case DTV_BANDWIDTH_HZ:
- c->bandwidth_hz = tvp->u.data;
+ c->bandwidth_hz = data;
break;
case DTV_INVERSION:
- c->inversion = tvp->u.data;
+ c->inversion = data;
break;
case DTV_SYMBOL_RATE:
- c->symbol_rate = tvp->u.data;
+ c->symbol_rate = data;
break;
case DTV_INNER_FEC:
- c->fec_inner = tvp->u.data;
+ c->fec_inner = data;
break;
case DTV_PILOT:
- c->pilot = tvp->u.data;
+ c->pilot = data;
break;
case DTV_ROLLOFF:
- c->rolloff = tvp->u.data;
+ c->rolloff = data;
break;
case DTV_DELIVERY_SYSTEM:
- r = dvbv5_set_delivery_system(fe, tvp->u.data);
+ r = dvbv5_set_delivery_system(fe, data);
break;
case DTV_VOLTAGE:
- c->voltage = tvp->u.data;
+ c->voltage = data;
r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
(void *)c->voltage);
break;
case DTV_TONE:
- c->sectone = tvp->u.data;
+ 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 = tvp->u.data;
+ c->code_rate_HP = data;
break;
case DTV_CODE_RATE_LP:
- c->code_rate_LP = tvp->u.data;
+ c->code_rate_LP = data;
break;
case DTV_GUARD_INTERVAL:
- c->guard_interval = tvp->u.data;
+ c->guard_interval = data;
break;
case DTV_TRANSMISSION_MODE:
- c->transmission_mode = tvp->u.data;
+ c->transmission_mode = data;
break;
case DTV_HIERARCHY:
- c->hierarchy = tvp->u.data;
+ c->hierarchy = data;
break;
case DTV_INTERLEAVING:
- c->interleaving = tvp->u.data;
+ c->interleaving = data;
break;

/* ISDB-T Support here */
case DTV_ISDBT_PARTIAL_RECEPTION:
- c->isdbt_partial_reception = tvp->u.data;
+ c->isdbt_partial_reception = data;
break;
case DTV_ISDBT_SOUND_BROADCASTING:
- c->isdbt_sb_mode = tvp->u.data;
+ c->isdbt_sb_mode = data;
break;
case DTV_ISDBT_SB_SUBCHANNEL_ID:
- c->isdbt_sb_subchannel = tvp->u.data;
+ c->isdbt_sb_subchannel = data;
break;
case DTV_ISDBT_SB_SEGMENT_IDX:
- c->isdbt_sb_segment_idx = tvp->u.data;
+ c->isdbt_sb_segment_idx = data;
break;
case DTV_ISDBT_SB_SEGMENT_COUNT:
- c->isdbt_sb_segment_count = tvp->u.data;
+ c->isdbt_sb_segment_count = data;
break;
case DTV_ISDBT_LAYER_ENABLED:
- c->isdbt_layer_enabled = tvp->u.data;
+ c->isdbt_layer_enabled = data;
break;
case DTV_ISDBT_LAYERA_FEC:
- c->layer[0].fec = tvp->u.data;
+ c->layer[0].fec = data;
break;
case DTV_ISDBT_LAYERA_MODULATION:
- c->layer[0].modulation = tvp->u.data;
+ c->layer[0].modulation = data;
break;
case DTV_ISDBT_LAYERA_SEGMENT_COUNT:
- c->layer[0].segment_count = tvp->u.data;
+ c->layer[0].segment_count = data;
break;
case DTV_ISDBT_LAYERA_TIME_INTERLEAVING:
- c->layer[0].interleaving = tvp->u.data;
+ c->layer[0].interleaving = data;
break;
case DTV_ISDBT_LAYERB_FEC:
- c->layer[1].fec = tvp->u.data;
+ c->layer[1].fec = data;
break;
case DTV_ISDBT_LAYERB_MODULATION:
- c->layer[1].modulation = tvp->u.data;
+ c->layer[1].modulation = data;
break;
case DTV_ISDBT_LAYERB_SEGMENT_COUNT:
- c->layer[1].segment_count = tvp->u.data;
+ c->layer[1].segment_count = data;
break;
case DTV_ISDBT_LAYERB_TIME_INTERLEAVING:
- c->layer[1].interleaving = tvp->u.data;
+ c->layer[1].interleaving = data;
break;
case DTV_ISDBT_LAYERC_FEC:
- c->layer[2].fec = tvp->u.data;
+ c->layer[2].fec = data;
break;
case DTV_ISDBT_LAYERC_MODULATION:
- c->layer[2].modulation = tvp->u.data;
+ c->layer[2].modulation = data;
break;
case DTV_ISDBT_LAYERC_SEGMENT_COUNT:
- c->layer[2].segment_count = tvp->u.data;
+ c->layer[2].segment_count = data;
break;
case DTV_ISDBT_LAYERC_TIME_INTERLEAVING:
- c->layer[2].interleaving = tvp->u.data;
+ c->layer[2].interleaving = data;
break;

/* Multistream support */
case DTV_STREAM_ID:
case DTV_DVBT2_PLP_ID_LEGACY:
- c->stream_id = tvp->u.data;
+ c->stream_id = data;
break;

/* ATSC-MH */
case DTV_ATSCMH_PARADE_ID:
- fe->dtv_property_cache.atscmh_parade_id = tvp->u.data;
+ fe->dtv_property_cache.atscmh_parade_id = data;
break;
case DTV_ATSCMH_RS_FRAME_ENSEMBLE:
- fe->dtv_property_cache.atscmh_rs_frame_ensemble = tvp->u.data;
+ fe->dtv_property_cache.atscmh_rs_frame_ensemble = data;
break;

case DTV_LNA:
- c->lna = tvp->u.data;
+ c->lna = data;
if (fe->ops.set_lna)
r = fe->ops.set_lna(fe);
if (r < 0)
@@ -1914,6 +1919,7 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
return r;
}

+
static int dvb_frontend_ioctl(struct file *file,
unsigned int cmd, void *parg)
{
@@ -1939,7 +1945,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;
@@ -1979,7 +1986,8 @@ static int dvb_frontend_ioctl_properties(struct file *file,
return PTR_ERR(tvp);

for (i = 0; i < tvps->num; i++) {
- err = dtv_property_process_set(fe, tvp + i, file);
+ err = dtv_property_process_set(fe, file,
+ (tvp + i)->cmd, (tvp + i)->u.data);
if (err < 0)
goto out;
(tvp + i)->result = err;
@@ -2026,7 +2034,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_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
--
2.7.4

2017-09-15 11:28:27

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH v2] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

Em Fri, 15 Sep 2017 06:18:20 -0400
Satendra Singh Thakur <[email protected]> escreveu:

> Hello Mr Chehab,
> Thanks for the comments.
> I have modified dtv_property_process_set and
> also added documentation.
> Please let me know if any further modifications required.

That's a way better. Just a few minor things to modify for it to be
ready for merging.

> Signed-off-by: Satendra Singh Thakur <[email protected]>
> ---
> .../media/uapi/dvb/fe-set-property-short.rst | 60 +++++++++
> Documentation/media/uapi/dvb/frontend_fcalls.rst | 1 +
> drivers/media/dvb-core/dvb_frontend.c | 143 +++++++++++++--------
> include/uapi/linux/dvb/frontend.h | 24 ++++
> 4 files changed, 173 insertions(+), 55 deletions(-)
> create mode 100644 Documentation/media/uapi/dvb/fe-set-property-short.rst
>
> diff --git a/Documentation/media/uapi/dvb/fe-set-property-short.rst b/Documentation/media/uapi/dvb/fe-set-property-short.rst
> new file mode 100644
> index 0000000..7da1e8d
> --- /dev/null
> +++ b/Documentation/media/uapi/dvb/fe-set-property-short.rst
> @@ -0,0 +1,60 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _FE_SET_PROPERTY_SHORT:
> +
> +**************************************
> +ioctl FE_SET_PROPERTY_SHORT
> +**************************************
> +
> +Name
> +====
> +
> +FE_SET_PROPERTY_SHORT sets one or more frontend properties.
> +
> +
> +Synopsis
> +========
> +
> +
> +.. c:function:: int ioctl( int fd, FE_SET_PROPERTY_SHORT, struct dtv_properties_short *argp )
> + :name: FE_SET_PROPERTY_SHORT
> +
> +
> +Arguments
> +=========
> +
> +``fd``
> + File descriptor returned by :ref:`open() <frontend_f_open>`.
> +
> +``argp``
> + pointer to struct :c:type:`dtv_properties_short`,
> + which is a shorter variant of struct dtv_properties.
> +
> +
> +Description
> +===========
> +
> +All DVB frontend devices support the ``FE_SET_PROPERTY_SHORT`` ioctl.
> +This ioctl is a shorter variant of ioctl FE_SET_PROPERTY.

Please use, instead:

:ref:`FE_SET_PROPERTY`

in order to generate cross-reference with the other ioctl.

Also, please add a note at fe-set-property.rst mentioning the
FE_SET_PROPERTY_SHORT variant.

Btw, I'm actually in doubt if the best wouldn't be to just add
this new ioctl to fe-get-property.rst.

> +The supported properties and statistics depend on the delivery system
> +and on the device:
> +
> +- ``FE_SET_PROPERTY_SHORT:``
> +
> + - This ioctl is used to set one or more frontend properties.
> +
> + - This is the basic command to request the frontend to tune into
> + some frequency and to start decoding the digital TV signal.
> +
> + - This call requires read/write access to the device.
> +
> + - At return, the values are updated to reflect the actual parameters
> + used.
> +
> +
> +Return Value
> +============
> +
> +On success 0 is returned, on error -1 and the ``errno`` variable is set
> +appropriately. The generic error codes are described at the
> +:ref:`Generic Error Codes <gen-errors>` chapter.
> diff --git a/Documentation/media/uapi/dvb/frontend_fcalls.rst b/Documentation/media/uapi/dvb/frontend_fcalls.rst
> index b03f9ca..a1246c6 100644
> --- a/Documentation/media/uapi/dvb/frontend_fcalls.rst
> +++ b/Documentation/media/uapi/dvb/frontend_fcalls.rst
> @@ -14,6 +14,7 @@ Frontend Function Calls
> fe-get-info
> fe-read-status
> fe-get-property
> + fe-set-property-short
> fe-diseqc-reset-overload
> fe-diseqc-send-master-cmd
> fe-diseqc-recv-slave-reply
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index e3fff8f..6616474 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1738,23 +1738,28 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
> return emulate_delivery_system(fe, delsys);
> }
>
> +/**
> + * dtv_property_process_set

Nitpick: please add a short description for the function.

> + * @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 a single dtv property
> + * It assigns property value to corresponding member of
> + * property-cache structure
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> static int dtv_property_process_set(struct dvb_frontend *fe,
> - struct dtv_property *tvp,
> - struct file *file)
> + struct file *file,
> + u32 cmd, u32 data)
> {
> int r = 0;
> struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>
> - /* Allow the frontend to validate incoming properties */
> - if (fe->ops.set_property) {
> - r = fe->ops.set_property(fe, tvp);
> - if (r < 0)
> - return r;
> - }
> -
> - dtv_property_dump(fe, true, tvp);
> -

It makes sense to be able to also debug FE_SET_FRONTEND_SHORT. As now
the we don't have a struct dtv_property pointer here anymore, we won't
be able to share this function anymore with FE_GET_FRONTEND.

So, we'll need to rename the existing function to dtv_get_property_dump(),
removing the second argument, and add here something like:

if (!cmd || cmd > DTV_MAX_COMMAND)
dev_warn(fe->dvb->device, "%s: SET cmd 0x%08x undefined\n",
__func__, cmd);
else
dev_dbg(fe->dvb->device,
"%s: SET cmd 0x%08x (%s) to 0x%08x\n",
__func__, cmd, dtv_cmds[cmd].name, data);

> - switch(tvp->cmd) {
> + switch (cmd) {
> case DTV_CLEAR:
> /*
> * Reset a cache of data specific to the frontend here. This does
> @@ -1767,140 +1772,140 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
> * tunerequest so we can pass validation in the FE_SET_FRONTEND
> * ioctl.
> */
> - c->state = tvp->cmd;
> + 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 = tvp->u.data;
> + c->frequency = data;
> break;
> case DTV_MODULATION:
> - c->modulation = tvp->u.data;
> + c->modulation = data;
> break;
> case DTV_BANDWIDTH_HZ:
> - c->bandwidth_hz = tvp->u.data;
> + c->bandwidth_hz = data;
> break;
> case DTV_INVERSION:
> - c->inversion = tvp->u.data;
> + c->inversion = data;
> break;
> case DTV_SYMBOL_RATE:
> - c->symbol_rate = tvp->u.data;
> + c->symbol_rate = data;
> break;
> case DTV_INNER_FEC:
> - c->fec_inner = tvp->u.data;
> + c->fec_inner = data;
> break;
> case DTV_PILOT:
> - c->pilot = tvp->u.data;
> + c->pilot = data;
> break;
> case DTV_ROLLOFF:
> - c->rolloff = tvp->u.data;
> + c->rolloff = data;
> break;
> case DTV_DELIVERY_SYSTEM:
> - r = dvbv5_set_delivery_system(fe, tvp->u.data);
> + r = dvbv5_set_delivery_system(fe, data);
> break;
> case DTV_VOLTAGE:
> - c->voltage = tvp->u.data;
> + c->voltage = data;
> r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
> (void *)c->voltage);
> break;
> case DTV_TONE:
> - c->sectone = tvp->u.data;
> + 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 = tvp->u.data;
> + c->code_rate_HP = data;
> break;
> case DTV_CODE_RATE_LP:
> - c->code_rate_LP = tvp->u.data;
> + c->code_rate_LP = data;
> break;
> case DTV_GUARD_INTERVAL:
> - c->guard_interval = tvp->u.data;
> + c->guard_interval = data;
> break;
> case DTV_TRANSMISSION_MODE:
> - c->transmission_mode = tvp->u.data;
> + c->transmission_mode = data;
> break;
> case DTV_HIERARCHY:
> - c->hierarchy = tvp->u.data;
> + c->hierarchy = data;
> break;
> case DTV_INTERLEAVING:
> - c->interleaving = tvp->u.data;
> + c->interleaving = data;
> break;
>
> /* ISDB-T Support here */
> case DTV_ISDBT_PARTIAL_RECEPTION:
> - c->isdbt_partial_reception = tvp->u.data;
> + c->isdbt_partial_reception = data;
> break;
> case DTV_ISDBT_SOUND_BROADCASTING:
> - c->isdbt_sb_mode = tvp->u.data;
> + c->isdbt_sb_mode = data;
> break;
> case DTV_ISDBT_SB_SUBCHANNEL_ID:
> - c->isdbt_sb_subchannel = tvp->u.data;
> + c->isdbt_sb_subchannel = data;
> break;
> case DTV_ISDBT_SB_SEGMENT_IDX:
> - c->isdbt_sb_segment_idx = tvp->u.data;
> + c->isdbt_sb_segment_idx = data;
> break;
> case DTV_ISDBT_SB_SEGMENT_COUNT:
> - c->isdbt_sb_segment_count = tvp->u.data;
> + c->isdbt_sb_segment_count = data;
> break;
> case DTV_ISDBT_LAYER_ENABLED:
> - c->isdbt_layer_enabled = tvp->u.data;
> + c->isdbt_layer_enabled = data;
> break;
> case DTV_ISDBT_LAYERA_FEC:
> - c->layer[0].fec = tvp->u.data;
> + c->layer[0].fec = data;
> break;
> case DTV_ISDBT_LAYERA_MODULATION:
> - c->layer[0].modulation = tvp->u.data;
> + c->layer[0].modulation = data;
> break;
> case DTV_ISDBT_LAYERA_SEGMENT_COUNT:
> - c->layer[0].segment_count = tvp->u.data;
> + c->layer[0].segment_count = data;
> break;
> case DTV_ISDBT_LAYERA_TIME_INTERLEAVING:
> - c->layer[0].interleaving = tvp->u.data;
> + c->layer[0].interleaving = data;
> break;
> case DTV_ISDBT_LAYERB_FEC:
> - c->layer[1].fec = tvp->u.data;
> + c->layer[1].fec = data;
> break;
> case DTV_ISDBT_LAYERB_MODULATION:
> - c->layer[1].modulation = tvp->u.data;
> + c->layer[1].modulation = data;
> break;
> case DTV_ISDBT_LAYERB_SEGMENT_COUNT:
> - c->layer[1].segment_count = tvp->u.data;
> + c->layer[1].segment_count = data;
> break;
> case DTV_ISDBT_LAYERB_TIME_INTERLEAVING:
> - c->layer[1].interleaving = tvp->u.data;
> + c->layer[1].interleaving = data;
> break;
> case DTV_ISDBT_LAYERC_FEC:
> - c->layer[2].fec = tvp->u.data;
> + c->layer[2].fec = data;
> break;
> case DTV_ISDBT_LAYERC_MODULATION:
> - c->layer[2].modulation = tvp->u.data;
> + c->layer[2].modulation = data;
> break;
> case DTV_ISDBT_LAYERC_SEGMENT_COUNT:
> - c->layer[2].segment_count = tvp->u.data;
> + c->layer[2].segment_count = data;
> break;
> case DTV_ISDBT_LAYERC_TIME_INTERLEAVING:
> - c->layer[2].interleaving = tvp->u.data;
> + c->layer[2].interleaving = data;
> break;
>
> /* Multistream support */
> case DTV_STREAM_ID:
> case DTV_DVBT2_PLP_ID_LEGACY:
> - c->stream_id = tvp->u.data;
> + c->stream_id = data;
> break;
>
> /* ATSC-MH */
> case DTV_ATSCMH_PARADE_ID:
> - fe->dtv_property_cache.atscmh_parade_id = tvp->u.data;
> + fe->dtv_property_cache.atscmh_parade_id = data;
> break;
> case DTV_ATSCMH_RS_FRAME_ENSEMBLE:
> - fe->dtv_property_cache.atscmh_rs_frame_ensemble = tvp->u.data;
> + fe->dtv_property_cache.atscmh_rs_frame_ensemble = data;
> break;
>
> case DTV_LNA:
> - c->lna = tvp->u.data;
> + c->lna = data;
> if (fe->ops.set_lna)
> r = fe->ops.set_lna(fe);
> if (r < 0)


> @@ -1914,6 +1919,7 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
> return r;
> }
>
> +
> static int dvb_frontend_ioctl(struct file *file,
> unsigned int cmd, void *parg)
> {

Nitpick: please remove this hunk. No need to add an extra blank line here.

> @@ -1939,7 +1945,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))

Nitpick: adjust alignment for the second line to:

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;
> @@ -1979,7 +1986,8 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> return PTR_ERR(tvp);
>
> for (i = 0; i < tvps->num; i++) {
> - err = dtv_property_process_set(fe, tvp + i, file);
> + err = dtv_property_process_set(fe, file,
> + (tvp + i)->cmd, (tvp + i)->u.data);

Nitpick: adjust alignment:

err = dtv_property_process_set(fe, file,
(tvp + i)->cmd,
(tvp + i)->u.data);


> if (err < 0)
> goto out;
> (tvp + i)->result = err;
> @@ -2026,7 +2034,32 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> err = -EFAULT;
> goto out;
> }
> -
> + /* New ioctl for optimizing property set
> + */

Just use a single line for the comment. The above violates Kernel
coding style, e. g.:

/* 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);

Nitpick: avoid lines with more than 80 columns. In this specific case:

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))

Adjust alignment.

> + 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_process_set(fe, file,
> + (tvp_short + i)->cmd, (tvp_short + i)->data);

Adjust alignment.

> + 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

Use &struct dtv_property, in order to generate cross-references at html/pdf
output. Also, in order to keep it coherent with other descriptions, please
an hyphen and add a blank line after description.

> + * @cmd: Property type
> + * @data: Property value

Please use the same terms as defined at struct dtv_property description.

E. g. the kernel-doc markup should be something like:

/**
* @struct dtv_property_short - A shorter version of &struct dtv_property
*
* @cmd: Digital TV command.
* @data: An unsigned 32-bits number.
*/

PS.: Don't forget to test if the produced output is ok with:

make htmldocs

you'll need to install Sphinx for it to work. If you're writing the
patches against the latest Linus tree, if you call it without having
Sphinx, a helper script will be called and will print the
commands to install it.

The output for those kernel-doc markups will be at:

Documentation/output/media/uapi/dvb/frontend-header.html

> + */
> +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
> + */

Same comments I did for the previous kernel-doc tags apply here.

> +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

2017-09-18 08:16:00

by Satendra Singh Thakur

[permalink] [raw]
Subject: [PATCH v3] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

Hello Mr Chehab,
Thanks for the review.
I have modified the code and documentation as per your comments.
Please let me know if this patch is ready for merging.

Signed-off-by: Satendra Singh Thakur <[email protected]>
---
Documentation/media/uapi/dvb/fe-get-property.rst | 24 ++-
drivers/media/dvb-core/dvb_frontend.c | 191 ++++++++++++++---------
include/uapi/linux/dvb/frontend.h | 24 +++
3 files changed, 158 insertions(+), 81 deletions(-)

diff --git a/Documentation/media/uapi/dvb/fe-get-property.rst b/Documentation/media/uapi/dvb/fe-get-property.rst
index 015d4db..b63a588 100644
--- a/Documentation/media/uapi/dvb/fe-get-property.rst
+++ b/Documentation/media/uapi/dvb/fe-get-property.rst
@@ -2,14 +2,14 @@

.. _FE_GET_PROPERTY:

-**************************************
-ioctl FE_SET_PROPERTY, FE_GET_PROPERTY
-**************************************
+**************************************************************
+ioctl FE_SET_PROPERTY, FE_GET_PROPERTY, FE_SET_PROPERTY_SHORT
+**************************************************************

Name
====

-FE_SET_PROPERTY - FE_GET_PROPERTY - FE_SET_PROPERTY sets one or more frontend properties. - FE_GET_PROPERTY returns one or more frontend properties.
+FE_SET_PROPERTY and FE_SET_PROPERTY_SHORT set one or more frontend properties. FE_GET_PROPERTY returns one or more frontend properties.


Synopsis
@@ -21,6 +21,8 @@ Synopsis
.. c:function:: int ioctl( int fd, FE_SET_PROPERTY, struct dtv_properties *argp )
:name: FE_SET_PROPERTY

+.. c:function:: int ioctl( int fd, FE_SET_PROPERTY_SHORT, struct dtv_properties_short *argp )
+ :name: FE_SET_PROPERTY_SHORT

Arguments
=========
@@ -29,15 +31,16 @@ Arguments
File descriptor returned by :ref:`open() <frontend_f_open>`.

``argp``
- pointer to struct :c:type:`dtv_properties`
+ Pointer to struct :c:type:`dtv_properties` or
+ struct :c:type:`dtv_properties_short`.


Description
===========

-All DVB frontend devices support the ``FE_SET_PROPERTY`` and
-``FE_GET_PROPERTY`` ioctls. The supported properties and statistics
-depends on the delivery system and on the device:
+All DVB frontend devices support the ``FE_SET_PROPERTY``, ``FE_GET_PROPERTY``
+and ``FE_SET_PROPERTY_SHORT`` ioctls. The supported properties and
+statistics depends on the delivery system and on the device:

- ``FE_SET_PROPERTY:``

@@ -60,6 +63,11 @@ depends on the delivery system and on the device:

- This call only requires read-only access to the device.

+- ``FE_SET_PROPERTY_SHORT:``
+
+ - This ioctl is similar to FE_SET_PROPERTY ioctl mentioned above
+ except that the arguments of the former utilize a struct :c:type:`dtv_property_short`
+ which is smaller in size.

Return Value
============
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index e3fff8f..3e49127 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1081,28 +1081,25 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 1] = {
_DTV_CMD(DTV_STAT_TOTAL_BLOCK_COUNT, 0, 0),
};

-static void dtv_property_dump(struct dvb_frontend *fe,
- bool is_set,
+static void dtv_get_property_dump(struct dvb_frontend *fe,
struct dtv_property *tvp)
{
int i;

if (tvp->cmd <= 0 || tvp->cmd > DTV_MAX_COMMAND) {
- dev_warn(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x undefined\n",
- __func__,
- is_set ? "SET" : "GET",
- tvp->cmd);
+ dev_warn(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x undefined\n"
+ , __func__,
+ tvp->cmd);
return;
}

- dev_dbg(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x (%s)\n", __func__,
- is_set ? "SET" : "GET",
- tvp->cmd,
- dtv_cmds[tvp->cmd].name);
+ dev_dbg(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x (%s)\n", __func__,
+ tvp->cmd,
+ dtv_cmds[tvp->cmd].name);

if (dtv_cmds[tvp->cmd].buffer) {
dev_dbg(fe->dvb->device, "%s: tvp.u.buffer.len = 0x%02x\n",
- __func__, tvp->u.buffer.len);
+ __func__, tvp->u.buffer.len);

for(i = 0; i < tvp->u.buffer.len; i++)
dev_dbg(fe->dvb->device,
@@ -1515,7 +1512,7 @@ static int dtv_property_process_get(struct dvb_frontend *fe,
return r;
}

- dtv_property_dump(fe, false, tvp);
+ dtv_get_property_dump(fe, tvp);

return 0;
}
@@ -1738,23 +1735,35 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
return emulate_delivery_system(fe, delsys);
}

+/**
+ * dtv_property_process_set - Sets a single DTV property
+ * @fe: Pointer to &struct dvb_frontend
+ * @file: Pointer to &struct file
+ * @cmd: Digital TV command
+ * @data: An unsigned 32-bits number
+ *
+ * This routine assigns the property
+ * value to the corresponding member of
+ * &struct dtv_frontend_properties
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
static int dtv_property_process_set(struct dvb_frontend *fe,
- struct dtv_property *tvp,
- struct file *file)
+ struct file *file,
+ u32 cmd, u32 data)
{
int r = 0;
struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-
- /* Allow the frontend to validate incoming properties */
- if (fe->ops.set_property) {
- r = fe->ops.set_property(fe, tvp);
- if (r < 0)
- return r;
- }
-
- dtv_property_dump(fe, true, tvp);
-
- switch(tvp->cmd) {
+ /** Dump DTV command name and value*/
+ if (!cmd || cmd > DTV_MAX_COMMAND)
+ dev_warn(fe->dvb->device, "%s: SET cmd 0x%08x undefined\n",
+ __func__, cmd);
+ else
+ dev_dbg(fe->dvb->device,
+ "%s: SET cmd 0x%08x (%s) to 0x%08x\n",
+ __func__, cmd, dtv_cmds[cmd].name, data);
+ switch (cmd) {
case DTV_CLEAR:
/*
* Reset a cache of data specific to the frontend here. This does
@@ -1767,140 +1776,140 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
* tunerequest so we can pass validation in the FE_SET_FRONTEND
* ioctl.
*/
- c->state = tvp->cmd;
+ 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 = tvp->u.data;
+ c->frequency = data;
break;
case DTV_MODULATION:
- c->modulation = tvp->u.data;
+ c->modulation = data;
break;
case DTV_BANDWIDTH_HZ:
- c->bandwidth_hz = tvp->u.data;
+ c->bandwidth_hz = data;
break;
case DTV_INVERSION:
- c->inversion = tvp->u.data;
+ c->inversion = data;
break;
case DTV_SYMBOL_RATE:
- c->symbol_rate = tvp->u.data;
+ c->symbol_rate = data;
break;
case DTV_INNER_FEC:
- c->fec_inner = tvp->u.data;
+ c->fec_inner = data;
break;
case DTV_PILOT:
- c->pilot = tvp->u.data;
+ c->pilot = data;
break;
case DTV_ROLLOFF:
- c->rolloff = tvp->u.data;
+ c->rolloff = data;
break;
case DTV_DELIVERY_SYSTEM:
- r = dvbv5_set_delivery_system(fe, tvp->u.data);
+ r = dvbv5_set_delivery_system(fe, data);
break;
case DTV_VOLTAGE:
- c->voltage = tvp->u.data;
+ c->voltage = data;
r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
(void *)c->voltage);
break;
case DTV_TONE:
- c->sectone = tvp->u.data;
+ 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 = tvp->u.data;
+ c->code_rate_HP = data;
break;
case DTV_CODE_RATE_LP:
- c->code_rate_LP = tvp->u.data;
+ c->code_rate_LP = data;
break;
case DTV_GUARD_INTERVAL:
- c->guard_interval = tvp->u.data;
+ c->guard_interval = data;
break;
case DTV_TRANSMISSION_MODE:
- c->transmission_mode = tvp->u.data;
+ c->transmission_mode = data;
break;
case DTV_HIERARCHY:
- c->hierarchy = tvp->u.data;
+ c->hierarchy = data;
break;
case DTV_INTERLEAVING:
- c->interleaving = tvp->u.data;
+ c->interleaving = data;
break;

/* ISDB-T Support here */
case DTV_ISDBT_PARTIAL_RECEPTION:
- c->isdbt_partial_reception = tvp->u.data;
+ c->isdbt_partial_reception = data;
break;
case DTV_ISDBT_SOUND_BROADCASTING:
- c->isdbt_sb_mode = tvp->u.data;
+ c->isdbt_sb_mode = data;
break;
case DTV_ISDBT_SB_SUBCHANNEL_ID:
- c->isdbt_sb_subchannel = tvp->u.data;
+ c->isdbt_sb_subchannel = data;
break;
case DTV_ISDBT_SB_SEGMENT_IDX:
- c->isdbt_sb_segment_idx = tvp->u.data;
+ c->isdbt_sb_segment_idx = data;
break;
case DTV_ISDBT_SB_SEGMENT_COUNT:
- c->isdbt_sb_segment_count = tvp->u.data;
+ c->isdbt_sb_segment_count = data;
break;
case DTV_ISDBT_LAYER_ENABLED:
- c->isdbt_layer_enabled = tvp->u.data;
+ c->isdbt_layer_enabled = data;
break;
case DTV_ISDBT_LAYERA_FEC:
- c->layer[0].fec = tvp->u.data;
+ c->layer[0].fec = data;
break;
case DTV_ISDBT_LAYERA_MODULATION:
- c->layer[0].modulation = tvp->u.data;
+ c->layer[0].modulation = data;
break;
case DTV_ISDBT_LAYERA_SEGMENT_COUNT:
- c->layer[0].segment_count = tvp->u.data;
+ c->layer[0].segment_count = data;
break;
case DTV_ISDBT_LAYERA_TIME_INTERLEAVING:
- c->layer[0].interleaving = tvp->u.data;
+ c->layer[0].interleaving = data;
break;
case DTV_ISDBT_LAYERB_FEC:
- c->layer[1].fec = tvp->u.data;
+ c->layer[1].fec = data;
break;
case DTV_ISDBT_LAYERB_MODULATION:
- c->layer[1].modulation = tvp->u.data;
+ c->layer[1].modulation = data;
break;
case DTV_ISDBT_LAYERB_SEGMENT_COUNT:
- c->layer[1].segment_count = tvp->u.data;
+ c->layer[1].segment_count = data;
break;
case DTV_ISDBT_LAYERB_TIME_INTERLEAVING:
- c->layer[1].interleaving = tvp->u.data;
+ c->layer[1].interleaving = data;
break;
case DTV_ISDBT_LAYERC_FEC:
- c->layer[2].fec = tvp->u.data;
+ c->layer[2].fec = data;
break;
case DTV_ISDBT_LAYERC_MODULATION:
- c->layer[2].modulation = tvp->u.data;
+ c->layer[2].modulation = data;
break;
case DTV_ISDBT_LAYERC_SEGMENT_COUNT:
- c->layer[2].segment_count = tvp->u.data;
+ c->layer[2].segment_count = data;
break;
case DTV_ISDBT_LAYERC_TIME_INTERLEAVING:
- c->layer[2].interleaving = tvp->u.data;
+ c->layer[2].interleaving = data;
break;

/* Multistream support */
case DTV_STREAM_ID:
case DTV_DVBT2_PLP_ID_LEGACY:
- c->stream_id = tvp->u.data;
+ c->stream_id = data;
break;

/* ATSC-MH */
case DTV_ATSCMH_PARADE_ID:
- fe->dtv_property_cache.atscmh_parade_id = tvp->u.data;
+ fe->dtv_property_cache.atscmh_parade_id = data;
break;
case DTV_ATSCMH_RS_FRAME_ENSEMBLE:
- fe->dtv_property_cache.atscmh_rs_frame_ensemble = tvp->u.data;
+ fe->dtv_property_cache.atscmh_rs_frame_ensemble = data;
break;

case DTV_LNA:
- c->lna = tvp->u.data;
+ c->lna = data;
if (fe->ops.set_lna)
r = fe->ops.set_lna(fe);
if (r < 0)
@@ -1939,7 +1948,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;
@@ -1966,8 +1976,10 @@ static int dvb_frontend_ioctl_properties(struct file *file,
dev_dbg(fe->dvb->device, "%s:\n", __func__);

if (cmd == FE_SET_PROPERTY) {
- dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
- dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
+ dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+ __func__, tvps->num);
+ dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+ __func__, tvps->props);

/* Put an arbitrary limit on the number of messages that can
* be sent at once */
@@ -1979,20 +1991,25 @@ static int dvb_frontend_ioctl_properties(struct file *file,
return PTR_ERR(tvp);

for (i = 0; i < tvps->num; i++) {
- err = dtv_property_process_set(fe, tvp + i, file);
+ err = dtv_property_process_set(fe, file,
+ (tvp + i)->cmd,
+ (tvp + i)->u.data);
if (err < 0)
goto out;
(tvp + i)->result = err;
}

if (c->state == DTV_TUNE)
- dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
+ dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n",
+ __func__);

} else if (cmd == FE_GET_PROPERTY) {
struct dtv_frontend_properties getp = fe->dtv_property_cache;

- dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
- dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
+ dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+ __func__, tvps->num);
+ dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+ __func__, tvps->props);

/* Put an arbitrary limit on the number of messages that can
* be sent at once */
@@ -2026,7 +2043,35 @@ 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_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..9d96dab 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: Digital TV command.
+ * @data: An unsigned 32-bits number.
+ */
+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
--
2.7.4

2017-09-18 09:36:12

by Satendra Singh Thakur

[permalink] [raw]
Subject: [PATCH v4] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

Hello Mr Chehab,
It seems that there is a mismatch among tab spacing
in local patch on my PC, the patch in email
and the patch in lkml site.
This is causing alignment problem. Even if I fix alignment problem
in my PC, alignment is different in lkml and email.
Anyway, I have run checkpatch and got 0 err and warnings
Please review it.
Thanks for the patience.

Signed-off-by: Satendra Singh Thakur <[email protected]>
---
Documentation/media/uapi/dvb/fe-get-property.rst | 24 ++-
drivers/media/dvb-core/dvb_frontend.c | 191 ++++++++++++++---------
include/uapi/linux/dvb/frontend.h | 24 +++
3 files changed, 158 insertions(+), 81 deletions(-)

diff --git a/Documentation/media/uapi/dvb/fe-get-property.rst b/Documentation/media/uapi/dvb/fe-get-property.rst
index 015d4db..b63a588 100644
--- a/Documentation/media/uapi/dvb/fe-get-property.rst
+++ b/Documentation/media/uapi/dvb/fe-get-property.rst
@@ -2,14 +2,14 @@

.. _FE_GET_PROPERTY:

-**************************************
-ioctl FE_SET_PROPERTY, FE_GET_PROPERTY
-**************************************
+**************************************************************
+ioctl FE_SET_PROPERTY, FE_GET_PROPERTY, FE_SET_PROPERTY_SHORT
+**************************************************************

Name
====

-FE_SET_PROPERTY - FE_GET_PROPERTY - FE_SET_PROPERTY sets one or more frontend properties. - FE_GET_PROPERTY returns one or more frontend properties.
+FE_SET_PROPERTY and FE_SET_PROPERTY_SHORT set one or more frontend properties. FE_GET_PROPERTY returns one or more frontend properties.


Synopsis
@@ -21,6 +21,8 @@ Synopsis
.. c:function:: int ioctl( int fd, FE_SET_PROPERTY, struct dtv_properties *argp )
:name: FE_SET_PROPERTY

+.. c:function:: int ioctl( int fd, FE_SET_PROPERTY_SHORT, struct dtv_properties_short *argp )
+ :name: FE_SET_PROPERTY_SHORT

Arguments
=========
@@ -29,15 +31,16 @@ Arguments
File descriptor returned by :ref:`open() <frontend_f_open>`.

``argp``
- pointer to struct :c:type:`dtv_properties`
+ Pointer to struct :c:type:`dtv_properties` or
+ struct :c:type:`dtv_properties_short`.


Description
===========

-All DVB frontend devices support the ``FE_SET_PROPERTY`` and
-``FE_GET_PROPERTY`` ioctls. The supported properties and statistics
-depends on the delivery system and on the device:
+All DVB frontend devices support the ``FE_SET_PROPERTY``, ``FE_GET_PROPERTY``
+and ``FE_SET_PROPERTY_SHORT`` ioctls. The supported properties and
+statistics depends on the delivery system and on the device:

- ``FE_SET_PROPERTY:``

@@ -60,6 +63,11 @@ depends on the delivery system and on the device:

- This call only requires read-only access to the device.

+- ``FE_SET_PROPERTY_SHORT:``
+
+ - This ioctl is similar to FE_SET_PROPERTY ioctl mentioned above
+ except that the arguments of the former utilize a struct :c:type:`dtv_property_short`
+ which is smaller in size.

Return Value
============
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index e3fff8f..7c96197 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1081,28 +1081,25 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 1] = {
_DTV_CMD(DTV_STAT_TOTAL_BLOCK_COUNT, 0, 0),
};

-static void dtv_property_dump(struct dvb_frontend *fe,
- bool is_set,
+static void dtv_get_property_dump(struct dvb_frontend *fe,
struct dtv_property *tvp)
{
int i;

if (tvp->cmd <= 0 || tvp->cmd > DTV_MAX_COMMAND) {
- dev_warn(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x undefined\n",
- __func__,
- is_set ? "SET" : "GET",
- tvp->cmd);
+ dev_warn(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x undefined\n"
+ , __func__,
+ tvp->cmd);
return;
}

- dev_dbg(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x (%s)\n", __func__,
- is_set ? "SET" : "GET",
- tvp->cmd,
- dtv_cmds[tvp->cmd].name);
+ dev_dbg(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x (%s)\n", __func__,
+ tvp->cmd,
+ dtv_cmds[tvp->cmd].name);

if (dtv_cmds[tvp->cmd].buffer) {
dev_dbg(fe->dvb->device, "%s: tvp.u.buffer.len = 0x%02x\n",
- __func__, tvp->u.buffer.len);
+ __func__, tvp->u.buffer.len);

for(i = 0; i < tvp->u.buffer.len; i++)
dev_dbg(fe->dvb->device,
@@ -1515,7 +1512,7 @@ static int dtv_property_process_get(struct dvb_frontend *fe,
return r;
}

- dtv_property_dump(fe, false, tvp);
+ dtv_get_property_dump(fe, tvp);

return 0;
}
@@ -1738,23 +1735,35 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
return emulate_delivery_system(fe, delsys);
}

+/**
+ * dtv_property_process_set - Sets a single DTV property
+ * @fe: Pointer to &struct dvb_frontend
+ * @file: Pointer to &struct file
+ * @cmd: Digital TV command
+ * @data: An unsigned 32-bits number
+ *
+ * This routine assigns the property
+ * value to the corresponding member of
+ * &struct dtv_frontend_properties
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
static int dtv_property_process_set(struct dvb_frontend *fe,
- struct dtv_property *tvp,
- struct file *file)
+ struct file *file,
+ u32 cmd, u32 data)
{
int r = 0;
struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-
- /* Allow the frontend to validate incoming properties */
- if (fe->ops.set_property) {
- r = fe->ops.set_property(fe, tvp);
- if (r < 0)
- return r;
- }
-
- dtv_property_dump(fe, true, tvp);
-
- switch(tvp->cmd) {
+ /** Dump DTV command name and value*/
+ if (!cmd || cmd > DTV_MAX_COMMAND)
+ dev_warn(fe->dvb->device, "%s: SET cmd 0x%08x undefined\n",
+ __func__, cmd);
+ else
+ dev_dbg(fe->dvb->device,
+ "%s: SET cmd 0x%08x (%s) to 0x%08x\n",
+ __func__, cmd, dtv_cmds[cmd].name, data);
+ switch (cmd) {
case DTV_CLEAR:
/*
* Reset a cache of data specific to the frontend here. This does
@@ -1767,140 +1776,140 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
* tunerequest so we can pass validation in the FE_SET_FRONTEND
* ioctl.
*/
- c->state = tvp->cmd;
+ 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 = tvp->u.data;
+ c->frequency = data;
break;
case DTV_MODULATION:
- c->modulation = tvp->u.data;
+ c->modulation = data;
break;
case DTV_BANDWIDTH_HZ:
- c->bandwidth_hz = tvp->u.data;
+ c->bandwidth_hz = data;
break;
case DTV_INVERSION:
- c->inversion = tvp->u.data;
+ c->inversion = data;
break;
case DTV_SYMBOL_RATE:
- c->symbol_rate = tvp->u.data;
+ c->symbol_rate = data;
break;
case DTV_INNER_FEC:
- c->fec_inner = tvp->u.data;
+ c->fec_inner = data;
break;
case DTV_PILOT:
- c->pilot = tvp->u.data;
+ c->pilot = data;
break;
case DTV_ROLLOFF:
- c->rolloff = tvp->u.data;
+ c->rolloff = data;
break;
case DTV_DELIVERY_SYSTEM:
- r = dvbv5_set_delivery_system(fe, tvp->u.data);
+ r = dvbv5_set_delivery_system(fe, data);
break;
case DTV_VOLTAGE:
- c->voltage = tvp->u.data;
+ c->voltage = data;
r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
(void *)c->voltage);
break;
case DTV_TONE:
- c->sectone = tvp->u.data;
+ 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 = tvp->u.data;
+ c->code_rate_HP = data;
break;
case DTV_CODE_RATE_LP:
- c->code_rate_LP = tvp->u.data;
+ c->code_rate_LP = data;
break;
case DTV_GUARD_INTERVAL:
- c->guard_interval = tvp->u.data;
+ c->guard_interval = data;
break;
case DTV_TRANSMISSION_MODE:
- c->transmission_mode = tvp->u.data;
+ c->transmission_mode = data;
break;
case DTV_HIERARCHY:
- c->hierarchy = tvp->u.data;
+ c->hierarchy = data;
break;
case DTV_INTERLEAVING:
- c->interleaving = tvp->u.data;
+ c->interleaving = data;
break;

/* ISDB-T Support here */
case DTV_ISDBT_PARTIAL_RECEPTION:
- c->isdbt_partial_reception = tvp->u.data;
+ c->isdbt_partial_reception = data;
break;
case DTV_ISDBT_SOUND_BROADCASTING:
- c->isdbt_sb_mode = tvp->u.data;
+ c->isdbt_sb_mode = data;
break;
case DTV_ISDBT_SB_SUBCHANNEL_ID:
- c->isdbt_sb_subchannel = tvp->u.data;
+ c->isdbt_sb_subchannel = data;
break;
case DTV_ISDBT_SB_SEGMENT_IDX:
- c->isdbt_sb_segment_idx = tvp->u.data;
+ c->isdbt_sb_segment_idx = data;
break;
case DTV_ISDBT_SB_SEGMENT_COUNT:
- c->isdbt_sb_segment_count = tvp->u.data;
+ c->isdbt_sb_segment_count = data;
break;
case DTV_ISDBT_LAYER_ENABLED:
- c->isdbt_layer_enabled = tvp->u.data;
+ c->isdbt_layer_enabled = data;
break;
case DTV_ISDBT_LAYERA_FEC:
- c->layer[0].fec = tvp->u.data;
+ c->layer[0].fec = data;
break;
case DTV_ISDBT_LAYERA_MODULATION:
- c->layer[0].modulation = tvp->u.data;
+ c->layer[0].modulation = data;
break;
case DTV_ISDBT_LAYERA_SEGMENT_COUNT:
- c->layer[0].segment_count = tvp->u.data;
+ c->layer[0].segment_count = data;
break;
case DTV_ISDBT_LAYERA_TIME_INTERLEAVING:
- c->layer[0].interleaving = tvp->u.data;
+ c->layer[0].interleaving = data;
break;
case DTV_ISDBT_LAYERB_FEC:
- c->layer[1].fec = tvp->u.data;
+ c->layer[1].fec = data;
break;
case DTV_ISDBT_LAYERB_MODULATION:
- c->layer[1].modulation = tvp->u.data;
+ c->layer[1].modulation = data;
break;
case DTV_ISDBT_LAYERB_SEGMENT_COUNT:
- c->layer[1].segment_count = tvp->u.data;
+ c->layer[1].segment_count = data;
break;
case DTV_ISDBT_LAYERB_TIME_INTERLEAVING:
- c->layer[1].interleaving = tvp->u.data;
+ c->layer[1].interleaving = data;
break;
case DTV_ISDBT_LAYERC_FEC:
- c->layer[2].fec = tvp->u.data;
+ c->layer[2].fec = data;
break;
case DTV_ISDBT_LAYERC_MODULATION:
- c->layer[2].modulation = tvp->u.data;
+ c->layer[2].modulation = data;
break;
case DTV_ISDBT_LAYERC_SEGMENT_COUNT:
- c->layer[2].segment_count = tvp->u.data;
+ c->layer[2].segment_count = data;
break;
case DTV_ISDBT_LAYERC_TIME_INTERLEAVING:
- c->layer[2].interleaving = tvp->u.data;
+ c->layer[2].interleaving = data;
break;

/* Multistream support */
case DTV_STREAM_ID:
case DTV_DVBT2_PLP_ID_LEGACY:
- c->stream_id = tvp->u.data;
+ c->stream_id = data;
break;

/* ATSC-MH */
case DTV_ATSCMH_PARADE_ID:
- fe->dtv_property_cache.atscmh_parade_id = tvp->u.data;
+ fe->dtv_property_cache.atscmh_parade_id = data;
break;
case DTV_ATSCMH_RS_FRAME_ENSEMBLE:
- fe->dtv_property_cache.atscmh_rs_frame_ensemble = tvp->u.data;
+ fe->dtv_property_cache.atscmh_rs_frame_ensemble = data;
break;

case DTV_LNA:
- c->lna = tvp->u.data;
+ c->lna = data;
if (fe->ops.set_lna)
r = fe->ops.set_lna(fe);
if (r < 0)
@@ -1939,7 +1948,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;
@@ -1966,8 +1976,10 @@ static int dvb_frontend_ioctl_properties(struct file *file,
dev_dbg(fe->dvb->device, "%s:\n", __func__);

if (cmd == FE_SET_PROPERTY) {
- dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
- dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
+ dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+ __func__, tvps->num);
+ dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+ __func__, tvps->props);

/* Put an arbitrary limit on the number of messages that can
* be sent at once */
@@ -1979,20 +1991,25 @@ static int dvb_frontend_ioctl_properties(struct file *file,
return PTR_ERR(tvp);

for (i = 0; i < tvps->num; i++) {
- err = dtv_property_process_set(fe, tvp + i, file);
+ err = dtv_property_process_set(fe, file,
+ (tvp + i)->cmd,
+ (tvp + i)->u.data);
if (err < 0)
goto out;
(tvp + i)->result = err;
}

if (c->state == DTV_TUNE)
- dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
+ dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n",
+ __func__);

} else if (cmd == FE_GET_PROPERTY) {
struct dtv_frontend_properties getp = fe->dtv_property_cache;

- dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
- dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
+ dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+ __func__, tvps->num);
+ dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+ __func__, tvps->props);

/* Put an arbitrary limit on the number of messages that can
* be sent at once */
@@ -2026,7 +2043,35 @@ 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_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..9d96dab 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: Digital TV command.
+ * @data: An unsigned 32-bits number.
+ */
+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
--
2.7.4

2017-09-19 09:33:31

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH v4] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

Em Mon, 18 Sep 2017 05:35:45 -0400
Satendra Singh Thakur <[email protected]> escreveu:

> Hello Mr Chehab,
> It seems that there is a mismatch among tab spacing
> in local patch on my PC, the patch in email
> and the patch in lkml site.
> This is causing alignment problem. Even if I fix alignment problem
> in my PC, alignment is different in lkml and email.
> Anyway, I have run checkpatch and got 0 err and warnings
> Please review it.
> Thanks for the patience.

Please, always put patch description here.

>
> Signed-off-by: Satendra Singh Thakur <[email protected]>
> ---

If you have any comments to maintainers/reviewers, add here,
after the "---" line.

> Documentation/media/uapi/dvb/fe-get-property.rst | 24 ++-
> drivers/media/dvb-core/dvb_frontend.c | 191 ++++++++++++++---------
> include/uapi/linux/dvb/frontend.h | 24 +++
> 3 files changed, 158 insertions(+), 81 deletions(-)
>
> diff --git a/Documentation/media/uapi/dvb/fe-get-property.rst b/Documentation/media/uapi/dvb/fe-get-property.rst
> index 015d4db..b63a588 100644
> --- a/Documentation/media/uapi/dvb/fe-get-property.rst
> +++ b/Documentation/media/uapi/dvb/fe-get-property.rst
> @@ -2,14 +2,14 @@
>
> .. _FE_GET_PROPERTY:
>
> -**************************************
> -ioctl FE_SET_PROPERTY, FE_GET_PROPERTY
> -**************************************
> +**************************************************************
> +ioctl FE_SET_PROPERTY, FE_GET_PROPERTY, FE_SET_PROPERTY_SHORT
> +**************************************************************
>
> Name
> ====
>
> -FE_SET_PROPERTY - FE_GET_PROPERTY - FE_SET_PROPERTY sets one or more frontend properties. - FE_GET_PROPERTY returns one or more frontend properties.
> +FE_SET_PROPERTY and FE_SET_PROPERTY_SHORT set one or more frontend properties. FE_GET_PROPERTY returns one or more frontend properties.
>
>
> Synopsis
> @@ -21,6 +21,8 @@ Synopsis
> .. c:function:: int ioctl( int fd, FE_SET_PROPERTY, struct dtv_properties *argp )
> :name: FE_SET_PROPERTY
>
> +.. c:function:: int ioctl( int fd, FE_SET_PROPERTY_SHORT, struct dtv_properties_short *argp )
> + :name: FE_SET_PROPERTY_SHORT
>
> Arguments
> =========
> @@ -29,15 +31,16 @@ Arguments
> File descriptor returned by :ref:`open() <frontend_f_open>`.
>
> ``argp``
> - pointer to struct :c:type:`dtv_properties`
> + Pointer to struct :c:type:`dtv_properties` or
> + struct :c:type:`dtv_properties_short`.
>
>
> Description
> ===========
>
> -All DVB frontend devices support the ``FE_SET_PROPERTY`` and
> -``FE_GET_PROPERTY`` ioctls. The supported properties and statistics
> -depends on the delivery system and on the device:
> +All DVB frontend devices support the ``FE_SET_PROPERTY``, ``FE_GET_PROPERTY``
> +and ``FE_SET_PROPERTY_SHORT`` ioctls. The supported properties and
> +statistics depends on the delivery system and on the device:
>
> - ``FE_SET_PROPERTY:``
>
> @@ -60,6 +63,11 @@ depends on the delivery system and on the device:
>
> - This call only requires read-only access to the device.
>
> +- ``FE_SET_PROPERTY_SHORT:``
> +
> + - This ioctl is similar to FE_SET_PROPERTY ioctl mentioned above
> + except that the arguments of the former utilize a struct :c:type:`dtv_property_short`
> + which is smaller in size.
>
> Return Value
> ============

I'm still in doubt about something: what's the real requirement from userspace
to justify a new ioctl?

Userspace will still need to allocate the same memory in order to get back
the tuner parameters with FE_GET_PROPERTY and to get statistics. Also, tuning
happens just once per channel, so hardly this would bring *any* real
performance improvement while tuning into a channel.

> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index e3fff8f..7c96197 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1081,28 +1081,25 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 1] = {
> _DTV_CMD(DTV_STAT_TOTAL_BLOCK_COUNT, 0, 0),
> };
>
> -static void dtv_property_dump(struct dvb_frontend *fe,
> - bool is_set,
> +static void dtv_get_property_dump(struct dvb_frontend *fe,
> struct dtv_property *tvp)
> {
> int i;
>
> if (tvp->cmd <= 0 || tvp->cmd > DTV_MAX_COMMAND) {
> - dev_warn(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x undefined\n",
> - __func__,
> - is_set ? "SET" : "GET",
> - tvp->cmd);
> + dev_warn(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x undefined\n"
> + , __func__,
> + tvp->cmd);
> return;
> }
>
> - dev_dbg(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x (%s)\n", __func__,
> - is_set ? "SET" : "GET",
> - tvp->cmd,
> - dtv_cmds[tvp->cmd].name);
> + dev_dbg(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x (%s)\n", __func__,
> + tvp->cmd,
> + dtv_cmds[tvp->cmd].name);
>
> if (dtv_cmds[tvp->cmd].buffer) {
> dev_dbg(fe->dvb->device, "%s: tvp.u.buffer.len = 0x%02x\n",
> - __func__, tvp->u.buffer.len);
> + __func__, tvp->u.buffer.len);
>
> for(i = 0; i < tvp->u.buffer.len; i++)
> dev_dbg(fe->dvb->device,
> @@ -1515,7 +1512,7 @@ static int dtv_property_process_get(struct dvb_frontend *fe,
> return r;
> }
>
> - dtv_property_dump(fe, false, tvp);
> + dtv_get_property_dump(fe, tvp);
>
> return 0;
> }
> @@ -1738,23 +1735,35 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
> return emulate_delivery_system(fe, delsys);
> }
>
> +/**
> + * dtv_property_process_set - Sets a single DTV property
> + * @fe: Pointer to &struct dvb_frontend
> + * @file: Pointer to &struct file
> + * @cmd: Digital TV command
> + * @data: An unsigned 32-bits number
> + *
> + * This routine assigns the property
> + * value to the corresponding member of
> + * &struct dtv_frontend_properties
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> static int dtv_property_process_set(struct dvb_frontend *fe,
> - struct dtv_property *tvp,
> - struct file *file)
> + struct file *file,
> + u32 cmd, u32 data)
> {
> int r = 0;
> struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> -
> - /* Allow the frontend to validate incoming properties */
> - if (fe->ops.set_property) {
> - r = fe->ops.set_property(fe, tvp);
> - if (r < 0)
> - return r;
> - }
> -
> - dtv_property_dump(fe, true, tvp);
> -
> - switch(tvp->cmd) {
> + /** Dump DTV command name and value*/
> + if (!cmd || cmd > DTV_MAX_COMMAND)
> + dev_warn(fe->dvb->device, "%s: SET cmd 0x%08x undefined\n",
> + __func__, cmd);
> + else
> + dev_dbg(fe->dvb->device,
> + "%s: SET cmd 0x%08x (%s) to 0x%08x\n",
> + __func__, cmd, dtv_cmds[cmd].name, data);
> + switch (cmd) {
> case DTV_CLEAR:
> /*
> * Reset a cache of data specific to the frontend here. This does
> @@ -1767,140 +1776,140 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
> * tunerequest so we can pass validation in the FE_SET_FRONTEND
> * ioctl.
> */
> - c->state = tvp->cmd;
> + 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 = tvp->u.data;
> + c->frequency = data;
> break;
> case DTV_MODULATION:
> - c->modulation = tvp->u.data;
> + c->modulation = data;
> break;
> case DTV_BANDWIDTH_HZ:
> - c->bandwidth_hz = tvp->u.data;
> + c->bandwidth_hz = data;
> break;
> case DTV_INVERSION:
> - c->inversion = tvp->u.data;
> + c->inversion = data;
> break;
> case DTV_SYMBOL_RATE:
> - c->symbol_rate = tvp->u.data;
> + c->symbol_rate = data;
> break;
> case DTV_INNER_FEC:
> - c->fec_inner = tvp->u.data;
> + c->fec_inner = data;
> break;
> case DTV_PILOT:
> - c->pilot = tvp->u.data;
> + c->pilot = data;
> break;
> case DTV_ROLLOFF:
> - c->rolloff = tvp->u.data;
> + c->rolloff = data;
> break;
> case DTV_DELIVERY_SYSTEM:
> - r = dvbv5_set_delivery_system(fe, tvp->u.data);
> + r = dvbv5_set_delivery_system(fe, data);
> break;
> case DTV_VOLTAGE:
> - c->voltage = tvp->u.data;
> + c->voltage = data;
> r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
> (void *)c->voltage);
> break;
> case DTV_TONE:
> - c->sectone = tvp->u.data;
> + 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 = tvp->u.data;
> + c->code_rate_HP = data;
> break;
> case DTV_CODE_RATE_LP:
> - c->code_rate_LP = tvp->u.data;
> + c->code_rate_LP = data;
> break;
> case DTV_GUARD_INTERVAL:
> - c->guard_interval = tvp->u.data;
> + c->guard_interval = data;
> break;
> case DTV_TRANSMISSION_MODE:
> - c->transmission_mode = tvp->u.data;
> + c->transmission_mode = data;
> break;
> case DTV_HIERARCHY:
> - c->hierarchy = tvp->u.data;
> + c->hierarchy = data;
> break;
> case DTV_INTERLEAVING:
> - c->interleaving = tvp->u.data;
> + c->interleaving = data;
> break;
>
> /* ISDB-T Support here */
> case DTV_ISDBT_PARTIAL_RECEPTION:
> - c->isdbt_partial_reception = tvp->u.data;
> + c->isdbt_partial_reception = data;
> break;
> case DTV_ISDBT_SOUND_BROADCASTING:
> - c->isdbt_sb_mode = tvp->u.data;
> + c->isdbt_sb_mode = data;
> break;
> case DTV_ISDBT_SB_SUBCHANNEL_ID:
> - c->isdbt_sb_subchannel = tvp->u.data;
> + c->isdbt_sb_subchannel = data;
> break;
> case DTV_ISDBT_SB_SEGMENT_IDX:
> - c->isdbt_sb_segment_idx = tvp->u.data;
> + c->isdbt_sb_segment_idx = data;
> break;
> case DTV_ISDBT_SB_SEGMENT_COUNT:
> - c->isdbt_sb_segment_count = tvp->u.data;
> + c->isdbt_sb_segment_count = data;
> break;
> case DTV_ISDBT_LAYER_ENABLED:
> - c->isdbt_layer_enabled = tvp->u.data;
> + c->isdbt_layer_enabled = data;
> break;
> case DTV_ISDBT_LAYERA_FEC:
> - c->layer[0].fec = tvp->u.data;
> + c->layer[0].fec = data;
> break;
> case DTV_ISDBT_LAYERA_MODULATION:
> - c->layer[0].modulation = tvp->u.data;
> + c->layer[0].modulation = data;
> break;
> case DTV_ISDBT_LAYERA_SEGMENT_COUNT:
> - c->layer[0].segment_count = tvp->u.data;
> + c->layer[0].segment_count = data;
> break;
> case DTV_ISDBT_LAYERA_TIME_INTERLEAVING:
> - c->layer[0].interleaving = tvp->u.data;
> + c->layer[0].interleaving = data;
> break;
> case DTV_ISDBT_LAYERB_FEC:
> - c->layer[1].fec = tvp->u.data;
> + c->layer[1].fec = data;
> break;
> case DTV_ISDBT_LAYERB_MODULATION:
> - c->layer[1].modulation = tvp->u.data;
> + c->layer[1].modulation = data;
> break;
> case DTV_ISDBT_LAYERB_SEGMENT_COUNT:
> - c->layer[1].segment_count = tvp->u.data;
> + c->layer[1].segment_count = data;
> break;
> case DTV_ISDBT_LAYERB_TIME_INTERLEAVING:
> - c->layer[1].interleaving = tvp->u.data;
> + c->layer[1].interleaving = data;
> break;
> case DTV_ISDBT_LAYERC_FEC:
> - c->layer[2].fec = tvp->u.data;
> + c->layer[2].fec = data;
> break;
> case DTV_ISDBT_LAYERC_MODULATION:
> - c->layer[2].modulation = tvp->u.data;
> + c->layer[2].modulation = data;
> break;
> case DTV_ISDBT_LAYERC_SEGMENT_COUNT:
> - c->layer[2].segment_count = tvp->u.data;
> + c->layer[2].segment_count = data;
> break;
> case DTV_ISDBT_LAYERC_TIME_INTERLEAVING:
> - c->layer[2].interleaving = tvp->u.data;
> + c->layer[2].interleaving = data;
> break;
>
> /* Multistream support */
> case DTV_STREAM_ID:
> case DTV_DVBT2_PLP_ID_LEGACY:
> - c->stream_id = tvp->u.data;
> + c->stream_id = data;
> break;
>
> /* ATSC-MH */
> case DTV_ATSCMH_PARADE_ID:
> - fe->dtv_property_cache.atscmh_parade_id = tvp->u.data;
> + fe->dtv_property_cache.atscmh_parade_id = data;
> break;
> case DTV_ATSCMH_RS_FRAME_ENSEMBLE:
> - fe->dtv_property_cache.atscmh_rs_frame_ensemble = tvp->u.data;
> + fe->dtv_property_cache.atscmh_rs_frame_ensemble = data;
> break;
>
> case DTV_LNA:
> - c->lna = tvp->u.data;
> + c->lna = data;
> if (fe->ops.set_lna)
> r = fe->ops.set_lna(fe);
> if (r < 0)
> @@ -1939,7 +1948,8 @@ static int dvb_frontend_ioctl(struct file *file,
> return -EPERM;
> }

The above optimization seem OK. Still, it is better to place it on
a separate patch.

>
> - 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;

Assuming that you provide a good reason why we need this ioctl, this
should be on the second patch with the new ioctl.

> @@ -1966,8 +1976,10 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> dev_dbg(fe->dvb->device, "%s:\n", __func__);
>
> if (cmd == FE_SET_PROPERTY) {
> - dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
> - dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
> + dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> + __func__, tvps->num);
> + dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> + __func__, tvps->props);
>
> /* Put an arbitrary limit on the number of messages that can
> * be sent at once */
> @@ -1979,20 +1991,25 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> return PTR_ERR(tvp);
>
> for (i = 0; i < tvps->num; i++) {
> - err = dtv_property_process_set(fe, tvp + i, file);
> + err = dtv_property_process_set(fe, file,
> + (tvp + i)->cmd,
> + (tvp + i)->u.data);
> if (err < 0)
> goto out;
> (tvp + i)->result = err;
> }
>
> if (c->state == DTV_TUNE)
> - dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
> + dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n",
> + __func__);
>
> } else if (cmd == FE_GET_PROPERTY) {
> struct dtv_frontend_properties getp = fe->dtv_property_cache;
>
> - dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
> - dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
> + dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> + __func__, tvps->num);
> + dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> + __func__, tvps->props);
>
> /* Put an arbitrary limit on the number of messages that can
> * be sent at once */
> @@ -2026,7 +2043,35 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> err = -EFAULT;
> goto out;
> }

Indentation is still wrong. Are you setting tab to 8?

> -
> + /* 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_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);

Place the new ioctl code on the second patch.

> } else
> err = -EOPNOTSUPP;
>
> diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
> index 00a20cd..9d96dab 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: Digital TV command.
> + * @data: An unsigned 32-bits number.
> + */
> +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

Place the new ioctl code on the second patch.


Thanks,
Mauro

2017-09-20 05:15:45

by Satendra Singh Thakur

[permalink] [raw]
Subject: [PATCH v5] [DVB][FRONTEND] Modified the code related to FE_SET_PROPERTY ioctl

-Since all the properties in the func dtv_property_process_set
are 4 bytes, We have added 2 new arguments u32 cmd and u32 data;
in place of older argument struct dtv_property *tvp.
-We have removed property validation in function
dtv_property_process_set because most of the frontend drivers in
linux source are not using the method ops.set_property.
-In place of dtv_property_dump, we have added own logic
to dump cmd and data of the incoming property
-We have also re-named dtv_property_dump to dtv_get_property_dump.
-We have modified logs in this func to be suitable only for getting
properties.
Signed-off-by: Satendra Singh Thakur <[email protected]>
---
drivers/media/dvb-core/dvb_frontend.c | 135 ++++++++++++++++++----------------
1 file changed, 73 insertions(+), 62 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 2fcba16..013476e 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1092,22 +1092,19 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 1] = {
_DTV_CMD(DTV_STAT_TOTAL_BLOCK_COUNT, 0, 0),
};

-static void dtv_property_dump(struct dvb_frontend *fe,
- bool is_set,
+static void dtv_get_property_dump(struct dvb_frontend *fe,
struct dtv_property *tvp)
{
int i;

if (tvp->cmd <= 0 || tvp->cmd > DTV_MAX_COMMAND) {
- dev_warn(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x undefined\n",
- __func__,
- is_set ? "SET" : "GET",
+ dev_warn(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x undefined\n"
+ , __func__,
tvp->cmd);
return;
}

- dev_dbg(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x (%s)\n", __func__,
- is_set ? "SET" : "GET",
+ dev_dbg(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x (%s)\n", __func__,
tvp->cmd,
dtv_cmds[tvp->cmd].name);

@@ -1526,7 +1523,7 @@ static int dtv_property_process_get(struct dvb_frontend *fe,
return r;
}

- dtv_property_dump(fe, false, tvp);
+ dtv_get_property_dump(fe, tvp);

return 0;
}
@@ -1749,23 +1746,35 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
return emulate_delivery_system(fe, delsys);
}

+/**
+ * dtv_property_process_set - Sets a single DTV property
+ * @fe: Pointer to &struct dvb_frontend
+ * @file: Pointer to &struct file
+ * @cmd: Digital TV command
+ * @data: An unsigned 32-bits number
+ *
+ * This routine assigns the property
+ * value to the corresponding member of
+ * &struct dtv_frontend_properties
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
static int dtv_property_process_set(struct dvb_frontend *fe,
- struct dtv_property *tvp,
- struct file *file)
+ struct file *file,
+ u32 cmd, u32 data)
{
int r = 0;
struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-
- /* Allow the frontend to validate incoming properties */
- if (fe->ops.set_property) {
- r = fe->ops.set_property(fe, tvp);
- if (r < 0)
- return r;
- }
-
- dtv_property_dump(fe, true, tvp);
-
- switch(tvp->cmd) {
+ /** Dump DTV command name and value*/
+ if (!cmd || cmd > DTV_MAX_COMMAND)
+ dev_warn(fe->dvb->device, "%s: SET cmd 0x%08x undefined\n",
+ __func__, cmd);
+ else
+ dev_dbg(fe->dvb->device,
+ "%s: SET cmd 0x%08x (%s) to 0x%08x\n",
+ __func__, cmd, dtv_cmds[cmd].name, data);
+ switch (cmd) {
case DTV_CLEAR:
/*
* Reset a cache of data specific to the frontend here. This does
@@ -1778,140 +1787,140 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
* tunerequest so we can pass validation in the FE_SET_FRONTEND
* ioctl.
*/
- c->state = tvp->cmd;
+ 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 = tvp->u.data;
+ c->frequency = data;
break;
case DTV_MODULATION:
- c->modulation = tvp->u.data;
+ c->modulation = data;
break;
case DTV_BANDWIDTH_HZ:
- c->bandwidth_hz = tvp->u.data;
+ c->bandwidth_hz = data;
break;
case DTV_INVERSION:
- c->inversion = tvp->u.data;
+ c->inversion = data;
break;
case DTV_SYMBOL_RATE:
- c->symbol_rate = tvp->u.data;
+ c->symbol_rate = data;
break;
case DTV_INNER_FEC:
- c->fec_inner = tvp->u.data;
+ c->fec_inner = data;
break;
case DTV_PILOT:
- c->pilot = tvp->u.data;
+ c->pilot = data;
break;
case DTV_ROLLOFF:
- c->rolloff = tvp->u.data;
+ c->rolloff = data;
break;
case DTV_DELIVERY_SYSTEM:
- r = dvbv5_set_delivery_system(fe, tvp->u.data);
+ r = dvbv5_set_delivery_system(fe, data);
break;
case DTV_VOLTAGE:
- c->voltage = tvp->u.data;
+ c->voltage = data;
r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
(void *)c->voltage);
break;
case DTV_TONE:
- c->sectone = tvp->u.data;
+ 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 = tvp->u.data;
+ c->code_rate_HP = data;
break;
case DTV_CODE_RATE_LP:
- c->code_rate_LP = tvp->u.data;
+ c->code_rate_LP = data;
break;
case DTV_GUARD_INTERVAL:
- c->guard_interval = tvp->u.data;
+ c->guard_interval = data;
break;
case DTV_TRANSMISSION_MODE:
- c->transmission_mode = tvp->u.data;
+ c->transmission_mode = data;
break;
case DTV_HIERARCHY:
- c->hierarchy = tvp->u.data;
+ c->hierarchy = data;
break;
case DTV_INTERLEAVING:
- c->interleaving = tvp->u.data;
+ c->interleaving = data;
break;

/* ISDB-T Support here */
case DTV_ISDBT_PARTIAL_RECEPTION:
- c->isdbt_partial_reception = tvp->u.data;
+ c->isdbt_partial_reception = data;
break;
case DTV_ISDBT_SOUND_BROADCASTING:
- c->isdbt_sb_mode = tvp->u.data;
+ c->isdbt_sb_mode = data;
break;
case DTV_ISDBT_SB_SUBCHANNEL_ID:
- c->isdbt_sb_subchannel = tvp->u.data;
+ c->isdbt_sb_subchannel = data;
break;
case DTV_ISDBT_SB_SEGMENT_IDX:
- c->isdbt_sb_segment_idx = tvp->u.data;
+ c->isdbt_sb_segment_idx = data;
break;
case DTV_ISDBT_SB_SEGMENT_COUNT:
- c->isdbt_sb_segment_count = tvp->u.data;
+ c->isdbt_sb_segment_count = data;
break;
case DTV_ISDBT_LAYER_ENABLED:
- c->isdbt_layer_enabled = tvp->u.data;
+ c->isdbt_layer_enabled = data;
break;
case DTV_ISDBT_LAYERA_FEC:
- c->layer[0].fec = tvp->u.data;
+ c->layer[0].fec = data;
break;
case DTV_ISDBT_LAYERA_MODULATION:
- c->layer[0].modulation = tvp->u.data;
+ c->layer[0].modulation = data;
break;
case DTV_ISDBT_LAYERA_SEGMENT_COUNT:
- c->layer[0].segment_count = tvp->u.data;
+ c->layer[0].segment_count = data;
break;
case DTV_ISDBT_LAYERA_TIME_INTERLEAVING:
- c->layer[0].interleaving = tvp->u.data;
+ c->layer[0].interleaving = data;
break;
case DTV_ISDBT_LAYERB_FEC:
- c->layer[1].fec = tvp->u.data;
+ c->layer[1].fec = data;
break;
case DTV_ISDBT_LAYERB_MODULATION:
- c->layer[1].modulation = tvp->u.data;
+ c->layer[1].modulation = data;
break;
case DTV_ISDBT_LAYERB_SEGMENT_COUNT:
- c->layer[1].segment_count = tvp->u.data;
+ c->layer[1].segment_count = data;
break;
case DTV_ISDBT_LAYERB_TIME_INTERLEAVING:
- c->layer[1].interleaving = tvp->u.data;
+ c->layer[1].interleaving = data;
break;
case DTV_ISDBT_LAYERC_FEC:
- c->layer[2].fec = tvp->u.data;
+ c->layer[2].fec = data;
break;
case DTV_ISDBT_LAYERC_MODULATION:
- c->layer[2].modulation = tvp->u.data;
+ c->layer[2].modulation = data;
break;
case DTV_ISDBT_LAYERC_SEGMENT_COUNT:
- c->layer[2].segment_count = tvp->u.data;
+ c->layer[2].segment_count = data;
break;
case DTV_ISDBT_LAYERC_TIME_INTERLEAVING:
- c->layer[2].interleaving = tvp->u.data;
+ c->layer[2].interleaving = data;
break;

/* Multistream support */
case DTV_STREAM_ID:
case DTV_DVBT2_PLP_ID_LEGACY:
- c->stream_id = tvp->u.data;
+ c->stream_id = data;
break;

/* ATSC-MH */
case DTV_ATSCMH_PARADE_ID:
- fe->dtv_property_cache.atscmh_parade_id = tvp->u.data;
+ fe->dtv_property_cache.atscmh_parade_id = data;
break;
case DTV_ATSCMH_RS_FRAME_ENSEMBLE:
- fe->dtv_property_cache.atscmh_rs_frame_ensemble = tvp->u.data;
+ fe->dtv_property_cache.atscmh_rs_frame_ensemble = data;
break;

case DTV_LNA:
- c->lna = tvp->u.data;
+ c->lna = data;
if (fe->ops.set_lna)
r = fe->ops.set_lna(fe);
if (r < 0)
@@ -1990,7 +1999,9 @@ static int dvb_frontend_ioctl_properties(struct file *file,
return PTR_ERR(tvp);

for (i = 0; i < tvps->num; i++) {
- err = dtv_property_process_set(fe, tvp + i, file);
+ err = dtv_property_process_set(fe, file,
+ (tvp + i)->cmd,
+ (tvp + i)->u.data);
if (err < 0)
goto out;
(tvp + i)->result = err;
--
2.7.4

2017-09-20 09:50:42

by Satendra Singh Thakur

[permalink] [raw]
Subject: [PATCH v5 2/2] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

-For setting one frontend property , one FE_SET_PROPERTY ioctl is called
-Since, size of struct dtv_property is 72 bytes, this ioctl requires
---allocating 72 bytes of memory in user space
---allocating 72 bytes of memory in kernel space
---copying 72 bytes of data from user space to kernel space
-However, for all the properties, only 8 out of 72 bytes are used
for setting the property
-Four bytes are needed for specifying property type and another 4 for
property value
-Moreover, there are 2 properties DTV_CLEAR and DTV_TUNE which use
only 4 bytes for property name
---They don't use property value
-Therefore, we have defined new short variant/forms/version of currently
used structures for such 8 byte properties.
-This results in 89% (8*100/72) of memory saving in user and kernel space
each.
-This also results in faster copy (8 bytes as compared to 72 bytes) from
user to kernel space
-We have added new ioctl FE_SET_PROPERTY_SHORT which utilizes above
mentioned new property structures
-This ioctl can co-exist with present ioctl FE_SET_PROPERTY
-If the apps wish to use shorter forms they can use
proposed FE_SET_PROPERTY_SHORT, rest of them can continue to use
current versions FE_SET_PROPERTY

Signed-off-by: Satendra Singh Thakur <[email protected]>
---
Documentation/media/uapi/dvb/fe-get-property.rst | 24 +++++++++++------
drivers/media/dvb-core/dvb_frontend.c | 33 +++++++++++++++++++++++-
include/uapi/linux/dvb/frontend.h | 24 +++++++++++++++++
3 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/Documentation/media/uapi/dvb/fe-get-property.rst b/Documentation/media/uapi/dvb/fe-get-property.rst
index 948d2ba..efe95af 100644
--- a/Documentation/media/uapi/dvb/fe-get-property.rst
+++ b/Documentation/media/uapi/dvb/fe-get-property.rst
@@ -2,14 +2,14 @@

.. _FE_GET_PROPERTY:

-**************************************
-ioctl FE_SET_PROPERTY, FE_GET_PROPERTY
-**************************************
+**************************************************************
+ioctl FE_SET_PROPERTY, FE_GET_PROPERTY, FE_SET_PROPERTY_SHORT
+**************************************************************

Name
====

-FE_SET_PROPERTY - FE_GET_PROPERTY - FE_SET_PROPERTY sets one or more frontend properties. - FE_GET_PROPERTY returns one or more frontend properties.
+FE_SET_PROPERTY and FE_SET_PROPERTY_SHORT set one or more frontend properties. FE_GET_PROPERTY returns one or more frontend properties.


Synopsis
@@ -21,6 +21,8 @@ Synopsis
.. c:function:: int ioctl( int fd, FE_SET_PROPERTY, struct dtv_properties *argp )
:name: FE_SET_PROPERTY

+.. c:function:: int ioctl( int fd, FE_SET_PROPERTY_SHORT, struct dtv_properties_short *argp )
+ :name: FE_SET_PROPERTY_SHORT

Arguments
=========
@@ -29,15 +31,16 @@ Arguments
File descriptor returned by :ref:`open() <frontend_f_open>`.

``argp``
- Pointer to struct :c:type:`dtv_properties`.
+ Pointer to struct :c:type:`dtv_properties` or
+ struct :c:type:`dtv_properties_short`.


Description
===========

-All Digital TV frontend devices support the ``FE_SET_PROPERTY`` and
-``FE_GET_PROPERTY`` ioctls. The supported properties and statistics
-depends on the delivery system and on the device:
+All Digital TV frontend devices support the ``FE_SET_PROPERTY``, ``FE_GET_PROPERTY``
+and ``FE_SET_PROPERTY_SHORT`` ioctls. The supported properties and
+statistics depends on the delivery system and on the device:

- ``FE_SET_PROPERTY:``

@@ -60,6 +63,11 @@ depends on the delivery system and on the device:

- This call only requires read-only access to the device.

+- ``FE_SET_PROPERTY_SHORT:``
+
+ - This ioctl is similar to FE_SET_PROPERTY ioctl mentioned above
+ except that the arguments of the former utilize a struct :c:type:`dtv_property_short`
+ which is smaller in size.

Return Value
============
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 2fcba16..a6a5340 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1925,6 +1925,7 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
return r;
}

+
static int dvb_frontend_ioctl(struct file *file,
unsigned int cmd, void *parg)
{
@@ -1950,7 +1951,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;
@@ -2037,6 +2039,35 @@ 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_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 861cacd..dedee0d 100644
--- a/include/uapi/linux/dvb/frontend.h
+++ b/include/uapi/linux/dvb/frontend.h
@@ -857,6 +857,17 @@ struct dtv_property {
int result;
} __attribute__ ((packed));

+/**
+ * struct dtv_property_short - A shorter version of &struct dtv_property
+ *
+ * @cmd: Digital TV command.
+ * @data: An unsigned 32-bits number.
+ */
+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

@@ -871,6 +882,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;
+};
+
/*
* When set, this flag will disable any zigzagging or other "normal" tuning
* behavior. Additionally, there will be no automatic monitoring of the lock
@@ -906,6 +929,7 @@ struct dtv_properties {

#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)

#if defined(__DVB_CORE__) || !defined(__KERNEL__)

--
2.7.4