Re: [RFC PATCH] vb2: Stop allocating 'alloc_ctx', just set the device instead

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hello,

On 2015-12-15 11:14, Hans Verkuil wrote:
> On 12/15/15 11:01, Marek Szyprowski wrote:
>> On 2015-12-14 16:40, Laurent Pinchart wrote:
>>> On Monday 14 December 2015 15:36:04 Hans Verkuil wrote:
>>>> (Before I post this as the 'final' patch and CC all the driver developers
>>>> that are affected, I'd like to do an RFC post first. I always hated the
>>>> alloc context for obfuscating what is really going on, but let's see what
>>>> others think).
>>>>
>>>>
>>>> Instead of allocating a struct that contains just a single device pointer,
>>>> just pass that device pointer around. This avoids having to check for
>>>> memory allocation errors and is much easier to understand since it makes
>>>> explicit what was hidden in an opaque handle before.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>> As most devices use the same allocation context for all planes, wouldn't it
>>> make sense to just store the struct device pointer in the queue structure ?
>>> The oddball driver that requires different allocation contexts (I'm thinking
>>> about s5p-mfc here, there might be a couple more) would have to set the
>>> allocation contexts properly in the queue_setup handler, but for all other
>>> devices you could just remove that code completely.
>> This seams a reasonable approach. vb2 was written with special (or strange ;)
>> requirements in mind to align it with Exynos HW. However after some time it
>> turned out that most device drivers are simple and don't need fancy handling
>> of allocator context, so this definitely can be simplified. It also turned out
>> also that there is no real 'context' for vb2 memory allocators, although some
>> out-of-tree code (used in Android kernels) use some more advanced structures
>> there. Maybe it will be enough to let drivers to change defaults in queue_setup
>> and ensure that there is a 'void *alloc_ctx_priv' placeholder for allocator
>> specific data.
>>
>> There is one more advantage of moving struct device * to vb2_queue. One can
>> then change all debugs to use dev_info/err/dbg infrastructure, so the logs will
>> be significantly easier to read, especially when more than one media device is
>> used.
> I hadn't thought of that. That's nice indeed. However, it would require that
> vb2-vmalloc drivers also set the device pointer.

They have some kind of virtual struct device (or fake platform device) 
registered
somewhere anyway, so this should not be a problem. In case of usb 
devices, they can
use struct device from that usb device. In both cases this will make the 
logs much
easier to read.

> I think I'll work on this a bit more, clearly it is the right direction to go.

Thanks!

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland