Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add multiple backend driver support #351

Merged
merged 3 commits into from Dec 25, 2019
Merged

Conversation

@XinfengZhang
Copy link
Contributor

XinfengZhang commented Dec 10, 2019

enable multiple driver support, if there are no vaSetDriverName is called or environment variable LIBVA_DRIVER_NAME is set. driver will select driver from g_driver_name_map, if there are i965 and iHD.
libva will try to load iHD firstly, if it failed. then it will load i965.

@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch 2 times, most recently from b1a1590 to 7af8faa Dec 10, 2019
@XinfengZhang XinfengZhang requested review from xhaihao and dvrogozh Dec 10, 2019
@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch from 7af8faa to 7758968 Dec 12, 2019
va/drm/va_drm_utils.c Outdated Show resolved Hide resolved
va/va_backend.h Show resolved Hide resolved
va/drm/va_drm_utils.c Outdated Show resolved Hide resolved
va/va.c Outdated Show resolved Hide resolved
va/va.c Outdated Show resolved Hide resolved
@xhaihao

This comment has been minimized.

Copy link
Contributor

xhaihao commented Dec 17, 2019

Could you add something in the commit log?

@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch 2 times, most recently from 5def9c1 to 83df99b Dec 17, 2019
@XinfengZhang

This comment has been minimized.

Copy link
Contributor Author

XinfengZhang commented Dec 17, 2019

I already add the description in commit log, will add more in the PR description .

@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch from 83df99b to 1ce2041 Dec 19, 2019
va/drm/va_drm_utils.c Show resolved Hide resolved
@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch from 1ce2041 to 82839c2 Dec 19, 2019
Copy link
Collaborator

dvrogozh left a comment

I don't understand from the code how this is supposed to work. Can you, please, add some description of the algorithm, preferably as a comment in the code, but at least in the commit message. Are there any expectations from the drivers to have some kind of devices id white/black lists?

@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch 3 times, most recently from a0d09e9 to dbe3079 Dec 20, 2019
Copy link
Contributor

xhaihao left a comment

LGTM

@XinfengZhang

This comment has been minimized.

Copy link
Contributor Author

XinfengZhang commented Dec 20, 2019

@dvrogozh I will add some comments in the code, but this change does not touch DID, it just check whether vaIntialize sucess, if no, it will check next candidate in the table g_driver_name_map

@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch from dbe3079 to d4e0631 Dec 20, 2019
Copy link
Collaborator

onabiull left a comment

LGTM

va/va.c Outdated
vaStatus = va_getDriverName(dpy, &driver_name);

/*get backend driver candidate number, by default the value should be 1*/
vaStatus = va_getDriverCandidateNum(dpy, &candidate_num);

This comment has been minimized.

Copy link
@dvrogozh

dvrogozh Dec 24, 2019

Collaborator

To me this function name and surrounding comments are misleading. You use 'candidate' while actually you get number of candiates (plural). Can we express this in the function name and comments? Like: va_getNumDriverCandidates

@@ -51,11 +51,20 @@ va_DisplayContextDestroy(VADisplayContextP pDisplayContext)
free(pDisplayContext->pDriverContext);
free(pDisplayContext);
}
static VAStatus va_DisplayContextGetCandidateNum(

This comment has been minimized.

Copy link
@dvrogozh

dvrogozh Dec 24, 2019

Collaborator

candidates, not candidate. And I would probably reorder: va_DisplayContextGetNumCandidates. Here and below for this and other cases like that...

va/va.c Outdated
candidate_num = 1;
}
/*get driver name , get first driver candidate name, then try to load it*/
vaStatus = va_getDriverName(dpy, &driver_name, candidate_index);

This comment has been minimized.

Copy link
@dvrogozh

dvrogozh Dec 24, 2019

Collaborator

This code blows my mind... That's not about your changes, but about code which existed before... For example, can ctx->override_driver_name be changed in va_getDriverName() function? and if not - WHY we are calling va_getDriverName before it and not after?

Why we don't start process with analysis whether user specified LIBVA_DRIVER_NAME? I.e. from:
if (driver_name_env && (geteuid() == getuid()))

I kind of feel that this code can be rewritten to be much easier...

This comment has been minimized.

Copy link
@XinfengZhang

XinfengZhang Dec 24, 2019

Author Contributor

@dvrogozh , I have to revert to previous change, because i965 driver need drm is authenticated, so va_drm_authenticate should be called anyway, and drm_state->auth_type = VA_DRM_AUTH_CUSTOM; should be set, if we refactor this part, it will not call this part with some conditions

This comment has been minimized.

Copy link
@XinfengZhang

XinfengZhang Dec 24, 2019

Author Contributor

another solution is mv authenticate operation into vaGetNumCandidates function . I just update code.

va/va.c Outdated
@@ -659,6 +678,8 @@ VAStatus vaInitialize (
{
const char *driver_name_env = NULL;
char *driver_name = NULL;
int candidate_num = 1;

This comment has been minimized.

Copy link
@dvrogozh

dvrogozh Dec 24, 2019

Collaborator

num_candidates, plural

@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch from d4e0631 to 04ccfb6 Dec 24, 2019
@@ -48,14 +48,39 @@ static const struct driver_name_map g_driver_name_map[] = {
{ NULL, 0, NULL }
};

/* Returns the VA driver candidate num for the active display*/
VAStatus
VA_DRM_GetCandidatesNum(VADriverContextP ctx, int * candidate_num)

This comment has been minimized.

Copy link
@dvrogozh

dvrogozh Dec 24, 2019

Collaborator

s/candidate_num/num_candidates/


DLL_HIDDEN
VAStatus
VA_DRM_GetCandidatesNum(VADriverContextP ctx, int * candidate_num);

This comment has been minimized.

Copy link
@dvrogozh

dvrogozh Dec 24, 2019

Collaborator

s/candidate_num/num_candidates/

@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch from 04ccfb6 to 12fd057 Dec 24, 2019
Copy link
Collaborator

dvrogozh left a comment

just minor comments. I like this approach better. Generally approved, but would be nice to address few minor comments.

va/va.c Outdated
static VAStatus va_getDriverCandidatesNum(VADisplay dpy, int *candidates_num)
{
VADisplayContextP pDisplayContext = (VADisplayContextP)dpy;
*candidates_num = 1;

This comment has been minimized.

Copy link
@dvrogozh

dvrogozh Dec 24, 2019

Collaborator

move few lines below, please. This intermix code with declarations...

{
struct drm_state * const drm_state = ctx->drm_state;
drmVersionPtr drm_version;
int num_of_candidate = 0;

This comment has been minimized.

Copy link
@dvrogozh

dvrogozh Dec 24, 2019

Collaborator

num_of_candidates

@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch 2 times, most recently from 66a6ede to 1680b79 Dec 24, 2019
@XinfengZhang

This comment has been minimized.

Copy link
Contributor Author

XinfengZhang commented Dec 24, 2019

@dvrogozh , I have to revert to previous change, because i965 driver need drm is authenticated, so va_drm_authenticate should be called anyway, and drm_state->auth_type = VA_DRM_AUTH_CUSTOM; should be set, if we refactor this part, it will not call this part with some conditions , so I still need the complex logic inside vaInitialize

@XinfengZhang XinfengZhang force-pushed the XinfengZhang:driver_sel branch from 1680b79 to 2cdef94 Dec 24, 2019
@XinfengZhang

This comment has been minimized.

Copy link
Contributor Author

XinfengZhang commented Dec 24, 2019

another solution is mv authenticate operation into vaGetNumCandidates function . I just update code.

add multiple backend driver support part 1
add new function pointer in va_backend.h

vaGetCandidatesNum is used to get driver candidates number
vaGetDriverNameByIndex is used to get driver name by candidate index

and basic implementation of va_getDriverCandidateNum and va_getDriverNameByIndex

Signed-off-by: Carl Zhang <carl.zhang@intel.com>
add multiple backend driver support part 2
try to open driver one by one
1. get candidates number
2. try to load driver one by one until one driver loaded successfully

Signed-off-by: Carl Zhang <carl.zhang@intel.com>
add multiple backend driver support part 3
enable vaGetDriverNameByIndex for DRM
add comments for the implementation
mv authenticate operation into vaGetNumCandidates function
from vaGetDriverName.

Signed-off-by: Carl Zhang <carl.zhang@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.