[RFC 0/4] Don't do tuning zigzag using the very same frequency

[RFC 0/4] Don't do tuning zigzag using the very same frequency

[Mauro Carvalho Chehab]

Marc reported on IRC that the zigzag code is trying to tune several times using
the same frequency with si2168. Well, this is not how this would be supposed
to do: it should try with different frequencies each time.

Change the core to use the one-shot mode if the frontend doesn't report a
frequency step. This will default to the current behavior, except that tuning
should be faster.

Yet, probably the right thing to do is to implement a frequency shift at such
frontends, as otherwise  tuning may have problems. So, produce a warning
on such cases, in order for the FE driver to be fixed.

Mauro Carvalho Chehab (4):
  media: atomisp: fix identation at I2C Kconfig menu
  media: atomisp: fix help message for ISP2401 selection
  media: dvb_frontend: move algo-specific settings to a function
  media: dvb_frontend: disable zigzag mode if not possible

 drivers/media/dvb-core/dvb_frontend.c         | 231 ++++++++++--------
 drivers/staging/media/atomisp/Kconfig         |   2 +-
 drivers/staging/media/atomisp/i2c/Kconfig     |  74 +++---
 .../staging/media/atomisp/i2c/ov5693/Kconfig  |  12 -
 4 files changed, 171 insertions(+), 148 deletions(-)
 delete mode 100644 drivers/staging/media/atomisp/i2c/ov5693/Kconfig

-- 
2.26.2

[RFC 1/4] media: atomisp: fix identation at I2C Kconfig menu

[Mauro Carvalho Chehab]

There are several bad whitespacing usage there. Remove them.

While here, place all Kconfig options for sensors at the
same place.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/atomisp/i2c/Kconfig     | 74 +++++++++++--------
 .../staging/media/atomisp/i2c/ov5693/Kconfig  | 12 ---
 2 files changed, 42 insertions(+), 44 deletions(-)
 delete mode 100644 drivers/staging/media/atomisp/i2c/ov5693/Kconfig

diff --git a/drivers/staging/media/atomisp/i2c/Kconfig b/drivers/staging/media/atomisp/i2c/Kconfig
index 7c7f0fc090b3..a772b833a85f 100644
--- a/drivers/staging/media/atomisp/i2c/Kconfig
+++ b/drivers/staging/media/atomisp/i2c/Kconfig
@@ -3,53 +3,51 @@
 # Kconfig for sensor drivers
 #
 
-source "drivers/staging/media/atomisp/i2c/ov5693/Kconfig"
-
 config VIDEO_ATOMISP_OV2722
-       tristate "OVT ov2722 sensor support"
+	tristate "OVT ov2722 sensor support"
 	depends on ACPI
-       depends on I2C && VIDEO_V4L2
+	depends on I2C && VIDEO_V4L2
 	help
-	 This is a Video4Linux2 sensor-level driver for the OVT
-	 OV2722 raw camera.
+	  This is a Video4Linux2 sensor-level driver for the OVT
+	  OV2722 raw camera.
 
-	 OVT is a 2M raw sensor.
+	  OVT is a 2M raw sensor.
 
-	 It currently only works with the atomisp driver.
+	  It currently only works with the atomisp driver.
 
 config VIDEO_ATOMISP_GC2235
-       tristate "Galaxy gc2235 sensor support"
+	tristate "Galaxy gc2235 sensor support"
 	depends on ACPI
-       depends on I2C && VIDEO_V4L2
+	depends on I2C && VIDEO_V4L2
 	help
-	 This is a Video4Linux2 sensor-level driver for the OVT
-	 GC2235 raw camera.
+	  This is a Video4Linux2 sensor-level driver for the OVT
+	  GC2235 raw camera.
 
-	 GC2235 is a 2M raw sensor.
+	  GC2235 is a 2M raw sensor.
 
-	 It currently only works with the atomisp driver.
+	  It currently only works with the atomisp driver.
 
 config VIDEO_ATOMISP_MSRLIST_HELPER
-       tristate "Helper library to load, parse and apply large register lists."
-       depends on I2C
+	tristate "Helper library to load, parse and apply large register lists."
+	depends on I2C
 	help
-	 This is a helper library to be used from a sensor driver to load, parse
-	 and apply large register lists.
+	  This is a helper library to be used from a sensor driver to load, parse
+	  and apply large register lists.
 
-	 To compile this driver as a module, choose M here: the
-	 module will be called libmsrlisthelper.
+	  To compile this driver as a module, choose M here: the
+	  module will be called libmsrlisthelper.
 
 config VIDEO_ATOMISP_MT9M114
-       tristate "Aptina mt9m114 sensor support"
+	tristate "Aptina mt9m114 sensor support"
 	depends on ACPI
-       depends on I2C && VIDEO_V4L2
+	depends on I2C && VIDEO_V4L2
 	help
-	 This is a Video4Linux2 sensor-level driver for the Micron
-	 mt9m114 1.3 Mpixel camera.
+	  This is a Video4Linux2 sensor-level driver for the Micron
+	  mt9m114 1.3 Mpixel camera.
 
-	 mt9m114 is video camera sensor.
+	  mt9m114 is video camera sensor.
 
-	 It currently only works with the atomisp driver.
+	  It currently only works with the atomisp driver.
 
 config VIDEO_ATOMISP_GC0310
 	tristate "GC0310 sensor support"
@@ -60,16 +58,28 @@ config VIDEO_ATOMISP_GC0310
 	  GC0310 0.3MP sensor.
 
 config VIDEO_ATOMISP_OV2680
-       tristate "Omnivision OV2680 sensor support"
+	tristate "Omnivision OV2680 sensor support"
 	depends on ACPI
-       depends on I2C && VIDEO_V4L2
+	depends on I2C && VIDEO_V4L2
 	help
-	 This is a Video4Linux2 sensor-level driver for the Omnivision
-	 OV2680 raw camera.
+	  This is a Video4Linux2 sensor-level driver for the Omnivision
+	  OV2680 raw camera.
 
-	 ov2680 is a 2M raw sensor.
+	  ov2680 is a 2M raw sensor.
 
-	 It currently only works with the atomisp driver.
+	  It currently only works with the atomisp driver.
+
+config VIDEO_ATOMISP_OV5693
+	tristate "Omnivision ov5693 sensor support"
+	depends on ACPI
+	depends on I2C && VIDEO_V4L2
+	help
+	  This is a Video4Linux2 sensor-level driver for the Micron
+	  ov5693 5 Mpixel camera.
+
+	  ov5693 is video camera sensor.
+
+	  It currently only works with the atomisp driver.
 
 #
 # Kconfig for flash drivers
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/Kconfig b/drivers/staging/media/atomisp/i2c/ov5693/Kconfig
deleted file mode 100644
index c8d09f416c35..000000000000
--- a/drivers/staging/media/atomisp/i2c/ov5693/Kconfig
+++ /dev/null
@@ -1,12 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0
-config VIDEO_ATOMISP_OV5693
-       tristate "Omnivision ov5693 sensor support"
-	depends on ACPI
-       depends on I2C && VIDEO_V4L2
-	help
-	 This is a Video4Linux2 sensor-level driver for the Micron
-	 ov5693 5 Mpixel camera.
-
-	 ov5693 is video camera sensor.
-
-	 It currently only works with the atomisp driver.
-- 
2.26.2

[RFC 2/4] media: atomisp: fix help message for ISP2401 selection

[Mauro Carvalho Chehab]

I'm pretty sure I named this right, but it sounds that I ended
doing something weird maybe while solving some conflict.

So, fix the title of this config var.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/atomisp/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/Kconfig b/drivers/staging/media/atomisp/Kconfig
index fea06cb0eb48..37577bb72998 100644
--- a/drivers/staging/media/atomisp/Kconfig
+++ b/drivers/staging/media/atomisp/Kconfig
@@ -22,7 +22,7 @@ config VIDEO_ATOMISP
 	  module will be called atomisp
 
 config VIDEO_ATOMISP_ISP2401
-	bool "VIDEO_ATOMISP_ISP2401"
+	bool "Use Intel Atom ISP on Cherrytail/Anniedale (ISP2401)"
 	depends on VIDEO_ATOMISP
 	help
 	  Enable support for Atom ISP2401-based boards.
-- 
2.26.2

[RFC 3/4] media: dvb_frontend: move algo-specific settings to a function

[Mauro Carvalho Chehab]

As we're planning to call this code on a separate place, let's
fist move it to a different function.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/dvb-core/dvb_frontend.c | 90 +++++++++++++++------------
 1 file changed, 49 insertions(+), 41 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 06ea30a689d7..ed85dc2a9183 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1790,6 +1790,54 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
 	return emulate_delivery_system(fe, delsys);
 }
 
+static void prepare_tuning_algo_parameters(struct dvb_frontend *fe)
+{
+	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
+	struct dvb_frontend_private *fepriv = fe->frontend_priv;
+	struct dvb_frontend_tune_settings fetunesettings;
+
+	/* get frontend-specific tuning settings */
+	memset(&fetunesettings, 0, sizeof(struct dvb_frontend_tune_settings));
+	if (fe->ops.get_tune_settings && (fe->ops.get_tune_settings(fe, &fetunesettings) == 0)) {
+		fepriv->min_delay = (fetunesettings.min_delay_ms * HZ) / 1000;
+		fepriv->max_drift = fetunesettings.max_drift;
+		fepriv->step_size = fetunesettings.step_size;
+	} else {
+		/* default values */
+		switch (c->delivery_system) {
+		case SYS_DVBS:
+		case SYS_DVBS2:
+		case SYS_ISDBS:
+		case SYS_TURBO:
+		case SYS_DVBC_ANNEX_A:
+		case SYS_DVBC_ANNEX_C:
+			fepriv->min_delay = HZ / 20;
+			fepriv->step_size = c->symbol_rate / 16000;
+			fepriv->max_drift = c->symbol_rate / 2000;
+			break;
+		case SYS_DVBT:
+		case SYS_DVBT2:
+		case SYS_ISDBT:
+		case SYS_DTMB:
+			fepriv->min_delay = HZ / 20;
+			fepriv->step_size = dvb_frontend_get_stepsize(fe) * 2;
+			fepriv->max_drift = (dvb_frontend_get_stepsize(fe) * 2) + 1;
+			break;
+		default:
+			/*
+			 * FIXME: This sounds wrong! if freqency_stepsize is
+			 * defined by the frontend, why not use it???
+			 */
+			fepriv->min_delay = HZ / 20;
+			fepriv->step_size = 0; /* no zigzag */
+			fepriv->max_drift = 0;
+			break;
+		}
+	}
+	if (dvb_override_tune_delay > 0)
+		fepriv->min_delay = (dvb_override_tune_delay * HZ) / 1000;
+}
+
 /**
  * dtv_property_process_set -  Sets a single DTV property
  * @fe:		Pointer to &struct dvb_frontend
@@ -2182,7 +2230,6 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
 {
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-	struct dvb_frontend_tune_settings fetunesettings;
 	u32 rolloff = 0;
 
 	if (dvb_frontend_check_parameters(fe) < 0)
@@ -2260,46 +2307,7 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
 	if (c->hierarchy == HIERARCHY_NONE && c->code_rate_LP == FEC_NONE)
 		c->code_rate_LP = FEC_AUTO;
 
-	/* get frontend-specific tuning settings */
-	memset(&fetunesettings, 0, sizeof(struct dvb_frontend_tune_settings));
-	if (fe->ops.get_tune_settings && (fe->ops.get_tune_settings(fe, &fetunesettings) == 0)) {
-		fepriv->min_delay = (fetunesettings.min_delay_ms * HZ) / 1000;
-		fepriv->max_drift = fetunesettings.max_drift;
-		fepriv->step_size = fetunesettings.step_size;
-	} else {
-		/* default values */
-		switch (c->delivery_system) {
-		case SYS_DVBS:
-		case SYS_DVBS2:
-		case SYS_ISDBS:
-		case SYS_TURBO:
-		case SYS_DVBC_ANNEX_A:
-		case SYS_DVBC_ANNEX_C:
-			fepriv->min_delay = HZ / 20;
-			fepriv->step_size = c->symbol_rate / 16000;
-			fepriv->max_drift = c->symbol_rate / 2000;
-			break;
-		case SYS_DVBT:
-		case SYS_DVBT2:
-		case SYS_ISDBT:
-		case SYS_DTMB:
-			fepriv->min_delay = HZ / 20;
-			fepriv->step_size = dvb_frontend_get_stepsize(fe) * 2;
-			fepriv->max_drift = (dvb_frontend_get_stepsize(fe) * 2) + 1;
-			break;
-		default:
-			/*
-			 * FIXME: This sounds wrong! if freqency_stepsize is
-			 * defined by the frontend, why not use it???
-			 */
-			fepriv->min_delay = HZ / 20;
-			fepriv->step_size = 0; /* no zigzag */
-			fepriv->max_drift = 0;
-			break;
-		}
-	}
-	if (dvb_override_tune_delay > 0)
-		fepriv->min_delay = (dvb_override_tune_delay * HZ) / 1000;
+	prepare_tuning_algo_parameters(fe);
 
 	fepriv->state = FESTATE_RETUNE;
 
-- 
2.26.2

Re: [RFC 3/4] media: dvb_frontend: move algo-specific settings to a function

[Marc Gonzalez]

On 17/06/2020 20:52, Mauro Carvalho Chehab wrote:

> As we're planning to call this code on a separate place, let's

s/on/from/
Suggest: "to call this code from somewhere else"

> fist move it to a different function.

s/fist/first
Suggest: "first move it to its own function"

> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 90 +++++++++++++++------------
>  1 file changed, 49 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 06ea30a689d7..ed85dc2a9183 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1790,6 +1790,54 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
>  	return emulate_delivery_system(fe, delsys);
>  }
>  
> +static void prepare_tuning_algo_parameters(struct dvb_frontend *fe)
> +{
> +	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> +	struct dvb_frontend_private *fepriv = fe->frontend_priv;
> +	struct dvb_frontend_tune_settings fetunesettings;

Suggest: fetunesettings = { 0 };
then we can remove the memset() below.

> +
> +	/* get frontend-specific tuning settings */
> +	memset(&fetunesettings, 0, sizeof(struct dvb_frontend_tune_settings));
> +	if (fe->ops.get_tune_settings && (fe->ops.get_tune_settings(fe, &fetunesettings) == 0)) {
> +		fepriv->min_delay = (fetunesettings.min_delay_ms * HZ) / 1000;
> +		fepriv->max_drift = fetunesettings.max_drift;
> +		fepriv->step_size = fetunesettings.step_size;
> +	} else {
> +		/* default values */
> +		switch (c->delivery_system) {
> +		case SYS_DVBS:
> +		case SYS_DVBS2:
> +		case SYS_ISDBS:
> +		case SYS_TURBO:
> +		case SYS_DVBC_ANNEX_A:
> +		case SYS_DVBC_ANNEX_C:
> +			fepriv->min_delay = HZ / 20;
> +			fepriv->step_size = c->symbol_rate / 16000;
> +			fepriv->max_drift = c->symbol_rate / 2000;
> +			break;
> +		case SYS_DVBT:
> +		case SYS_DVBT2:
> +		case SYS_ISDBT:
> +		case SYS_DTMB:
> +			fepriv->min_delay = HZ / 20;
> +			fepriv->step_size = dvb_frontend_get_stepsize(fe) * 2;
> +			fepriv->max_drift = (dvb_frontend_get_stepsize(fe) * 2) + 1;
> +			break;
> +		default:
> +			/*
> +			 * FIXME: This sounds wrong! if freqency_stepsize is
> +			 * defined by the frontend, why not use it???
> +			 */
> +			fepriv->min_delay = HZ / 20;
> +			fepriv->step_size = 0; /* no zigzag */
> +			fepriv->max_drift = 0;
> +			break;
> +		}
> +	}
> +	if (dvb_override_tune_delay > 0)
> +		fepriv->min_delay = (dvb_override_tune_delay * HZ) / 1000;
> +}
> +
>  /**
>   * dtv_property_process_set -  Sets a single DTV property
>   * @fe:		Pointer to &struct dvb_frontend
> @@ -2182,7 +2230,6 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
>  {
>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>  	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> -	struct dvb_frontend_tune_settings fetunesettings;
>  	u32 rolloff = 0;
>  
>  	if (dvb_frontend_check_parameters(fe) < 0)
> @@ -2260,46 +2307,7 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
>  	if (c->hierarchy == HIERARCHY_NONE && c->code_rate_LP == FEC_NONE)
>  		c->code_rate_LP = FEC_AUTO;
>  
> -	/* get frontend-specific tuning settings */
> -	memset(&fetunesettings, 0, sizeof(struct dvb_frontend_tune_settings));
> -	if (fe->ops.get_tune_settings && (fe->ops.get_tune_settings(fe, &fetunesettings) == 0)) {
> -		fepriv->min_delay = (fetunesettings.min_delay_ms * HZ) / 1000;
> -		fepriv->max_drift = fetunesettings.max_drift;
> -		fepriv->step_size = fetunesettings.step_size;
> -	} else {
> -		/* default values */
> -		switch (c->delivery_system) {
> -		case SYS_DVBS:
> -		case SYS_DVBS2:
> -		case SYS_ISDBS:
> -		case SYS_TURBO:
> -		case SYS_DVBC_ANNEX_A:
> -		case SYS_DVBC_ANNEX_C:
> -			fepriv->min_delay = HZ / 20;
> -			fepriv->step_size = c->symbol_rate / 16000;
> -			fepriv->max_drift = c->symbol_rate / 2000;
> -			break;
> -		case SYS_DVBT:
> -		case SYS_DVBT2:
> -		case SYS_ISDBT:
> -		case SYS_DTMB:
> -			fepriv->min_delay = HZ / 20;
> -			fepriv->step_size = dvb_frontend_get_stepsize(fe) * 2;
> -			fepriv->max_drift = (dvb_frontend_get_stepsize(fe) * 2) + 1;
> -			break;

As an aside, I find it confusing that there are 3 sources for the stepsize.
1) fe->ops.get_tune_settings().step_size
2) fe->ops.info.frequency_stepsize_hz
3) fe->ops.tuner_ops.info.frequency_step_hz

> -		default:
> -			/*
> -			 * FIXME: This sounds wrong! if freqency_stepsize is
> -			 * defined by the frontend, why not use it???
> -			 */
> -			fepriv->min_delay = HZ / 20;
> -			fepriv->step_size = 0; /* no zigzag */
> -			fepriv->max_drift = 0;
> -			break;
> -		}
> -	}
> -	if (dvb_override_tune_delay > 0)
> -		fepriv->min_delay = (dvb_override_tune_delay * HZ) / 1000;
> +	prepare_tuning_algo_parameters(fe);

LGTM

Reviewed-by: Marc Gonzalez <marc.w.gonzalez@free.fr>

[RFC 4/4] media: dvb_frontend: disable zigzag mode if not possible

[Mauro Carvalho Chehab]

For the zigzag to work, the core needs to have a frequency
shift. Without that, the zigzag code will just try re-tuning
several times at the very same frequency, with seems wrong.

So, add a warning when this happens, and fall back to the
single-shot mode.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/dvb-core/dvb_frontend.c | 141 +++++++++++++++-----------
 1 file changed, 79 insertions(+), 62 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index ed85dc2a9183..cb577924121e 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -642,6 +642,9 @@ static void dvb_frontend_wakeup(struct dvb_frontend *fe)
 	wake_up_interruptible(&fepriv->wait_queue);
 }
 
+static u32 dvb_frontend_get_stepsize(struct dvb_frontend *fe);
+static void prepare_tuning_algo_parameters(struct dvb_frontend *fe);
+
 static int dvb_frontend_thread(void *data)
 {
 	struct dvb_frontend *fe = data;
@@ -696,78 +699,92 @@ static int dvb_frontend_thread(void *data)
 			fepriv->reinitialise = 0;
 		}
 
-		/* do an iteration of the tuning loop */
-		if (fe->ops.get_frontend_algo) {
+		if (fe->ops.get_frontend_algo)
 			algo = fe->ops.get_frontend_algo(fe);
-			switch (algo) {
-			case DVBFE_ALGO_HW:
-				dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_HW\n", __func__);
+		else
+			algo = DVBFE_ALGO_SW;
 
-				if (fepriv->state & FESTATE_RETUNE) {
-					dev_dbg(fe->dvb->device, "%s: Retune requested, FESTATE_RETUNE\n", __func__);
-					re_tune = true;
-					fepriv->state = FESTATE_TUNED;
-				} else {
-					re_tune = false;
-				}
+		/* do an iteration of the tuning loop */
+		switch (algo) {
+		case DVBFE_ALGO_SW:
+			prepare_tuning_algo_parameters(fe);
 
-				if (fe->ops.tune)
-					fe->ops.tune(fe, re_tune, fepriv->tune_mode_flags, &fepriv->delay, &s);
-
-				if (s != fepriv->status && !(fepriv->tune_mode_flags & FE_TUNE_MODE_ONESHOT)) {
-					dev_dbg(fe->dvb->device, "%s: state changed, adding current state\n", __func__);
-					dvb_frontend_add_event(fe, s);
-					fepriv->status = s;
-				}
-				break;
-			case DVBFE_ALGO_SW:
+			if (fepriv->max_drift) {
 				dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_SW\n", __func__);
 				dvb_frontend_swzigzag(fe);
 				break;
-			case DVBFE_ALGO_CUSTOM:
-				dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_CUSTOM, state=%d\n", __func__, fepriv->state);
-				if (fepriv->state & FESTATE_RETUNE) {
-					dev_dbg(fe->dvb->device, "%s: Retune requested, FESTAT_RETUNE\n", __func__);
-					fepriv->state = FESTATE_TUNED;
+			}
+
+			/*
+			 * See prepare_tuning_algo_parameters():
+			 *   - Some standards may not use zigzag.
+			 */
+			if (!dvb_frontend_get_stepsize(fe))
+				dev_warn(fe->dvb->device,
+					"disabling sigzag, as frontend doesn't set frequency step size\n");
+
+			/* fall through */
+		case DVBFE_ALGO_HW:
+			dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_HW\n", __func__);
+
+			if (fepriv->state & FESTATE_RETUNE) {
+				dev_dbg(fe->dvb->device, "%s: Retune requested, FESTATE_RETUNE\n", __func__);
+				re_tune = true;
+				fepriv->state = FESTATE_TUNED;
+			} else {
+				re_tune = false;
+			}
+
+			if (fe->ops.tune)
+				fe->ops.tune(fe, re_tune, fepriv->tune_mode_flags, &fepriv->delay, &s);
+
+			if (s != fepriv->status && !(fepriv->tune_mode_flags & FE_TUNE_MODE_ONESHOT)) {
+				dev_dbg(fe->dvb->device, "%s: state changed, adding current state\n", __func__);
+				dvb_frontend_add_event(fe, s);
+				fepriv->status = s;
+			}
+			break;
+		case DVBFE_ALGO_CUSTOM:
+			dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_CUSTOM, state=%d\n", __func__, fepriv->state);
+			if (fepriv->state & FESTATE_RETUNE) {
+				dev_dbg(fe->dvb->device, "%s: Retune requested, FESTAT_RETUNE\n", __func__);
+				fepriv->state = FESTATE_TUNED;
+			}
+			/* Case where we are going to search for a carrier
+			    * User asked us to retune again for some reason, possibly
+			    * requesting a search with a new set of parameters
+			    */
+			if (fepriv->algo_status & DVBFE_ALGO_SEARCH_AGAIN) {
+				if (fe->ops.search) {
+					fepriv->algo_status = fe->ops.search(fe);
+					/* We did do a search as was requested, the flags are
+					    * now unset as well and has the flags wrt to search.
+					    */
+				} else {
+					fepriv->algo_status &= ~DVBFE_ALGO_SEARCH_AGAIN;
 				}
-				/* Case where we are going to search for a carrier
-				 * User asked us to retune again for some reason, possibly
-				 * requesting a search with a new set of parameters
-				 */
-				if (fepriv->algo_status & DVBFE_ALGO_SEARCH_AGAIN) {
-					if (fe->ops.search) {
-						fepriv->algo_status = fe->ops.search(fe);
-						/* We did do a search as was requested, the flags are
-						 * now unset as well and has the flags wrt to search.
-						 */
-					} else {
-						fepriv->algo_status &= ~DVBFE_ALGO_SEARCH_AGAIN;
-					}
-				}
-				/* Track the carrier if the search was successful */
-				if (fepriv->algo_status != DVBFE_ALGO_SEARCH_SUCCESS) {
+			}
+			/* Track the carrier if the search was successful */
+			if (fepriv->algo_status != DVBFE_ALGO_SEARCH_SUCCESS) {
+				fepriv->algo_status |= DVBFE_ALGO_SEARCH_AGAIN;
+				fepriv->delay = HZ / 2;
+			}
+			dtv_property_legacy_params_sync(fe, c, &fepriv->parameters_out);
+			fe->ops.read_status(fe, &s);
+			if (s != fepriv->status) {
+				dvb_frontend_add_event(fe, s); /* update event list */
+				fepriv->status = s;
+				if (!(s & FE_HAS_LOCK)) {
+					fepriv->delay = HZ / 10;
 					fepriv->algo_status |= DVBFE_ALGO_SEARCH_AGAIN;
-					fepriv->delay = HZ / 2;
+				} else {
+					fepriv->delay = 60 * HZ;
 				}
-				dtv_property_legacy_params_sync(fe, c, &fepriv->parameters_out);
-				fe->ops.read_status(fe, &s);
-				if (s != fepriv->status) {
-					dvb_frontend_add_event(fe, s); /* update event list */
-					fepriv->status = s;
-					if (!(s & FE_HAS_LOCK)) {
-						fepriv->delay = HZ / 10;
-						fepriv->algo_status |= DVBFE_ALGO_SEARCH_AGAIN;
-					} else {
-						fepriv->delay = 60 * HZ;
-					}
-				}
-				break;
-			default:
-				dev_dbg(fe->dvb->device, "%s: UNDEFINED ALGO !\n", __func__);
-				break;
 			}
-		} else {
-			dvb_frontend_swzigzag(fe);
+			break;
+		default:
+			dev_dbg(fe->dvb->device, "%s: UNDEFINED ALGO !\n", __func__);
+			break;
 		}
 	}
 
-- 
2.26.2

Re: [RFC 4/4] media: dvb_frontend: disable zigzag mode if not possible

[Marc Gonzalez]

On 17/06/2020 20:52, Mauro Carvalho Chehab wrote:

> For the zigzag to work, the core needs to have a frequency
> shift. Without that, the zigzag code will just try re-tuning
> several times at the very same frequency, with seems wrong.

s/with/which

Suggest: "the core requires a frequency shift value"

> So, add a warning when this happens, and fall back to the
> single-shot mode.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 141 +++++++++++++++-----------
>  1 file changed, 79 insertions(+), 62 deletions(-)

It's hard to discern in the diff what is just white-space adjustment
from one less tab, and what is new code that requires more scrutiny.
I'll try applying the patch, and then diff -w.
Yes, that's much better.

> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index ed85dc2a9183..cb577924121e 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -642,6 +642,9 @@ static void dvb_frontend_wakeup(struct dvb_frontend *fe)
>  	wake_up_interruptible(&fepriv->wait_queue);
>  }
>  
> +static u32 dvb_frontend_get_stepsize(struct dvb_frontend *fe);
> +static void prepare_tuning_algo_parameters(struct dvb_frontend *fe);
> +
>  static int dvb_frontend_thread(void *data)
>  {
>  	struct dvb_frontend *fe = data;
> @@ -696,78 +699,92 @@ static int dvb_frontend_thread(void *data)
>  			fepriv->reinitialise = 0;
>  		}
>  
> -		/* do an iteration of the tuning loop */
> -		if (fe->ops.get_frontend_algo) {
> +		if (fe->ops.get_frontend_algo)
>  			algo = fe->ops.get_frontend_algo(fe);
> -			switch (algo) {
> -			case DVBFE_ALGO_HW:
> -				dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_HW\n", __func__);
> +		else
> +			algo = DVBFE_ALGO_SW;
>  
> -				if (fepriv->state & FESTATE_RETUNE) {
> -					dev_dbg(fe->dvb->device, "%s: Retune requested, FESTATE_RETUNE\n", __func__);
> -					re_tune = true;
> -					fepriv->state = FESTATE_TUNED;
> -				} else {
> -					re_tune = false;
> -				}
> +		/* do an iteration of the tuning loop */
> +		switch (algo) {
> +		case DVBFE_ALGO_SW:
> +			prepare_tuning_algo_parameters(fe);
>  
> -				if (fe->ops.tune)
> -					fe->ops.tune(fe, re_tune, fepriv->tune_mode_flags, &fepriv->delay, &s);
> -
> -				if (s != fepriv->status && !(fepriv->tune_mode_flags & FE_TUNE_MODE_ONESHOT)) {
> -					dev_dbg(fe->dvb->device, "%s: state changed, adding current state\n", __func__);
> -					dvb_frontend_add_event(fe, s);
> -					fepriv->status = s;
> -				}
> -				break;
> -			case DVBFE_ALGO_SW:
> +			if (fepriv->max_drift) {
>  				dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_SW\n", __func__);
>  				dvb_frontend_swzigzag(fe);
>  				break;
> -			case DVBFE_ALGO_CUSTOM:
> -				dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_CUSTOM, state=%d\n", __func__, fepriv->state);
> -				if (fepriv->state & FESTATE_RETUNE) {
> -					dev_dbg(fe->dvb->device, "%s: Retune requested, FESTAT_RETUNE\n", __func__);
> -					fepriv->state = FESTATE_TUNED;
> +			}
> +
> +			/*
> +			 * See prepare_tuning_algo_parameters():
> +			 *   - Some standards may not use zigzag.
> +			 */
> +			if (!dvb_frontend_get_stepsize(fe))
> +				dev_warn(fe->dvb->device,
> +					"disabling sigzag, as frontend doesn't set frequency step size\n");

s/sigzag/zigzag

I don't understand why you're calling dvb_frontend_get_stepsize() again?
prepare_tuning_algo_parameters() already tried its best to set fepriv->step_size

Why not just:

	if (fepriv->max_drift)
		do the zigzag
	else
		warn that zigzag is disabled

> +
> +			/* fall through */

Why would you want to fall through from DVBFE_ALGO_SW to DVBFE_ALGO_HW?
I think this changes the behavior before the patch.

Re: [RFC 4/4] media: dvb_frontend: disable zigzag mode if not possible

[Mauro Carvalho Chehab]

Em Thu, 18 Jun 2020 11:50:54 +0200
Marc Gonzalez <marc.w.gonzalez@free.fr> escreveu:

> On 17/06/2020 20:52, Mauro Carvalho Chehab wrote:
> 
> > For the zigzag to work, the core needs to have a frequency
> > shift. Without that, the zigzag code will just try re-tuning
> > several times at the very same frequency, with seems wrong.  
> 
> s/with/which
> 
> Suggest: "the core requires a frequency shift value"
> 
> > So, add a warning when this happens, and fall back to the
> > single-shot mode.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/media/dvb-core/dvb_frontend.c | 141 +++++++++++++++-----------
> >  1 file changed, 79 insertions(+), 62 deletions(-)  
> 
> It's hard to discern in the diff what is just white-space adjustment
> from one less tab, and what is new code that requires more scrutiny.
> I'll try applying the patch, and then diff -w.
> Yes, that's much better.
> 
> > diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> > index ed85dc2a9183..cb577924121e 100644
> > --- a/drivers/media/dvb-core/dvb_frontend.c
> > +++ b/drivers/media/dvb-core/dvb_frontend.c
> > @@ -642,6 +642,9 @@ static void dvb_frontend_wakeup(struct dvb_frontend *fe)
> >  	wake_up_interruptible(&fepriv->wait_queue);
> >  }
> >  
> > +static u32 dvb_frontend_get_stepsize(struct dvb_frontend *fe);
> > +static void prepare_tuning_algo_parameters(struct dvb_frontend *fe);
> > +
> >  static int dvb_frontend_thread(void *data)
> >  {
> >  	struct dvb_frontend *fe = data;
> > @@ -696,78 +699,92 @@ static int dvb_frontend_thread(void *data)
> >  			fepriv->reinitialise = 0;
> >  		}
> >  
> > -		/* do an iteration of the tuning loop */
> > -		if (fe->ops.get_frontend_algo) {
> > +		if (fe->ops.get_frontend_algo)
> >  			algo = fe->ops.get_frontend_algo(fe);
> > -			switch (algo) {
> > -			case DVBFE_ALGO_HW:
> > -				dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_HW\n", __func__);
> > +		else
> > +			algo = DVBFE_ALGO_SW;
> >  
> > -				if (fepriv->state & FESTATE_RETUNE) {
> > -					dev_dbg(fe->dvb->device, "%s: Retune requested, FESTATE_RETUNE\n", __func__);
> > -					re_tune = true;
> > -					fepriv->state = FESTATE_TUNED;
> > -				} else {
> > -					re_tune = false;
> > -				}
> > +		/* do an iteration of the tuning loop */
> > +		switch (algo) {
> > +		case DVBFE_ALGO_SW:
> > +			prepare_tuning_algo_parameters(fe);
> >  
> > -				if (fe->ops.tune)
> > -					fe->ops.tune(fe, re_tune, fepriv->tune_mode_flags, &fepriv->delay, &s);
> > -
> > -				if (s != fepriv->status && !(fepriv->tune_mode_flags & FE_TUNE_MODE_ONESHOT)) {
> > -					dev_dbg(fe->dvb->device, "%s: state changed, adding current state\n", __func__);
> > -					dvb_frontend_add_event(fe, s);
> > -					fepriv->status = s;
> > -				}
> > -				break;
> > -			case DVBFE_ALGO_SW:
> > +			if (fepriv->max_drift) {
> >  				dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_SW\n", __func__);
> >  				dvb_frontend_swzigzag(fe);
> >  				break;
> > -			case DVBFE_ALGO_CUSTOM:
> > -				dev_dbg(fe->dvb->device, "%s: Frontend ALGO = DVBFE_ALGO_CUSTOM, state=%d\n", __func__, fepriv->state);
> > -				if (fepriv->state & FESTATE_RETUNE) {
> > -					dev_dbg(fe->dvb->device, "%s: Retune requested, FESTAT_RETUNE\n", __func__);
> > -					fepriv->state = FESTATE_TUNED;
> > +			}
> > +
> > +			/*
> > +			 * See prepare_tuning_algo_parameters():
> > +			 *   - Some standards may not use zigzag.
> > +			 */
> > +			if (!dvb_frontend_get_stepsize(fe))
> > +				dev_warn(fe->dvb->device,
> > +					"disabling sigzag, as frontend doesn't set frequency step size\n");  
> 
> s/sigzag/zigzag
> 
> I don't understand why you're calling dvb_frontend_get_stepsize() again?
> prepare_tuning_algo_parameters() already tried its best to set fepriv->step_size
> 
> Why not just:
> 
> 	if (fepriv->max_drift)
> 		do the zigzag
> 	else
> 		warn that zigzag is disabled
> 
> > +
> > +			/* fall through */  
> 
> Why would you want to fall through from DVBFE_ALGO_SW to DVBFE_ALGO_HW?
> I think this changes the behavior before the patch.

I double-checked this patch. What happens is that there are 3 
types of DVB devices:

1. Devices where the Zigzag happens at the hardware level,
   automatically (DVBFE_ALGO_HW). All they need is to call 
   fe->ops.tune() logic once;

2. Devices that have their own hardware-assisted zigzag logic.
   Those are handled via DVBFE_ALGO_CUSTOM logic. Those use
   an special callback: fe->ops.search(fe).

3. Devices that require the Kernel to do zigzag (DVBFE_ALGO_SW).
   Those should set max_drift and other fields, in order to
   setup the zigzag steps.

In other words, a device driver which uses DVBFE_ALGO_SW should 
provide the vars that are needed for the zigzag to work, as
otherwise, the software zigzag would be just wasting time, as
it won't be different than a device driver using DVBFE_ALGO_HW.

What the above patch does is to generate a warning when
DVBFE_ALGO_SW is used without setting the frequency shift,
which is an uAPI/kAPI violation. On such cases, it will fallback
to DVBFE_ALGO_HW.

The main issue is that testing this patch is not trivial.
As you pointed, it can cause regressions. So, instead of this
patch, I'll merge one that will just print a warning. We need
to fix the frontend drivers case by case.

Thanks,
Mauro

[PATCH] media: dvb_frontend: warn if frontend driver has API issues

[Mauro Carvalho Chehab]

the kAPI for a frontend can use 3 different tuning methods:

1. The hardware tracks internally frequency shifts via its
   own internal zigzag logic;
2. The hardware has a custom zigzag method, implemented via
   fe search() ops;
3. The hardware doesn't have any internal zigzag logic. So,
   the Kernel needs to implement it.

Drivers that use the in-kernel software zigzag are required to
provide some parameters for the zigzag code to work. Failing
to do that will just make the Kernel to tune several times
to the very same frequency, delaying the tuning time for
no good reason. This is actually a kAPI violation
(and an uAPI one, as the frequency shift is exported to the
uAPI).

Emit a warning on such case, as the driver needs to be fixed.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/dvb-core/dvb_frontend.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 6aaa4d5a504d..0c0af4bd2256 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -481,6 +481,11 @@ static void dvb_frontend_swzigzag(struct dvb_frontend *fe)
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache, tmp;
 
+
+	if (fepriv->max_drift)
+		dev_warn_once(fe->dvb->device,
+			      "Frontend requested software zigzag, but didn't set the frequency step size\n");
+
 	/* if we've got no parameters, just keep idling */
 	if (fepriv->state & FESTATE_IDLE) {
 		fepriv->delay = 3 * HZ;
-- 
2.30.2

Re: [RFC 0/4] Don't do tuning zigzag using the very same frequency

[Mauro Carvalho Chehab]

Em Wed, 17 Jun 2020 20:52:10 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Marc reported on IRC that the zigzag code is trying to tune several times using
> the same frequency with si2168. Well, this is not how this would be supposed
> to do: it should try with different frequencies each time.
> 
> Change the core to use the one-shot mode if the frontend doesn't report a
> frequency step. This will default to the current behavior, except that tuning
> should be faster.
> 
> Yet, probably the right thing to do is to implement a frequency shift at such
> frontends, as otherwise  tuning may have problems. So, produce a warning
> on such cases, in order for the FE driver to be fixed.
> 


> Mauro Carvalho Chehab (4):
>   media: atomisp: fix identation at I2C Kconfig menu
>   media: atomisp: fix help message for ISP2401 selection

Those two patches are unrelated. Please ignore it on the context of this RFC.


Thanks,
Mauro