[Intel-gfx] [PATCH i-g-t 2/7] lib/igt_kms: refactor atomic properties load/commit
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Wed Mar 23 13:57:25 UTC 2016
On 23/03/16 12:13, Marius Vlad wrote:
> On Wed, Mar 23, 2016 at 11:38:05AM +0000, Lionel Landwerlin wrote:
>> All properties can be listed regardless of whether we're dealing with
>> an atomic enabled driver, so load all of the properties ids regardless
>> at init time.
>>
>> Also, we can have only one function loading the property ids and reuse
>> that for different DRM objects.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> ---
>> lib/igt_kms.c | 185 +++++++++++++++++++++-------------------------------------
>> lib/igt_kms.h | 21 +++----
>> 2 files changed, 76 insertions(+), 130 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 5ef89cf..3321738 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -173,85 +173,31 @@ static const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>> };
>>
>> /*
>> - * Retrieve all the properies specified in props_name and store them into
>> - * plane->atomic_props_plane.
>> + * Retrieve all the properies specified in props_name from object_id
> typo.
Thanks.
>> + * and store them into prop_ids.
>> */
>> static void
>> -igt_atomic_fill_plane_props(igt_display_t *display, igt_plane_t *plane,
>> - int num_props, const char **prop_names)
>> +igt_atomic_fill_props(igt_display_t *display,
>> + uint32_t object_id, uint32_t object_type,
>> + uint32_t *prop_ids, const char **prop_names,
>> + int num_props)
>> {
>> drmModeObjectPropertiesPtr props;
>> - int i, j, fd;
>> -
>> - fd = display->drm_fd;
>> + int i, j;
>>
>> - props = drmModeObjectGetProperties(fd, plane->drm_plane->plane_id, DRM_MODE_OBJECT_PLANE);
>> + props = drmModeObjectGetProperties(display->drm_fd,
>> + object_id, object_type);
>> igt_assert(props);
>>
>> for (i = 0; i < props->count_props; i++) {
>> drmModePropertyPtr prop =
>> - drmModeGetProperty(fd, props->props[i]);
>> + drmModeGetProperty(display->drm_fd, props->props[i]);
>>
>> for (j = 0; j < num_props; j++) {
>> if (strcmp(prop->name, prop_names[j]) != 0)
>> continue;
>>
>> - plane->atomic_props_plane[j] = props->props[i];
>> - break;
>> - }
>> -
>> - drmModeFreeProperty(prop);
>> - }
>> -
>> - drmModeFreeObjectProperties(props);
>> -}
>> -
>> -/*
>> - * Retrieve all the properies specified in props_name and store them into
>> - * config->atomic_props_crtc and config->atomic_props_connector.
>> - */
>> -static void
>> -igt_atomic_fill_props(igt_display_t *display, igt_output_t *output,
>> - int num_crtc_props, const char **crtc_prop_names,
>> - int num_connector_props, const char **conn_prop_names)
>> -{
>> - drmModeObjectPropertiesPtr props;
>> - int i, j, fd;
>> -
>> - fd = display->drm_fd;
>> -
>> - props = drmModeObjectGetProperties(fd, output->config.crtc->crtc_id, DRM_MODE_OBJECT_CRTC);
>> - igt_assert(props);
>> -
>> - for (i = 0; i < props->count_props; i++) {
>> - drmModePropertyPtr prop =
>> - drmModeGetProperty(fd, props->props[i]);
>> -
>> - for (j = 0; j < num_crtc_props; j++) {
>> - if (strcmp(prop->name, crtc_prop_names[j]) != 0)
>> - continue;
>> -
>> - output->config.atomic_props_crtc[j] = props->props[i];
>> - break;
>> - }
>> -
>> - drmModeFreeProperty(prop);
>> - }
>> -
>> - drmModeFreeObjectProperties(props);
>> - props = NULL;
>> - props = drmModeObjectGetProperties(fd, output->config.connector->connector_id, DRM_MODE_OBJECT_CONNECTOR);
>> - igt_assert(props);
>> -
>> - for (i = 0; i < props->count_props; i++) {
>> - drmModePropertyPtr prop =
>> - drmModeGetProperty(fd, props->props[i]);
>> -
>> - for (j = 0; j < num_connector_props; j++) {
>> - if (strcmp(prop->name, conn_prop_names[j]) != 0)
>> - continue;
>> -
>> - output->config.atomic_props_connector[j] = props->props[i];
>> + prop_ids[j] = props->props[i];
>> break;
>> }
>>
>> @@ -259,7 +205,6 @@ igt_atomic_fill_props(igt_display_t *display, igt_output_t *output,
>> }
>>
>> drmModeFreeObjectProperties(props);
>> -
>> }
>>
>> const unsigned char* igt_kms_get_alt_edid(void)
>> @@ -1118,8 +1063,9 @@ static void igt_output_refresh(igt_output_t *output)
>> kmstest_pipe_name(output->config.pipe));
>>
>> display->pipes_in_use |= 1 << output->config.pipe;
>> - igt_atomic_fill_props(display, output, IGT_NUM_CRTC_PROPS, igt_crtc_prop_names,
>> - IGT_NUM_CONNECTOR_PROPS, igt_connector_prop_names);
>> + igt_atomic_fill_props(display, output->id, DRM_MODE_OBJECT_CONNECTOR,
>> + output->properties, igt_connector_prop_names,
>> + IGT_NUM_CONNECTOR_PROPS);
> Does it make sense to call it if !display->atomic?
I can't think of a reason not to full the properties array all the time.
It makes the initialization simpler and we use these rather than having
rotation_property/background_property/degamma_lut_property/etc...
>
>> }
>>
>> static bool
>> @@ -1189,7 +1135,6 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>> drmModeRes *resources;
>> drmModePlaneRes *plane_resources;
>> int i;
>> - int is_atomic = 0;
>>
>> memset(display, 0, sizeof(igt_display_t));
>>
>> @@ -1207,7 +1152,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>> display->n_pipes = resources->count_crtcs;
>>
>> drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
>> - is_atomic = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1);
>> + display->is_atomic = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0;
>> plane_resources = drmModeGetPlaneResources(display->drm_fd);
>> igt_assert(plane_resources);
>>
>> @@ -1265,10 +1210,12 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>>
>> plane->pipe = pipe;
>> plane->drm_plane = drm_plane;
>> - if (is_atomic == 0) {
>> - display->is_atomic = 1;
>> - igt_atomic_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
>> - }
>> + igt_atomic_fill_props(display,
>> + plane->drm_plane->plane_id,
>> + DRM_MODE_OBJECT_PLANE,
>> + plane->properties,
>> + igt_plane_prop_names,
>> + IGT_NUM_PLANE_PROPS);
>>
>> get_plane_property(display->drm_fd, drm_plane->plane_id,
>> "rotation",
>> @@ -1316,6 +1263,36 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>> /* planes = 1 primary, (p-1) sprites, 1 cursor */
>> pipe->n_planes = p + 1;
>>
>> + igt_atomic_fill_props(display, pipe->crtc_id, DRM_MODE_OBJECT_CRTC,
>> + pipe->properties, igt_crtc_prop_names,
>> + IGT_NUM_CRTC_PROPS);
>> + if (pipe->crtc_id) {
>> + uint64_t prop_value;
>> +
>> + get_crtc_property(display->drm_fd, pipe->crtc_id,
>> + "background_color",
>> + &pipe->background_property,
>> + &prop_value,
>> + NULL);
>> + pipe->background = (uint32_t)prop_value;
>> + get_crtc_property(display->drm_fd, pipe->crtc_id,
>> + "DEGAMMA_LUT",
>> + &pipe->degamma_property,
>> + NULL,
>> + NULL);
>> + get_crtc_property(display->drm_fd, pipe->crtc_id,
>> + "CTM",
>> + &pipe->ctm_property,
>> + NULL,
>> + NULL);
>> + get_crtc_property(display->drm_fd, pipe->crtc_id,
>> + "GAMMA_LUT",
>> + &pipe->gamma_property,
>> + NULL,
>> + NULL);
>> + }
>> +
>> +
>> /* make sure we don't overflow the plane array */
>> igt_assert(pipe->n_planes <= IGT_MAX_PLANES);
>> }
>> @@ -1330,7 +1307,6 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>> igt_assert(display->outputs);
>>
>> for (i = 0; i < display->n_outputs; i++) {
>> - int j;
>> igt_output_t *output = &display->outputs[i];
>>
>> /*
>> @@ -1342,35 +1318,6 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>> output->display = display;
>>
>> igt_output_refresh(output);
>> -
>> - for (j = 0; j < display->n_pipes; j++) {
>> - uint64_t prop_value;
>> - igt_pipe_t *pipe = &display->pipes[j];
>> -
>> - if (output->config.crtc) {
>> - get_crtc_property(display->drm_fd, output->config.crtc->crtc_id,
>> - "background_color",
>> - &pipe->background_property,
>> - &prop_value,
>> - NULL);
>> - pipe->background = (uint32_t)prop_value;
>> - get_crtc_property(display->drm_fd, output->config.crtc->crtc_id,
>> - "DEGAMMA_LUT",
>> - &pipe->degamma_property,
>> - NULL,
>> - NULL);
>> - get_crtc_property(display->drm_fd, output->config.crtc->crtc_id,
>> - "CTM",
>> - &pipe->ctm_property,
>> - NULL,
>> - NULL);
>> - get_crtc_property(display->drm_fd, output->config.crtc->crtc_id,
>> - "GAMMA_LUT",
>> - &pipe->gamma_property,
>> - NULL,
>> - NULL);
>> - }
>> - }
>> }
>>
>> drmModeFreePlaneResources(plane_resources);
>> @@ -1945,22 +1892,19 @@ static int igt_output_commit(igt_output_t *output,
>> /*
>> * Add crtc property changes to the atomic property set
>> */
>> -static void igt_atomic_prepare_crtc_commit(igt_output_t *output, drmModeAtomicReq *req)
>> +static void igt_atomic_prepare_pipe_commit(igt_pipe_t *pipe, drmModeAtomicReq *req)
>> {
>> + if (pipe->background_changed)
>> + igt_atomic_populate_pipe_req(req, pipe, IGT_CRTC_BACKGROUND, pipe->background);
>>
>> - igt_pipe_t *pipe_obj = igt_output_get_driving_pipe(output);
>> -
>> - if (pipe_obj->background_changed)
>> - igt_atomic_populate_crtc_req(req, output, IGT_CRTC_BACKGROUND, pipe_obj->background);
>> -
>> - if (pipe_obj->color_mgmt_changed) {
>> - igt_atomic_populate_crtc_req(req, output, IGT_CRTC_DEGAMMA_LUT, pipe_obj->degamma_blob);
>> - igt_atomic_populate_crtc_req(req, output, IGT_CRTC_CTM, pipe_obj->ctm_blob);
>> - igt_atomic_populate_crtc_req(req, output, IGT_CRTC_GAMMA_LUT, pipe_obj->gamma_blob);
>> + if (pipe->color_mgmt_changed) {
>> + igt_atomic_populate_pipe_req(req, pipe, IGT_CRTC_DEGAMMA_LUT, pipe->degamma_blob);
>> + igt_atomic_populate_pipe_req(req, pipe, IGT_CRTC_CTM, pipe->ctm_blob);
>> + igt_atomic_populate_pipe_req(req, pipe, IGT_CRTC_GAMMA_LUT, pipe->gamma_blob);
>> }
>>
>> - pipe_obj->background_changed = false;
>> - pipe_obj->color_mgmt_changed = false;
>> + pipe->background_changed = false;
>> + pipe->color_mgmt_changed = false;
>> }
>>
>> /*
>> @@ -2001,10 +1945,14 @@ static int igt_atomic_commit(igt_display_t *display)
>> igt_plane_t *plane;
>> enum pipe pipe;
>>
>> + pipe_obj = igt_output_get_driving_pipe(output);
>> +
>> + pipe = pipe_obj->pipe;
>> +
>> /*
>> * Add CRTC Properties to the property set
>> */
>> - igt_atomic_prepare_crtc_commit(output, req);
>> + igt_atomic_prepare_pipe_commit(pipe_obj, req);
>>
>> /*
>> * Add Connector Properties to the property set
>> @@ -2012,9 +1960,6 @@ static int igt_atomic_commit(igt_display_t *display)
>> igt_atomic_prepare_connector_commit(output, req);
>>
>>
>> - pipe_obj = igt_output_get_driving_pipe(output);
>> -
>> - pipe = pipe_obj->pipe;
>>
>> for_each_plane_on_pipe(display, pipe, plane) {
>> igt_atomic_prepare_plane_commit(plane, output, req);
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index 8ae1192..9f04f72 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -129,8 +129,6 @@ struct kmstest_connector_config {
>> bool connector_scaling_mode_changed;
>> uint64_t connector_dpms;
>> bool connector_dpms_changed;
>> - uint32_t atomic_props_crtc[IGT_NUM_CRTC_PROPS];
>> - uint32_t atomic_props_connector[IGT_NUM_CONNECTOR_PROPS];
>> int crtc_idx;
>> int pipe;
>> };
>> @@ -246,7 +244,7 @@ typedef struct {
>> /* panning offset within the fb */
>> unsigned int pan_x, pan_y;
>> igt_rotation_t rotation;
>> - uint32_t atomic_props_plane[IGT_NUM_PLANE_PROPS];
>> + uint32_t properties[IGT_NUM_PLANE_PROPS];
>> } igt_plane_t;
>>
>> struct igt_pipe {
>> @@ -269,6 +267,8 @@ struct igt_pipe {
>> uint32_t color_mgmt_changed : 1;
>>
>> uint32_t crtc_id;
>> +
>> + uint32_t properties[IGT_NUM_CRTC_PROPS];
>> };
> So the macro remain as CRTCs?
Ah sorry, I should leave igt_atomic_populate_crtc_req untouched in this
commit and move rename in the other one.
Following Daniel's comment, it might not need to be changed at all.
>
>>
>> typedef struct {
>> @@ -281,6 +281,7 @@ typedef struct {
>> unsigned long pending_crtc_idx_mask;
>> bool use_override_mode;
>> drmModeModeInfo override_mode;
>> + uint32_t properties[IGT_NUM_CONNECTOR_PROPS];
>> } igt_output_t;
>>
>> struct igt_display {
>> @@ -355,18 +356,18 @@ void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
>> */
>> #define igt_atomic_populate_plane_req(req, plane, prop, value) \
>> igt_assert_lt(0, drmModeAtomicAddProperty(req, plane->drm_plane->plane_id,\
>> - plane->atomic_props_plane[prop], value))
>> + plane->properties[prop], value))
>>
>> /**
>> - * igt_atomic_populate_crtc_req:
>> + * igt_atomic_populate_pipe_req:
>> * @req: A pointer to drmModeAtomicReq
>> - * @output: A pointer igt_output_t
>> + * @pipe: A pointer igt_pipe_t
>> * @prop: one of igt_atomic_crtc_properties
>> * @value: the value to add
>> */
>> -#define igt_atomic_populate_crtc_req(req, output, prop, value) \
>> - igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.crtc->crtc_id,\
>> - output->config.atomic_props_crtc[prop], value))
>> +#define igt_atomic_populate_pipe_req(req, pipe, prop, value) \
>> + igt_assert_lt(0, drmModeAtomicAddProperty(req, pipe->crtc_id,\
>> + pipe->properties[prop], value))
>> /**
>> * igt_atomic_populate_connector_req:
>> * @req: A pointer to drmModeAtomicReq
>> @@ -376,7 +377,7 @@ void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
>> */
>> #define igt_atomic_populate_connector_req(req, output, prop, value) \
>> igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,\
>> - output->config.atomic_props_connector[prop], value))
>> + output->properties[prop], value))
>>
>> void igt_enable_connectors(void);
>> void igt_reset_connectors(void);
>> --
>> 2.8.0.rc3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list