Re: [RFC PATCH v0] Add tw5864 driver

2016-03-11 Thread Andrey Utkin 
On Fri, 11 Mar 2016 10:59:02 +0100
Hans Verkuil  wrote:
> While userspace may specify FIELD_ANY when setting a format, the
> driver should always map that to a specific field setting and should
> never return FIELD_ANY back to userspace.
> 
> In this case, the 'field' field of the v4l2_buffer struct has
> FIELD_ANY which means it is not set correctly (or at all) in the
> driver.
> 
> It's a common mistake, which is why v4l2-compliance tests for it :-)

Thanks for great guidance Hans, finally I have solved all issues.

You can review latest state at tw5864 branch, also you can review
changelog of v4l2-compliance fixing at tags tw5864_pre_1.11,
tw5864_pre_1.10 of https://github.com/bluecherrydvr/linux .

I will make a final internal review before submission, and try
to submit the driver for inclusion.

Everybody is appreciated to make any comments even before submission,
the actual code to review is at tw5864 branch.

Re: [RFC PATCH v0] Add tw5864 driver

2016-03-11 Thread Hans Verkuil 
On 03/11/2016 09:40 AM, Andrey Utkin wrote:
> On Fri, 11 Mar 2016 09:00:18 +0100
> Hans Verkuil  wrote:
>> The reason is likely to be the tw5864_queue_setup function which has
>> not been updated to handle CREATE_BUFS support correctly. It should
>> look like this:
>>
>> static int tw5864_queue_setup(struct vb2_queue *q,
>>unsigned int *num_buffers,
>>unsigned int *num_planes, unsigned int
>> sizes[], void *alloc_ctxs[])
>> {
>>  struct tw5864_input *dev = vb2_get_drv_priv(q);
>>
>>  if (q->num_buffers + *num_buffers < 12)
>>  *num_buffers = 12 - q->num_buffers;
>>
>>  alloc_ctxs[0] = dev->alloc_ctx;
>>  if (*num_planes)
>>  return sizes[0] < H264_VLC_BUF_SIZE ? -EINVAL : 0;
>>
>>  sizes[0] = H264_VLC_BUF_SIZE;
>>  *num_planes = 1;
>>
>>  return 0;
>> }
> 
> Thanks for suggestion, but now the failure looks this way:
> 
> Streaming ioctls:
> test read/write: OK
> fail: v4l2-test-buffers.cpp(297): g_field() == V4L2_FIELD_ANY

While userspace may specify FIELD_ANY when setting a format, the driver should
always map that to a specific field setting and should never return FIELD_ANY
back to userspace.

In this case, the 'field' field of the v4l2_buffer struct has FIELD_ANY which
means it is not set correctly (or at all) in the driver.

It's a common mistake, which is why v4l2-compliance tests for it :-)

Regards,

Hans

> fail: v4l2-test-buffers.cpp(703): buf.check(q, last_seq)
> fail: v4l2-test-buffers.cpp(976): captureBufs(node, q, m2m_q, 
> frame_count, false)
> test MMAP: FAIL
> 
> Will check that later. If you have any suggestions, I would be very
> grateful.
> --
> 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 v0] Add tw5864 driver

2016-03-11 Thread Andrey Utkin 
On Fri, 11 Mar 2016 09:00:18 +0100
Hans Verkuil  wrote:
> The reason is likely to be the tw5864_queue_setup function which has
> not been updated to handle CREATE_BUFS support correctly. It should
> look like this:
> 
> static int tw5864_queue_setup(struct vb2_queue *q,
> unsigned int *num_buffers,
> unsigned int *num_planes, unsigned int
> sizes[], void *alloc_ctxs[])
> {
>   struct tw5864_input *dev = vb2_get_drv_priv(q);
> 
>   if (q->num_buffers + *num_buffers < 12)
>   *num_buffers = 12 - q->num_buffers;
> 
>   alloc_ctxs[0] = dev->alloc_ctx;
>   if (*num_planes)
>   return sizes[0] < H264_VLC_BUF_SIZE ? -EINVAL : 0;
> 
>   sizes[0] = H264_VLC_BUF_SIZE;
>   *num_planes = 1;
> 
>   return 0;
> }

Thanks for suggestion, but now the failure looks this way:

Streaming ioctls:
test read/write: OK
fail: v4l2-test-buffers.cpp(297): g_field() == V4L2_FIELD_ANY
fail: v4l2-test-buffers.cpp(703): buf.check(q, last_seq)
fail: v4l2-test-buffers.cpp(976): captureBufs(node, q, m2m_q, 
frame_count, false)
test MMAP: FAIL

Will check that later. If you have any suggestions, I would be very
grateful.

Re: [RFC PATCH v0] Add tw5864 driver

2016-03-11 Thread Hans Verkuil 
On 03/09/2016 03:29 PM, Andrey Utkin wrote:
> Hi Hans!
> 
> Some improvements took place on the driver, including cleaner
> v4l2-compliance tests passing. But there's a single test failure I
> don't understand.
> 
> In the code of v4l2-compliance, it seems like an API
> call CREATE_BUFS is supposed to fail with EINVAL. But in case of my
> driver, which simply uses vb2_ioctl_create_bufs(), the call returns 0.
> 
> Please review the below report.
> 
> The report is produced by fresh v4l-utils from git:
> v4l-utils-1.10.0-59-g1388c0a
> 
> The actual code which was tested is at tag release/tw5864/1.10 of
> https://github.com/bluecherrydvr/linux.git , see
> drivers/staging/media/tw5864 . If we sort this out, I am considering
> resubmit the driver for merging upstream.

The reason is likely to be the tw5864_queue_setup function which has not been
updated to handle CREATE_BUFS support correctly. It should look like this:

static int tw5864_queue_setup(struct vb2_queue *q,
  unsigned int *num_buffers,
  unsigned int *num_planes, unsigned int sizes[],
  void *alloc_ctxs[])
{
struct tw5864_input *dev = vb2_get_drv_priv(q);

if (q->num_buffers + *num_buffers < 12)
*num_buffers = 12 - q->num_buffers;

alloc_ctxs[0] = dev->alloc_ctx;
if (*num_planes)
return sizes[0] < H264_VLC_BUF_SIZE ? -EINVAL : 0;

sizes[0] = H264_VLC_BUF_SIZE;
*num_planes = 1;

return 0;
}

> 
> There's also another issue with v4l2-compliance. At some moment the
> driver receives S_PARM command with timeperframe 0/0, by which reason I
> added special handling, but I guess there's something out of my
> knowledge which caused this and which should be fixed.

A timeperframe value of 0/0 is valid and the driver is supposed to replace it
with the default timeperframe. Which is why v4l2-compliance tests for this
corner case.

Regards,

Hans

> 
> 
>  # v4l2-compliance -d /dev/video6 -s
> Driver Info:
> Driver name   : tw5864
> Card type : TW5864 Encoder 2
> Bus info  : PCI::06:05.0
> Driver version: 4.5.0
> Capabilities  : 0x8521
> Video Capture
> Read/Write
> Streaming
> Extended Pix Format
> Device Capabilities
> Device Caps   : 0x0521
> Video Capture
> Read/Write
> Streaming
> Extended Pix Format
> 
> Compliance test for device /dev/video6 (not using libv4l2):
> 
> Required ioctls:
> test VIDIOC_QUERYCAP: OK
> 
> Allow for multiple opens:
> test second video open: OK
> test VIDIOC_QUERYCAP: OK
> test VIDIOC_G/S_PRIORITY: OK
> 
> Debug ioctls:
> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> test VIDIOC_LOG_STATUS: OK
> 
> Input ioctls:
> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> test VIDIOC_ENUMAUDIO: OK (Not Supported)
> test VIDIOC_G/S/ENUMINPUT: OK
> test VIDIOC_G/S_AUDIO: OK (Not Supported)
> Inputs: 1 Audio Inputs: 0 Tuners: 0
> 
> Output ioctls:
> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> Outputs: 0 Audio Outputs: 0 Modulators: 0
> 
> Input/Output configuration ioctls:
> test VIDIOC_ENUM/G/S/QUERY_STD: OK
> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> test VIDIOC_G/S_EDID: OK (Not Supported)
> 
> Test input 0:
> 
> Control ioctls:
> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
> test VIDIOC_QUERYCTRL: OK
> test VIDIOC_G/S_CTRL: OK
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 11 Private Controls: 0
> 
> Format ioctls:
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK
> test VIDIOC_G_FBUF: OK (Not Supported)
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> test VIDIOC_S_FMT: OK
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> test Scaling: OK (Not Supported)
> 
> Codec ioctls:
> test VIDIOC_(TRY_)ENCO

Re: [RFC PATCH v0] Add tw5864 driver

2016-03-09 Thread Andrey Utkin 
Hi Hans!

Some improvements took place on the driver, including cleaner
v4l2-compliance tests passing. But there's a single test failure I
don't understand.

In the code of v4l2-compliance, it seems like an API
call CREATE_BUFS is supposed to fail with EINVAL. But in case of my
driver, which simply uses vb2_ioctl_create_bufs(), the call returns 0.

Please review the below report.

The report is produced by fresh v4l-utils from git:
v4l-utils-1.10.0-59-g1388c0a

The actual code which was tested is at tag release/tw5864/1.10 of
https://github.com/bluecherrydvr/linux.git , see
drivers/staging/media/tw5864 . If we sort this out, I am considering
resubmit the driver for merging upstream.

There's also another issue with v4l2-compliance. At some moment the
driver receives S_PARM command with timeperframe 0/0, by which reason I
added special handling, but I guess there's something out of my
knowledge which caused this and which should be fixed.


 # v4l2-compliance -d /dev/video6 -s
Driver Info:
Driver name   : tw5864
Card type : TW5864 Encoder 2
Bus info  : PCI::06:05.0
Driver version: 4.5.0
Capabilities  : 0x8521
Video Capture
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0521
Video Capture
Read/Write
Streaming
Extended Pix Format

Compliance test for device /dev/video6 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 11 Private Controls: 0

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

Test input 0:

Streaming ioctls:
test read/write: OK
fail: v4l2-test-buffers.cpp(959): ret != EINVAL
test MMAP: FAIL
test USERPTR: OK (Not Supported)
test DMABUF: Cannot test, specify --expbuf-device


Total: 45, Succeeded: 44, Failed: 1, Warnings: 0
[ERR]
14:14:15root@tw ~
 # gdb v4l2-compliance 
GNU gdb (Ubuntu 7.10-1ubuntu3) 7.10
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
 This is free software: you are free
to change and redistribute it. There is NO WARRANTY, to the extent
permitted by law.  Type "show copying" and "show warranty" for details.
This GDB was configured as "i686-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:

Re: [RFC PATCH v0] Add tw5864 driver

2016-02-08 Thread Hans Verkuil 
On 02/08/2016 11:23 AM, Andrey Utkin wrote:
> On Mon, Feb 8, 2016 at 11:58 AM, Hans Verkuil  wrote:
>> Hi Andrey,
>>
>> Hmm, it looks like I forgot to reply. Sorry about that.
> 
> Thank you very much anyway.
> 
>> I wouldn't change the memcpy: in my experience it is very useful to get a
>> well-formed compressed stream out of the hardware. And the overhead of
>> having to do a memcpy is a small price to pay and on modern CPUs should
>> be barely noticeable for SDTV inputs.
> 
> So there's no usecase for scatter-gather approach, right?

The only advantage scatter-gather would bring is more efficient memory usage:
dma-contig requires physically contiguous memory, dma-sg doesn't. If you need
a lot of contiguous memory you can run into out-of-memory situations. The
alternative is to build a kernel with CMA enabled and reserve memory that way.

dma-sg doesn't have these problems, so that can be a good alternative, but it
comes at the price of higher complexity.

> 
>> I don't believe that the lockups you see are related to the memcpy as
>> such. The trace says that a cpu is stuck for 22s, no way that is related
>> to something like that. It looks more like a deadlock somewhere.
> 
> There was a locking issue (lack of _irqsave) and was fixed since then.
> 
>> Regarding the compliance tests: don't pass VB2_USERPTR (doesn't work well
>> with videobuf2-dma-contig). Also add vidioc_expbuf = vb2_ioctl_expbuf for
>> the DMABUF support. That should clear up some of the errors you see.
> 
> Thank you!
> 

Regards,

Hans

Re: [RFC PATCH v0] Add tw5864 driver

2016-02-08 Thread Andrey Utkin 
On Mon, Feb 8, 2016 at 11:58 AM, Hans Verkuil  wrote:
> Hi Andrey,
>
> Hmm, it looks like I forgot to reply. Sorry about that.

Thank you very much anyway.

> I wouldn't change the memcpy: in my experience it is very useful to get a
> well-formed compressed stream out of the hardware. And the overhead of
> having to do a memcpy is a small price to pay and on modern CPUs should
> be barely noticeable for SDTV inputs.

So there's no usecase for scatter-gather approach, right?

> I don't believe that the lockups you see are related to the memcpy as
> such. The trace says that a cpu is stuck for 22s, no way that is related
> to something like that. It looks more like a deadlock somewhere.

There was a locking issue (lack of _irqsave) and was fixed since then.

> Regarding the compliance tests: don't pass VB2_USERPTR (doesn't work well
> with videobuf2-dma-contig). Also add vidioc_expbuf = vb2_ioctl_expbuf for
> the DMABUF support. That should clear up some of the errors you see.

Thank you!

-- 
Bluecherry developer.

Re: [RFC PATCH v0] Add tw5864 driver

2016-02-08 Thread Hans Verkuil 
Hi Andrey,

Hmm, it looks like I forgot to reply. Sorry about that.

On 01/15/2016 03:13 AM, Andrey Utkin wrote:
> On Mon, Jan 11, 2016 at 12:52 PM, Hans Verkuil  wrote:
>> Did you also test with v4l2-compliance? Before I accept the driver I want to 
>> see the
>> output of 'v4l2-compliance' and 'v4l2-compliance -s'. Basically there 
>> shouldn't be
>> any failures.
>>
>> I did a quick scan over the source and I saw nothing big. When you post the 
>> new
>> version I will go over it more carefully and do a proper review.
> 
> Thank you for review.
> You can find output of v4l2-compliance and v4l2-compliance -s tests
> below (v4l-utils-1.8.0-42-gf1348b4).
> There are some issues, I will work on them.
> Could you please advise how could I optimize high-volume data passing
> to userspace apps?
> In one installation with 16 boards, when all streams are set to be 30
> FPS all-I-frames (workaround for another P-frames quality issue), the
> host struggles from regular lockups, see message immediately below
> (4.2.0 kernel though, DKMS package with slightly modified code to
> match older API).
> I am not fully sure it is related to my driver, but I guess memcpy-ish
> approach to preparing video frames may be very CPU-expensive. 30 FPS
> all-I-frames video stream is like 3 megabytes per second.
> Of course this is not very good situation anyway, and I should either
> fix original issue or find some workarounds, at last to lower
> framerate. But still, optimization of data passing using some
> appropriate API like scatter-gather should be done I believe. I need
> hints how to do that right.
> Thanks in advance.

If I understand it correctly the problem is that you need to insert h264
headers in front of the data, and for that you need to do a memcpy, for now
at least. We are looking into mechanisms to pass the header separately from
the video frame data, but I don't dare give an ETA when that arrives in
the kernel.

That said, it makes no sense to use H264 with I frames only, it defeats
the purpose of H264. I would concentrate on making the encoder work correcly
as that will dramatically reduce the amount of data to memcpy. For SDTV
inputs you can get good quality using an MPEG-2 encoder with just 1 MB/s
in my experience. H264 should reduce that further. And doing a memcpy for
worst-case 4*16 = 64 MB/s should not be a problem at all for modern CPUs.

Frankly, even 3*64 = 192 MB/s as you have now shouldn't pose a problem.

I wouldn't change the memcpy: in my experience it is very useful to get a
well-formed compressed stream out of the hardware. And the overhead of
having to do a memcpy is a small price to pay and on modern CPUs should
be barely noticeable for SDTV inputs.

I don't believe that the lockups you see are related to the memcpy as
such. The trace says that a cpu is stuck for 22s, no way that is related
to something like that. It looks more like a deadlock somewhere.

Regarding the compliance tests: don't pass VB2_USERPTR (doesn't work well
with videobuf2-dma-contig). Also add vidioc_expbuf = vb2_ioctl_expbuf for
the DMABUF support. That should clear up some of the errors you see.

Regards,

Hans

> 
> Jan 13 19:35:20 cctv kernel: [34202.715069] NMI watchdog: BUG: soft
> lockup - CPU#2 stuck for 22s! [khugepaged:69]
> Jan 13 19:35:20 cctv kernel: [34202.715071] Modules linked in: ib_iser
> rdma_cm iw_cm ib_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp
> libiscsi_tcp libiscsi scsi_transport_iscsi ast ttm drm_kms_helper drm
> syscopyarea sysfillrect sysimgblt xfs libcrc32c ipmi_ssif joydev
> input_leds intel_rapl iosf_mbi x86_pkg_temp_thermal intel_powerclamp
> coretemp kvm_intel kvm crct10dif_pclmul crc32_pclmul aesni_intel
> aes_x86_64 lrw gf128mul glue_helper ablk_helper cryptd sb_edac
> edac_core lpc_ich mei_me mei shpchp wmi tw5864(OE) solo6x10
> videobuf2_dma_contig videobuf2_dma_sg videobuf2_memops videobuf2_core
> v4l2_common videodev media ipmi_si 8250_fintek ipmi_msghandler snd_pcm
> snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device
> snd_timer snd acpi_power_meter acpi_pad soundcore mac_hid parport_pc
> ppdev lp parport hid_generic usbhid hid igb i2c_algo_bit ahci dca ptp
> libahci megaraid_sas pps_core
> Jan 13 19:35:20 cctv kernel: [34202.715105] CPU: 2 PID: 69 Comm:
> khugepaged Tainted: GW  OEL  4.2.0-23-generic
> #28~14.04.1-Ubuntu
> Jan 13 19:35:20 cctv kernel: [34202.715106] Hardware name: Supermicro
> Super Server/X10SRi-F, BIOS 2.0 12/17/2015
> Jan 13 19:35:20 cctv kernel: [34202.715107] task: 88046d7a6040 ti:
> 88046d0f4000 task.ti: 88046d0f4000
> Jan 13 19:35:20 cctv kernel: [34202.715108] RIP:
> 0010:[]  []
> smp_call_function_many+0x1f9/0x250
> Jan 13 19:35:20 cctv kernel: [34202.715112] RSP: 0018:88046d0f7b58
>  EFLAGS: 0202
> Jan 13 19:35:20 cctv kernel: [34202.715112] RAX: 0003 RBX:
>  RCX: 88047fd1a870
> Jan 13 19:35:20 cctv kernel: [34202.715113] RDX: 0004 RSI:
> 0100 RDI: fff

Re: [RFC PATCH v0] Add tw5864 driver

2016-01-03 Thread Andrey Utkin 
On Sun, Jan 3, 2016 at 5:47 AM, Joe Perches  wrote:
> several of these have unnecessary parentheses

Thanks, fixed.

> Maybe use bool a bit more

Thanks, fixed.

> or maybe just use fls

Thanks, fls() fit greatly, rewritten the function with compatibility testing.

>> +static inline int bs_size_ue(unsigned int val)
>> +{
>> + int i_size = 0;
>> + static const int i_size0_254[255] = {
>
> Same sort of thing

Dropped this procedure because it is not used.
Thanks.

>> diff --git a/drivers/staging/media/tw5864/tw5864-config.c 
>> b/drivers/staging/media/tw5864/tw5864-config.c
> []
>> +u8 tw_indir_readb(struct tw5864_dev *dev, u16 addr)
>> +{
>> + int timeout = 3;
>
> misleading name, retries would be more proper,
> or maybe use real timed loops.

Thanks, renamed to "retries".

> This seems a little repetitive.

Thanks, reworked.

> u16?

Thanks, fixed.

> odd indentation

Indeed. For some mysterious reason, vim + syntastic insists on this way. Fixed.

>> +#ifdef DEBUG
>> + dev_dbg(&input->root->pci->dev,
>> + "input %d, frame md stats: min %u, max %u, avg %u, cells above 
>> threshold: %u\n",
>> + input->input_number, min, max, sum / md_cells,
>> + cnt_above_thresh);
>> +#endif
>
> unnecessary #ifdef

Not quite. This debug printout works with variables which are declared
in another "#ifdef DEBUG" clause. And it turns out that dev_dbg is
compiled not only if DEBUG is declared, so when I remove this ifdef, I
get "undefined variable" errors. It seems it is compiled if this
condition is met:

#if (defined DEBUG) || (defined CONFIG_DYNAMIC_DEBUG)

so I can wrap my stats variables into this statement instead. But such
change is not equivalent - I guess CONFIG_DYNAMIC_DEBUG is common to
be enabled, so debug stats will be always calculated, even when module
is not under debug. Except if I use DEFINE_DYNAMIC_DEBUG_METADATA etc.
in my code. Please let me know if this can be sorted out in cleaner
way.



On Sun, Jan 3, 2016 at 7:38 AM, Leon Romanovsky  wrote:
> On Sun, Jan 03, 2016 at 03:41:42AM +0200, Andrey Utkin wrote:
> 
>> +/*
>> + *  TW5864 driver - Exp-Golomb code functions
>> + *
>> + *  Copyright (C) 2015 Bluecherry, LLC 
>> + *  Copyright (C) 2015 Andrey Utkin 
>
> I doubt that you have contract with your employer which permits you to
> claim copyright on the work/product.

Thank you for commenting.
I have previously asked my employer to review copyright statment, and
he told this is fine.
Now I have requrested him again with reference to your comment.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v0] Add tw5864 driver

2016-01-02 Thread Leon Romanovsky 
On Sun, Jan 03, 2016 at 03:41:42AM +0200, Andrey Utkin wrote:

> +/*
> + *  TW5864 driver - Exp-Golomb code functions
> + *
> + *  Copyright (C) 2015 Bluecherry, LLC 
> + *  Copyright (C) 2015 Andrey Utkin 

I doubt that you have contract with your employer which permits you to
claim copyright on the work/product.
Signed-of-by != copyright.


> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + */
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v0] Add tw5864 driver

2016-01-02 Thread Joe Perches 
On Sun, 2016-01-03 at 03:41 +0200, Andrey Utkin wrote:
> (Disclaimer up to scissors mark)
> 
> Please be so kind to take a look at a new driver.

trivial comments only:
> diff --git a/drivers/staging/media/tw5864/tw5864-bs.h 
> b/drivers/staging/media/tw5864/tw5864-bs.h
[]
> +static inline int bs_pos(struct bs *s)
> +{
> + return (8 * (s->p - s->p_start) + 8 - s->i_left);
> +}

several of these have unnecessary parentheses

> +static inline int bs_eof(struct bs *s)
> +{
> + return (s->p >= s->p_end ? 1 : 0);
> +}

Maybe use bool a bit more
> +/* golomb functions */
> +static inline void bs_write_ue(struct bs *s, u32 val)
> +{
> + int i_size = 0;
> + static const int i_size0_255[256] = {

Maybe use s8 instead of int to reduce object size

> + 1, 1, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 4, 4, 5, 5, 5,
> + 5, 5,
> + 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5,
> + 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6,

Perhaps it'd be clearer to use gcc's ranged initializer syntax

[  0 ...   1] = 1,
[  2 ...   3] = 2,
[  4 ...   7] = 3,
[  8 ...  15] = 4,
[ 16 ...  31] = 5,
[ 32 ...  63] = 6,
etc...

or maybe just use fls

> +static inline int bs_size_ue(unsigned int val)
> +{
> + int i_size = 0;
> + static const int i_size0_254[255] = {

Same sort of thing

> diff --git a/drivers/staging/media/tw5864/tw5864-config.c 
> b/drivers/staging/media/tw5864/tw5864-config.c
[]
> +u8 tw_indir_readb(struct tw5864_dev *dev, u16 addr)
> +{
> + int timeout = 3;

misleading name, retries would be more proper,
or maybe use real timed loops.

> + u32 data = 0;
> +
> + addr <<= 2;
> +
> + while ((tw_readl(TW5864_IND_CTL) >> 31) && (timeout--))
> + ;
> + if (!timeout)
> + dev_err(&dev->pci->dev,
> + "tw_indir_writel() timeout before reading\n");
> +
> + tw_writel(TW5864_IND_CTL, addr | TW5864_ENABLE);
> +
> + timeout = 3;
> + while ((tw_readl(TW5864_IND_CTL) >> 31) && (timeout--))
> + ;
> + if (!timeout)
> + dev_err(&dev->pci->dev,
> + "tw_indir_writel() timeout at reading\n");
> +
> + data = tw_readl(TW5864_IND_DATA);
> + return data & 0xff;
> +}
[]
> +static size_t regs_dump(struct tw5864_dev *dev, char *buf, size_t size)
> +{
> + size_t count = 0;
> +
> + u32 reg_addr;
> + u32 value;
> +
> + for (reg_addr = 0x; (count < size) && (reg_addr <= 0x2FFC);
> +  reg_addr += 4) {
> + value = tw_readl(reg_addr);
> + count += scnprintf(buf + count, size - count,
> +    "[0x%05x] = 0x%08x\n", reg_addr, value);
> + }
> +
> + for (reg_addr = 0x4000; (count < size) && (reg_addr <= 0x4FFC);
> +  reg_addr += 4) {
> + value = tw_readl(reg_addr);
> + count += scnprintf(buf + count, size - count,
> +    "[0x%05x] = 0x%08x\n", reg_addr, value);
> + }
> +
> + for (reg_addr = 0x8000; (count < size) && (reg_addr <= 0x180DC);
> +  reg_addr += 4) {
> + value = tw_readl(reg_addr);
> + count += scnprintf(buf + count, size - count,
> +    "[0x%05x] = 0x%08x\n", reg_addr, value);
> + }
> +
> + for (reg_addr = 0x18100; (count < size) && (reg_addr <= 0x1817C);
> +  reg_addr += 4) {
> + value = tw_readl(reg_addr);
> + count += scnprintf(buf + count, size - count,
> +    "[0x%05x] = 0x%08x\n", reg_addr, value);
> + }
> +
> + for (reg_addr = 0x8; (count < size) && (reg_addr <= 0x87FFF);
> +  reg_addr += 4) {
> + value = tw_readl(reg_addr);
> + count += scnprintf(buf + count, size - count,
> +    "[0x%05x] = 0x%08x\n", reg_addr, value);
> + }

This seems a little repetitive.

> diff --git a/drivers/staging/media/tw5864/tw5864-tables.h 
> b/drivers/staging/media/tw5864/tw5864-tables.h
[]
> +static const u32 forward_quantization_table[QUANTIZATION_TABLE_LEN] = {

u16?


> + static const struct v4l2_ctrl_config tw5864_md_thresholds = {
> + .ops = &tw5864_ctrl_ops,
> + .id = V4L2_CID_DETECT_MD_THRESHOLD_GRID,
> + .dims = {MD_CELLS_HOR, MD_CELLS_VERT},
> + .def = 14,
> + /* See tw5864_md_metric_from_mvd() */
> + .max = 2 * 0x0f,
> + .step = 1,
> + };

odd indentation

> +#ifdef DEBUG
> + dev_dbg(&input->root->pci->dev,
> + "input %d, frame md stats: min %u, max %u, avg %u, cells above 
> threshold: %u\n",
> + input->input_number, min, max, sum / md_cells,
> + cnt_above_thresh);
> +#endif

unnecessary #ifdef

--
To unsubscribe from this list: send the line "unsubscri