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

Message ID 1461986229-11949-1-git-send-email-ismael@iodev.co.uk (mailing list archive)
State Superseded, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Ismael Luceno April 30, 2016, 3:17 a.m. UTC
  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(-)
  

Comments

Andrey Utkin May 4, 2016, 1:34 p.m. UTC | #1
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
--
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
  
Hans Verkuil May 4, 2016, 2:04 p.m. UTC | #2
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
> 
--
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 Utkin May 4, 2016, 2:22 p.m. UTC | #3
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.
--
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
  
Ismael Luceno May 4, 2016, 2:53 p.m. UTC | #4
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.
--
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
  
Ismael Luceno May 4, 2016, 3:09 p.m. UTC | #5
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.
--
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
  
Hans Verkuil May 4, 2016, 3:41 p.m. UTC | #6
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
--
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
  

Patch

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;