From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
To: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH i-g-t 2/7] lib/igt_kms: refactor atomic properties load/commit
Date: Wed, 23 Mar 2016 13:57:25 +0000 [thread overview]
Message-ID: <56F2A0C5.1050403@intel.com> (raw)
In-Reply-To: <20160323121350.GA31168@mcvlad-wk.rb.intel.com>
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@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-03-23 13:57 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-23 11:38 [PATCH i-g-t 0/7] lib/igt_kms cleanups Lionel Landwerlin
2016-03-23 11:38 ` [PATCH i-g-t 1/7] lib/igt_kms: fix clearing up connector/pipe state on atomic commit Lionel Landwerlin
2016-03-23 11:38 ` [PATCH i-g-t 2/7] lib/igt_kms: refactor atomic properties load/commit Lionel Landwerlin
2016-03-23 12:13 ` Marius Vlad
2016-03-23 13:57 ` Lionel Landwerlin [this message]
2016-03-23 11:38 ` [PATCH i-g-t 3/7] lib/igt_kms: don't store rotation property id twice Lionel Landwerlin
2016-03-23 11:38 ` [PATCH i-g-t 4/7] lib/igt_kms: don't store background " Lionel Landwerlin
2016-03-23 11:38 ` [PATCH i-g-t 5/7] lib/igt_kms: don't store color management properties ids twice Lionel Landwerlin
2016-03-23 11:38 ` [PATCH i-g-t 6/7] lib/igt_kms: rename igt_crtc_set_background Lionel Landwerlin
2016-03-23 12:27 ` Daniel Vetter
2016-03-23 14:39 ` Lionel Landwerlin
2016-03-23 11:38 ` [PATCH i-g-t 7/7] lib/igt_kms: add some missing documentation Lionel Landwerlin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56F2A0C5.1050403@intel.com \
--to=lionel.g.landwerlin@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.