minor fixes for driver name rework #731
Conversation
| @@ -686,6 +686,7 @@ static VAStatus va_new_opendriver(VADisplay dpy) | |||
| if (vaStatus != VA_STATUS_SUCCESS) { | |||
| /* Print and error yet continue, as per the above ordering note */ | |||
| va_errorMessage(dpy, "vaGetDriverNames() failed with %s\n", vaErrorStr(vaStatus)); | |||
| num_drivers = 0; | |||
There was a problem hiding this comment.
Small nit: let's prefix the commit message with va: .
va/drm/va_drm_utils.c
Outdated
| while (va_drivers[count]) { | ||
| if (count < MAX_NAMES && count < *num_drivers) | ||
| while (count < MAX_NAMES && va_drivers[count]) { | ||
| if (count < *num_drivers) | ||
| drivers[count] = strdup(va_drivers[count]); |
There was a problem hiding this comment.
The original code was essentially mixing two conflicting premises:
- there will be a sentinel - aka the
while (va_drivers[count]) - there will not be a sentinel - aka
count < MAX_NAMES
It would be a lot easier to read if we use
for ( ; count < MAX_NAMES && count < *num_drivers; count++)
drivers[count] = strdup(va_drivers[count]);
Can we use ^^ instead? Plus, please prefix the commit message with drm:
There was a problem hiding this comment.
Can we use ^^ instead? Plus, please prefix the commit message with
drm:
yes, will change the commit message, but
for ( ; count < MAX_NAMES && count < *num_drivers; count++)
drivers[count] = strdup(va_drivers[count]);
is not enough , if map[i].va_drivers only have 1 element. the pointer map[i].va_drivers[1] is nullptr.
for example, for second map[1], map1.va_drivers[0] = "pvr" map1.va_drivers[1] = null
we always need check drivers[count] is not null.
There was a problem hiding this comment.
Note to self: don't write any code before having coffee.
You're right. The following should fix that right?
for ( ; count < MAX_NAMES && va_drivers[count] && count < *num_drivers; count++)
drivers[count] = strdup(va_drivers[count]);
There was a problem hiding this comment.
of course , but there are a little difference need you to be aware of
if the input parameter --- *num_drivers is 1, the drm_driver is i915.
previous implementation:
num_drivers will be 2 finally, even only first "iHD" will be inserted into drivers[]
after
'''
for ( ; count < MAX_NAMES && va_drivers[count] && count < *num_drivers; count++)
drivers[count] = strdup(va_drivers[count]);
'''
num_drivers will be 1
seems , previous implementation will return the real candidates number, but only first is valid
:)
There was a problem hiding this comment.
The API was aimed to take an array + its size as input. Then each implementation should populate the array, up-to the original length and return the actual length. If I'm understanding correctly it sounds like we had bug in the original.
That said, just had a cup of coffee so it would be better if I have another look + send a patch documenting the API later today.
Thanks
There was a problem hiding this comment.
updated. thanks
Signed-off-by: Carl Zhang <[email protected]>
map is a array, which could be accessed by index, even the index is out of range. Fixes intel#722 Signed-off-by: Carl Zhang <[email protected]>
XinfengZhang
force-pushed
the
fix_722
branch
2 times, most recently
from
831b653
to
b113c3a
Compare
|
Looks like other backends need a similar fix. Let me fix that up... /me writes a patch. Edit: opened #733 |
No description provided.