Re: [PATCH 0/3] Add media bdisp driver for stihxxx platforms

[Hans Verkuil <hverkuil@xs4all.nl>]

On 04/27/2015 06:25 PM, Hans Verkuil wrote:
> Hi Fabien,
> 
> Thank you for this driver! Good to see V4L2 support for this SoC.
> 
> I did a quick initial scan over the driver and there are a few things that
> need to be addressed:
> 
> - I think bdisp as the driver name is a bit generic, perhaps something like
>   stih4xx-bdisp might be more appropriate. Similar to the exynos-* drivers.
> 
> - Replace cropcap/g_crop/s_crop by the g/s_selection ioctls. The old ioctls
>   are no longer supported for new drivers (the v4l2 core will automatically
>   add support for those ioctls if g/s_selection is implemented in the driver).
>   Read careful how crop and compose rectangles are used in a m2m device. I
>   would expect that you implement cropping for the BUF_TYPE_VIDEO_OUTPUT side
>   (i.e. memory to hardware) and implement composing for the BUF_TYPE_VIDEO_CAPTURE
>   side (i.e. hardware to memory).
> 
>   If the hardware also support composition for output or cropping for capture,
>   then let me know: in that case you will likely have to implement support for
>   V4L2_SEL_TGT_NATIVE_SIZE as well.
> 
> - Several ioctl and fop helpers were added to media/v4l2-mem2mem.h (e.g.
>   v4l2_m2m_ioctl_reqbufs, v4l2_m2m_fop_mmap, etc.). Use these instead of
>   rolling your own.

Two more comments:

- You can drop the desc field from struct bdisp_format: a patch was merged in
  media_tree.git that sets the VIDIOC_ENUM_FMT description in the v4l2 core,
  so it can be dropped from drivers.

- I noticed that you call video_device_release, but you shouldn't since struct
  video_device is embedded in a larger struct. video_device_release attempts
  to kfree the video_device, which obviously is wrong. Just remove that call.

Regards,

	Hans

> 
> - I would like to see the output of these v4l2-compliance commands:
> 
> 	v4l2-compliance
> 	v4l2-compliance -s
> 	v4l2-compliance -f
> 
>   In all fairness: mem2mem devices are not often tested using v4l2-compliance
>   and there may be problems testing this (-f will likely fail), but I still
>   like to see the output so I know what works and what doesn't.
> 
>   Please use the latest v4l2-compliance code from the v4l-utils.git repository.
>   I won't accept the driver unless I see the results of these compliance tests:
>   running this is required for new drivers since it is a great way of verifying
>   the completeness of your driver.
> 
> Regards,
> 
> 	Hans
> 
> On 04/27/2015 05:56 PM, Fabien Dessenne wrote:
>> This series of patches adds the support of v4l2 2D blitter driver for
>> STMicroelectronics SOC.
>>
>> The following features are supported and tested:
>> - Color format conversion (RGB32, RGB24, RGB16, NV12, YUV420P)
>> - Copy
>> - Scale
>> - Flip
>> - Deinterlace
>> - Wide (4K) picture support
>> - Crop
>>
>> This driver uses the v4l2 mem2mem framework and its implementation was largely
>> inspired by the Exynos G-Scaler (exynos-gsc) driver.
>>
>> The driver is mainly implemented across two files:
>> - bdisp-v4l2.c
>> - bdisp-hw.c
>> bdisp-v4l2.c uses v4l2_m2m to manage the V4L2 interface with the userland. It
>> calls the HW services that are implemented in bdisp-hw.c.
>>
>> The additional bdisp-debug.c file manages some debugfs entries.
>>
>> Fabien Dessenne (3):
>>   [media] bdisp: add DT bindings documentation
>>   [media] bdisp: 2D blitter driver using v4l2 mem2mem framework
>>   [media] bdisp: add debug file system
>>
>>  .../devicetree/bindings/media/st,stih4xx.txt       |   32 +
>>  drivers/media/platform/Kconfig                     |   10 +
>>  drivers/media/platform/Makefile                    |    2 +
>>  drivers/media/platform/bdisp/Kconfig               |    9 +
>>  drivers/media/platform/bdisp/Makefile              |    3 +
>>  drivers/media/platform/bdisp/bdisp-debug.c         |  668 +++++++++
>>  drivers/media/platform/bdisp/bdisp-filter.h        |  346 +++++
>>  drivers/media/platform/bdisp/bdisp-hw.c            |  823 +++++++++++
>>  drivers/media/platform/bdisp/bdisp-reg.h           |  235 +++
>>  drivers/media/platform/bdisp/bdisp-v4l2.c          | 1492 ++++++++++++++++++++
>>  drivers/media/platform/bdisp/bdisp.h               |  220 +++
>>  11 files changed, 3840 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/st,stih4xx.txt
>>  create mode 100644 drivers/media/platform/bdisp/Kconfig
>>  create mode 100644 drivers/media/platform/bdisp/Makefile
>>  create mode 100644 drivers/media/platform/bdisp/bdisp-debug.c
>>  create mode 100644 drivers/media/platform/bdisp/bdisp-filter.h
>>  create mode 100644 drivers/media/platform/bdisp/bdisp-hw.c
>>  create mode 100644 drivers/media/platform/bdisp/bdisp-reg.h
>>  create mode 100644 drivers/media/platform/bdisp/bdisp-v4l2.c
>>  create mode 100644 drivers/media/platform/bdisp/bdisp.h
>>
> 
> --
> 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
>