Skip to content

add device information query/set interface - #503

Closed
XinfengZhang wants to merge 1 commit into
intel:masterfrom
XinfengZhang:device_param
Closed

add device information query/set interface#503
XinfengZhang wants to merge 1 commit into
intel:masterfrom
XinfengZhang:device_param

Conversation

@XinfengZhang

Copy link
Copy Markdown
Contributor

this interface is used to detect device information
device information may include:
memory locations, memory regions, deviceIDs etc.
it is common attributes between vaContext.

query interface:
this function should be called after vaInitialize

set interface:
this function should be called after vaInitialize
and before any other VAAPI functions, such as vaCreateSurfaces, etc.

Signed-off-by: Carl Zhang carl.zhang@intel.com

Comment threadva/va.h
Comment threadva/va.h Outdated
/**
* Query all attributes for a display handle
* The caller must provide an "attrib_list" array, the num_attibs should be set.
* if the The actual number of attributes < *num_attibs, the actual number of

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix text: "if the The". Should be: "If the". Mind capital I.

Comment threadva/va.h Outdated
* Query all attributes for a display handle
* The caller must provide an "attrib_list" array, the num_attibs should be set.
* if the The actual number of attributes < *num_attibs, the actual number of
* attributes will be returned in *num_attibs. if the *num_attibs < actual number

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capital I in if

Comment threadva/va.h
* unsupported.
*/

VAStatus vaQueryDeviceAttributes(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be named Query or Get?

Comment threadva/va.h Outdated

/**
* Set Device configuration by this function, only when the device attribues is writable
* if APP want to use this function to set device configuration, this function must be

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if APP want - is that a new sentence? add . and capitalize I.

Comment threadva/va.h Outdated
/**
* Set Device configuration by this function, only when the device attribues is writable
* if APP want to use this function to set device configuration, this function must be
* called after vaInitialize and before any other VAAPI function. if this function is called

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/if/If/

this interface is used to detect device information
device information may include:
memory locations, memory regions, deviceIDs etc.
it is common attributes between vaContext.
query interface:
this function should be called after vaInitialize
set interface:
this function should be called after vaInitialize
and before any other VAAPI functions, such as vaCreateSurfaces, etc.
Signed-off-by: Carl Zhang <carl.zhang@intel.com>

@dvrogozhdvrogozh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to finish discussion in #505 before merging this one since similar terms like local_memory, regions and indices are discussed here as well.

@fhvwy : fyi, you might wish to look into this PR as well

Comment threadva/va.c

VAStatus vaSetDeviceAttributes(
VADisplay dpy,
VADeviceAttrib *attrib_list,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that we already have vaGetDisplayAttributes/vaSetDisplayAttributes to control the
attributes of display. I think we can extend that API, no need to introduce another "Device"
concept.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial concept for this API was slightly different, in particular consideration was to have attribs settable before vaInitialize. In this sense old API did not work for us... But then concept has changed we forgot to evaluate the new API vs. existing one.

@XinfengZhang : indeed, 2 sets of APIs start to look weird. Should we converge and just declare couple new display attributes?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether display attributes is suitable , because current display attributes seems just related with "Display" not adaptor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes, considering that adaptor/display is kind of merged thing in libva, and I would say even on Linux device exposure level (since we can use /dev/dri/cardN for display and encoding). So, having all attributes together seems ok to me - they just configure different aspects of the same entity.

Comment threadva/va.h
/** \brief current region index read - write
* vaQueryDeviceAttibutes , it will return pre-assigned memory region.
* vaSetDeviceAttributes, it is used indicate which memory region is selected*/
uint32_t current_memory_region : 4;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does these "regions" represent the tile of one GPU node?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@XinfengZhang@HeJunyan@dvrogozh