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

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

[Ismael Luceno]

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

Based on patches by Andrey Utkin <andrey.utkin@corp.bluecherry.net>.

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

[PATCH 2/2] solo6x10: Simplify solo_enum_ext_input

[Ismael Luceno]

Additionally, now it specifies which channels it's showing.

Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
---
 drivers/media/pci/solo6x10/solo6x10-v4l2.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2.c b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
index 721ff53..935c1b6 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
@@ -386,26 +386,17 @@ static int solo_querycap(struct file *file, void  *priv,
 static int solo_enum_ext_input(struct solo_dev *solo_dev,
 			       struct v4l2_input *input)
 {
-	static const char * const dispnames_1[] = { "4UP" };
-	static const char * const dispnames_2[] = { "4UP-1", "4UP-2" };
-	static const char * const dispnames_5[] = {
-		"4UP-1", "4UP-2", "4UP-3", "4UP-4", "16UP"
-	};
-	const char * const *dispnames;
-
-	if (input->index >= (solo_dev->nr_chans + solo_dev->nr_ext))
-		return -EINVAL;
-
-	if (solo_dev->nr_ext == 5)
-		dispnames = dispnames_5;
-	else if (solo_dev->nr_ext == 2)
-		dispnames = dispnames_2;
-	else
-		dispnames = dispnames_1;
+	int ext = input->index - solo_dev->nr_chans;
+	unsigned int nup, first;
 
-	snprintf(input->name, sizeof(input->name), "Multi %s",
-		 dispnames[input->index - solo_dev->nr_chans]);
+	if (ext >= solo_dev->nr_ext)
+		return -EINVAL;
 
+	nup   = (ext == 4) ? 16 : 4;
+	first = (ext & 3) << 2;
+	snprintf(input->name, sizeof(input->name),
+		 "Multi %d-up (cameras %d-%d)",
+		 nup, first + 1, first + nup);
 	return 0;
 }
 
-- 
2.8.0

Re: [PATCH 2/2] solo6x10: Simplify solo_enum_ext_input

[Hans Verkuil]

Hi Ismael,

On 04/30/2016 05:17 AM, Ismael Luceno wrote:
> Additionally, now it specifies which channels it's showing.
> 
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> ---
>  drivers/media/pci/solo6x10/solo6x10-v4l2.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2.c b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
> index 721ff53..935c1b6 100644
> --- a/drivers/media/pci/solo6x10/solo6x10-v4l2.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
> @@ -386,26 +386,17 @@ static int solo_querycap(struct file *file, void  *priv,
>  static int solo_enum_ext_input(struct solo_dev *solo_dev,
>  			       struct v4l2_input *input)
>  {
> -	static const char * const dispnames_1[] = { "4UP" };
> -	static const char * const dispnames_2[] = { "4UP-1", "4UP-2" };
> -	static const char * const dispnames_5[] = {
> -		"4UP-1", "4UP-2", "4UP-3", "4UP-4", "16UP"
> -	};
> -	const char * const *dispnames;
> -
> -	if (input->index >= (solo_dev->nr_chans + solo_dev->nr_ext))
> -		return -EINVAL;
> -
> -	if (solo_dev->nr_ext == 5)
> -		dispnames = dispnames_5;
> -	else if (solo_dev->nr_ext == 2)
> -		dispnames = dispnames_2;
> -	else
> -		dispnames = dispnames_1;
> +	int ext = input->index - solo_dev->nr_chans;
> +	unsigned int nup, first;
>  
> -	snprintf(input->name, sizeof(input->name), "Multi %s",
> -		 dispnames[input->index - solo_dev->nr_chans]);
> +	if (ext >= solo_dev->nr_ext)
> +		return -EINVAL;
>  
> +	nup   = (ext == 4) ? 16 : 4;
> +	first = (ext & 3) << 2;
> +	snprintf(input->name, sizeof(input->name),
> +		 "Multi %d-up (cameras %d-%d)",
> +		 nup, first + 1, first + nup);

Shouldn't this be: nup, first + 1, nup);

Now it displays cameras as 1-5, 2-6, 3-7, 4-8 if I am not mistaken.

Regards,

	Hans

>  	return 0;
>  }
>  
> 

Re: [PATCH 2/2] solo6x10: Simplify solo_enum_ext_input

[Ismael Luceno]

On 04/Mai/2016 10:02, Hans Verkuil wrote:
> Hi Ismael,
> 
> On 04/30/2016 05:17 AM, Ismael Luceno wrote:
> > Additionally, now it specifies which channels it's showing.
> > 
> > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> > ---
> >  drivers/media/pci/solo6x10/solo6x10-v4l2.c | 27 +++++++++------------------
> >  1 file changed, 9 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2.c b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
> > index 721ff53..935c1b6 100644
> > --- a/drivers/media/pci/solo6x10/solo6x10-v4l2.c
> > +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
> > @@ -386,26 +386,17 @@ static int solo_querycap(struct file *file, void  *priv,
> >  static int solo_enum_ext_input(struct solo_dev *solo_dev,
> >  			       struct v4l2_input *input)
> >  {
> > -	static const char * const dispnames_1[] = { "4UP" };
> > -	static const char * const dispnames_2[] = { "4UP-1", "4UP-2" };
> > -	static const char * const dispnames_5[] = {
> > -		"4UP-1", "4UP-2", "4UP-3", "4UP-4", "16UP"
> > -	};
> > -	const char * const *dispnames;
> > -
> > -	if (input->index >= (solo_dev->nr_chans + solo_dev->nr_ext))
> > -		return -EINVAL;
> > -
> > -	if (solo_dev->nr_ext == 5)
> > -		dispnames = dispnames_5;
> > -	else if (solo_dev->nr_ext == 2)
> > -		dispnames = dispnames_2;
> > -	else
> > -		dispnames = dispnames_1;
> > +	int ext = input->index - solo_dev->nr_chans;
> > +	unsigned int nup, first;
> >  
> > -	snprintf(input->name, sizeof(input->name), "Multi %s",
> > -		 dispnames[input->index - solo_dev->nr_chans]);
> > +	if (ext >= solo_dev->nr_ext)
> > +		return -EINVAL;
> >  
> > +	nup   = (ext == 4) ? 16 : 4;
> > +	first = (ext & 3) << 2;
> > +	snprintf(input->name, sizeof(input->name),
> > +		 "Multi %d-up (cameras %d-%d)",
> > +		 nup, first + 1, first + nup);
> 
> Shouldn't this be: nup, first + 1, nup);
> 
> Now it displays cameras as 1-5, 2-6, 3-7, 4-8 if I am not mistaken.

Hi Hans.

The var "first" takes the values {0, 4, 8, 12}, so the code is correct,
it displays: 1-4, 5-8, 9-12, 13-16, or 1-16.

Regards.

Re: [PATCH 2/2] solo6x10: Simplify solo_enum_ext_input

[Hans Verkuil]

On 05/04/2016 01:42 PM, Ismael Luceno wrote:
> On 04/Mai/2016 10:02, Hans Verkuil wrote:
>> Hi Ismael,
>>
>> On 04/30/2016 05:17 AM, Ismael Luceno wrote:
>>> Additionally, now it specifies which channels it's showing.
>>>
>>> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
>>> ---
>>>  drivers/media/pci/solo6x10/solo6x10-v4l2.c | 27 +++++++++------------------
>>>  1 file changed, 9 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2.c b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
>>> index 721ff53..935c1b6 100644
>>> --- a/drivers/media/pci/solo6x10/solo6x10-v4l2.c
>>> +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
>>> @@ -386,26 +386,17 @@ static int solo_querycap(struct file *file, void  *priv,
>>>  static int solo_enum_ext_input(struct solo_dev *solo_dev,
>>>  			       struct v4l2_input *input)
>>>  {
>>> -	static const char * const dispnames_1[] = { "4UP" };
>>> -	static const char * const dispnames_2[] = { "4UP-1", "4UP-2" };
>>> -	static const char * const dispnames_5[] = {
>>> -		"4UP-1", "4UP-2", "4UP-3", "4UP-4", "16UP"
>>> -	};
>>> -	const char * const *dispnames;
>>> -
>>> -	if (input->index >= (solo_dev->nr_chans + solo_dev->nr_ext))
>>> -		return -EINVAL;
>>> -
>>> -	if (solo_dev->nr_ext == 5)
>>> -		dispnames = dispnames_5;
>>> -	else if (solo_dev->nr_ext == 2)
>>> -		dispnames = dispnames_2;
>>> -	else
>>> -		dispnames = dispnames_1;
>>> +	int ext = input->index - solo_dev->nr_chans;
>>> +	unsigned int nup, first;
>>>  
>>> -	snprintf(input->name, sizeof(input->name), "Multi %s",
>>> -		 dispnames[input->index - solo_dev->nr_chans]);
>>> +	if (ext >= solo_dev->nr_ext)
>>> +		return -EINVAL;
>>>  
>>> +	nup   = (ext == 4) ? 16 : 4;
>>> +	first = (ext & 3) << 2;
>>> +	snprintf(input->name, sizeof(input->name),
>>> +		 "Multi %d-up (cameras %d-%d)",
>>> +		 nup, first + 1, first + nup);
>>
>> Shouldn't this be: nup, first + 1, nup);
>>
>> Now it displays cameras as 1-5, 2-6, 3-7, 4-8 if I am not mistaken.
> 
> Hi Hans.
> 
> The var "first" takes the values {0, 4, 8, 12}, so the code is correct,
> it displays: 1-4, 5-8, 9-12, 13-16, or 1-16.

Ah, now I see what you do. Can you add a little comment here? For example:

/* Construct the text: cameras 1-4, 5-8, 9-12, 13-16, or 1-16 */

It's a bit obfuscated otherwise.

Thanks!

	Hans

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

[Andrey Utkin]

On Sat, Apr 30, 2016 at 12:17:08AM -0300, Ismael Luceno wrote:
> Such frame size is met in practice. Also report oversized frames.
> 
> Based on patches by Andrey Utkin <andrey.utkin@corp.bluecherry.net>.

If it is based on my patches([1] [2]), then why you claim authorship and
why you don't let me know (at last CCing me)?

Do you know that 200 KiB is not the limit, just as previous value? I
haven't researched subj deep enough to figure out proven good value for
new buffer size.

It's both laughable and infuriating for me to spectate your behaviour of
"stealth driver developer".
You have added yourself back to driver maintainers in MAINTAINERS file
after your quit without letting us know.
You are not affiliated with Bluecherry for two years, and you are not
informed about how the driver is working in production on customers
setups. So you are not aware what are real issues with it. BTW do you
still have a sample of actual hardware? Yeah, I agree that this can be
argument against Bluecherry and lack of openness in its bug tracking.
But you are also not open and not collaborating.

The point of my accusation to you is that you seem to be just gaining
"kernel developer" score for nobody's (except your CV's) benefit.
Development and maintenance is what Hans Verkuil, Krzysztof Halasa and
others do to this driver, but not this.

Sorry to be harsh.

[1] https://github.com/bluecherrydvr/solo6x10/commit/5cd985087362e2e524b3e44504eea791ae7cda7e
[2] https://github.com/bluecherrydvr/solo6x10/commit/3b437f79c40438eb09bb2d5dbcfe67dbc94648ed

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

[Hans Verkuil]

OK, I've dropped Ismael's patch from my pull request to Mauro until this
is resolved.

Who actually maintains this driver? I would say Andrey since he works for
bluecherry. If Ismael is no longer affiliated (either officially or unofficially)
with bluecherry, then his name should be removed.

BTW, looking at the MAINTAINERS file I see two email addresses for Andrey,
neither of which is the fastmail.com address this email came from.

Ismael, please keep the correct author if it isn't you. And a CC to Andrey
certainly doesn't hurt.

Andrey, it might be a good idea to post such fixes to the mailinglist sooner,
both to prevent situations like this and to keep the diffs between mainline
and your internal code as small as possible.

I don't really care who posts fixes as long as authorship info etc. remains
intact and the code is obviously an improvement.

Regards,

	Hans

On 05/04/2016 03:34 PM, Andrey Utkin wrote:
> On Sat, Apr 30, 2016 at 12:17:08AM -0300, Ismael Luceno wrote:
>> Such frame size is met in practice. Also report oversized frames.
>>
>> Based on patches by Andrey Utkin <andrey.utkin@corp.bluecherry.net>.
> 
> If it is based on my patches([1] [2]), then why you claim authorship and
> why you don't let me know (at last CCing me)?
> 
> Do you know that 200 KiB is not the limit, just as previous value? I
> haven't researched subj deep enough to figure out proven good value for
> new buffer size.
> 
> It's both laughable and infuriating for me to spectate your behaviour of
> "stealth driver developer".
> You have added yourself back to driver maintainers in MAINTAINERS file
> after your quit without letting us know.
> You are not affiliated with Bluecherry for two years, and you are not
> informed about how the driver is working in production on customers
> setups. So you are not aware what are real issues with it. BTW do you
> still have a sample of actual hardware? Yeah, I agree that this can be
> argument against Bluecherry and lack of openness in its bug tracking.
> But you are also not open and not collaborating.
> 
> The point of my accusation to you is that you seem to be just gaining
> "kernel developer" score for nobody's (except your CV's) benefit.
> Development and maintenance is what Hans Verkuil, Krzysztof Halasa and
> others do to this driver, but not this.
> 
> Sorry to be harsh.
> 
> [1] https://github.com/bluecherrydvr/solo6x10/commit/5cd985087362e2e524b3e44504eea791ae7cda7e
> [2] https://github.com/bluecherrydvr/solo6x10/commit/3b437f79c40438eb09bb2d5dbcfe67dbc94648ed
> 

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

[Andrey Utkin]

On Wed, May 4, 2016 at 5:04 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> BTW, looking at the MAINTAINERS file I see two email addresses for Andrey,
> neither of which is the fastmail.com address this email came from.

Now I'm replying from corporate email.

> Andrey, it might be a good idea to post such fixes to the mailinglist sooner,
> both to prevent situations like this and to keep the diffs between mainline
> and your internal code as small as possible.

In a word - we would do what is possible to achieve that, but there's
little time
and little incentive for that.
The codebases have already diverged a lot, having unique sets of runtime bugs.
And this exact issue alone is not resolved yet in a good way and is
not actually critical.
Merging would require a lot of working time. And it is complicated by
the fact that
there's not going to be any new manufacturing orders (the minimal order quantity
is too high for Bluecherry), and that we have picked tw5864 as
reachable for retail orders.

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

[Ismael Luceno]

On 04/May/2016 17:22, Andrey Utkin wrote:
> On Wed, May 4, 2016 at 5:04 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > BTW, looking at the MAINTAINERS file I see two email addresses for Andrey,
> > neither of which is the fastmail.com address this email came from.
> 
> Now I'm replying from corporate email.
> 
> > Andrey, it might be a good idea to post such fixes to the mailinglist sooner,
> > both to prevent situations like this and to keep the diffs between mainline
> > and your internal code as small as possible.
> 
> In a word - we would do what is possible to achieve that, but
> there's little time and little incentive for that.
> The codebases have already diverged a lot, having unique sets of runtime bugs.

The divergence is due to the vb2 porting effort by Hans. History
was not properly conserved, but it is otherwise compatible, and most
changes can still be ported back and forth without trouble.

> And this exact issue alone is not resolved yet in a good way and is
> not actually critical.
> Merging would require a lot of working time. And it is complicated by
> the fact that there's not going to be any new manufacturing orders
> (the minimal order quantity is too high for Bluecherry), and that
> we have picked tw5864 as reachable for retail orders.

Right now it would be easy to keep the two in sync, there has been
not many changes since I left, and if changes are integrated into
mainline first, there will be no further issues.

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

[Hans Verkuil]

On 05/04/2016 04:22 PM, Andrey Utkin wrote:
> On Wed, May 4, 2016 at 5:04 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> BTW, looking at the MAINTAINERS file I see two email addresses for Andrey,
>> neither of which is the fastmail.com address this email came from.
> 
> Now I'm replying from corporate email.
> 
>> Andrey, it might be a good idea to post such fixes to the mailinglist sooner,
>> both to prevent situations like this and to keep the diffs between mainline
>> and your internal code as small as possible.
> 
> In a word - we would do what is possible to achieve that, but there's
> little time
> and little incentive for that.
> The codebases have already diverged a lot, having unique sets of runtime bugs.
> And this exact issue alone is not resolved yet in a good way and is
> not actually critical.
> Merging would require a lot of working time. And it is complicated by
> the fact that
> there's not going to be any new manufacturing orders (the minimal order quantity
> is too high for Bluecherry), and that we have picked tw5864 as
> reachable for retail orders.
> 

It sounds more like the status of this driver is "Odd Fixes" and not "Supported"
as it says today. From the MAINTAINERS file:

        S: Status, one of the following:
           Supported:   Someone is actually paid to look after this.
           Maintained:  Someone actually looks after it.
           Odd Fixes:   It has a maintainer but they don't have time to do
                        much other than throw the odd patch in. See below..
           Orphan:      No current maintainer [but maybe you could take the
                        role as you write your new code].
           Obsolete:    Old code. Something tagged obsolete generally means
                        it has been replaced by a better system and you
                        should be using that.

Yes, Ismael should have kept your authorship, but it won't be the first time
someone forgets that. Heck, I've forgotten it on occasion. Just point that out
politely and let Ismael post a new version of this patch.

Don't look for conspiracies when it is just a mistake. Relax, peace on earth,
don't worry, be happy and all that. Bad for everyone's blood pressure to get
so worked up about these things.

Looking at the recent history of this driver the patch contributions seem to
be more-or-less equally distributed between Krzysztof, Andrey and Ismael, not
that there are all that many.

So if Ismael is merging in some of your patches for free in his own time, then
I'd say why not? Again, provided correct authorship is maintained.

Regards,

	Hans

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

[Ismael Luceno]

On 04/May/2016 16:34, Andrey Utkin wrote:
> On Sat, Apr 30, 2016 at 12:17:08AM -0300, Ismael Luceno wrote:
> > Such frame size is met in practice. Also report oversized frames.
> > 
> > Based on patches by Andrey Utkin <andrey.utkin@corp.bluecherry.net>.
> 
> If it is based on my patches([1] [2]), then why you claim authorship and
> why you don't let me know (at last CCing me)?

Wasn't my intention, I gave you credit, I just merged the changes
and reworked the warning and commit message.

The whole point to have solo6x10 mainlined was to get rid of the
out-of-tree driver and convert the DKMS package to use media_build,
thus mainline should be kept in sync, so why are you not submitting
the patches yourself? I should have nothing to do with that.

> Do you know that 200 KiB is not the limit, just as previous value? I
> haven't researched subj deep enough to figure out proven good value for
> new buffer size.

I know, I know it depends on the quantization matrix, and it should
be possible to infer the limit, but like you I didn't do the research,
the difference is that I don't get paid to do it anymore.

> It's both laughable and infuriating for me to spectate your behaviour of
> "stealth driver developer".
> You have added yourself back to driver maintainers in MAINTAINERS file
> after your quit without letting us know.

Why the attack?

I didn't quit, I was dismissed, and the remaining ~5k USD I'm owed
was never paid. Curtis: any comment on that?

Also, I don't see what's the problem in re-adding myself, and I
don't understand why I was removed in the first place, it's not up
to Bluecherry, is it?

> You are not affiliated with Bluecherry for two years, and you are not
> informed about how the driver is working in production on customers
> setups. So you are not aware what are real issues with it. BTW do you
> still have a sample of actual hardware? Yeah, I agree that this can be
> argument against Bluecherry and lack of openness in its bug tracking.

You attend Bluecherry customers' needs because you're part of
Bluecherry; like you said I'm not, and I certainly don't get paid to
care about the out-of-tree driver issues or it's bug tracker.

And yes, I still have the hardware.

> But you are also not open and not collaborating.

What do you think I should do? Seriously, I don't get it.

> The point of my accusation to you is that you seem to be just gaining
> "kernel developer" score for nobody's (except your CV's) benefit.
> Development and maintenance is what Hans Verkuil, Krzysztof Halasa and
> others do to this driver, but not this.
> 
> Sorry to be harsh.

I think you're the only one keeping such a score, and I never claimed
my work being more or superior to anyone's else.

You're out of your mind, man.