Re: [PATCH v10 0/8] Add MT8173 Video Encoder Driver and VPU Driver

[Hans Verkuil]

On 05/04/2016 10:19 AM, tiffany lin wrote:
> Hi Hans,
> 
> On Wed, 2016-05-04 at 09:32 +0200, Hans Verkuil wrote:
>> Hi Tiffany,
>>
>> On 05/03/2016 12:11 PM, Tiffany Lin wrote:
>>> ==============
>>>  Introduction
>>> ==============
>>>
>>> The purpose of this series is to add the driver for video codec hw embedded in the Mediatek's MT8173 SoCs.
>>> Mediatek Video Codec is able to handle video encoding of in a range of formats.
>>>
>>> This patch series also include VPU driver. Mediatek Video Codec driver rely on VPU driver to load,
>>> communicate with VPU.
>>>
>>> Internally the driver uses videobuf2 framework and MTK IOMMU and MTK SMI both have been merged in v4.6-rc1.
>>> This patch series need [PATCH v15 8/8] memory: mtk-smi: export mtk_smi_larb_get/put[1] to build as module
>>>
>>> [1]http://lists.infradead.org/pipermail/linux-mediatek/2016-April/005173.html
>>>
>>> ==================
>>>  Device interface
>>> ==================
>>>
>>> In principle the driver bases on v4l2 memory-to-memory framework:
>>> it provides a single video node and each opened file handle gets its own private context with separate
>>> buffer queues. Each context consist of 2 buffer queues: OUTPUT (for source buffers, i.e. raw video
>>> frames) and CAPTURE (for destination buffers, i.e. encoded video frames).
>>>
>>> ==============================
>>>  VPU (Video Processor Unit)
>>> ==============================
>>> The VPU driver for hw video codec embedded in Mediatek's MT8173 SOCs.
>>> It is able to handle video decoding/encoding in a range of formats.
>>> The driver provides with VPU firmware download, memory management and the communication interface between CPU and VPU.
>>> For VPU initialization, it will create virtual memory for CPU access and physical address for VPU hw device access. 
>>> When a decode/encode instance opens a device node, vpu driver will download vpu firmware to the device.
>>> A decode/encode instant will decode/encode a frame using VPU interface to interrupt vpu to handle decoding/encoding jobs.
>>>
>>> Please have a look at the code and comments will be very much appreciated.
>>>
>>> Change in v10:
>>> 1. Fix smatch/sparse error message
>>> 2. Add depends on ARM || ARM64 and MTK_IOMMU in Kconfig
>>
>> I don't like the ARM or ARM64 dependency since that makes COMPILE_TEST harder.
>>
>> I removed it here and for the VPU and everything still compiles fine with
>> sparse and smatch. So I'll drop those dependencies.
>>
>> Why is the MTK_IOMMU dependency needed? Just curious.
>>
> This is because we need MTK_IOMMU module to get dma continuous memory
> for HW.
> 
>> Note that sparse still complains about this:
>>
>> media-git/drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c:261:24: warning: incorrect type in assignment (different base types)
>>
>> I think this code should be rewritten. Something like this would work
>> (if I understand the code correctly):
>>
>> u32 tag = (bs_hdr_len << 5) | 0x10 | not_key;
>>
>> ac_tag[0] = tag & 0xff;
>> ac_tag[1] = (tag >> 8) & 0xff;
>> ac_tag[2] = (tag >> 16) & 0xff;
>>
>> It's perhaps a bit more verbose, but a lot easier to understand.
>>
> 
>> I'm accepting this driver but I'll remove the ARM || ARM64 dependency. Can
>> you post a follow-up patch for this sparse warning?
>>
> 
> Got it. We will provide a patch to fix this.
> I was curious why my sparse do not show this warning.
> Is there any parameter I need to set except C=1?

I'm running it with these parameters: C=2 CF="-D__CHECK_ENDIAN__"

I suspect it is the CHECK_ENDIAN flag that does it.

Regards,

	Hans