Re: [PATCH v2 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Hans Verkuil]

On 06/28/16 13:48, Andrey Utkin wrote:
> On Mon, Jun 27, 2016 at 11:12:42AM +0200, Hans Verkuil wrote:
>> Andrey,
>>
>> Since you are the original author, can you give me your Signed-off-by line?
> 
> No, as increasing buffer size by few kilobytes doesn't change anything. I've
> increased it from 200 to 204, then found new occurances of the issue,
> then increased it again and again by few kilobytes. Then I got that this
> is not a (nice) solution, and have never came back to this. Maybe
> doubling current buffer size would make users forget about this, but I'm
> not sure maintainers would be glad with such patch.

I don't care. Right now it doesn't work. The cause is that the buffers are
too small to handle the worst-case situation. So if doubling the size makes
it work, then that's perfectly OK. Memory is cheap these days. If it will
fail, then that's much worse than consuming a few meg more.

Ideally you can calculate what the worst-case size is, but I expect that to
be quite difficult if not impossible.

Regards,

Hans
--
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: [PATCH v2 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Andrey Utkin]

On Mon, Jun 27, 2016 at 11:12:42AM +0200, Hans Verkuil wrote:
> Andrey,
> 
> Since you are the original author, can you give me your Signed-off-by line?

No, as increasing buffer size by few kilobytes doesn't change anything. I've
increased it from 200 to 204, then found new occurances of the issue,
then increased it again and again by few kilobytes. Then I got that this
is not a (nice) solution, and have never came back to this. Maybe
doubling current buffer size would make users forget about this, but I'm
not sure maintainers would be glad with such patch.
--
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: [PATCH v2 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Hans Verkuil]

Andrey,

Since you are the original author, can you give me your Signed-off-by line?

My apologies for the very late reply, it's been very busy lately and I am
finally catching up...

Thanks!

Hans

On 05/04/2016 06:21 PM, Ismael Luceno wrote:
> From: Andrey Utkin 
> 
> Such frame size is met in practice. Also report oversized frames.
> 
> [ismael: Reworked warning and commit message]
> 
> Signed-off-by: Ismael Luceno 
> ---
>  drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
> b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> index 67a14c4..f98017b 100644
> --- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> @@ -33,7 +33,7 @@
>  #include "solo6x10-jpeg.h"
>  
>  #define MIN_VID_BUFFERS  2
> -#define FRAME_BUF_SIZE   (196 * 1024)
> +#define FRAME_BUF_SIZE   (200 * 1024)
>  #define MP4_QS   16
>  #define DMA_ALIGN4096
>  
> @@ -323,8 +323,11 @@ static int solo_send_desc(struct solo_enc_dev *solo_enc, 
> int skip,
>   int i;
>   int ret;
>  
> - if (WARN_ON_ONCE(size > FRAME_BUF_SIZE))
> + if (WARN_ON_ONCE(size > FRAME_BUF_SIZE)) {
> + dev_warn(_dev->pdev->dev,
> +  "oversized frame (%d bytes)\n", size);
>   return -EINVAL;
> + }
>  
>   solo_enc->desc_count = 1;
>  
> 
--
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: [PATCH v2 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Ismael Luceno]

On 05/Mai/2016 00:14, Andrey Utkin wrote:
> On Wed, May 04, 2016 at 01:21:20PM -0300, Ismael Luceno wrote:
> > From: Andrey Utkin 
> > 
> > Such frame size is met in practice. Also report oversized frames.
> > 
> > [ismael: Reworked warning and commit message]
> > 
> > Signed-off-by: Ismael Luceno 
> 
> I object against merging the first part.
> 
> > ---
> >  drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
> > b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> > index 67a14c4..f98017b 100644
> > --- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> > +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> > @@ -33,7 +33,7 @@
> >  #include "solo6x10-jpeg.h"
> >  
> >  #define MIN_VID_BUFFERS2
> > -#define FRAME_BUF_SIZE (196 * 1024)
> > +#define FRAME_BUF_SIZE (200 * 1024)
> 
> Please don't push this.
> It doesn't matter whether there are 196 or 200 KiB because there happen
> bigger frames.
> I don't remember details so I cannot point to all time max frame size.
> AFAIK this issue appeared on one particular customer installation. I
> don't monitor it closely right now. I think I have compiled custom
> package for that setup with FRAME_BUF_SIZE increased much more (maybe
> 10x?).

I don't quite remember the overscan, but the maximum should be around
1.2MB, so yes. If the QM hasn't been tweaked, then the image must
be terrible.
--
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: [PATCH v2 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Andrey Utkin]

On Wed, May 04, 2016 at 01:21:20PM -0300, Ismael Luceno wrote:
> From: Andrey Utkin 
> 
> Such frame size is met in practice. Also report oversized frames.
> 
> [ismael: Reworked warning and commit message]
> 
> Signed-off-by: Ismael Luceno 

I object against merging the first part.

> ---
>  drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
> b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> index 67a14c4..f98017b 100644
> --- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> @@ -33,7 +33,7 @@
>  #include "solo6x10-jpeg.h"
>  
>  #define MIN_VID_BUFFERS  2
> -#define FRAME_BUF_SIZE   (196 * 1024)
> +#define FRAME_BUF_SIZE   (200 * 1024)

Please don't push this.
It doesn't matter whether there are 196 or 200 KiB because there happen
bigger frames.
I don't remember details so I cannot point to all time max frame size.
AFAIK this issue appeared on one particular customer installation. I
don't monitor it closely right now. I think I have compiled custom
package for that setup with FRAME_BUF_SIZE increased much more (maybe
10x?).
--
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

[PATCH v2 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Ismael Luceno]

From: Andrey Utkin 

Such frame size is met in practice. Also report oversized frames.

[ismael: Reworked warning and commit message]

Signed-off-by: Ismael Luceno 
---
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
index 67a14c4..f98017b 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
@@ -33,7 +33,7 @@
 #include "solo6x10-jpeg.h"
 
 #define MIN_VID_BUFFERS2
-#define FRAME_BUF_SIZE (196 * 1024)
+#define FRAME_BUF_SIZE (200 * 1024)
 #define MP4_QS 16
 #define DMA_ALIGN  4096
 
@@ -323,8 +323,11 @@ static int solo_send_desc(struct solo_enc_dev *solo_enc, 
int skip,
int i;
int ret;
 
-   if (WARN_ON_ONCE(size > FRAME_BUF_SIZE))
+   if (WARN_ON_ONCE(size > FRAME_BUF_SIZE)) {
+   dev_warn(_dev->pdev->dev,
+"oversized frame (%d bytes)\n", size);
return -EINVAL;
+   }
 
solo_enc->desc_count = 1;
 
-- 
2.8.0

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