Re: [RFC 0/9] Unrestricted media entity ID range support

[Sakari Ailus]

Hi Mauro,

Mauro Carvalho Chehab wrote:

Em Sun, 20 Sep 2015 10:26:09 +0300
Sakari Ailus  escreveu:


Hi Mauro,

Mauro Carvalho Chehab wrote:

Hi Sakari,

On Fri, 11 Sep 2015 13:09:03 +0300
Sakari Ailus  wrote:


Hi all,

This patchset adds an API for managing entity enumerations, i.e.
storing a bit of information per entity. The entity ID is no longer
limited to small integers (e.g. 31 or 63), but
MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
instead of a fixed number.

If the number of entities in a real use case grows beyond the
capabilities of the approach, the algorithm may be changed. But most
importantly, the API is used to perform the same operation everywhere
it's done.

I'm sending this for review only, the code itself is untested.

I haven't entirely made up my mind on the fourth patch. It could be
dropped, as it uses the API for a somewhat different purpose.



Sorry for not reviewing this earlier, but I'm traveling this week to
China, and I was having some troubles with the Internet. Btw, I don't
have my notebook here (just a chromebook without the media tree).
So, please consider this as just a preliminary review.

I won't be comment this series patch by patch, because it is really
painful to do it while here with this Internet.

Also, I want to discuss the patch series as a hole.

  From what we've agreed last week, we won't be using a separate ID
range for the entity ID, but this patch series is actually adding
it, and, IMHO, using a confusing nomenclature: instead of calling the
entity ID range as "entity_id" at the media_device struct, you're
now calling it "low_id". That sounds confusing to me. If you think
we should keep a separate range for entities, calling it as
"entity_id" is clearer.


It's *not* the entity ID. It's a number used internally to keep track of
the entities, and only used for this purpose, nothing else. If you look
at the patch, the number of places where low_id is used is very limited.
Individual drivers do not and must not access the low_id field.

The underlying algorithm for keeping track of entities does not change,
but that could be changed later on without affecting the users of the
alrogithm. --- See patch 3.

There are two main reasons for this:

1. No need to implement a new algorithm which would be less efficient
but could cope in cases we do not have yet; this can be done later on,
as patch 3 adds an API to access the information without making
assumptions on the implementation.


If this is an internal number used only by the graph traversal
algorithm, then the implementation doesn't seem correct. I mean:
such number should be generated internally when starting the
graph traversal algorithm, and it would be better to store such
graph-traversal internal algorithm data on a separate struct.


In that case the number would become enumeration specific. I have no 
problem in that, but if a single entity specific number works for the 
purpose, I think that could be used as well. Using something else than a 
constant value per entity quickly will require memory allocation, and 
graph traversal is performance critical in many cases.





2. It does solve the problem. The graph object IDs of the entities can
be large without adversely affecting the functionality of existing drivers.


Right now, with just those 9 patches, it doesn't ;)

I mean: if I apply your patches after the MC next gen ones and try to use the
graph traversal, it will do the wrong thing for hybrid TV cards, as the number
of entities there are more than 64. Ok, easy to fix after this series by
just changing the value of MEDIA_ENTITY_MAX_LOW_ID, but this will only
work while we don't implement dynamic support.


Fair enough. Still, it didn't work before either: the graph traversal 
algorithm depends on entity ID not exceeding MEDIA_ENTITY_ENUM_MAX_ID 
(which this set renames).


I still consider that a separate problem.





Also, you said at the low_id comment that if an entity is
unregistered and then re-registered, it would preserve the same


I never claimed that, and the patchset does not implement that either.


That's what I understood from this comment:

Rename the macro as it no longer is a maximum ID that an entity can 
have,
but a low ID which is used for internal purposes of enumeration. This is
the maximum number of concurrent entities that may exist in a media
device.

If this is the "max number of concurrent entities", you need to reassign
those numbers when entities are removed.

If this is not what you're meaning, then fix the patch description
to let it clearer.

I guess then that "low ID" is actually an upper bound for that
graph-traversal only control number. We should really not use the word "ID"
here, as this is not an ID. it is just some index/control number that will
be granted to be below some upper bound.


Index sounds good to me, it's indeed clearer. I 

Re: [RFC 0/9] Unrestricted media entity ID range support

[Mauro Carvalho Chehab]

Em Wed, 23 Sep 2015 00:56:05 +0300
Sakari Ailus  escreveu:

> Hi Mauro,
> 
> Mauro Carvalho Chehab wrote:
> > Em Sun, 20 Sep 2015 10:26:09 +0300
> > Sakari Ailus  escreveu:
> >
> >> Hi Mauro,
> >>
> >> Mauro Carvalho Chehab wrote:
> >>> Hi Sakari,
> >>>
> >>> On Fri, 11 Sep 2015 13:09:03 +0300
> >>> Sakari Ailus  wrote:
> >>>
>  Hi all,
> 
>  This patchset adds an API for managing entity enumerations, i.e.
>  storing a bit of information per entity. The entity ID is no longer
>  limited to small integers (e.g. 31 or 63), but
>  MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
>  instead of a fixed number.
> 
>  If the number of entities in a real use case grows beyond the
>  capabilities of the approach, the algorithm may be changed. But most
>  importantly, the API is used to perform the same operation everywhere
>  it's done.
> 
>  I'm sending this for review only, the code itself is untested.
> 
>  I haven't entirely made up my mind on the fourth patch. It could be
>  dropped, as it uses the API for a somewhat different purpose.
> 
> >>>
> >>> Sorry for not reviewing this earlier, but I'm traveling this week to
> >>> China, and I was having some troubles with the Internet. Btw, I don't
> >>> have my notebook here (just a chromebook without the media tree).
> >>> So, please consider this as just a preliminary review.
> >>>
> >>> I won't be comment this series patch by patch, because it is really
> >>> painful to do it while here with this Internet.
> >>>
> >>> Also, I want to discuss the patch series as a hole.
> >>>
> >>>   From what we've agreed last week, we won't be using a separate ID
> >>> range for the entity ID, but this patch series is actually adding
> >>> it, and, IMHO, using a confusing nomenclature: instead of calling the
> >>> entity ID range as "entity_id" at the media_device struct, you're
> >>> now calling it "low_id". That sounds confusing to me. If you think
> >>> we should keep a separate range for entities, calling it as
> >>> "entity_id" is clearer.
> >>
> >> It's *not* the entity ID. It's a number used internally to keep track of
> >> the entities, and only used for this purpose, nothing else. If you look
> >> at the patch, the number of places where low_id is used is very limited.
> >> Individual drivers do not and must not access the low_id field.
> >>
> >> The underlying algorithm for keeping track of entities does not change,
> >> but that could be changed later on without affecting the users of the
> >> alrogithm. --- See patch 3.
> >>
> >> There are two main reasons for this:
> >>
> >> 1. No need to implement a new algorithm which would be less efficient
> >> but could cope in cases we do not have yet; this can be done later on,
> >> as patch 3 adds an API to access the information without making
> >> assumptions on the implementation.
> >
> > If this is an internal number used only by the graph traversal
> > algorithm, then the implementation doesn't seem correct. I mean:
> > such number should be generated internally when starting the
> > graph traversal algorithm, and it would be better to store such
> > graph-traversal internal algorithm data on a separate struct.
> 
> In that case the number would become enumeration specific. I have no 
> problem in that, but if a single entity specific number works for the 
> purpose, I think that could be used as well. Using something else than a 
> constant value per entity quickly will require memory allocation, and 
> graph traversal is performance critical in many cases.

I guess a single loop at graph traversal start would solve it. Yet,
it means an extra O(n) to fill in the numbers, but the algorithm
that fills such numbers will be at least O(n) either here or at the
entity creation.

Perhaps one solution would be to have a "dirty" flag that would be
raised if a new entity would be created (and removed?). If dirty
flag is raised, it would do the count at the beginning of the graph
start.

> >
> >> 2. It does solve the problem. The graph object IDs of the entities can
> >> be large without adversely affecting the functionality of existing drivers.
> >
> > Right now, with just those 9 patches, it doesn't ;)
> >
> > I mean: if I apply your patches after the MC next gen ones and try to use 
> > the
> > graph traversal, it will do the wrong thing for hybrid TV cards, as the 
> > number
> > of entities there are more than 64. Ok, easy to fix after this series by
> > just changing the value of MEDIA_ENTITY_MAX_LOW_ID, but this will only
> > work while we don't implement dynamic support.
> 
> Fair enough. Still, it didn't work before either: the graph traversal 
> algorithm depends on entity ID not exceeding MEDIA_ENTITY_ENUM_MAX_ID 
> (which this set renames).

Well, for existing drivers that use graph traversal, this number is
not exceeded.

> I still 

Re: [RFC 0/9] Unrestricted media entity ID range support

[Mauro Carvalho Chehab]

Em Sun, 20 Sep 2015 10:26:09 +0300
Sakari Ailus  escreveu:

> Hi Mauro,
> 
> Mauro Carvalho Chehab wrote:
> > Hi Sakari,
> >
> > On Fri, 11 Sep 2015 13:09:03 +0300
> > Sakari Ailus  wrote:
> >
> >> Hi all,
> >>
> >> This patchset adds an API for managing entity enumerations, i.e.
> >> storing a bit of information per entity. The entity ID is no longer
> >> limited to small integers (e.g. 31 or 63), but
> >> MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
> >> instead of a fixed number.
> >>
> >> If the number of entities in a real use case grows beyond the
> >> capabilities of the approach, the algorithm may be changed. But most
> >> importantly, the API is used to perform the same operation everywhere
> >> it's done.
> >>
> >> I'm sending this for review only, the code itself is untested.
> >>
> >> I haven't entirely made up my mind on the fourth patch. It could be
> >> dropped, as it uses the API for a somewhat different purpose.
> >>
> >
> > Sorry for not reviewing this earlier, but I'm traveling this week to
> > China, and I was having some troubles with the Internet. Btw, I don't
> > have my notebook here (just a chromebook without the media tree).
> > So, please consider this as just a preliminary review.
> >
> > I won't be comment this series patch by patch, because it is really
> > painful to do it while here with this Internet.
> >
> > Also, I want to discuss the patch series as a hole.
> >
> >  From what we've agreed last week, we won't be using a separate ID
> > range for the entity ID, but this patch series is actually adding
> > it, and, IMHO, using a confusing nomenclature: instead of calling the
> > entity ID range as "entity_id" at the media_device struct, you're
> > now calling it "low_id". That sounds confusing to me. If you think
> > we should keep a separate range for entities, calling it as
> > "entity_id" is clearer.
> 
> It's *not* the entity ID. It's a number used internally to keep track of 
> the entities, and only used for this purpose, nothing else. If you look 
> at the patch, the number of places where low_id is used is very limited. 
> Individual drivers do not and must not access the low_id field.
> 
> The underlying algorithm for keeping track of entities does not change, 
> but that could be changed later on without affecting the users of the 
> alrogithm. --- See patch 3.
> 
> There are two main reasons for this:
> 
> 1. No need to implement a new algorithm which would be less efficient 
> but could cope in cases we do not have yet; this can be done later on, 
> as patch 3 adds an API to access the information without making 
> assumptions on the implementation.

If this is an internal number used only by the graph traversal
algorithm, then the implementation doesn't seem correct. I mean:
such number should be generated internally when starting the
graph traversal algorithm, and it would be better to store such
graph-traversal internal algorithm data on a separate struct.

> 2. It does solve the problem. The graph object IDs of the entities can 
> be large without adversely affecting the functionality of existing drivers.

Right now, with just those 9 patches, it doesn't ;)

I mean: if I apply your patches after the MC next gen ones and try to use the
graph traversal, it will do the wrong thing for hybrid TV cards, as the number
of entities there are more than 64. Ok, easy to fix after this series by
just changing the value of MEDIA_ENTITY_MAX_LOW_ID, but this will only
work while we don't implement dynamic support.

> >
> > Also, you said at the low_id comment that if an entity is
> > unregistered and then re-registered, it would preserve the same
> 
> I never claimed that, and the patchset does not implement that either.

That's what I understood from this comment:

Rename the macro as it no longer is a maximum ID that an entity can 
have,
but a low ID which is used for internal purposes of enumeration. This is
the maximum number of concurrent entities that may exist in a media
device.

If this is the "max number of concurrent entities", you need to reassign
those numbers when entities are removed.

If this is not what you're meaning, then fix the patch description
to let it clearer.

I guess then that "low ID" is actually an upper bound for that
graph-traversal only control number. We should really not use the word "ID"
here, as this is not an ID. it is just some index/control number that will
be granted to be below some upper bound.

> If an entity is unregistered and registered again, from the MC point of 
> view it is not the same entity. We do not keep track of entities that 
> are not registered with MC, do we?
> 
> > entity ID. That doesn't seem easy to implement, as we would need
> > to track those previously-used ID. On the other hand, if we just
> > re-use a previously released ID for some other entity, this can
> > be a problem, as userspace may 

Re: [RFC 0/9] Unrestricted media entity ID range support

[Sakari Ailus]

Hi Mauro,

Mauro Carvalho Chehab wrote:

Hi Sakari,

On Fri, 11 Sep 2015 13:09:03 +0300
Sakari Ailus  wrote:


Hi all,

This patchset adds an API for managing entity enumerations, i.e.
storing a bit of information per entity. The entity ID is no longer
limited to small integers (e.g. 31 or 63), but
MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
instead of a fixed number.

If the number of entities in a real use case grows beyond the
capabilities of the approach, the algorithm may be changed. But most
importantly, the API is used to perform the same operation everywhere
it's done.

I'm sending this for review only, the code itself is untested.

I haven't entirely made up my mind on the fourth patch. It could be
dropped, as it uses the API for a somewhat different purpose.



Sorry for not reviewing this earlier, but I'm traveling this week to
China, and I was having some troubles with the Internet. Btw, I don't
have my notebook here (just a chromebook without the media tree).
So, please consider this as just a preliminary review.

I won't be comment this series patch by patch, because it is really
painful to do it while here with this Internet.

Also, I want to discuss the patch series as a hole.

 From what we've agreed last week, we won't be using a separate ID
range for the entity ID, but this patch series is actually adding
it, and, IMHO, using a confusing nomenclature: instead of calling the
entity ID range as "entity_id" at the media_device struct, you're
now calling it "low_id". That sounds confusing to me. If you think
we should keep a separate range for entities, calling it as
"entity_id" is clearer.


It's *not* the entity ID. It's a number used internally to keep track of 
the entities, and only used for this purpose, nothing else. If you look 
at the patch, the number of places where low_id is used is very limited. 
Individual drivers do not and must not access the low_id field.


The underlying algorithm for keeping track of entities does not change, 
but that could be changed later on without affecting the users of the 
alrogithm. --- See patch 3.


There are two main reasons for this:

1. No need to implement a new algorithm which would be less efficient 
but could cope in cases we do not have yet; this can be done later on, 
as patch 3 adds an API to access the information without making 
assumptions on the implementation.


2. It does solve the problem. The graph object IDs of the entities can 
be large without adversely affecting the functionality of existing drivers.




Also, you said at the low_id comment that if an entity is
unregistered and then re-registered, it would preserve the same


I never claimed that, and the patchset does not implement that either.

If an entity is unregistered and registered again, from the MC point of 
view it is not the same entity. We do not keep track of entities that 
are not registered with MC, do we?



entity ID. That doesn't seem easy to implement, as we would need
to track those previously-used ID. On the other hand, if we just
re-use a previously released ID for some other entity, this can
be a problem, as userspace may not be aware of such changes and
might be asking the Kernel to do the wrong thing.


Let's not do that then. This is why we have graph object IDs.



So, I can't see how non-monotonically incremented numbers would
work here.

Finally, the changes you did still rely on having the ID limited
to a well-defined, hardcoded number (MEDIA_ENTITY_MAX_LOW_ID).

I can see this working only if:

- We keep a separate range ID for entities (so, having a minimum
   of two ranges);

- the entity maximum ID is defined by the driver (as the number
   of entities is actually dependent on the hardware);


The case is that we do not have a driver requiring more entities. Once 
we do, we can implement a new algorithm for the purpose. Memory 
allocation will be required, but that's a separate matter from the graph 
object ID of the entities, which is the problem this patchset was 
intended to solve.




- some other mechanism would be available for drivers that
   would support dynamic entity creation.

So, I don't see how this would solve the problems that we
discussed at the last week IRC chats.

Am I missing something?


This set indeed solves a problem, and that problem was unrestricting the 
graph object ID of the entities. There are other problems remaining 
before entities can be e.g. unregistered one by one, but they are 
separate problems.




Regards,
Mauro

PS.: sparse already complains on two places at the media-entity where
bitmaps are declared at the stack. With max entities equal to 64,
that's not an issue, but if we change to a higher number, those will
need to be dynamically allocated, in order to avoid stack overflows.
I didn't see any patches touching that.


I agree. The aim of the set was not to increase the number of concurrent 
entities. That is a separate problem which can be 

Re: [RFC 0/9] Unrestricted media entity ID range support

[Mauro Carvalho Chehab]

Hi Sakari,

On Fri, 11 Sep 2015 13:09:03 +0300
Sakari Ailus  wrote:

> Hi all,
> 
> This patchset adds an API for managing entity enumerations, i.e.
> storing a bit of information per entity. The entity ID is no longer
> limited to small integers (e.g. 31 or 63), but
> MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
> instead of a fixed number.
> 
> If the number of entities in a real use case grows beyond the
> capabilities of the approach, the algorithm may be changed. But most
> importantly, the API is used to perform the same operation everywhere
> it's done.
> 
> I'm sending this for review only, the code itself is untested.
> 
> I haven't entirely made up my mind on the fourth patch. It could be
> dropped, as it uses the API for a somewhat different purpose.
> 

Sorry for not reviewing this earlier, but I'm traveling this week to
China, and I was having some troubles with the Internet. Btw, I don't
have my notebook here (just a chromebook without the media tree).
So, please consider this as just a preliminary review.

I won't be comment this series patch by patch, because it is really
painful to do it while here with this Internet.

Also, I want to discuss the patch series as a hole.

>From what we've agreed last week, we won't be using a separate ID
range for the entity ID, but this patch series is actually adding
it, and, IMHO, using a confusing nomenclature: instead of calling the
entity ID range as "entity_id" at the media_device struct, you're
now calling it "low_id". That sounds confusing to me. If you think
we should keep a separate range for entities, calling it as 
"entity_id" is clearer.

Also, you said at the low_id comment that if an entity is
unregistered and then re-registered, it would preserve the same
entity ID. That doesn't seem easy to implement, as we would need
to track those previously-used ID. On the other hand, if we just
re-use a previously released ID for some other entity, this can
be a problem, as userspace may not be aware of such changes and
might be asking the Kernel to do the wrong thing.

So, I can't see how non-monotonically incremented numbers would
work here.

Finally, the changes you did still rely on having the ID limited
to a well-defined, hardcoded number (MEDIA_ENTITY_MAX_LOW_ID).

I can see this working only if:

- We keep a separate range ID for entities (so, having a minimum
  of two ranges);

- the entity maximum ID is defined by the driver (as the number
  of entities is actually dependent on the hardware);

- some other mechanism would be available for drivers that
  would support dynamic entity creation.

So, I don't see how this would solve the problems that we
discussed at the last week IRC chats.

Am I missing something?

Regards,
Mauro

PS.: sparse already complains on two places at the media-entity where
bitmaps are declared at the stack. With max entities equal to 64,
that's not an issue, but if we change to a higher number, those will
need to be dynamically allocated, in order to avoid stack overflows.
I didn't see any patches touching that.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 0/9] Unrestricted media entity ID range support

[Sakari Ailus]

Hi all,

This patchset adds an API for managing entity enumerations, i.e. storing a
bit of information per entity. The entity ID is no longer limited to small
integers (e.g. 31 or 63), but MEDIA_ENTITY_MAX_LOW_ID. The drivers are
also converted to use that instead of a fixed number.

If the number of entities in a real use case grows beyond the capabilities
of the approach, the algorithm may be changed. But most importantly, the
API is used to perform the same operation everywhere it's done.

I'm sending this for review only, the code itself is untested.

I haven't entirely made up my mind on the fourth patch. It could be
dropped, as it uses the API for a somewhat different purpose.

-- 
Kind regards,
Sakari

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html