| Message ID | 1462378881-16625-1-git-send-email-ismael@iodev.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 04, 2016 at 01:21:20PM -0300, Ismael Luceno wrote: > From: Andrey Utkin <andrey.utkin@corp.bluecherry.net> > > Such frame size is met in practice. Also report oversized frames. > > [ismael: Reworked warning and commit message] > > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <andrey.utkin@corp.bluecherry.net> > > > > Such frame size is met in practice. Also report oversized frames. > > > > [ismael: Reworked warning and commit message] > > > > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk> > > 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?). 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <andrey.utkin@corp.bluecherry.net> > > Such frame size is met in practice. Also report oversized frames. > > [ismael: Reworked warning and commit message] > > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk> > --- > 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_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(&solo_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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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_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(&solo_dev->pdev->dev, + "oversized frame (%d bytes)\n", size); return -EINVAL; + } solo_enc->desc_count = 1;