* [RFC] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation [not found] <CGME20170914095941epcas5p3520a04d543890249b4952fea48747276@epcas5p3.samsung.com> @ 2017-09-14 9:59 ` Satendra Singh Thakur 2017-09-14 20:50 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 12+ messages in thread From: Satendra Singh Thakur @ 2017-09-14 9:59 UTC (permalink / raw) To: mchehab, max.kellermann, sakari.ailus, mingo, hans.verkuil, yamada.masahiro, shuah Cc: satendra.t, linux-media, linux-kernel, taeyoung0432.lee, jackee.lee, hemanshu.s, p.awasthi, siddharth.s, madhur.verma -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 <satendra.t@samsung.com> --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-14 9:59 ` [RFC] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation Satendra Singh Thakur @ 2017-09-14 20:50 ` Mauro Carvalho Chehab 2017-09-15 5:50 ` [PATCH v1] " Satendra Singh Thakur 2017-09-15 8:28 ` [RFC] " Honza Petrouš 0 siblings, 2 replies; 12+ messages in thread From: Mauro Carvalho Chehab @ 2017-09-14 20:50 UTC (permalink / raw) To: Satendra Singh Thakur Cc: mchehab, max.kellermann, sakari.ailus, mingo, hans.verkuil, yamada.masahiro, shuah, linux-media, linux-kernel, taeyoung0432.lee, jackee.lee, hemanshu.s, p.awasthi, siddharth.s, madhur.verma Hi Satendra, Em Thu, 14 Sep 2017 05:59:27 -0400 Satendra Singh Thakur <satendra.t@samsung.com> 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 <satendra.t@samsung.com> > --- > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-14 20:50 ` Mauro Carvalho Chehab @ 2017-09-15 5:50 ` Satendra Singh Thakur 2017-09-15 8:49 ` Mauro Carvalho Chehab 2017-09-15 8:28 ` [RFC] " Honza Petrouš 1 sibling, 1 reply; 12+ messages in thread From: Satendra Singh Thakur @ 2017-09-15 5:50 UTC (permalink / raw) To: mchehab, max.kellermann, sakari.ailus, mingo, hans.verkuil, yamada.masahiro, shuah Cc: satendra.t, linux-media, linux-kernel, taeyoung0432.lee, jackee.lee, hemanshu.s, madhur.verma 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 <satendra.t@samsung.com> --- drivers/media/dvb-core/dvb_frontend.c | 212 +++++++++++++++++++++++++++++++++- include/uapi/linux/dvb/frontend.h | 24 ++++ 2 files changed, 234 insertions(+), 2 deletions(-) diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index e3fff8f..bc35a86 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -1914,6 +1914,188 @@ static int dtv_property_process_set(struct dvb_frontend *fe, return r; } +/** + * dtv_property_short_process_set + * @fe: Pointer to struct dvb_frontend + * @file: Pointer to struct file + * @cmd: Property name + * @data: Property value + * + * helper function for dvb_frontend_ioctl_properties, + * which can be used to set dtv property using ioctl + * cmd FE_SET_PROPERTY_SHORT. + * It assigns property value to corresponding member of + * property-cache structure + * This func is a variant of the func dtv_property_process_set + * Returns: + * Zero on success, negative errno on failure. + */ +static int dtv_property_short_process_set(struct dvb_frontend *fe, + struct file *file, + u32 cmd, u32 data) +{ + int r = 0; + struct dtv_frontend_properties *c = &fe->dtv_property_cache; + switch (cmd) { + case DTV_CLEAR: + /* + * Reset a cache of data specific to the frontend here. This + * does not effect hardware. + */ + dvb_frontend_clear_cache(fe); + break; + 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-15 5:50 ` [PATCH v1] " Satendra Singh Thakur @ 2017-09-15 8:49 ` Mauro Carvalho Chehab 2017-09-15 10:18 ` [PATCH v2] " Satendra Singh Thakur 0 siblings, 1 reply; 12+ messages in thread From: Mauro Carvalho Chehab @ 2017-09-15 8:49 UTC (permalink / raw) To: Satendra Singh Thakur Cc: mchehab, max.kellermann, sakari.ailus, mingo, hans.verkuil, yamada.masahiro, shuah, linux-media, linux-kernel, taeyoung0432.lee, jackee.lee, hemanshu.s, madhur.verma Em Fri, 15 Sep 2017 01:50:29 -0400 Satendra Singh Thakur <satendra.t@samsung.com> escreveu: > Hello Mr Chehab, > Thanks for reviewing the patch. > I have modified the patch as per your comments. > Please check if it looks fine now. Forgot to mention, but, for the patch series to be applied, you'll need to also patch Documentation/media/uapi/dvb. > Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com> > --- > drivers/media/dvb-core/dvb_frontend.c | 212 +++++++++++++++++++++++++++++++++- > include/uapi/linux/dvb/frontend.h | 24 ++++ > 2 files changed, 234 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c > index e3fff8f..bc35a86 100644 > --- a/drivers/media/dvb-core/dvb_frontend.c > +++ b/drivers/media/dvb-core/dvb_frontend.c > @@ -1914,6 +1914,188 @@ static int dtv_property_process_set(struct dvb_frontend *fe, > return r; > } > > +/** > + * dtv_property_short_process_set > + * @fe: Pointer to struct dvb_frontend > + * @file: Pointer to struct file > + * @cmd: Property name > + * @data: Property value > + * > + * helper function for dvb_frontend_ioctl_properties, > + * which can be used to set dtv property using ioctl > + * cmd FE_SET_PROPERTY_SHORT. > + * It assigns property value to corresponding member of > + * property-cache structure > + * This func is a variant of the func dtv_property_process_set > + * Returns: > + * Zero on success, negative errno on failure. > + */ > +static int dtv_property_short_process_set(struct dvb_frontend *fe, > + struct file *file, > + u32 cmd, u32 data) > +{ > + int r = 0; > + struct dtv_frontend_properties *c = &fe->dtv_property_cache; > + switch (cmd) { > + case DTV_CLEAR: > + /* > + * Reset a cache of data specific to the frontend here. This > + * does not effect hardware. > + */ > + dvb_frontend_clear_cache(fe); > + break; No, you got me wrong. What I want is to have just *one* switch that would work for both the normal and the short version of this ioctl. In other words, I was expecting you to patch the existing dtv_property_process_set() for it to take u32 cmd and u32 data as arguments and to be used by both FE_SET_PROPERTY and FE_SET_PROPERTY_SHORT handling. > + case DTV_TUNE: > + /* interpret the cache of data, build either a traditional > + * frontend tune request so we can pass validation in the > + * FE_SET_FRONTEND ioctl. > + */ > + c->state = cmd; > + dev_dbg(fe->dvb->device, "%s: Finalised property cache\n", > + __func__); > + > + r = dtv_set_frontend(fe); > + break; > + case DTV_FREQUENCY: > + c->frequency = data; > + break; > + case DTV_MODULATION: > + c->modulation = data; > + break; > + case DTV_BANDWIDTH_HZ: > + c->bandwidth_hz = data; > + break; > + case DTV_INVERSION: > + c->inversion = data; > + break; > + case DTV_SYMBOL_RATE: > + c->symbol_rate = data; > + break; > + case DTV_INNER_FEC: > + c->fec_inner = data; > + break; > + case DTV_PILOT: > + c->pilot = data; > + break; > + case DTV_ROLLOFF: > + c->rolloff = data; > + break; > + case DTV_DELIVERY_SYSTEM: > + r = dvbv5_set_delivery_system(fe, data); > + break; > + case DTV_VOLTAGE: > + c->voltage = data; > + r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE, > + (void *)c->voltage); > + break; > + case DTV_TONE: > + c->sectone = data; > + r = dvb_frontend_ioctl_legacy(file, FE_SET_TONE, > + (void *)c->sectone); > + break; > + case DTV_CODE_RATE_HP: > + c->code_rate_HP = data; > + break; > + case DTV_CODE_RATE_LP: > + c->code_rate_LP = data; > + break; > + case DTV_GUARD_INTERVAL: > + c->guard_interval = data; > + break; > + case DTV_TRANSMISSION_MODE: > + c->transmission_mode = data; > + break; > + case DTV_HIERARCHY: > + c->hierarchy = data; > + break; > + case DTV_INTERLEAVING: > + c->interleaving = data; > + break; > + > + /* ISDB-T Support here */ > + case DTV_ISDBT_PARTIAL_RECEPTION: > + c->isdbt_partial_reception = data; > + break; > + case DTV_ISDBT_SOUND_BROADCASTING: > + c->isdbt_sb_mode = data; > + break; > + case DTV_ISDBT_SB_SUBCHANNEL_ID: > + c->isdbt_sb_subchannel = data; > + break; > + case DTV_ISDBT_SB_SEGMENT_IDX: > + c->isdbt_sb_segment_idx = data; > + break; > + case DTV_ISDBT_SB_SEGMENT_COUNT: > + c->isdbt_sb_segment_count = data; > + break; > + case DTV_ISDBT_LAYER_ENABLED: > + c->isdbt_layer_enabled = data; > + break; > + case DTV_ISDBT_LAYERA_FEC: > + c->layer[0].fec = data; > + break; > + case DTV_ISDBT_LAYERA_MODULATION: > + c->layer[0].modulation = data; > + break; > + case DTV_ISDBT_LAYERA_SEGMENT_COUNT: > + c->layer[0].segment_count = data; > + break; > + case DTV_ISDBT_LAYERA_TIME_INTERLEAVING: > + c->layer[0].interleaving = data; > + break; > + case DTV_ISDBT_LAYERB_FEC: > + c->layer[1].fec = data; > + break; > + case DTV_ISDBT_LAYERB_MODULATION: > + c->layer[1].modulation = data; > + break; > + case DTV_ISDBT_LAYERB_SEGMENT_COUNT: > + c->layer[1].segment_count = data; > + break; > + case DTV_ISDBT_LAYERB_TIME_INTERLEAVING: > + c->layer[1].interleaving = data; > + break; > + case DTV_ISDBT_LAYERC_FEC: > + c->layer[2].fec = data; > + break; > + case DTV_ISDBT_LAYERC_MODULATION: > + c->layer[2].modulation = data; > + break; > + case DTV_ISDBT_LAYERC_SEGMENT_COUNT: > + c->layer[2].segment_count = data; > + break; > + case DTV_ISDBT_LAYERC_TIME_INTERLEAVING: > + c->layer[2].interleaving = data; > + break; > + > + /* Multistream support */ > + case DTV_STREAM_ID: > + case DTV_DVBT2_PLP_ID_LEGACY: > + c->stream_id = data; > + break; > + > + /* ATSC-MH */ > + case DTV_ATSCMH_PARADE_ID: > + fe->dtv_property_cache.atscmh_parade_id = data; > + break; > + case DTV_ATSCMH_RS_FRAME_ENSEMBLE: > + fe->dtv_property_cache.atscmh_rs_frame_ensemble = data; > + break; > + > + case DTV_LNA: > + c->lna = data; > + if (fe->ops.set_lna) > + r = fe->ops.set_lna(fe); > + if (r < 0) > + c->lna = LNA_AUTO; > + break; > + > + default: > + return -EINVAL; > + } > + > + return r; > +} > + > static int dvb_frontend_ioctl(struct file *file, > unsigned int cmd, void *parg) > { > @@ -1939,7 +2121,8 @@ static int dvb_frontend_ioctl(struct file *file, > return -EPERM; > } > > - if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY)) > + if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY) > + || (cmd == FE_SET_PROPERTY_SHORT)) > err = dvb_frontend_ioctl_properties(file, cmd, parg); > else { > c->state = DTV_UNDEFINED; > @@ -2026,7 +2209,32 @@ static int dvb_frontend_ioctl_properties(struct file *file, > err = -EFAULT; > goto out; > } > - > + /* New ioctl for optimizing property set > + */ > + } else if (cmd == FE_SET_PROPERTY_SHORT) { > + struct dtv_property_short *tvp_short = NULL; > + struct dtv_properties_short *tvps_short = parg; > + > + dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps_short->num); > + dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps_short->props); > + if ((!tvps_short->num) || > + (tvps_short->num > DTV_IOCTL_MAX_MSGS)) > + return -EINVAL; > + tvp_short = memdup_user(tvps_short->props, > + tvps_short->num * sizeof(*tvp_short)); > + if (IS_ERR(tvp_short)) > + return PTR_ERR(tvp_short); > + for (i = 0; i < tvps_short->num; i++) { > + err = dtv_property_short_process_set(fe, file, > + (tvp_short + i)->cmd, (tvp_short + i)->data); > + if (err < 0) { > + kfree(tvp_short); > + return err; > + } > + } > + if (c->state == DTV_TUNE) > + dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__); > + kfree(tvp_short); > } else > err = -EOPNOTSUPP; > > diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h > index 00a20cd..aa82179 100644 > --- a/include/uapi/linux/dvb/frontend.h > +++ b/include/uapi/linux/dvb/frontend.h > @@ -476,6 +476,17 @@ struct dtv_property { > int result; > } __attribute__ ((packed)); > > +/** > + * @struct dtv_property_short > + * A shorter version of struct dtv_property > + * @cmd: Property type > + * @data: Property value > + */ > +struct dtv_property_short { > + __u32 cmd; > + __u32 data; > +}; > + > /* num of properties cannot exceed DTV_IOCTL_MAX_MSGS per ioctl */ > #define DTV_IOCTL_MAX_MSGS 64 > > @@ -484,6 +495,18 @@ struct dtv_properties { > struct dtv_property *props; > }; > > +/** > + * @struct dtv_properties_short > + * A variant of struct dtv_properties > + * to support struct dtv_property_short > + * @num: Number of properties > + * @props: Pointer to struct dtv_property_short > + */ > +struct dtv_properties_short { > + __u32 num; > + struct dtv_property_short *props; > +}; > + > #if defined(__DVB_CORE__) || !defined (__KERNEL__) > > /* > @@ -565,6 +588,7 @@ struct dvb_frontend_event { > > #define FE_SET_PROPERTY _IOW('o', 82, struct dtv_properties) > #define FE_GET_PROPERTY _IOR('o', 83, struct dtv_properties) > +#define FE_SET_PROPERTY_SHORT _IOW('o', 84, struct dtv_properties_short) > > /** > * When set, this flag will disable any zigzagging or other "normal" tuning Thanks, Mauro ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-15 8:49 ` Mauro Carvalho Chehab @ 2017-09-15 10:18 ` Satendra Singh Thakur 2017-09-15 11:28 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 12+ messages in thread From: Satendra Singh Thakur @ 2017-09-15 10:18 UTC (permalink / raw) To: mchehab, max.kellermann, sakari.ailus, mingo, hans.verkuil, yamada.masahiro, shuah Cc: satendra.t, linux-media, linux-kernel, taeyoung0432.lee, jackee.lee, hemanshu.s, madhur.verma 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 <satendra.t@samsung.com> --- .../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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-15 10:18 ` [PATCH v2] " Satendra Singh Thakur @ 2017-09-15 11:28 ` Mauro Carvalho Chehab 2017-09-18 8:15 ` [PATCH v3] " Satendra Singh Thakur 0 siblings, 1 reply; 12+ messages in thread From: Mauro Carvalho Chehab @ 2017-09-15 11:28 UTC (permalink / raw) To: Satendra Singh Thakur Cc: mchehab, max.kellermann, sakari.ailus, mingo, hans.verkuil, yamada.masahiro, shuah, linux-media, linux-kernel, taeyoung0432.lee, jackee.lee, hemanshu.s, madhur.verma Em Fri, 15 Sep 2017 06:18:20 -0400 Satendra Singh Thakur <satendra.t@samsung.com> 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 <satendra.t@samsung.com> > --- > .../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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-15 11:28 ` Mauro Carvalho Chehab @ 2017-09-18 8:15 ` Satendra Singh Thakur 2017-09-18 9:35 ` [PATCH v4] " Satendra Singh Thakur 0 siblings, 1 reply; 12+ messages in thread From: Satendra Singh Thakur @ 2017-09-18 8:15 UTC (permalink / raw) To: mchehab, max.kellermann, sakari.ailus, mingo, hans.verkuil, yamada.masahiro, shuah Cc: satendra.t, linux-media, linux-kernel, taeyoung0432.lee, jackee.lee, p.awasthi, hemanshu.s, madhur.verma 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 <satendra.t@samsung.com> --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v4] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-18 8:15 ` [PATCH v3] " Satendra Singh Thakur @ 2017-09-18 9:35 ` Satendra Singh Thakur 2017-09-19 9:33 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 12+ messages in thread From: Satendra Singh Thakur @ 2017-09-18 9:35 UTC (permalink / raw) To: mchehab; +Cc: satendra.t, linux-media, linux-kernel 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 <satendra.t@samsung.com> --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-18 9:35 ` [PATCH v4] " Satendra Singh Thakur @ 2017-09-19 9:33 ` Mauro Carvalho Chehab 2017-09-20 5:15 ` [PATCH v5] [DVB][FRONTEND] Modified the code related to FE_SET_PROPERTY ioctl Satendra Singh Thakur 0 siblings, 1 reply; 12+ messages in thread From: Mauro Carvalho Chehab @ 2017-09-19 9:33 UTC (permalink / raw) To: Satendra Singh Thakur; +Cc: mchehab, linux-media, linux-kernel Em Mon, 18 Sep 2017 05:35:45 -0400 Satendra Singh Thakur <satendra.t@samsung.com> 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 <satendra.t@samsung.com> > --- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5] [DVB][FRONTEND] Modified the code related to FE_SET_PROPERTY ioctl 2017-09-19 9:33 ` Mauro Carvalho Chehab @ 2017-09-20 5:15 ` Satendra Singh Thakur 2017-09-20 9:50 ` [PATCH v5 2/2] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation Satendra Singh Thakur 0 siblings, 1 reply; 12+ messages in thread From: Satendra Singh Thakur @ 2017-09-20 5:15 UTC (permalink / raw) To: mchehab; +Cc: satendra.t, linux-media, linux-kernel -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 <satendra.t@samsung.com> --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v5 2/2] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-20 5:15 ` [PATCH v5] [DVB][FRONTEND] Modified the code related to FE_SET_PROPERTY ioctl Satendra Singh Thakur @ 2017-09-20 9:50 ` Satendra Singh Thakur 0 siblings, 0 replies; 12+ messages in thread From: Satendra Singh Thakur @ 2017-09-20 9:50 UTC (permalink / raw) To: mchehab; +Cc: satendra.t, linux-media, linux-kernel -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 <satendra.t@samsung.com> --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation 2017-09-14 20:50 ` Mauro Carvalho Chehab 2017-09-15 5:50 ` [PATCH v1] " Satendra Singh Thakur @ 2017-09-15 8:28 ` Honza Petrouš 1 sibling, 0 replies; 12+ messages in thread From: Honza Petrouš @ 2017-09-15 8:28 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Satendra Singh Thakur, mchehab, Max Kellermann, Sakari Ailus, Ingo Molnar, hans.verkuil, Masahiro Yamada, Shuah Khan, linux-media@vger.kernel.org, linux-kernel, taeyoung0432.lee, jackee.lee, hemanshu.s, p.awasthi, siddharth.s, madhur.verma 2017-09-14 22:50 GMT+02:00 Mauro Carvalho Chehab <mchehab@s-opensource.com>: > Hi Satendra, > > Em Thu, 14 Sep 2017 05:59:27 -0400 > Satendra Singh Thakur <satendra.t@samsung.com> 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-09-20 9:50 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20170914095941epcas5p3520a04d543890249b4952fea48747276@epcas5p3.samsung.com>
2017-09-14 9:59 ` [RFC] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation Satendra Singh Thakur
2017-09-14 20:50 ` Mauro Carvalho Chehab
2017-09-15 5:50 ` [PATCH v1] " Satendra Singh Thakur
2017-09-15 8:49 ` Mauro Carvalho Chehab
2017-09-15 10:18 ` [PATCH v2] " Satendra Singh Thakur
2017-09-15 11:28 ` Mauro Carvalho Chehab
2017-09-18 8:15 ` [PATCH v3] " Satendra Singh Thakur
2017-09-18 9:35 ` [PATCH v4] " Satendra Singh Thakur
2017-09-19 9:33 ` Mauro Carvalho Chehab
2017-09-20 5:15 ` [PATCH v5] [DVB][FRONTEND] Modified the code related to FE_SET_PROPERTY ioctl Satendra Singh Thakur
2017-09-20 9:50 ` [PATCH v5 2/2] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation Satendra Singh Thakur
2017-09-15 8:28 ` [RFC] " Honza Petrouš
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox