Message-ID: <f673844e-e2a5-3d4c-df8a-eb71100b626d@maciej.szmigiero.name>
Date:   Wed, 24 Apr 2019 22:02:45 +0200
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Michael Krufky <mkrufky@...uxtv.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Andy Walls <awalls@...metrocast.net>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-media@...r.kernel.org
Subject: Re: [PATCH v9 2/4] [media] cxusb: implement Medion MD95700 digital /
 analog coexistence

On 24.04.2019 14:45, Hans Verkuil wrote:
> On 4/16/19 1:38 AM, Maciej S. Szmigiero wrote:
>> On 12.04.2019 10:56, Hans Verkuil wrote:
>>> On 3/30/19 12:51 AM, Maciej S. Szmigiero wrote:
>>>> This patch prepares cxusb driver for supporting the analog part of
>>>> Medion 95700 (previously only the digital - DVB - mode was supported).
>>>>
>>>> Specifically, it adds support for:
>>>> * switching the device between analog and digital modes of operation,
>>>> * enforcing that only one mode is active at the same time due to hardware
>>>> limitations.
>>>>
>>>> Actual implementation of the analog mode will be provided by the next
>>>> commit.
>>>>
>>>> Signed-off-by: Maciej S. Szmigiero <mail@...iej.szmigiero.name>
>>>> ---
(..)
>>>> index 8056053c9ab0..01987ec5e0c5 100644
>>>> --- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
>>>> +++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
>>>> @@ -15,6 +15,7 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed *dvbdmxfeed, int onoff)
>>>>  {
>>>>  	struct dvb_usb_adapter *adap = dvbdmxfeed->demux->priv;
>>>>  	int newfeedcount, ret;
>>>> +	bool streaming_ctrl_no_urb;
>>>>  
>>>>  	if (adap == NULL)
>>>>  		return -ENODEV;
>>>> @@ -24,12 +25,16 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed *dvbdmxfeed, int onoff)
>>>>  		return -EINVAL;
>>>>  	}
>>>>  
>>>> +	streaming_ctrl_no_urb = adap->props.fe[adap->active_fe].caps &
>>>> +		DVB_USB_ADAP_STREAMING_CTRL_NO_URB;
>>>>  	newfeedcount = adap->feedcount + (onoff ? 1 : -1);
>>>>  
>>>>  	/* stop feed before setting a new pid if there will be no pid anymore */
>>>>  	if (newfeedcount == 0) {
>>>>  		deb_ts("stop feeding\n");
>>>> -		usb_urb_kill(&adap->fe_adap[adap->active_fe].stream);
>>>> +
>>>> +		if (streaming_ctrl_no_urb)
>>>
>>> Is this test right? Shouldn't it be !streaming_ctrl_no_urb in order to keep the
>>> current (non-medion) behavior?
>>>
>>>> +			usb_urb_kill(&adap->fe_adap[adap->active_fe].stream);
>>>>  
>>>>  		if (adap->props.fe[adap->active_fe].streaming_ctrl != NULL) {
>>>>  			ret = adap->props.fe[adap->active_fe].streaming_ctrl(adap, 0);
>>>> @@ -38,6 +43,9 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed *dvbdmxfeed, int onoff)
>>>>  				return ret;
>>>>  			}
>>>>  		}
>>>> +
>>>> +		if (!streaming_ctrl_no_urb)
>>>
>>> And then this would have to be inverted as well.
>>
>> The newly added flag "DVB_USB_ADAP_STREAMING_CTRL_NO_URB" has the
>> following meaning:
>> If it is set the order of operations in dvb_usb_ctrl_feed() looks like
>> this:
>> 1) Call the driver streaming_ctrl(1) callback to enable streaming,
>> 2) Submit DVB data URBs,
>> (streaming happens)
>> 3) Kill DVB data URBs,
>> 4) Call the driver streaming_ctrl(0) callback to disable streaming.
>>
>> This is needed for Medion because:
>> a) The device could already be open in the analog mode when
>> streaming_ctrl(1) tries to acquire it in the digital mode.
>> In this case it is important that no DVB URBs are scheduled before
>> giving the callback chance to report an error,
>>
>> b) The device could get open in the analog mode as soon as
>> streaming_ctrl(0) drops the last digital mode reference to it so all
>> DVB data URB must have already been terminated at this point.
>>
>> If the aforementioned flag is unset, however, the order of operations in
>> dvb_usb_ctrl_feed() looks like this:
>> 1) Submit DVB data URBs,
>> 2) Call the driver streaming_ctrl(1) callback to enable streaming,
>> (streaming happens)
>> 3) Call the driver streaming_ctrl(0) callback to disable streaming,
>> 4) Kill DVB data URBs.
>>
>> Previously, the order was always like this:
>> 1) Submit DVB data URBs,
>> 2) Call the driver streaming_ctrl(1) callback to enable streaming,
>> (streaming happens)
>> 3) Kill DVB data URBs,
>> 4) Call the driver streaming_ctrl(0) callback to disable streaming.
>>
>> You can see that this was asymmetric - streaming_ctrl(1) is called with
>> data URBs already active but they are killed before streaming_ctrl(0)
>> gets called, so switching only the submit part on "STREAMING_CTRL_NO_URB"
>> would make this flag operation only half-correct.
>>
>> I have audited existing drivers that use dvb-usb framework and none of
>> them do anything besides a synchronous USB control transfer or a
>> synchronous USB bulk transfer to an endpoint different from the one that
>> DVB data uses in streaming_ctrl(0) callback.
>> Additionally, streaming_ctrl(1) and streaming_ctrl(0) paths are usually
>> very similar in the driver code, so if streaming_ctrl(1) runs fine with
>> data URBs being already active there is no reason to think
>> streaming_ctrl(0) will have a problem with this.
> 
> Is there anything wrong with always doing:
> 
> 1) Call the driver streaming_ctrl(1) callback to enable streaming,
> 2) Submit DVB data URBs,
> (streaming happens)
> 3) Kill DVB data URBs,
> 4) Call the driver streaming_ctrl(0) callback to disable streaming.

In principle this should be fine - looking at the existing drivers
doesn't reveal anything in their streaming_ctrl(1) callbacks that could
depend on URBs submission order.

> Regards,
> 
> 	Hans
> 

Thanks,
Maciej

Powered by blists - more mailing lists