[PATCH] [RFC] em28xx: Fix dual transport stream use

[PATCH] [RFC] em28xx: Fix dual transport stream use

[Brad Love]

When dual transport stream support was added the call to set
alt mode on the USB interface was moved to em28xx_dvb_init.
This was reported to break streaming for a device, so the
call was re-added to em28xx_start_streaming.

Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950")

This regression fix however broke dual transport stream support.
When a tuner starts streaming it sets alt mode on the USB interface.
The problem is both tuners share the same USB interface, so when
the second tuner becomes active and sets alt mode on the interface
it kills streaming on the other port.

It was suggested add a refcount somewhere and only set alt mode if
no tuners are currently active on the interface. This requires
sharing some state amongst both tuner devices, with appropriate
locking.

What I've done here is the following:
- Add a usage_count pointer to struct em28xx
- Share usage_count between both em28xx devices
- Only set alt mode if usage_count is zero
- Increment usage_count when each tuner becomes active
- Decrement usage_count when a tuner becomes idle

With usage_count in the main em28xx struct, locking is handled as
follows:
- if a secondary tuner exists, lock dev->dev_next->lock
- if no secondary tuner exists, lock dev->lock

By using the above scheme a single tuner device, will lock itself,
the first tuner in a dual tuner device will lock the second tuner,
and the second tuner in a dual tuner device will lock itself aka
the second tuner instance.

This is a perhaps a little hacky, which is why I've added the RFC.
A quick solution was required though, so I don't fix a couple
newer Hauppauge devices, just to break a lot of older ones.


Brad Love (1):
  em28xx: Fix dual transport stream operation

 drivers/media/usb/em28xx/em28xx-cards.c |  6 ++++-
 drivers/media/usb/em28xx/em28xx-dvb.c   | 47 +++++++++++++++++++++++++++++++--
 drivers/media/usb/em28xx/em28xx.h       |  1 +
 3 files changed, 51 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH] em28xx: Fix dual transport stream operation

[Brad Love]

Addresses the following, which introduced a regression itself:

Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950")

The regression fix breaks dual transport stream support. When a
tuner starts streaming it sets alt mode on the USB interface.
The problem is both tuners share the same USB interface, so when
the second tuner becomes active and sets alt mode on the interface
it kills streaming on the other port.

It was suggested to add a refcounter and only set alt mode if no
tuners are currently active on the interface. This requires
sharing some state amongst both tuner devices, with appropriate
locking.

What I've done here is the following:
- Add a usage_count pointer to struct em28xx
- Share usage_count between both em28xx devices
- Only set alt mode if usage_count is zero
- Increment usage_count when each tuner becomes active
- Decrement usage_count when a tuner becomes idle

With usage_count in the main em28xx struct, locking is handled as
follows:
- if a secondary tuner exists, lock dev->dev_next->lock
- if no secondary tuner exists, lock dev->lock

By using the above scheme a single tuner device, will lock itself,
the first tuner in a dual tuner device will lock the second tuner,
and the second tuner in a dual tuner device will lock itself aka
the second tuner instance.

Signed-off-by: Brad Love <brad@nextdimension.cc>
---
 drivers/media/usb/em28xx/em28xx-cards.c |  6 ++++-
 drivers/media/usb/em28xx/em28xx-dvb.c   | 47 +++++++++++++++++++++++++++++++--
 drivers/media/usb/em28xx/em28xx.h       |  1 +
 3 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 48bc505..91f9787 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3374,8 +3374,10 @@ void em28xx_free_device(struct kref *ref)
 	if (!dev->disconnected)
 		em28xx_release_resources(dev);
 
-	if (dev->ts == PRIMARY_TS)
+	if (dev->ts == PRIMARY_TS) {
 		kfree(dev->alt_max_pkt_size_isoc);
+		kfree(dev->usage_count);
+	}
 
 	kfree(dev);
 }
@@ -3793,6 +3795,8 @@ static int em28xx_usb_probe(struct usb_interface *intf,
 	dev->is_audio_only = has_vendor_audio && !(has_video || has_dvb);
 	dev->has_video = has_video;
 	dev->ifnum = ifnum;
+	dev->usage_count = kcalloc(1, sizeof(unsigned int), GFP_KERNEL);
+	*dev->usage_count = 0;
 
 	dev->ts = PRIMARY_TS;
 	snprintf(dev->name, 28, "em28xx");
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index b778d8a..3a67831 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -218,7 +218,19 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		dvb_alt = dev->dvb_alt_isoc;
 	}
 
-	usb_set_interface(udev, dev->ifnum, dvb_alt);
+	if (dev->dev_next)
+		mutex_lock(&dev->dev_next->lock);
+	else
+		mutex_lock(&dev->lock);
+
+	if (*dev->usage_count == 0)
+		usb_set_interface(udev, dev->ifnum, dvb_alt);
+
+	if (dev->dev_next)
+		mutex_unlock(&dev->dev_next->lock);
+	else
+		mutex_unlock(&dev->lock);
+
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
@@ -250,6 +262,8 @@ static int em28xx_start_feed(struct dvb_demux_feed *feed)
 {
 	struct dvb_demux *demux  = feed->demux;
 	struct em28xx_dvb *dvb = demux->priv;
+	struct em28xx_i2c_bus *i2c_bus = dvb->adapter.priv;
+	struct em28xx *dev = i2c_bus->dev;
 	int rc, ret;
 
 	if (!demux->dmx.frontend)
@@ -263,6 +277,19 @@ static int em28xx_start_feed(struct dvb_demux_feed *feed)
 		ret = em28xx_start_streaming(dvb);
 		if (ret < 0)
 			rc = ret;
+		else {
+			if (dev->dev_next)
+				mutex_lock(&dev->dev_next->lock);
+			else
+				mutex_lock(&dev->lock);
+
+			*dev->usage_count = *dev->usage_count + 1;
+
+			if (dev->dev_next)
+				mutex_unlock(&dev->dev_next->lock);
+			else
+				mutex_unlock(&dev->lock);
+		}
 	}
 
 	mutex_unlock(&dvb->lock);
@@ -273,14 +300,30 @@ static int em28xx_stop_feed(struct dvb_demux_feed *feed)
 {
 	struct dvb_demux *demux  = feed->demux;
 	struct em28xx_dvb *dvb = demux->priv;
+	struct em28xx_i2c_bus *i2c_bus = dvb->adapter.priv;
+	struct em28xx *dev = i2c_bus->dev;
 	int err = 0;
 
 	mutex_lock(&dvb->lock);
 	dvb->nfeeds--;
 
-	if (!dvb->nfeeds)
+	if (!dvb->nfeeds) {
 		err = em28xx_stop_streaming(dvb);
 
+		if (dev->dev_next)
+			mutex_lock(&dev->dev_next->lock);
+		else
+			mutex_lock(&dev->lock);
+
+		*dev->usage_count = *dev->usage_count - 1;
+
+		if (dev->dev_next)
+			mutex_unlock(&dev->dev_next->lock);
+		else
+			mutex_unlock(&dev->lock);
+	}
+
+
 	mutex_unlock(&dvb->lock);
 	return err;
 }
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 953caac..d245c4f 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -775,6 +775,7 @@ struct em28xx {
 
 	struct em28xx	*dev_next;
 	int ts;
+	unsigned int *usage_count;
 };
 
 #define kref_to_dev(d) container_of(d, struct em28xx, ref)
-- 
2.7.4

Re: [PATCH] em28xx: Fix dual transport stream operation

[Brad Love]

Apologies, forgot to note this patch requires:

https://patchwork.kernel.org/patch/10492053/



On 2018-06-27 14:41, Brad Love wrote:
> Addresses the following, which introduced a regression itself:
>
> Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950")
>
> The regression fix breaks dual transport stream support. When a
> tuner starts streaming it sets alt mode on the USB interface.
> The problem is both tuners share the same USB interface, so when
> the second tuner becomes active and sets alt mode on the interface
> it kills streaming on the other port.
>
> It was suggested to add a refcounter and only set alt mode if no
> tuners are currently active on the interface. This requires
> sharing some state amongst both tuner devices, with appropriate
> locking.
>
> What I've done here is the following:
> - Add a usage_count pointer to struct em28xx
> - Share usage_count between both em28xx devices
> - Only set alt mode if usage_count is zero
> - Increment usage_count when each tuner becomes active
> - Decrement usage_count when a tuner becomes idle
>
> With usage_count in the main em28xx struct, locking is handled as
> follows:
> - if a secondary tuner exists, lock dev->dev_next->lock
> - if no secondary tuner exists, lock dev->lock
>
> By using the above scheme a single tuner device, will lock itself,
> the first tuner in a dual tuner device will lock the second tuner,
> and the second tuner in a dual tuner device will lock itself aka
> the second tuner instance.
>
> Signed-off-by: Brad Love <brad@nextdimension.cc>
> ---
>  drivers/media/usb/em28xx/em28xx-cards.c |  6 ++++-
>  drivers/media/usb/em28xx/em28xx-dvb.c   | 47 +++++++++++++++++++++++++++++++--
>  drivers/media/usb/em28xx/em28xx.h       |  1 +
>  3 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 48bc505..91f9787 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -3374,8 +3374,10 @@ void em28xx_free_device(struct kref *ref)
>  	if (!dev->disconnected)
>  		em28xx_release_resources(dev);
>  
> -	if (dev->ts == PRIMARY_TS)
> +	if (dev->ts == PRIMARY_TS) {
>  		kfree(dev->alt_max_pkt_size_isoc);
> +		kfree(dev->usage_count);
> +	}
>  
>  	kfree(dev);
>  }
> @@ -3793,6 +3795,8 @@ static int em28xx_usb_probe(struct usb_interface *intf,
>  	dev->is_audio_only = has_vendor_audio && !(has_video || has_dvb);
>  	dev->has_video = has_video;
>  	dev->ifnum = ifnum;
> +	dev->usage_count = kcalloc(1, sizeof(unsigned int), GFP_KERNEL);
> +	*dev->usage_count = 0;
>  
>  	dev->ts = PRIMARY_TS;
>  	snprintf(dev->name, 28, "em28xx");
> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> index b778d8a..3a67831 100644
> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> @@ -218,7 +218,19 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
>  		dvb_alt = dev->dvb_alt_isoc;
>  	}
>  
> -	usb_set_interface(udev, dev->ifnum, dvb_alt);
> +	if (dev->dev_next)
> +		mutex_lock(&dev->dev_next->lock);
> +	else
> +		mutex_lock(&dev->lock);
> +
> +	if (*dev->usage_count == 0)
> +		usb_set_interface(udev, dev->ifnum, dvb_alt);
> +
> +	if (dev->dev_next)
> +		mutex_unlock(&dev->dev_next->lock);
> +	else
> +		mutex_unlock(&dev->lock);
> +
>  	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
>  	if (rc < 0)
>  		return rc;
> @@ -250,6 +262,8 @@ static int em28xx_start_feed(struct dvb_demux_feed *feed)
>  {
>  	struct dvb_demux *demux  = feed->demux;
>  	struct em28xx_dvb *dvb = demux->priv;
> +	struct em28xx_i2c_bus *i2c_bus = dvb->adapter.priv;
> +	struct em28xx *dev = i2c_bus->dev;
>  	int rc, ret;
>  
>  	if (!demux->dmx.frontend)
> @@ -263,6 +277,19 @@ static int em28xx_start_feed(struct dvb_demux_feed *feed)
>  		ret = em28xx_start_streaming(dvb);
>  		if (ret < 0)
>  			rc = ret;
> +		else {
> +			if (dev->dev_next)
> +				mutex_lock(&dev->dev_next->lock);
> +			else
> +				mutex_lock(&dev->lock);
> +
> +			*dev->usage_count = *dev->usage_count + 1;
> +
> +			if (dev->dev_next)
> +				mutex_unlock(&dev->dev_next->lock);
> +			else
> +				mutex_unlock(&dev->lock);
> +		}
>  	}
>  
>  	mutex_unlock(&dvb->lock);
> @@ -273,14 +300,30 @@ static int em28xx_stop_feed(struct dvb_demux_feed *feed)
>  {
>  	struct dvb_demux *demux  = feed->demux;
>  	struct em28xx_dvb *dvb = demux->priv;
> +	struct em28xx_i2c_bus *i2c_bus = dvb->adapter.priv;
> +	struct em28xx *dev = i2c_bus->dev;
>  	int err = 0;
>  
>  	mutex_lock(&dvb->lock);
>  	dvb->nfeeds--;
>  
> -	if (!dvb->nfeeds)
> +	if (!dvb->nfeeds) {
>  		err = em28xx_stop_streaming(dvb);
>  
> +		if (dev->dev_next)
> +			mutex_lock(&dev->dev_next->lock);
> +		else
> +			mutex_lock(&dev->lock);
> +
> +		*dev->usage_count = *dev->usage_count - 1;
> +
> +		if (dev->dev_next)
> +			mutex_unlock(&dev->dev_next->lock);
> +		else
> +			mutex_unlock(&dev->lock);
> +	}
> +
> +
>  	mutex_unlock(&dvb->lock);
>  	return err;
>  }
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 953caac..d245c4f 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -775,6 +775,7 @@ struct em28xx {
>  
>  	struct em28xx	*dev_next;
>  	int ts;
> +	unsigned int *usage_count;
>  };
>  
>  #define kref_to_dev(d) container_of(d, struct em28xx, ref)

Re: [PATCH] [RFC] em28xx: Fix dual transport stream use

[Mauro Carvalho Chehab]

Em Wed, 27 Jun 2018 14:41:22 -0500
Brad Love <brad@nextdimension.cc> escreveu:

> When dual transport stream support was added the call to set
> alt mode on the USB interface was moved to em28xx_dvb_init.
> This was reported to break streaming for a device, so the
> call was re-added to em28xx_start_streaming.
> 
> Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950")
> 
> This regression fix however broke dual transport stream support.
> When a tuner starts streaming it sets alt mode on the USB interface.
> The problem is both tuners share the same USB interface, so when
> the second tuner becomes active and sets alt mode on the interface
> it kills streaming on the other port.
> 
> It was suggested add a refcount somewhere and only set alt mode if
> no tuners are currently active on the interface. This requires
> sharing some state amongst both tuner devices, with appropriate
> locking.
> 
> What I've done here is the following:
> - Add a usage_count pointer to struct em28xx
> - Share usage_count between both em28xx devices
> - Only set alt mode if usage_count is zero
> - Increment usage_count when each tuner becomes active
> - Decrement usage_count when a tuner becomes idle
> 
> With usage_count in the main em28xx struct, locking is handled as
> follows:
> - if a secondary tuner exists, lock dev->dev_next->lock
> - if no secondary tuner exists, lock dev->lock
> 
> By using the above scheme a single tuner device, will lock itself,
> the first tuner in a dual tuner device will lock the second tuner,
> and the second tuner in a dual tuner device will lock itself aka
> the second tuner instance.
> 
> This is a perhaps a little hacky, which is why I've added the RFC.
> A quick solution was required though, so I don't fix a couple
> newer Hauppauge devices, just to break a lot of older ones.

Hi Brad,

I didn't look at the patches, just at the description. IMHO,
the proposed approach won't work.

I suspect that it will require a redesign of the alternate setting
logic, but we have to handle first with the regressions.

So, I'll break this into two separate issues

1) HVR-950 x dual mode tuners regression

HVR-950 uses a model of em28xx chipset that doesn't support dual 
tuners (em2884 - I think) and has a tuner defined for analog TV. 

Even on chipsets that supports dual tuners, most of the designs 
come with just one.

An easier fix would be check if the device supports dual tuner.
That could be done either by checking dev->chip_id of via an
extra bit at em28xx cards struct:
	unsigned int has_dual_tuners:1;
that would indicate if the device has dual tuners.

Then, decide where the alternate settings logic will happen.

That would allow a simple patch that will fix regressions with
single tuner devices while keeping Digital TV support for dual
tuners working.

A fix like that should be easy to do and to backport to older
Kernels.

IMHO, we should do it as a first approach to solve the issue,
and send with c/c to -stable.

2) Fixing the alt logic to better support dual-streaming devices

After thinking about the code changes to support dual tuner devices,
I suspect that something was missing there for the dual tuner
devices.

On a single tuner device, the alternate should be set considering
those scenarios (enumerated from less bandwidth to high bandwidth):

	0) Device is not streaming
	1) FM mode - just em28xx-audio sets the hardware to stream
	   audio via ALSA.
	2) Digital TV
	3) Webcam, Analog TV or Capture stream - I'll call it ATV below
	   for simplicity.

For all scenarios, the alternate is chosen to be the closest one to
the required bandwidth, in order to reduce latency. Still, the latency
can be somewhat painful on em28xx-based cameras.

It should be noticed that, for ATV, userspace can start streaming audio
video in any order, e. g. it can start streaming audio, then switch
to TV and start streaming video. It can also do the reverse: start
streaming video, and then start audio. The current logic was designed
and tested to support this.

I'm not sure if em28xx can support the secondary tuner on all
three different modes. If it can, with 2 tuners, that expands to 16
different combinations:

	======== =======
	tuner 0  tuner 1
	======== =======
	none     none
	none     FM
	none     DTV
	none     ATV
	FM       none
	FM       FM
	FM       DTV
	FM       ATV
	DTV      none
	DTV      FM
	DTV      DTV
	DTV      ATV
	ATV      none
	ATV      FM
	ATV      DTV
	ATV      ATV
	======== =======

Worse than that, when one tuner is active and another one starts to be
used, it may need to change the alternate, with affects the first tuner.
Not sure what happens with the pending URBs if the alternate is changed
at runtime. If this is not allowed, the driver will need to cancel all
pending URBs, switch the alternate and re-submit URBs for the first
active tuner.

The logic should also consider the case where both tuners are streamed
and one stops. Perhaps it could just keep it using the dual-tuner
alternate until both stops.

In any case, for proper alternate setting, I'm expecting to see a patch
that would touch em28xx-dvb, em28xx-audio and em28xx-v4l2, as
whatever logic it needs, it has to consider all supported combinations.

Such patch would likely be too complex for -stable.

Thanks,
Mauro

Re: [PATCH] [RFC] em28xx: Fix dual transport stream use

[Brad Love]

Hi Mauro,


On 2018-06-28 04:10, Mauro Carvalho Chehab wrote:
> Em Wed, 27 Jun 2018 14:41:22 -0500
> Brad Love <brad@nextdimension.cc> escreveu:
>
>> When dual transport stream support was added the call to set
>> alt mode on the USB interface was moved to em28xx_dvb_init.
>> This was reported to break streaming for a device, so the
>> call was re-added to em28xx_start_streaming.
>>
>> Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950")
>>
>> This regression fix however broke dual transport stream support.
>> When a tuner starts streaming it sets alt mode on the USB interface.
>> The problem is both tuners share the same USB interface, so when
>> the second tuner becomes active and sets alt mode on the interface
>> it kills streaming on the other port.
>>
>> It was suggested add a refcount somewhere and only set alt mode if
>> no tuners are currently active on the interface. This requires
>> sharing some state amongst both tuner devices, with appropriate
>> locking.
>>
>> What I've done here is the following:
>> - Add a usage_count pointer to struct em28xx
>> - Share usage_count between both em28xx devices
>> - Only set alt mode if usage_count is zero
>> - Increment usage_count when each tuner becomes active
>> - Decrement usage_count when a tuner becomes idle
>>
>> With usage_count in the main em28xx struct, locking is handled as
>> follows:
>> - if a secondary tuner exists, lock dev->dev_next->lock
>> - if no secondary tuner exists, lock dev->lock
>>
>> By using the above scheme a single tuner device, will lock itself,
>> the first tuner in a dual tuner device will lock the second tuner,
>> and the second tuner in a dual tuner device will lock itself aka
>> the second tuner instance.
>>
>> This is a perhaps a little hacky, which is why I've added the RFC.
>> A quick solution was required though, so I don't fix a couple
>> newer Hauppauge devices, just to break a lot of older ones.
> Hi Brad,
>
> I didn't look at the patches, just at the description. IMHO,
> the proposed approach won't work.
>
> I suspect that it will require a redesign of the alternate setting
> logic, but we have to handle first with the regressions.
>
> So, I'll break this into two separate issues
>
> 1) HVR-950 x dual mode tuners regression
>
> HVR-950 uses a model of em28xx chipset that doesn't support dual 
> tuners (em2884 - I think) and has a tuner defined for analog TV. 
>
> Even on chipsets that supports dual tuners, most of the designs 
> come with just one.
>
> An easier fix would be check if the device supports dual tuner.
> That could be done either by checking dev->chip_id of via an
> extra bit at em28xx cards struct:
> 	unsigned int has_dual_tuners:1;
> that would indicate if the device has dual tuners.
>
> Then, decide where the alternate settings logic will happen.
>
> That would allow a simple patch that will fix regressions with
> single tuner devices while keeping Digital TV support for dual
> tuners working.
>
> A fix like that should be easy to do and to backport to older
> Kernels.
>
> IMHO, we should do it as a first approach to solve the issue,
> and send with c/c to -stable.
>
> 2) Fixing the alt logic to better support dual-streaming devices
>
> After thinking about the code changes to support dual tuner devices,
> I suspect that something was missing there for the dual tuner
> devices.
>
> On a single tuner device, the alternate should be set considering
> those scenarios (enumerated from less bandwidth to high bandwidth):
>
> 	0) Device is not streaming
> 	1) FM mode - just em28xx-audio sets the hardware to stream
> 	   audio via ALSA.
> 	2) Digital TV
> 	3) Webcam, Analog TV or Capture stream - I'll call it ATV below
> 	   for simplicity.
>
> For all scenarios, the alternate is chosen to be the closest one to
> the required bandwidth, in order to reduce latency. Still, the latency
> can be somewhat painful on em28xx-based cameras.
>
> It should be noticed that, for ATV, userspace can start streaming audio
> video in any order, e. g. it can start streaming audio, then switch
> to TV and start streaming video. It can also do the reverse: start
> streaming video, and then start audio. The current logic was designed
> and tested to support this.
>
> I'm not sure if em28xx can support the secondary tuner on all
> three different modes. If it can, with 2 tuners, that expands to 16
> different combinations:
>
> 	======== =======
> 	tuner 0  tuner 1
> 	======== =======
> 	none     none
> 	none     FM
> 	none     DTV
> 	none     ATV
> 	FM       none
> 	FM       FM
> 	FM       DTV
> 	FM       ATV
> 	DTV      none
> 	DTV      FM
> 	DTV      DTV
> 	DTV      ATV
> 	ATV      none
> 	ATV      FM
> 	ATV      DTV
> 	ATV      ATV
> 	======== =======
>
> Worse than that, when one tuner is active and another one starts to be
> used, it may need to change the alternate, with affects the first tuner.
> Not sure what happens with the pending URBs if the alternate is changed
> at runtime. If this is not allowed, the driver will need to cancel all
> pending URBs, switch the alternate and re-submit URBs for the first
> active tuner.
>
> The logic should also consider the case where both tuners are streamed
> and one stops. Perhaps it could just keep it using the dual-tuner
> alternate until both stops.
>
> In any case, for proper alternate setting, I'm expecting to see a patch
> that would touch em28xx-dvb, em28xx-audio and em28xx-v4l2, as
> whatever logic it needs, it has to consider all supported combinations.
>
> Such patch would likely be too complex for -stable.
>
> Thanks,
> Mauro

Thanks for the in depth comments. I'm fully aware that my quick hack was
likely not acceptable, but I have to provide builds that support DualHD
as well as older (far more DualHD than older, but still) Hauppauge
hardware, so I needed something to handle the regression asap. I won't
be able to do an extensive driver wide audit of the alt mode setting
until August due to travel.

Both Isoc and Bulk DualHD models are reported working perfectly fine in
dvb, without setting alt mode in em28xx_start_streaming and only setting
it once in em28xx_dvb_init. Whereas the em28xx-video driver sets alt
mode to 0 on close, neither em28xx-audio nor em28xx-dvb do so, perhaps
this is why DualHD maintain operation? I was tempted to only set alt
mode in em28xx_start_streaming if dev->board->has_dual_ts is false, but
I was not sure if there were some cases where an Isoc DualHD would fail
if alt mode were not set in em28xx_start_streaming.

Cheers,

Brad

Re: [PATCH] [RFC] em28xx: Fix dual transport stream use

[Brad Love]

Hi Mauro,


On 2018-06-28 09:23, Brad Love wrote:
> Hi Mauro,
>
>
> On 2018-06-28 04:10, Mauro Carvalho Chehab wrote:
>> Em Wed, 27 Jun 2018 14:41:22 -0500
>> Brad Love <brad@nextdimension.cc> escreveu:
>>
>>> When dual transport stream support was added the call to set
>>> alt mode on the USB interface was moved to em28xx_dvb_init.
>>> This was reported to break streaming for a device, so the
>>> call was re-added to em28xx_start_streaming.
>>>
>>> Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950")
>>>
>>> This regression fix however broke dual transport stream support.
>>> When a tuner starts streaming it sets alt mode on the USB interface.
>>> The problem is both tuners share the same USB interface, so when
>>> the second tuner becomes active and sets alt mode on the interface
>>> it kills streaming on the other port.
>>>
>>> It was suggested add a refcount somewhere and only set alt mode if
>>> no tuners are currently active on the interface. This requires
>>> sharing some state amongst both tuner devices, with appropriate
>>> locking.
>>>
>>> What I've done here is the following:
>>> - Add a usage_count pointer to struct em28xx
>>> - Share usage_count between both em28xx devices
>>> - Only set alt mode if usage_count is zero
>>> - Increment usage_count when each tuner becomes active
>>> - Decrement usage_count when a tuner becomes idle
>>>
>>> With usage_count in the main em28xx struct, locking is handled as
>>> follows:
>>> - if a secondary tuner exists, lock dev->dev_next->lock
>>> - if no secondary tuner exists, lock dev->lock
>>>
>>> By using the above scheme a single tuner device, will lock itself,
>>> the first tuner in a dual tuner device will lock the second tuner,
>>> and the second tuner in a dual tuner device will lock itself aka
>>> the second tuner instance.
>>>
>>> This is a perhaps a little hacky, which is why I've added the RFC.
>>> A quick solution was required though, so I don't fix a couple
>>> newer Hauppauge devices, just to break a lot of older ones.
>> Hi Brad,
>>
>> I didn't look at the patches, just at the description. IMHO,
>> the proposed approach won't work.
>>
>> I suspect that it will require a redesign of the alternate setting
>> logic, but we have to handle first with the regressions.
>>
>> So, I'll break this into two separate issues
>>
>> 1) HVR-950 x dual mode tuners regression
>>
>> HVR-950 uses a model of em28xx chipset that doesn't support dual 
>> tuners (em2884 - I think) and has a tuner defined for analog TV. 
>>
>> Even on chipsets that supports dual tuners, most of the designs 
>> come with just one.
>>
>> An easier fix would be check if the device supports dual tuner.
>> That could be done either by checking dev->chip_id of via an
>> extra bit at em28xx cards struct:
>> 	unsigned int has_dual_tuners:1;
>> that would indicate if the device has dual tuners.
>>
>> Then, decide where the alternate settings logic will happen.
>>
>> That would allow a simple patch that will fix regressions with
>> single tuner devices while keeping Digital TV support for dual
>> tuners working.
>>
>> A fix like that should be easy to do and to backport to older
>> Kernels.
>>
>> IMHO, we should do it as a first approach to solve the issue,
>> and send with c/c to -stable.
>>
>> 2) Fixing the alt logic to better support dual-streaming devices
>>
>> After thinking about the code changes to support dual tuner devices,
>> I suspect that something was missing there for the dual tuner
>> devices.
>>
>> On a single tuner device, the alternate should be set considering
>> those scenarios (enumerated from less bandwidth to high bandwidth):
>>
>> 	0) Device is not streaming
>> 	1) FM mode - just em28xx-audio sets the hardware to stream
>> 	   audio via ALSA.
>> 	2) Digital TV
>> 	3) Webcam, Analog TV or Capture stream - I'll call it ATV below
>> 	   for simplicity.
>>
>> For all scenarios, the alternate is chosen to be the closest one to
>> the required bandwidth, in order to reduce latency. Still, the latency
>> can be somewhat painful on em28xx-based cameras.
>>
>> It should be noticed that, for ATV, userspace can start streaming audio
>> video in any order, e. g. it can start streaming audio, then switch
>> to TV and start streaming video. It can also do the reverse: start
>> streaming video, and then start audio. The current logic was designed
>> and tested to support this.
>>
>> I'm not sure if em28xx can support the secondary tuner on all
>> three different modes. If it can, with 2 tuners, that expands to 16
>> different combinations:
>>
>> 	======== =======
>> 	tuner 0  tuner 1
>> 	======== =======
>> 	none     none
>> 	none     FM
>> 	none     DTV
>> 	none     ATV
>> 	FM       none
>> 	FM       FM
>> 	FM       DTV
>> 	FM       ATV
>> 	DTV      none
>> 	DTV      FM
>> 	DTV      DTV
>> 	DTV      ATV
>> 	ATV      none
>> 	ATV      FM
>> 	ATV      DTV
>> 	ATV      ATV
>> 	======== =======
>>
>> Worse than that, when one tuner is active and another one starts to be
>> used, it may need to change the alternate, with affects the first tuner.
>> Not sure what happens with the pending URBs if the alternate is changed
>> at runtime. If this is not allowed, the driver will need to cancel all
>> pending URBs, switch the alternate and re-submit URBs for the first
>> active tuner.
>>
>> The logic should also consider the case where both tuners are streamed
>> and one stops. Perhaps it could just keep it using the dual-tuner
>> alternate until both stops.
>>
>> In any case, for proper alternate setting, I'm expecting to see a patch
>> that would touch em28xx-dvb, em28xx-audio and em28xx-v4l2, as
>> whatever logic it needs, it has to consider all supported combinations.
>>
>> Such patch would likely be too complex for -stable.
>>
>> Thanks,
>> Mauro
> Thanks for the in depth comments. I'm fully aware that my quick hack was
> likely not acceptable, but I have to provide builds that support DualHD
> as well as older (far more DualHD than older, but still) Hauppauge
> hardware, so I needed something to handle the regression asap. I won't
> be able to do an extensive driver wide audit of the alt mode setting
> until August due to travel.
>
> Both Isoc and Bulk DualHD models are reported working perfectly fine in
> dvb, without setting alt mode in em28xx_start_streaming and only setting
> it once in em28xx_dvb_init. Whereas the em28xx-video driver sets alt
> mode to 0 on close, neither em28xx-audio nor em28xx-dvb do so, perhaps
> this is why DualHD maintain operation? I was tempted to only set alt
> mode in em28xx_start_streaming if dev->board->has_dual_ts is false, but
> I was not sure if there were some cases where an Isoc DualHD would fail
> if alt mode were not set in em28xx_start_streaming.
>
> Cheers,
>
> Brad
>

A v2 patch has been submitted that addresses *only* the regression. If a
device is not a dual tuner model, then alt mode is not set in
start_streaming. DualHD models, both isoc and bulk, correctly work with
alt mode set once in dvb_init. So this is a much simpler patch, without
the extra hacky fluff.

Cheers,

Brad

Re: [PATCH] [RFC] em28xx: Fix dual transport stream use

[Mauro Carvalho Chehab]

Em Thu, 28 Jun 2018 12:33:41 -0500
Brad Love <brad@nextdimension.cc> escreveu:

> Hi Mauro,
> 
> 
> On 2018-06-28 09:23, Brad Love wrote:
> > Hi Mauro,
> >
> >
> > On 2018-06-28 04:10, Mauro Carvalho Chehab wrote:  
> >> Em Wed, 27 Jun 2018 14:41:22 -0500
> >> Brad Love <brad@nextdimension.cc> escreveu:
> >>  
> >>> When dual transport stream support was added the call to set
> >>> alt mode on the USB interface was moved to em28xx_dvb_init.
> >>> This was reported to break streaming for a device, so the
> >>> call was re-added to em28xx_start_streaming.
> >>>
> >>> Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950")
> >>>
> >>> This regression fix however broke dual transport stream support.
> >>> When a tuner starts streaming it sets alt mode on the USB interface.
> >>> The problem is both tuners share the same USB interface, so when
> >>> the second tuner becomes active and sets alt mode on the interface
> >>> it kills streaming on the other port.
> >>>
> >>> It was suggested add a refcount somewhere and only set alt mode if
> >>> no tuners are currently active on the interface. This requires
> >>> sharing some state amongst both tuner devices, with appropriate
> >>> locking.
> >>>
> >>> What I've done here is the following:
> >>> - Add a usage_count pointer to struct em28xx
> >>> - Share usage_count between both em28xx devices
> >>> - Only set alt mode if usage_count is zero
> >>> - Increment usage_count when each tuner becomes active
> >>> - Decrement usage_count when a tuner becomes idle
> >>>
> >>> With usage_count in the main em28xx struct, locking is handled as
> >>> follows:
> >>> - if a secondary tuner exists, lock dev->dev_next->lock
> >>> - if no secondary tuner exists, lock dev->lock
> >>>
> >>> By using the above scheme a single tuner device, will lock itself,
> >>> the first tuner in a dual tuner device will lock the second tuner,
> >>> and the second tuner in a dual tuner device will lock itself aka
> >>> the second tuner instance.
> >>>
> >>> This is a perhaps a little hacky, which is why I've added the RFC.
> >>> A quick solution was required though, so I don't fix a couple
> >>> newer Hauppauge devices, just to break a lot of older ones.  
> >> Hi Brad,
> >>
> >> I didn't look at the patches, just at the description. IMHO,
> >> the proposed approach won't work.
> >>
> >> I suspect that it will require a redesign of the alternate setting
> >> logic, but we have to handle first with the regressions.
> >>
> >> So, I'll break this into two separate issues
> >>
> >> 1) HVR-950 x dual mode tuners regression
> >>
> >> HVR-950 uses a model of em28xx chipset that doesn't support dual 
> >> tuners (em2884 - I think) and has a tuner defined for analog TV. 
> >>
> >> Even on chipsets that supports dual tuners, most of the designs 
> >> come with just one.
> >>
> >> An easier fix would be check if the device supports dual tuner.
> >> That could be done either by checking dev->chip_id of via an
> >> extra bit at em28xx cards struct:
> >> 	unsigned int has_dual_tuners:1;
> >> that would indicate if the device has dual tuners.
> >>
> >> Then, decide where the alternate settings logic will happen.
> >>
> >> That would allow a simple patch that will fix regressions with
> >> single tuner devices while keeping Digital TV support for dual
> >> tuners working.
> >>
> >> A fix like that should be easy to do and to backport to older
> >> Kernels.
> >>
> >> IMHO, we should do it as a first approach to solve the issue,
> >> and send with c/c to -stable.
> >>
> >> 2) Fixing the alt logic to better support dual-streaming devices
> >>
> >> After thinking about the code changes to support dual tuner devices,
> >> I suspect that something was missing there for the dual tuner
> >> devices.
> >>
> >> On a single tuner device, the alternate should be set considering
> >> those scenarios (enumerated from less bandwidth to high bandwidth):
> >>
> >> 	0) Device is not streaming
> >> 	1) FM mode - just em28xx-audio sets the hardware to stream
> >> 	   audio via ALSA.
> >> 	2) Digital TV
> >> 	3) Webcam, Analog TV or Capture stream - I'll call it ATV below
> >> 	   for simplicity.
> >>
> >> For all scenarios, the alternate is chosen to be the closest one to
> >> the required bandwidth, in order to reduce latency. Still, the latency
> >> can be somewhat painful on em28xx-based cameras.
> >>
> >> It should be noticed that, for ATV, userspace can start streaming audio
> >> video in any order, e. g. it can start streaming audio, then switch
> >> to TV and start streaming video. It can also do the reverse: start
> >> streaming video, and then start audio. The current logic was designed
> >> and tested to support this.
> >>
> >> I'm not sure if em28xx can support the secondary tuner on all
> >> three different modes. If it can, with 2 tuners, that expands to 16
> >> different combinations:
> >>
> >> 	======== =======
> >> 	tuner 0  tuner 1
> >> 	======== =======
> >> 	none     none
> >> 	none     FM
> >> 	none     DTV
> >> 	none     ATV
> >> 	FM       none
> >> 	FM       FM
> >> 	FM       DTV
> >> 	FM       ATV
> >> 	DTV      none
> >> 	DTV      FM
> >> 	DTV      DTV
> >> 	DTV      ATV
> >> 	ATV      none
> >> 	ATV      FM
> >> 	ATV      DTV
> >> 	ATV      ATV
> >> 	======== =======
> >>
> >> Worse than that, when one tuner is active and another one starts to be
> >> used, it may need to change the alternate, with affects the first tuner.
> >> Not sure what happens with the pending URBs if the alternate is changed
> >> at runtime. If this is not allowed, the driver will need to cancel all
> >> pending URBs, switch the alternate and re-submit URBs for the first
> >> active tuner.
> >>
> >> The logic should also consider the case where both tuners are streamed
> >> and one stops. Perhaps it could just keep it using the dual-tuner
> >> alternate until both stops.
> >>
> >> In any case, for proper alternate setting, I'm expecting to see a patch
> >> that would touch em28xx-dvb, em28xx-audio and em28xx-v4l2, as
> >> whatever logic it needs, it has to consider all supported combinations.
> >>
> >> Such patch would likely be too complex for -stable.
> >>
> >> Thanks,
> >> Mauro  
> > Thanks for the in depth comments. I'm fully aware that my quick hack was
> > likely not acceptable, but I have to provide builds that support DualHD
> > as well as older (far more DualHD than older, but still) Hauppauge
> > hardware, so I needed something to handle the regression asap. I won't
> > be able to do an extensive driver wide audit of the alt mode setting
> > until August due to travel.
> >
> > Both Isoc and Bulk DualHD models are reported working perfectly fine in
> > dvb, without setting alt mode in em28xx_start_streaming and only setting
> > it once in em28xx_dvb_init. Whereas the em28xx-video driver sets alt
> > mode to 0 on close, neither em28xx-audio nor em28xx-dvb do so, perhaps
> > this is why DualHD maintain operation? I was tempted to only set alt
> > mode in em28xx_start_streaming if dev->board->has_dual_ts is false, but
> > I was not sure if there were some cases where an Isoc DualHD would fail
> > if alt mode were not set in em28xx_start_streaming.
> >
> > Cheers,
> >
> > Brad
> >  
> 
> A v2 patch has been submitted that addresses *only* the regression. If a
> device is not a dual tuner model, then alt mode is not set in
> start_streaming. DualHD models, both isoc and bulk, correctly work with
> alt mode set once in dvb_init. So this is a much simpler patch, without
> the extra hacky fluff.

Yeah, patch seems a lot better, thanks!

As the only two dual tuner models we have right now has
dev->tuner_type = ABSENT and don't have connectors for S-video
or composite capture, e. g. INPUT(0)->type == 0, this patch should 
be enough for what we currently have.

Still, we'll need to address it in a different way if non-digital
TV modes are supported for dual tuner devices.

So, I would also add a patch like the enclosed one.

Thanks,
Mauro

[PATCH] em28xx-cards: disable V4L2 mode for dual tuners

Right now, the code that calculates alternate modes is not ready
for devices with dual tuners. That's ok, as we currently don't
have any such devices, but better to add a warning for such
case, as, if anyone adds such device, the logic will need to
be reviewed.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 6c8438311d3b..5178ab4f1a68 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3861,6 +3861,17 @@ static int em28xx_usb_probe(struct usb_interface *intf,
 		dev->has_video = false;
 	}
 
+	if (dev->board.has_dual_ts &&
+	    (dev->tuner_type != TUNER_ABSENT || INPUT(0)->type)) {
+		/*
+		 * The logic with sets alternate is not ready for dual-tuners
+		 * with analog modes.
+		 */
+		dev_err(&intf->dev,
+			"We currently don't support analog TV or stream capture on dual tuners.\n");
+		has_video = false;
+	}
+
 	/* Select USB transfer types to use */
 	if (has_video) {
 		if (!dev->analog_ep_isoc || (try_bulk && dev->analog_ep_bulk))

Re: [PATCH] [RFC] em28xx: Fix dual transport stream use

[Brad Love]

Hi Mauro,


On 2018-06-28 13:43, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Jun 2018 12:33:41 -0500
> Brad Love <brad@nextdimension.cc> escreveu:
>
>> Hi Mauro,
>>
>>
>> On 2018-06-28 09:23, Brad Love wrote:
>>> Hi Mauro,
>>>
>>>
>>> On 2018-06-28 04:10, Mauro Carvalho Chehab wrote:  
>>>> Em Wed, 27 Jun 2018 14:41:22 -0500
>>>> Brad Love <brad@nextdimension.cc> escreveu:
>>>>  
>>>>> When dual transport stream support was added the call to set
>>>>> alt mode on the USB interface was moved to em28xx_dvb_init.
>>>>> This was reported to break streaming for a device, so the
>>>>> call was re-added to em28xx_start_streaming.
>>>>>
>>>>> Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950")
>>>>>
>>>>> This regression fix however broke dual transport stream support.
>>>>> When a tuner starts streaming it sets alt mode on the USB interface.
>>>>> The problem is both tuners share the same USB interface, so when
>>>>> the second tuner becomes active and sets alt mode on the interface
>>>>> it kills streaming on the other port.
>>>>>
>>>>> It was suggested add a refcount somewhere and only set alt mode if
>>>>> no tuners are currently active on the interface. This requires
>>>>> sharing some state amongst both tuner devices, with appropriate
>>>>> locking.
>>>>>
>>>>> What I've done here is the following:
>>>>> - Add a usage_count pointer to struct em28xx
>>>>> - Share usage_count between both em28xx devices
>>>>> - Only set alt mode if usage_count is zero
>>>>> - Increment usage_count when each tuner becomes active
>>>>> - Decrement usage_count when a tuner becomes idle
>>>>>
>>>>> With usage_count in the main em28xx struct, locking is handled as
>>>>> follows:
>>>>> - if a secondary tuner exists, lock dev->dev_next->lock
>>>>> - if no secondary tuner exists, lock dev->lock
>>>>>
>>>>> By using the above scheme a single tuner device, will lock itself,
>>>>> the first tuner in a dual tuner device will lock the second tuner,
>>>>> and the second tuner in a dual tuner device will lock itself aka
>>>>> the second tuner instance.
>>>>>
>>>>> This is a perhaps a little hacky, which is why I've added the RFC.
>>>>> A quick solution was required though, so I don't fix a couple
>>>>> newer Hauppauge devices, just to break a lot of older ones.  
>>>> Hi Brad,
>>>>
>>>> I didn't look at the patches, just at the description. IMHO,
>>>> the proposed approach won't work.
>>>>
>>>> I suspect that it will require a redesign of the alternate setting
>>>> logic, but we have to handle first with the regressions.
>>>>
>>>> So, I'll break this into two separate issues
>>>>
>>>> 1) HVR-950 x dual mode tuners regression
>>>>
>>>> HVR-950 uses a model of em28xx chipset that doesn't support dual 
>>>> tuners (em2884 - I think) and has a tuner defined for analog TV. 
>>>>
>>>> Even on chipsets that supports dual tuners, most of the designs 
>>>> come with just one.
>>>>
>>>> An easier fix would be check if the device supports dual tuner.
>>>> That could be done either by checking dev->chip_id of via an
>>>> extra bit at em28xx cards struct:
>>>> 	unsigned int has_dual_tuners:1;
>>>> that would indicate if the device has dual tuners.
>>>>
>>>> Then, decide where the alternate settings logic will happen.
>>>>
>>>> That would allow a simple patch that will fix regressions with
>>>> single tuner devices while keeping Digital TV support for dual
>>>> tuners working.
>>>>
>>>> A fix like that should be easy to do and to backport to older
>>>> Kernels.
>>>>
>>>> IMHO, we should do it as a first approach to solve the issue,
>>>> and send with c/c to -stable.
>>>>
>>>> 2) Fixing the alt logic to better support dual-streaming devices
>>>>
>>>> After thinking about the code changes to support dual tuner devices,
>>>> I suspect that something was missing there for the dual tuner
>>>> devices.
>>>>
>>>> On a single tuner device, the alternate should be set considering
>>>> those scenarios (enumerated from less bandwidth to high bandwidth):
>>>>
>>>> 	0) Device is not streaming
>>>> 	1) FM mode - just em28xx-audio sets the hardware to stream
>>>> 	   audio via ALSA.
>>>> 	2) Digital TV
>>>> 	3) Webcam, Analog TV or Capture stream - I'll call it ATV below
>>>> 	   for simplicity.
>>>>
>>>> For all scenarios, the alternate is chosen to be the closest one to
>>>> the required bandwidth, in order to reduce latency. Still, the latency
>>>> can be somewhat painful on em28xx-based cameras.
>>>>
>>>> It should be noticed that, for ATV, userspace can start streaming audio
>>>> video in any order, e. g. it can start streaming audio, then switch
>>>> to TV and start streaming video. It can also do the reverse: start
>>>> streaming video, and then start audio. The current logic was designed
>>>> and tested to support this.
>>>>
>>>> I'm not sure if em28xx can support the secondary tuner on all
>>>> three different modes. If it can, with 2 tuners, that expands to 16
>>>> different combinations:
>>>>
>>>> 	======== =======
>>>> 	tuner 0  tuner 1
>>>> 	======== =======
>>>> 	none     none
>>>> 	none     FM
>>>> 	none     DTV
>>>> 	none     ATV
>>>> 	FM       none
>>>> 	FM       FM
>>>> 	FM       DTV
>>>> 	FM       ATV
>>>> 	DTV      none
>>>> 	DTV      FM
>>>> 	DTV      DTV
>>>> 	DTV      ATV
>>>> 	ATV      none
>>>> 	ATV      FM
>>>> 	ATV      DTV
>>>> 	ATV      ATV
>>>> 	======== =======
>>>>
>>>> Worse than that, when one tuner is active and another one starts to be
>>>> used, it may need to change the alternate, with affects the first tuner.
>>>> Not sure what happens with the pending URBs if the alternate is changed
>>>> at runtime. If this is not allowed, the driver will need to cancel all
>>>> pending URBs, switch the alternate and re-submit URBs for the first
>>>> active tuner.
>>>>
>>>> The logic should also consider the case where both tuners are streamed
>>>> and one stops. Perhaps it could just keep it using the dual-tuner
>>>> alternate until both stops.
>>>>
>>>> In any case, for proper alternate setting, I'm expecting to see a patch
>>>> that would touch em28xx-dvb, em28xx-audio and em28xx-v4l2, as
>>>> whatever logic it needs, it has to consider all supported combinations.
>>>>
>>>> Such patch would likely be too complex for -stable.
>>>>
>>>> Thanks,
>>>> Mauro  
>>> Thanks for the in depth comments. I'm fully aware that my quick hack was
>>> likely not acceptable, but I have to provide builds that support DualHD
>>> as well as older (far more DualHD than older, but still) Hauppauge
>>> hardware, so I needed something to handle the regression asap. I won't
>>> be able to do an extensive driver wide audit of the alt mode setting
>>> until August due to travel.
>>>
>>> Both Isoc and Bulk DualHD models are reported working perfectly fine in
>>> dvb, without setting alt mode in em28xx_start_streaming and only setting
>>> it once in em28xx_dvb_init. Whereas the em28xx-video driver sets alt
>>> mode to 0 on close, neither em28xx-audio nor em28xx-dvb do so, perhaps
>>> this is why DualHD maintain operation? I was tempted to only set alt
>>> mode in em28xx_start_streaming if dev->board->has_dual_ts is false, but
>>> I was not sure if there were some cases where an Isoc DualHD would fail
>>> if alt mode were not set in em28xx_start_streaming.
>>>
>>> Cheers,
>>>
>>> Brad
>>>  
>> A v2 patch has been submitted that addresses *only* the regression. If a
>> device is not a dual tuner model, then alt mode is not set in
>> start_streaming. DualHD models, both isoc and bulk, correctly work with
>> alt mode set once in dvb_init. So this is a much simpler patch, without
>> the extra hacky fluff.
> Yeah, patch seems a lot better, thanks!
>
> As the only two dual tuner models we have right now has
> dev->tuner_type = ABSENT and don't have connectors for S-video
> or composite capture, e. g. INPUT(0)->type == 0, this patch should 
> be enough for what we currently have.
>
> Still, we'll need to address it in a different way if non-digital
> TV modes are supported for dual tuner devices.
>
> So, I would also add a patch like the enclosed one.
>
> Thanks,
> Mauro
>
> [PATCH] em28xx-cards: disable V4L2 mode for dual tuners
>
> Right now, the code that calculates alternate modes is not ready
> for devices with dual tuners. That's ok, as we currently don't
> have any such devices, but better to add a warning for such
> case, as, if anyone adds such device, the logic will need to
> be reviewed.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
>
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 6c8438311d3b..5178ab4f1a68 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -3861,6 +3861,17 @@ static int em28xx_usb_probe(struct usb_interface *intf,
>  		dev->has_video = false;
>  	}
>  
> +	if (dev->board.has_dual_ts &&
> +	    (dev->tuner_type != TUNER_ABSENT || INPUT(0)->type)) {
> +		/*
> +		 * The logic with sets alternate is not ready for dual-tuners

Which, not with.


> +		 * with analog modes.
> +		 */
> +		dev_err(&intf->dev,
> +			"We currently don't support analog TV or stream capture on dual tuners.\n");
> +		has_video = false;
> +	}
> +
>  	/* Select USB transfer types to use */
>  	if (has_video) {
>  		if (!dev->analog_ep_isoc || (try_bulk && dev->analog_ep_bulk))
>
>


This extra patch finishes covering the situation nicely I think. Thanks.
I did make one comment inline above, for a spelling error.

Cheers,

Brad


Acked-by: Brad Love <brad@nextdimension.cc>