[v5,8/9] media: uvcvideo: Rename uvc_{un,}init_video()

  From: Kieran Bingham <kieran.bingham@ideasonboard.com>

We have both uvc_init_video() and uvc_video_init() calls which can be
quite confusing to determine the process for each. Now that video
uvc_video_enable() has been renamed to uvc_video_start_streaming(),
adapt these calls to suit the new flow.

Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
uvc_video_stop().

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_video.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
  

Hi Kieran,

Thank you for the patch.

On Tuesday, 6 November 2018 23:27:19 EET Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> We have both uvc_init_video() and uvc_video_init() calls which can be
> quite confusing to determine the process for each. Now that video
> uvc_video_enable() has been renamed to uvc_video_start_streaming(),
> adapt these calls to suit the new flow.
> 
> Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
> uvc_video_stop().

I agree that these functions are badly named and should be renamed. We are 
however entering the nitpicking territory :-) The two functions do more that 
starting and stopping, they also allocate and free URBs and the associated 
buffers. It could also be argued that they don't actually start and stop 
anything, as beyond URB management, they just queue the URBs initially and 
kill them. I thus wonder if we could come up with better names.

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 0d35e933856a..020022e6ade4 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1641,7 +1641,7 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
> *stream, /*
>   * Uninitialize isochronous/bulk URBs and free transfer buffers.
>   */
> -static void uvc_uninit_video(struct uvc_streaming *stream, int
> free_buffers) +static void uvc_video_stop(struct uvc_streaming *stream, int
> free_buffers) {
>  	struct uvc_urb *uvc_urb;
> 
> @@ -1718,7 +1718,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
> *stream,
> 
>  		urb = usb_alloc_urb(npackets, gfp_flags);
>  		if (urb == NULL) {
> -			uvc_uninit_video(stream, 1);
> +			uvc_video_stop(stream, 1);
>  			return -ENOMEM;
>  		}
> 
> @@ -1786,7 +1786,7 @@ static int uvc_init_video_bulk(struct uvc_streaming
> *stream,
> 
>  		urb = usb_alloc_urb(0, gfp_flags);
>  		if (urb == NULL) {
> -			uvc_uninit_video(stream, 1);
> +			uvc_video_stop(stream, 1);
>  			return -ENOMEM;
>  		}
> 
> @@ -1806,7 +1806,7 @@ static int uvc_init_video_bulk(struct uvc_streaming
> *stream, /*
>   * Initialize isochronous/bulk URBs and allocate transfer buffers.
>   */
> -static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
> +static int uvc_video_start(struct uvc_streaming *stream, gfp_t gfp_flags)
>  {
>  	struct usb_interface *intf = stream->intf;
>  	struct usb_host_endpoint *ep;
> @@ -1894,7 +1894,7 @@ static int uvc_init_video(struct uvc_streaming
> *stream, gfp_t gfp_flags) if (ret < 0) {
>  			uvc_printk(KERN_ERR, "Failed to submit URB %u "
>  					"(%d).\n", i, ret);
> -			uvc_uninit_video(stream, 1);
> +			uvc_video_stop(stream, 1);
>  			return ret;
>  		}
>  	}
> @@ -1925,7 +1925,7 @@ int uvc_video_suspend(struct uvc_streaming *stream)
>  		return 0;
> 
>  	stream->frozen = 1;
> -	uvc_uninit_video(stream, 0);
> +	uvc_video_stop(stream, 0);
>  	usb_set_interface(stream->dev->udev, stream->intfnum, 0);
>  	return 0;
>  }
> @@ -1961,7 +1961,7 @@ int uvc_video_resume(struct uvc_streaming *stream, int
> reset) if (ret < 0)
>  		return ret;
> 
> -	return uvc_init_video(stream, GFP_NOIO);
> +	return uvc_video_start(stream, GFP_NOIO);
>  }
> 
>  /* ------------------------------------------------------------------------
> @@ -2095,7 +2095,7 @@ int uvc_video_start_streaming(struct uvc_streaming
> *stream) if (ret < 0)
>  		goto error_commit;
> 
> -	ret = uvc_init_video(stream, GFP_KERNEL);
> +	ret = uvc_video_start(stream, GFP_KERNEL);
>  	if (ret < 0)
>  		goto error_video;
> 
> @@ -2111,7 +2111,7 @@ int uvc_video_start_streaming(struct uvc_streaming
> *stream)
> 
>  int uvc_video_stop_streaming(struct uvc_streaming *stream)
>  {
> -	uvc_uninit_video(stream, 1);
> +	uvc_video_stop(stream, 1);
>  	if (stream->intf->num_altsetting > 1) {
>  		usb_set_interface(stream->dev->udev,
>  				  stream->intfnum, 0);
  

Hi Laurent,

On 06/11/2018 23:13, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tuesday, 6 November 2018 23:27:19 EET Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> We have both uvc_init_video() and uvc_video_init() calls which can be
>> quite confusing to determine the process for each. Now that video
>> uvc_video_enable() has been renamed to uvc_video_start_streaming(),
>> adapt these calls to suit the new flow.
>>
>> Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
>> uvc_video_stop().
> 
> I agree that these functions are badly named and should be renamed. We are 
> however entering the nitpicking territory :-) The two functions do more that 
> starting and stopping, they also allocate and free URBs and the associated 
> buffers. It could also be argued that they don't actually start and stop 
> anything, as beyond URB management, they just queue the URBs initially and 
> kill them. I thus wonder if we could come up with better names.

Well the act of killing (poisoning now) the URBs will certainly stop the
stream, but I guess submitting the URBs isn't necessarily the key act to
starting the stream.

I believe that needs the interface to be set correctly, and the buffers
to be available?

Although - I've just double-checked uvc_{video_start,init_video}() and
that is indeed what it does?

 - start stats
 - Initialise endpoints
   - Perform allocations
 - Submit URBs

Am I missing something? Is there another step that is pivotal to
starting the USB packet/urb stream flow after this point ?


Is it not true that the USB stack will start processing data at
submitting URB completion callbacks after the end of uvc_video_start();
and will no longer process data at the end of uvc_video_stop() (and thus
no more completion callbacks)?

 (That's a real question to verify my interpretation)

To me - these functions feel like the real 'start' and 'stop' components
of the data stream - hence my choice in naming.


Is your concern that you would like the functions to be more descriptive
over their other actions such as? :

  uvc_video_initialise_start()
  uvc_video_allocate_init_start()

Or something else? (I don't think those two are good names though)

--
Kieran

>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  drivers/media/usb/uvc/uvc_video.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index 0d35e933856a..020022e6ade4 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1641,7 +1641,7 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
>> *stream, /*
>>   * Uninitialize isochronous/bulk URBs and free transfer buffers.
>>   */
>> -static void uvc_uninit_video(struct uvc_streaming *stream, int
>> free_buffers) +static void uvc_video_stop(struct uvc_streaming *stream, int
>> free_buffers) {
>>  	struct uvc_urb *uvc_urb;
>>
>> @@ -1718,7 +1718,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
>> *stream,
>>
>>  		urb = usb_alloc_urb(npackets, gfp_flags);
>>  		if (urb == NULL) {
>> -			uvc_uninit_video(stream, 1);
>> +			uvc_video_stop(stream, 1);
>>  			return -ENOMEM;
>>  		}
>>
>> @@ -1786,7 +1786,7 @@ static int uvc_init_video_bulk(struct uvc_streaming
>> *stream,
>>
>>  		urb = usb_alloc_urb(0, gfp_flags);
>>  		if (urb == NULL) {
>> -			uvc_uninit_video(stream, 1);
>> +			uvc_video_stop(stream, 1);
>>  			return -ENOMEM;
>>  		}
>>
>> @@ -1806,7 +1806,7 @@ static int uvc_init_video_bulk(struct uvc_streaming
>> *stream, /*
>>   * Initialize isochronous/bulk URBs and allocate transfer buffers.
>>   */
>> -static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
>> +static int uvc_video_start(struct uvc_streaming *stream, gfp_t gfp_flags)
>>  {
>>  	struct usb_interface *intf = stream->intf;
>>  	struct usb_host_endpoint *ep;
>> @@ -1894,7 +1894,7 @@ static int uvc_init_video(struct uvc_streaming
>> *stream, gfp_t gfp_flags) if (ret < 0) {
>>  			uvc_printk(KERN_ERR, "Failed to submit URB %u "
>>  					"(%d).\n", i, ret);
>> -			uvc_uninit_video(stream, 1);
>> +			uvc_video_stop(stream, 1);
>>  			return ret;
>>  		}
>>  	}
>> @@ -1925,7 +1925,7 @@ int uvc_video_suspend(struct uvc_streaming *stream)
>>  		return 0;
>>
>>  	stream->frozen = 1;
>> -	uvc_uninit_video(stream, 0);
>> +	uvc_video_stop(stream, 0);
>>  	usb_set_interface(stream->dev->udev, stream->intfnum, 0);
>>  	return 0;
>>  }
>> @@ -1961,7 +1961,7 @@ int uvc_video_resume(struct uvc_streaming *stream, int
>> reset) if (ret < 0)
>>  		return ret;
>>
>> -	return uvc_init_video(stream, GFP_NOIO);
>> +	return uvc_video_start(stream, GFP_NOIO);
>>  }
>>
>>  /* ------------------------------------------------------------------------
>> @@ -2095,7 +2095,7 @@ int uvc_video_start_streaming(struct uvc_streaming
>> *stream) if (ret < 0)
>>  		goto error_commit;
>>
>> -	ret = uvc_init_video(stream, GFP_KERNEL);
>> +	ret = uvc_video_start(stream, GFP_KERNEL);
>>  	if (ret < 0)
>>  		goto error_video;
>>
>> @@ -2111,7 +2111,7 @@ int uvc_video_start_streaming(struct uvc_streaming
>> *stream)
>>
>>  int uvc_video_stop_streaming(struct uvc_streaming *stream)
>>  {
>> -	uvc_uninit_video(stream, 1);
>> +	uvc_video_stop(stream, 1);
>>  	if (stream->intf->num_altsetting > 1) {
>>  		usb_set_interface(stream->dev->udev,
>>  				  stream->intfnum, 0);
> 
>
  

Hi Kieran,

On Wednesday, 7 November 2018 16:30:46 EET Kieran Bingham wrote:
> On 06/11/2018 23:13, Laurent Pinchart wrote:
> > On Tuesday, 6 November 2018 23:27:19 EET Kieran Bingham wrote:
> >> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> 
> >> We have both uvc_init_video() and uvc_video_init() calls which can be
> >> quite confusing to determine the process for each. Now that video
> >> uvc_video_enable() has been renamed to uvc_video_start_streaming(),
> >> adapt these calls to suit the new flow.
> >> 
> >> Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
> >> uvc_video_stop().
> > 
> > I agree that these functions are badly named and should be renamed. We are
> > however entering the nitpicking territory :-) The two functions do more
> > that starting and stopping, they also allocate and free URBs and the
> > associated buffers. It could also be argued that they don't actually
> > start and stop anything, as beyond URB management, they just queue the
> > URBs initially and kill them. I thus wonder if we could come up with
> > better names.
> 
> Well the act of killing (poisoning now) the URBs will certainly stop the
> stream, but I guess submitting the URBs isn't necessarily the key act to
> starting the stream.
> 
> I believe that needs the interface to be set correctly, and the buffers
> to be available?
> 
> Although - I've just double-checked uvc_{video_start,init_video}() and
> that is indeed what it does?
> 
>  - start stats
>  - Initialise endpoints
>    - Perform allocations
>  - Submit URBs
> 
> Am I missing something? Is there another step that is pivotal to
> starting the USB packet/urb stream flow after this point ?
> 
> 
> Is it not true that the USB stack will start processing data at
> submitting URB completion callbacks after the end of uvc_video_start();
> and will no longer process data at the end of uvc_video_stop() (and thus
> no more completion callbacks)?
> 
>  (That's a real question to verify my interpretation)
> 
> To me - these functions feel like the real 'start' and 'stop' components
> of the data stream - hence my choice in naming.

The other part of the start operation is committing the streaming parameters 
(see uvc_video_start_streaming()). For the stop operation it's issuing a 
SET_INTERFACE or CLEAR_FEATURE(HALT) request (see uvc_video_stop_streaming()).

> Is your concern that you would like the functions to be more descriptive
> over their other actions such as? :
> 
>   uvc_video_initialise_start()
>   uvc_video_allocate_init_start()
> 
> Or something else? (I don't think those two are good names though)

Probably something else :-) A possibly equally bad proposal would be 
uvc_video_start_transfer() and uvc_video_stop_transfer().
  

On Wed, 2018-11-07 at 22:25 +0200, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wednesday, 7 November 2018 16:30:46 EET Kieran Bingham wrote:
> > On 06/11/2018 23:13, Laurent Pinchart wrote:
> > > On Tuesday, 6 November 2018 23:27:19 EET Kieran Bingham wrote:
> > > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > 
> > > > We have both uvc_init_video() and uvc_video_init() calls which can be
> > > > quite confusing to determine the process for each. Now that video
> > > > uvc_video_enable() has been renamed to uvc_video_start_streaming(),
> > > > adapt these calls to suit the new flow.
> > > > 
> > > > Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
> > > > uvc_video_stop().
> > > 
> > > I agree that these functions are badly named and should be renamed. We are
> > > however entering the nitpicking territory :-) The two functions do more
> > > that starting and stopping, they also allocate and free URBs and the
> > > associated buffers. It could also be argued that they don't actually
> > > start and stop anything, as beyond URB management, they just queue the
> > > URBs initially and kill them. I thus wonder if we could come up with
> > > better names.
> > 
> > Well the act of killing (poisoning now) the URBs will certainly stop the
> > stream, but I guess submitting the URBs isn't necessarily the key act to
> > starting the stream.
> > 
> > I believe that needs the interface to be set correctly, and the buffers
> > to be available?
> > 
> > Although - I've just double-checked uvc_{video_start,init_video}() and
> > that is indeed what it does?
> > 
> >  - start stats
> >  - Initialise endpoints
> >    - Perform allocations
> >  - Submit URBs
> > 
> > Am I missing something? Is there another step that is pivotal to
> > starting the USB packet/urb stream flow after this point ?
> > 
> > 
> > Is it not true that the USB stack will start processing data at
> > submitting URB completion callbacks after the end of uvc_video_start();
> > and will no longer process data at the end of uvc_video_stop() (and thus
> > no more completion callbacks)?
> > 
> >  (That's a real question to verify my interpretation)
> > 
> > To me - these functions feel like the real 'start' and 'stop' components
> > of the data stream - hence my choice in naming.
> 
> The other part of the start operation is committing the streaming parameters 
> (see uvc_video_start_streaming()). For the stop operation it's issuing a 
> SET_INTERFACE or CLEAR_FEATURE(HALT) request (see uvc_video_stop_streaming()).
> 
> > Is your concern that you would like the functions to be more descriptive
> > over their other actions such as? :
> > 
> >   uvc_video_initialise_start()
> >   uvc_video_allocate_init_start()
> > 
> > Or something else? (I don't think those two are good names though)
> 
> Probably something else :-) A possibly equally bad proposal would be 
> uvc_video_start_transfer() and uvc_video_stop_transfer().

I think this is still better than what we have now.

At least it contains "transfer" to make it clear it deals with the
isoc/bulk transfer setup/teardown part of streaming, not actually
starting or stopping the device streaming.

regards
Philipp
  

On 09/11/2018 15:41, Philipp Zabel wrote:
> On Wed, 2018-11-07 at 22:25 +0200, Laurent Pinchart wrote:
>> Hi Kieran,
>>
>> On Wednesday, 7 November 2018 16:30:46 EET Kieran Bingham wrote:
>>> On 06/11/2018 23:13, Laurent Pinchart wrote:
>>>> On Tuesday, 6 November 2018 23:27:19 EET Kieran Bingham wrote:
>>>>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>>>
>>>>> We have both uvc_init_video() and uvc_video_init() calls which can be
>>>>> quite confusing to determine the process for each. Now that video
>>>>> uvc_video_enable() has been renamed to uvc_video_start_streaming(),
>>>>> adapt these calls to suit the new flow.
>>>>>
>>>>> Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
>>>>> uvc_video_stop().
>>>>
>>>> I agree that these functions are badly named and should be renamed. We are
>>>> however entering the nitpicking territory :-) The two functions do more
>>>> that starting and stopping, they also allocate and free URBs and the
>>>> associated buffers. It could also be argued that they don't actually
>>>> start and stop anything, as beyond URB management, they just queue the
>>>> URBs initially and kill them. I thus wonder if we could come up with
>>>> better names.
>>>
>>> Well the act of killing (poisoning now) the URBs will certainly stop the
>>> stream, but I guess submitting the URBs isn't necessarily the key act to
>>> starting the stream.
>>>
>>> I believe that needs the interface to be set correctly, and the buffers
>>> to be available?
>>>
>>> Although - I've just double-checked uvc_{video_start,init_video}() and
>>> that is indeed what it does?
>>>
>>>  - start stats
>>>  - Initialise endpoints
>>>    - Perform allocations
>>>  - Submit URBs
>>>
>>> Am I missing something? Is there another step that is pivotal to
>>> starting the USB packet/urb stream flow after this point ?
>>>
>>>
>>> Is it not true that the USB stack will start processing data at
>>> submitting URB completion callbacks after the end of uvc_video_start();
>>> and will no longer process data at the end of uvc_video_stop() (and thus
>>> no more completion callbacks)?
>>>
>>>  (That's a real question to verify my interpretation)
>>>
>>> To me - these functions feel like the real 'start' and 'stop' components
>>> of the data stream - hence my choice in naming.
>>
>> The other part of the start operation is committing the streaming parameters 
>> (see uvc_video_start_streaming()). For the stop operation it's issuing a 
>> SET_INTERFACE or CLEAR_FEATURE(HALT) request (see uvc_video_stop_streaming()).
>>
>>> Is your concern that you would like the functions to be more descriptive
>>> over their other actions such as? :
>>>
>>>   uvc_video_initialise_start()
>>>   uvc_video_allocate_init_start()
>>>
>>> Or something else? (I don't think those two are good names though)
>>
>> Probably something else :-) A possibly equally bad proposal would be 
>> uvc_video_start_transfer() and uvc_video_stop_transfer().
> 
> I think this is still better than what we have now.
> 
> At least it contains "transfer" to make it clear it deals with the
> isoc/bulk transfer setup/teardown part of streaming, not actually
> starting or stopping the device streaming.

Ok - uvc_video_start_transfer() and uvc_video_stop_transfer() it shall
be then :)

v6 with fixed stream object / workqueue lifetime and updates to the last
two patches coming ... soon :)
--
Kieran



> 
> regards
> Philipp
>