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@xxxxxxxxxxxxxxxx> 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
