by Sergey Senozhatsky

[permalink] [raw]

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

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

2020-03-27 11:33:44

by Tomasz Figa

[permalink] [raw]

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

On Wed, Mar 25, 2020 at 3:32 AM Sergey Senozhatsky
<[email protected]> wrote:
>
> 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?

My feeling is that there it's a bit of an abuse of the language
construct. Enum is expected to be an enumeration type, where the value
is always one and only one of the defined values at the same time.
Making a bitwise OR of several values makes the resulting value
outside of the enum specification.

AFAICT, the typical approach in the kernel is to just have a block of
#define statements to define the flags and have the flag names
prefixed with some consistent prefix, e.g. VB2_QUEUE_FLAG_. The flags
itself would be defined using BIT() so they can be used directly in
the bitwise arithmetics.

Best regards,
Tomasz