Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Sergey Senozhatsky]

On (20/03/24 07:17), Ezequiel Garcia wrote:
[..]
> > > > +static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem)
> > > > +{
> > > > +	if (!vb2_queue_allows_cache_hints(q))
> > > > +		return;
> > > > +
> > > > +	if (consistent_mem)
> > > > +		q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
> > > > +	else
> > > > +		q->dma_attrs |= DMA_ATTR_NON_CONSISTENT;
> > > > +}
> > > > +
> > > >   int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> > > > -		unsigned int *count)
> > > > +		bool consistent_mem, unsigned int *count)
> > > You extended the vb2_core_reqbufs accepting a new boolean that
> > > is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT
> > > but in the future some other flags might be added, and so I think it
> > > is better to replace the boolean with a u32 consisting of all the flags.
> > 
> > Don't have any objections. Can change the `bool' to `u32'.
> > 
> 
> or maybe an enum instead? That would help get a cleaner
> interface.

Hmm, interesting.

The flags in question can be from different, unrelated groups
(types), controlling various parts of the stack. Not necessarily
all of them are memory_consistency related. We can, for instance,
pass some additional flags to underlying memory allocators (contig,
sg), etc.

E.g.

	enum MEMORY_ATTR {
		MEM_NON_CONSISTENT,
		...
	};

	enum VMALLOC_ALLOCATOR_ATTR {
		DO_A_BARREL_ROLL,
		...
	};

	enum DMA_SG_ALLOCATOR_ATTR {
		WRITEBACK_TO_TAPE_DEVICE,
		...
	};

	enum DMA_CONTIG_ALLOCATOR_ATTR {
		PREFER_HTTPS,
		...
	};

and so on. We maybe can keep all of those in one enum (umm, what should
be the name?), and then either make sure that all of them are proper powers
of two

	enum AUX_FLAGS {
		MEM_NON_CONSISTENT		= (1 << 0),
		DO_A_BARREL_ROLL		= (1 << 1),
		WRITEBACK_TO_TAPE_DEVICE	= (1 << 2),
		PREFER_HTTPS			= (1 << 3),
	};

or
	enum AUX_FLAGS {
		MEM_NON_CONSISTENT		= 0,
		DO_A_BARREL_ROLL,
		WRITEBACK_TO_TAPE_DEVICE,
		PREFER_HTTPS,
	};

and make sure that those are not flags, but bits.
IOW, if (flags & BIT(MEM_NON_CONSISTENT)).


What do others think?

	-ss