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

[Hans Verkuil <hansverk@cisco.com>]

On 12/15/15 11:01, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-12-14 16:40, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> 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.

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

Regards,

	Hans