Rework device names

[evelikov]

The current GetDriver API is rather fragile and bonkers.

In particular it does a two-step pass - number vs names. That in itself
is not great, since it assumes the devices do not change in between.
Additionally there is the assumption of index X will match device (name)
X, which does not hold true.

The most prominent issue is the inability to provide accurate number and
list of names.

Instead, we can have a single API, that provides both pieces of data in
one do - shorter and simpler code - in reliable manner.

---

In other words: this replaces and deprecates vaGetNumCandidates && vaGetDriverNameByIndex in favour of vaGetDriverNames

This is the first step towards fixing #677

[evelikov]

@sivileri this MR reworks the driver-name API, conflicting with your other work. Would be great if we can merge this first, but at the least can you please check this doesn't explode on Windows. Thanks o/

@XinfengZhang @dvrogozh this is the first big step towards addressing the DRI3 regressions some people are seeing. Would be great if we can get to it sooner than later.

[ceyusa]

fwiw, it looks cleaner and robust.

What worries me is that drivers lists are scattered among different backend implementation files, and, iiuc, some drivers can be used by several backends. I wonder if we could centralized all of them in a single file so we could update that single structure for all at once. Anyway, this change can be done later.

[evelikov]

fwiw, it looks cleaner and robust.

What worries me is that drivers lists are scattered among different backend implementation files, and, iiuc, some drivers can be used by several backends. I wonder if we could centralized all of them in a single file so we could update that single structure for all at once. Anyway, this change can be done later.

IMHO we should probably spend our efforts into a proper mechanism (say something like the Vulkan ICD/json) to remove this hard-coding + add some order/priority guarantees. Which in itself is an important yet orthogonal task.

[evelikov]

@XinfengZhang @dvrogozh any comments?

[dvrogozh]

So, proposal is to completely replace 2 functions currently used to enumerate drivers:

$ cat va_backend.h
vaGetNumCandidates
vaGetDriverNameByIndex

with single one:

$ cat va_backend.h
vaGetNumCandidates // DEPRECATED =NULL
vaGetDriverNameByIndex // DEPRECATED =NULL
vaGetDriverNames

I think this highlight needs to be added to PR description to save time to understand what's being done in the PR.

[evelikov]

Rebased, fixed the reserved bits and added one line summary about the API change.

@dvrogozh can you have another look?

[dvrogozh]

Change looks good to me. Basically I am trying to imagine the case where something might go wrong, but without a success so far. I think this change needs to be extensively tried before the merge. Recommend to pick it up for validation. @XinfengZhang - please, take a look.

[XinfengZhang]

the change itself looks good, but from my perspective, we do need test it.
also, we could merge it after 2.18 release immediately, to keep it in master about 3 month to be tried by different users.

[evelikov]

@XinfengZhang when you say "we need to test it" can you elaborate who is "we" and what testing do you expect to be done?

There has been zero comments here for over 3 months and 2.18 was released 2 month ago. This makes me wonder if as-is libva is even considered maintained?

[XinfengZhang]

20 should be enough now, but does we need a check?

[evelikov]

What check are you looking for - can you provide a concrete example?

[XinfengZhang]

no need check now, it is impossible that {i915, {iHD, i965, i966 .... iHD1, iHD2...} exceed 20 backend UMD drivers for one KMD.

[XinfengZhang]

@evelikov could you help to rebase it?

[evelikov]

@XinfengZhang do you mind replying to my earlier question #682 (comment), thanks.

[XinfengZhang]

@XinfengZhang do you mind replying to my earlier question #682 (comment), thanks.

sorry for late response firstly, just like my previous comments, the change itself looks good to me
#682 (comment)
but that time , we are closing 2.18 release. so I want to keep it there and let more people try it.
and merge it after 2.18 release. but there are no more comments after that,
I got the notification from your comments of last week. and revisit it and try it again.
I apologize for this , I understand it is bad practice to a respected contributor, will avoid such bad practice in future

about why we did not include it in 2.18.
the code make the code more elegant , unify 2 steps to 1. it is very nice code,
but it is not urgent feature at that time, and it include more 19 commit, looks a little risky at that time.
and the Q1 release validation cycle already been triggered.

I tested it with intel media stack.

[evelikov]

While I appreciate the heartfelt apology, I am a firm believer that actions speak louder than words. Here's to this being the last instance.

MR has been updated and lightly tested - basically a simple s/getenv/secure_getenv/ change was required.

[XinfengZhang]

hi @dvrogozh , could you help to dismiss your changes request if your concern was addressed already and no further concerns.
I will merge it , thanks

[dvrogozh]

Looks good.