Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

2015-01-20 Thread William Towle 



On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote:

On Mon, 19 Jan 2015, William Towle wrote:

  in the patchset Ben linked to above we think
we have the appropriate loops: a for loop for queue_buf[], and
list_for_each_safe() for anything left in priv-capture; this is
consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with
pointers unlinked from priv-capture. This in turn suggests that we
are right not to call list_del_init() in both of
rcar_vin_stop_streaming()'s loops ... as long as I've correctly
interpreted the code and everyone's feedback thus far.


I'm referring to this comment by Hans Verkuil of 14 August last year:


I'm assuming all buffers that are queued to the driver via buf_queue() are
linked into priv-capture. So you would typically call vb2_buffer_done
when you are walking that list:

list_for_each_safe(buf_head, tmp, priv-capture) {
// usually you go from buf_head to the real buffer struct
// containing a vb2_buffer struct
vb2_buffer_done(buf-vb, VB2_BUF_STATE_ERROR);
list_del_init(buf_head);
}

Please use this rather than looking into internal vb2_queue
datastructures.


I think, that's the right way to implement that clean up loop.


Hi,
  I was describing the code in my latest patch; we start with
rcar_vin_stop_streaming() having a list_for_each_safe() loop like
that above, and leave that loop in place but add statements to it.

  I add another loop to rcar_vin_stop_streaming() [which you will
have seen -in the patch Ben published in my absence over Christmas-
was particularly inelegant initially], but it can't be rewritten in
the same way.  The new version is undeniably neater, though.

  We believe the latest patches take Hans' comment above into
account properly, and we are looking into his latest suggestion at
the moment.

Thanks again,
  Wills.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

2015-01-19 Thread William Towle 


On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote:


On Thu, 18 Dec 2014, Ben Hutchings wrote:

Well, I thought that too.  Will's submission from last week has that
change:
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009



Anyway, yes, that looks better! But I would still consider keeping buffers
on the list in .buf_clean(), in which case you can remove it. And walk the
list instead of the VB2 internal buffer array, as others have pointed out.


Hi Guennadi,
  Thanks for the clarification. Ian (when he was with us) did say it
was particularly difficult to understand WTH this driver was doing.

  Regarding your first point, if it's safe to skip the actions left
in rcar_vin_videobuf_release() then I will do a further rework to
remove it completely.

  Regarding your second, in the patchset Ben linked to above we think
we have the appropriate loops: a for loop for queue_buf[], and
list_for_each_safe() for anything left in priv-capture; this is
consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with
pointers unlinked from priv-capture. This in turn suggests that we
are right not to call list_del_init() in both of
rcar_vin_stop_streaming()'s loops ... as long as I've correctly
interpreted the code and everyone's feedback thus far.


Cheers,
  Wills.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

2015-01-19 Thread Guennadi Liakhovetski 
Hi,

On Mon, 19 Jan 2015, William Towle wrote:

 
 On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote:
 
On Thu, 18 Dec 2014, Ben Hutchings wrote:
   Well, I thought that too.  Will's submission from last week has that
   change:
   http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009
 
  Anyway, yes, that looks better! But I would still consider keeping buffers
  on the list in .buf_clean(), in which case you can remove it. And walk the
  list instead of the VB2 internal buffer array, as others have pointed out.
 
 Hi Guennadi,
   Thanks for the clarification. Ian (when he was with us) did say it
 was particularly difficult to understand WTH this driver was doing.
 
   Regarding your first point, if it's safe to skip the actions left
 in rcar_vin_videobuf_release() then I will do a further rework to
 remove it completely.
 
   Regarding your second, in the patchset Ben linked to above we think
 we have the appropriate loops: a for loop for queue_buf[], and
 list_for_each_safe() for anything left in priv-capture; this is
 consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with
 pointers unlinked from priv-capture. This in turn suggests that we
 are right not to call list_del_init() in both of
 rcar_vin_stop_streaming()'s loops ... as long as I've correctly
 interpreted the code and everyone's feedback thus far.

I'm referring to this comment by Hans Verkuil of 14 August last year:

 I'm assuming all buffers that are queued to the driver via buf_queue() are
 linked into priv-capture. So you would typically call vb2_buffer_done
 when you are walking that list:
 
   list_for_each_safe(buf_head, tmp, priv-capture) {
   // usually you go from buf_head to the real buffer struct
   // containing a vb2_buffer struct
   vb2_buffer_done(buf-vb, VB2_BUF_STATE_ERROR);
   list_del_init(buf_head);
   }
 
 Please use this rather than looking into internal vb2_queue 
 datastructures.

I think, that's the right way to implement that clean up loop.

Thanks
Guennadi
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

2015-01-19 Thread Hans Verkuil 
On 01/19/2015 03:11 PM, William Towle wrote:
 
 On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote:
 
 On Thu, 18 Dec 2014, Ben Hutchings wrote:
 Well, I thought that too.  Will's submission from last week has that
 change:
 http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009
 
 Anyway, yes, that looks better! But I would still consider keeping buffers
 on the list in .buf_clean(), in which case you can remove it. And walk the
 list instead of the VB2 internal buffer array, as others have pointed out.
 
 Hi Guennadi,
Thanks for the clarification. Ian (when he was with us) did say it
 was particularly difficult to understand WTH this driver was doing.
 
Regarding your first point, if it's safe to skip the actions left
 in rcar_vin_videobuf_release() then I will do a further rework to
 remove it completely.

Yes, that's safe. Just remove it altogether.

The buf_init and buf_release ops are matching ops that are normally only
used if you have to do per-buffer initialization and/or release. These
are only called when the buffer memory changes. In most drivers including
this one it's not needed at all.

The same is true for rcar_vin_videobuf_init: it's pointless since the
list initialization is done implicitly when you add the buffer to a
list with list_add_tail(). Just drop the function.

Regards,

Hans

 
Regarding your second, in the patchset Ben linked to above we think
 we have the appropriate loops: a for loop for queue_buf[], and
 list_for_each_safe() for anything left in priv-capture; this is
 consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with
 pointers unlinked from priv-capture. This in turn suggests that we
 are right not to call list_del_init() in both of
 rcar_vin_stop_streaming()'s loops ... as long as I've correctly
 interpreted the code and everyone's feedback thus far.
 
 
 Cheers,
Wills.
 --
 To unsubscribe from this list: send the line unsubscribe linux-media in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

2015-01-19 Thread Guennadi Liakhovetski 
On Mon, 19 Jan 2015, Ben Hutchings wrote:

 On Sun, 2015-01-18 at 22:23 +0100, Guennadi Liakhovetski wrote:
  On Thu, 18 Dec 2014, Ben Hutchings wrote:
  
   From: William Towle william.to...@codethink.co.uk
   
   Move the buffer state test in the .buf_cleanup handler into
   .stop_streaming so that a) the vb2_queue API is not subverted, and
   b) tracking of active-state buffers via priv-queue_buf[] is handled
   as early as is possible
  
  Huh... Sorry, patches 1, 2, 3, and 5 of this series look like a strange 
  way to get from the present state to the destination. They all have to be 
  merged IMHO. 
 [...]
 
 Well, I thought that too.  Will's submission from last week has that
 change:
 http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009

Hm... interesting, why I didn't get those mails in my INBOX, although I do 
see myself in CC... Only got them from the list, and no, I don't have 
avoid copies enabled.

Anyway, yes, that looks better! But I would still consider keeping buffers 
on the list in .buf_clean(), in which case you can remove it. And walk the 
list instead of the VB2 internal buffer array, as others have pointed out.

Thanks
Guennadi
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

2015-01-19 Thread Ben Hutchings 
On Sun, 2015-01-18 at 22:23 +0100, Guennadi Liakhovetski wrote:
 On Thu, 18 Dec 2014, Ben Hutchings wrote:
 
  From: William Towle william.to...@codethink.co.uk
  
  Move the buffer state test in the .buf_cleanup handler into
  .stop_streaming so that a) the vb2_queue API is not subverted, and
  b) tracking of active-state buffers via priv-queue_buf[] is handled
  as early as is possible
 
 Huh... Sorry, patches 1, 2, 3, and 5 of this series look like a strange 
 way to get from the present state to the destination. They all have to be 
 merged IMHO. 
[...]

Well, I thought that too.  Will's submission from last week has that
change:
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009

Ben.


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

2015-01-18 Thread Guennadi Liakhovetski 
On Thu, 18 Dec 2014, Ben Hutchings wrote:

 From: William Towle william.to...@codethink.co.uk
 
 Move the buffer state test in the .buf_cleanup handler into
 .stop_streaming so that a) the vb2_queue API is not subverted, and
 b) tracking of active-state buffers via priv-queue_buf[] is handled
 as early as is possible

Huh... Sorry, patches 1, 2, 3, and 5 of this series look like a strange 
way to get from the present state to the destination. They all have to be 
merged IMHO. 

Further, looking at vb2, I don't think active buffers should ever stay 
active until .buf_cleanup() is called. I think rcar_vin's 
.stop_streaming() should stop the hardware, wait until it's stopped, then 
walk all queued buffers and return errors for them, while removing them 
from the list at the same time. Then I don't think a .buf_cleanup() method 
is needed there at all.

Thanks
Guennadi

 
 Signed-off-by: William Towle william.to...@codethink.co.uk
 ---
  drivers/media/platform/soc_camera/rcar_vin.c |   36 
 ++
  1 file changed, 14 insertions(+), 22 deletions(-)
 
 diff --git a/drivers/media/platform/soc_camera/rcar_vin.c 
 b/drivers/media/platform/soc_camera/rcar_vin.c
 index 20dbedf..bf60074 100644
 --- a/drivers/media/platform/soc_camera/rcar_vin.c
 +++ b/drivers/media/platform/soc_camera/rcar_vin.c
 @@ -486,28 +486,8 @@ static void rcar_vin_videobuf_release(struct vb2_buffer 
 *vb)
   struct soc_camera_device *icd = soc_camera_from_vb2q(vb-vb2_queue);
   struct soc_camera_host *ici = to_soc_camera_host(icd-parent);
   struct rcar_vin_priv *priv = ici-priv;
 - unsigned int i;
 - int buf_in_use = 0;
 - spin_lock_irq(priv-lock);
 -
 - /* Is the buffer in use by the VIN hardware? */
 - for (i = 0; i  MAX_BUFFER_NUM; i++) {
 - if (priv-queue_buf[i] == vb) {
 - buf_in_use = 1;
 - break;
 - }
 - }
  
 - if (buf_in_use) {
 - rcar_vin_wait_stop_streaming(priv);
 -
 - /*
 -  * Capturing has now stopped. The buffer we have been asked
 -  * to release could be any of the current buffers in use, so
 -  * release all buffers that are in use by HW
 -  */
 - priv-queue_buf[i] = NULL;
 - }
 + spin_lock_irq(priv-lock);
  
   list_del_init(to_buf_list(vb));
  
 @@ -533,8 +513,20 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
   rcar_vin_wait_stop_streaming(priv);
  
   for (i = 0; i  vq-num_buffers; ++i)
 - if (vq-bufs[i]-state == VB2_BUF_STATE_ACTIVE)
 + if (vq-bufs[i]-state == VB2_BUF_STATE_ACTIVE) {
 + int j;
 +
 + /*  Is this a buffer we have told the
 +  *  hardware about? Update the associated
 +  *  list, if so
 +  */
 + for (j = 0; j  MAX_BUFFER_NUM; j++) {
 + if (priv-queue_buf[j] == vq-bufs[i]) {
 + priv-queue_buf[j] = NULL;
 + }
 + }
   vb2_buffer_done(vq-bufs[i], VB2_BUF_STATE_ERROR);
 + }
  
   list_for_each_safe(buf_head, tmp, priv-capture)
   list_del_init(buf_head);
 -- 
 1.7.10.4
 
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

2014-12-18 Thread Sergei Shtylyov 

Hello.

On 12/18/2014 05:50 PM, Ben Hutchings wrote:


From: William Towle william.to...@codethink.co.uk



Move the buffer state test in the .buf_cleanup handler into
.stop_streaming so that a) the vb2_queue API is not subverted, and
b) tracking of active-state buffers via priv-queue_buf[] is handled
as early as is possible



Signed-off-by: William Towle william.to...@codethink.co.uk
---
  drivers/media/platform/soc_camera/rcar_vin.c |   36 ++
  1 file changed, 14 insertions(+), 22 deletions(-)



diff --git a/drivers/media/platform/soc_camera/rcar_vin.c 
b/drivers/media/platform/soc_camera/rcar_vin.c
index 20dbedf..bf60074 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c

[...]

@@ -533,8 +513,20 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
rcar_vin_wait_stop_streaming(priv);

for (i = 0; i  vq-num_buffers; ++i)
-   if (vq-bufs[i]-state == VB2_BUF_STATE_ACTIVE)
+   if (vq-bufs[i]-state == VB2_BUF_STATE_ACTIVE) {
+   int j;
+
+   /*  Is this a buffer we have told the
+*  hardware about? Update the associated
+*  list, if so
+*/
+   for (j = 0; j  MAX_BUFFER_NUM; j++) {
+   if (priv-queue_buf[j] == vq-bufs[i]) {
+   priv-queue_buf[j] = NULL;
+   }


   Don't need {} here.


+   }
vb2_buffer_done(vq-bufs[i], VB2_BUF_STATE_ERROR);
+   }


WBR, Sergei

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html