[Intel-gfx] [PATCH] lib/igt_kms: Add COMIT_ATOMIC to igt_display_commit2()
Mayuresh Gharpure
mayuresh.s.gharpure at intel.com
Sun Jan 31 21:52:40 PST 2016
Hi Marius,
I've incorporated your review comments in :
https://patchwork.freedesktop.org/patch/72179/
One of the comments regarding CHECK_RETURN is already taken care in
do_display_commit method, after returning from igt_atomic_commit
Regards,
Mayuresh
On 1/29/2016 6:15 PM, Marius Vlad wrote:
> Hi,
> I still do not see FB_ID set to 0 when disabling the plane in
> igt_atomic_prepare_plane_commit().
>
> See http://lists.freedesktop.org/archives/intel-gfx/2016-January/085790.html
>
>
> On Fri, Jan 29, 2016 at 02:17:11PM +0530, Mayuresh Gharpure wrote:
>> Co-Author : Marius Vlad <marius.c.vlad at intel.com>
>>
>> So far we have had only two commit styles, COMMIT_LEGACY
>> and COMMIT_UNIVERSAL. This patch adds another commit style
>> COMMIT_ATOMIC which makes use of drmModeAtomicCommit()
>>
>> Signed-off-by: Mayuresh Gharpure <mayuresh.s.gharpure at intel.com>
>> ---
>> lib/igt_kms.c | 317 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> lib/igt_kms.h | 71 ++++++++++++-
>> 2 files changed, 386 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 90c8da7..73883d3 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -145,6 +145,120 @@ const unsigned char* igt_kms_get_base_edid(void)
>> *
>> * Returns: an alternate edid block
>> */
>> +static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>> + "SRC_X",
>> + "SRC_Y",
>> + "SRC_W",
>> + "SRC_H",
>> + "CRTC_X",
>> + "CRTC_Y",
>> + "CRTC_W",
>> + "CRTC_H",
>> + "FB_ID",
>> + "CRTC_ID",
>> + "type",
>> + "rotation"
>> +};
>> +
>> +static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>> + "background_color"
>> +};
>> +
>> +static const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>> + "scaling mode",
>> + "DPMS"
>> +};
>> +
>> +/*
>> + * Retrieve all the properies specified in props_name and store them into
>> + * plane->atomic_props_plane.
>> + */
>> +static void
>> +igt_atomic_fill_plane_props(igt_display_t *display, igt_plane_t *plane,
>> + int num_props, const char **prop_names)
>> +{
>> + drmModeObjectPropertiesPtr props;
>> + int i, j, fd;
>> +
>> + fd = display->drm_fd;
>> +
>> + props = drmModeObjectGetProperties(fd, plane->drm_plane->plane_id, DRM_MODE_OBJECT_PLANE);
>> + igt_assert(props);
>> +
>> + for (i = 0; i < props->count_props; i++) {
>> + drmModePropertyPtr prop =
>> + drmModeGetProperty(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];
>> + break;
>> + }
>> +
>> + drmModeFreeProperty(prop);
>> + }
>> +
>> + drmModeFreeObjectProperties(props);
>> +
>> +}
>> +
>> const unsigned char* igt_kms_get_alt_edid(void)
>> {
>> update_edid_csum(alt_edid);
>> @@ -952,6 +1066,8 @@ 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);
>> }
>>
>> static bool
>> @@ -1020,6 +1136,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>> {
>> drmModeRes *resources;
>> drmModePlaneRes *plane_resources;
>> + drmModeObjectPropertiesPtr props;
>> int i;
>>
>> memset(display, 0, sizeof(igt_display_t));
>> @@ -1094,6 +1211,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>>
>> plane->pipe = pipe;
>> plane->drm_plane = drm_plane;
>> + igt_atomic_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
>>
>> get_plane_property(display->drm_fd, drm_plane->plane_id,
>> "rotation",
>> @@ -1349,6 +1467,111 @@ static uint32_t igt_plane_get_fb_gem_handle(igt_plane_t *plane)
>> igt_assert(r == 0); \
>> }
>>
>> +
>> +
>> +
>> +/*
>> + * Add position and fb changes of a plane to the atomic property set
>> + */
>> +static int
>> +igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_output_t *output,
>> + drmModeAtomicReq *req)
>> +{
>> +
>> + igt_display_t *display = output->display;
>> + uint32_t fb_id, crtc_id;
>> + int ret;
>> + uint32_t src_x;
>> + uint32_t src_y;
>> + uint32_t src_w;
>> + uint32_t src_h;
>> + int32_t crtc_x;
>> + int32_t crtc_y;
>> + uint32_t crtc_w;
>> + uint32_t crtc_h;
>> +
>> + igt_assert(plane->drm_plane);
>> +
>> + /* it's an error to try an unsupported feature */
>> + igt_assert(igt_plane_supports_rotation(plane) ||
>> + !plane->rotation_changed);
>> +
>> + fb_id = igt_plane_get_fb_id(plane);
>> + crtc_id = output->config.crtc->crtc_id;
>> +
>> + if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {
>> +
>> + LOG(display,
>> + "%s: populating plane data: pipe %s, plane %d, disabling\n",
>> + igt_output_name(output),
>> + kmstest_pipe_name(output->config.pipe),
>> + plane->index);
>> +
>> + /* populate plane req */
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, 0);
>> +
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_X, IGT_FIXED(0, 0));
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_Y, IGT_FIXED(0, 0));
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_W, IGT_FIXED(0, 0));
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_H, IGT_FIXED(0, 0));
>> +
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_X, 0);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_Y, 0);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_W, 0);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_H, 0);
>> +
>> + } else if (plane->fb_changed || plane->position_changed ||
>> + plane->size_changed) {
>> +
>> + src_x = IGT_FIXED(plane->fb->src_x, 0); /* src_x */
>> + src_y = IGT_FIXED(plane->fb->src_y, 0); /* src_y */
>> + src_w = IGT_FIXED(plane->fb->src_w, 0); /* src_w */
>> + src_h = IGT_FIXED(plane->fb->src_h, 0); /* src_h */
>> + crtc_x = plane->crtc_x;
>> + crtc_y = plane->crtc_y;
>> + crtc_w = plane->crtc_w;
>> + crtc_h = plane->crtc_h;
>> +
>> + LOG(display,
>> + "%s: populating plane data: %s.%d, fb %u, src = (%d, %d) "
>> + "%ux%u dst = (%u, %u) %ux%u\n",
>> + igt_output_name(output),
>> + kmstest_pipe_name(output->config.pipe),
>> + plane->index,
>> + fb_id,
>> + src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
>> + crtc_x, crtc_y, crtc_w, crtc_h);
>> +
>> +
>> + /* populate plane req */
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, crtc_id);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_FB_ID, fb_id);
>> +
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_X, src_x);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_Y, src_y);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_W, src_w);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_H, src_h);
>> +
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_X, crtc_x);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_Y, crtc_y);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_W, crtc_w);
>> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_H, crtc_h);
>> +
>> + if (plane->rotation_changed)
>> + igt_atomic_populate_plane_req(req, plane,
>> + IGT_PLANE_ROTATION, plane->rotation);
>> +
>> + }
>> +
>> + plane->fb_changed = false;
>> + plane->position_changed = false;
>> + plane->size_changed = false;
>> +
>> + return 0;
>> +}
>> +
>> +
>> +
>> /*
>> * Commit position and fb changes to a DRM plane via the SetPlane ioctl; if the
>> * DRM call to program the plane fails, we'll either fail immediately (for
>> @@ -1601,7 +1824,7 @@ static int igt_plane_commit(igt_plane_t *plane,
>> return igt_primary_plane_commit_legacy(plane, output,
>> fail_on_error);
>> } else {
>> - return igt_drm_plane_commit(plane, output, fail_on_error);
>> + return igt_drm_plane_commit(plane, output, fail_on_error);
>> }
>> }
>>
>> @@ -1655,6 +1878,90 @@ static int igt_output_commit(igt_output_t *output,
>> return 0;
>> }
>>
>> +
>> +/*
>> + * Add crtc property changes to the atomic property set
>> + */
>> +static void igt_atomic_prepare_crtc_commit(igt_output_t *output, drmModeAtomicReq *req)
>> +{
>> +
>> + 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);
>> +
>> + /*
>> + * TODO: Add all crtc level properties here
>> + */
>> +
>> +}
>> +
>> +/*
>> + * Add connector property changes to the atomic property set
>> + */
>> +static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>> +{
>> +
>> + struct kmstest_connector_config *config = &output->config;
>> +
>> + if (config->connector_scaling_mode_changed)
>> + igt_atomic_populate_connector_req(req, output, IGT_CONNECTOR_SCALING_MODE, config->connector_scaling_mode);
>> +
>> + if (config->connector_dpms_changed)
>> + igt_atomic_populate_connector_req(req, output, IGT_CONNECTOR_DPMS, config->connector_dpms);
>> + /*
>> + * TODO: Add all other connector level properties here
>> + */
>> +
>> +}
>> +
>> +/*
>> + * Commit all the changes of all the planes,crtcs, connectors
>> + * atomically using drmModeAtomicCommit()
>> + */
>> +static int igt_atomic_commit(igt_display_t *display)
>> +{
>> +
>> + int ret = 0;
>> + drmModeAtomicReq *req;
>> + do_or_die(drmSetClientCap(display->drm_fd, DRM_CLIENT_CAP_ATOMIC, 1));
>> + int i = 0;
>> +
>> + req = drmModeAtomicAlloc();
>> + drmModeAtomicSetCursor(req, 0);
>> +
>> + for (i = 0; i < display->n_outputs; i++) {
>> + igt_output_t *output = &display->outputs[i];
>> + igt_pipe_t *pipe;
>> +
>> + if (!output->valid)
>> + continue;
>> +
>> + /*
>> + * Add CRTC Properties to the property set
>> + */
>> + igt_atomic_prepare_crtc_commit(output, req);
>> +
>> + /*
>> + * Add Connector Properties to the property set
>> + */
>> + igt_atomic_prepare_connector_commit(output, req);
>> +
>> +
>> + pipe = igt_output_get_driving_pipe(output);
>> +
>> + for (i = 0; i < pipe->n_planes; i++) {
>> + igt_plane_t *plane = &pipe->planes[i];
>> + igt_atomic_prepare_plane_commit(plane, output, req);
>> + }
>> +
>> + }
>> +
>> + ret = drmModeAtomicCommit(display->drm_fd, req, 0, NULL);
>> + drmModeAtomicFree(req);
>> + return ret;
>> +
>> +}
>> /*
>> * Commit all plane changes across all outputs of the display.
>> *
>> @@ -1674,6 +1981,14 @@ static int do_display_commit(igt_display_t *display,
>>
>> igt_display_refresh(display);
>>
>> + if (s == COMMIT_ATOMIC) {
>> +
>> + ret = igt_atomic_commit(display);
>> + CHECK_RETURN(ret, fail_on_error);
>> + return 0;
>> +
>> + }
>> +
>> for (i = 0; i < display->n_outputs; i++) {
>> igt_output_t *output = &display->outputs[i];
>>
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index 3f7add5..768a5e3 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -106,11 +106,28 @@ int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id);
>> void kmstest_set_vt_graphics_mode(void);
>> void kmstest_restore_vt_mode(void);
>>
>> +enum igt_atomic_crtc_properties {
>> + IGT_CRTC_BACKGROUND = 0,
>> + IGT_NUM_CRTC_PROPS
>> +};
>> +
>> +enum igt_atomic_connector_properties {
>> + IGT_CONNECTOR_SCALING_MODE = 0,
>> + IGT_CONNECTOR_DPMS,
>> + IGT_NUM_CONNECTOR_PROPS
>> +};
>> +
>> struct kmstest_connector_config {
>> drmModeCrtc *crtc;
>> drmModeConnector *connector;
>> drmModeEncoder *encoder;
>> drmModeModeInfo default_mode;
>> + uint64_t connector_scaling_mode;
>> + 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;
>> };
>> @@ -159,9 +176,28 @@ void kmstest_unset_all_crtcs(int drm_fd, drmModeResPtr resources);
>> enum igt_commit_style {
>> COMMIT_LEGACY = 0,
>> COMMIT_UNIVERSAL,
>> - /* We'll add atomic here eventually. */
>> + COMMIT_ATOMIC,
>> };
>>
>> +enum igt_atomic_plane_properties {
>> + IGT_PLANE_SRC_X = 0,
>> + IGT_PLANE_SRC_Y,
>> + IGT_PLANE_SRC_W,
>> + IGT_PLANE_SRC_H,
>> +
>> + IGT_PLANE_CRTC_X,
>> + IGT_PLANE_CRTC_Y,
>> + IGT_PLANE_CRTC_W,
>> + IGT_PLANE_CRTC_H,
>> +
>> + IGT_PLANE_FB_ID,
>> + IGT_PLANE_CRTC_ID,
>> + IGT_PLANE_TYPE,
>> + IGT_PLANE_ROTATION,
>> + IGT_NUM_PLANE_PROPS
>> +};
>> +
>> +
>> typedef struct igt_display igt_display_t;
>> typedef struct igt_pipe igt_pipe_t;
>> typedef uint32_t igt_fixed_t; /* 16.16 fixed point */
>> @@ -203,6 +239,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];
>> } igt_plane_t;
>>
>> struct igt_pipe {
>> @@ -284,6 +321,38 @@ void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
>>
>> #define IGT_FIXED(i,f) ((i) << 16 | (f))
>>
>> +/**
>> + * igt_atomic_populate_plane_req:
>> + * @req: A pointer to drmModeAtomicReq
>> + * @plane: A pointer igt_plane_t
>> + * @prop: one of igt_atomic_plane_properties
>> + * @value: the value to add
>> + */
>> +#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))
>> +
>> +/**
>> + * igt_atomic_populate_crtc_req:
>> + * @req: A pointer to drmModeAtomicReq
>> + * @output: A pointer igt_output_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))
>> +/**
>> + * igt_atomic_populate_connector_req:
>> + * @req: A pointer to drmModeAtomicReq
>> + * @output: A pointer igt_output_t
>> + * @prop: one of igt_atomic_connector_properties
>> + * @value: the value to add
>> + */
>> +#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))
>> +
>> void igt_enable_connectors(void);
>> void igt_reset_connectors(void);
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list