<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Maarten, <br>
<br>
Yeah that would be much simpler I think.<br>
<br>
Damien also looked at this patch over my shoulder and had
additional comments.<br>
So more or less proxying :<br>
<br>
- Not sure why we're exposing the new enums and the
igt_atomic_popuplate_* macros in igt_kms.h.<br>
After all we're not using them anywhere outside igt_kms.c.<br>
<br>
- We have a couple of properties ids stored in igt_kms.h
(background_property & rotation_property), it would make sense
to get rid of those and use the new created arrays instead.<br>
<br>
Cheers,<br>
<br>
-<br>
Lionel
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<br>
<br>
On 08/03/16 15:56, Maarten Lankhorst wrote:<br>
</div>
<blockquote cite="mid:56DEF62E.1080401@linux.intel.com" type="cite">
<pre wrap="">Op 07-03-16 om 13:50 schreef Lionel Landwerlin:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Pratik,
I'm really looking forward to get this merged.
Just a few comments on the plane commit part below.
</pre>
</blockquote>
<pre wrap="">
Would the following diff to the patch satisfy you? It would clean up things a lot.
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 1f738e14f52f..454ff7552b4a 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1534,14 +1534,6 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_output_t *output,
igt_display_t *display = output->display;
uint32_t fb_id, crtc_id;
- 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);
@@ -1552,54 +1544,30 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_output_t *output,
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_FB_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);
+ 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);
+
+ if (plane->fb_changed) {
+ igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, fb_id ? crtc_id : 0);
igt_atomic_populate_plane_req(req, plane, IGT_PLANE_FB_ID, fb_id);
+ }
+
+ if (plane->position_changed || plane->size_changed) {
+ uint32_t src_x = IGT_FIXED(plane->fb->src_x, 0); /* src_x */
+ uint32_t src_y = IGT_FIXED(plane->fb->src_y, 0); /* src_y */
+ uint32_t src_w = IGT_FIXED(plane->fb->src_w, 0); /* src_w */
+ uint32_t src_h = IGT_FIXED(plane->fb->src_h, 0); /* src_h */
+ int32_t crtc_x = plane->crtc_x;
+ int32_t crtc_y = plane->crtc_y;
+ uint32_t crtc_w = plane->crtc_w;
+ uint32_t crtc_h = plane->crtc_h;
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);
@@ -1610,18 +1578,15 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_output_t *output,
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);
-
}
+ 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;</pre>
</blockquote>
<br>
I missed that, but I think we need rotation_changed = false; here
too.<br>
<br>
<blockquote cite="mid:56DEF62E.1080401@linux.intel.com" type="cite">
<pre wrap="">
-
- return;
}
</pre>
</blockquote>
<br>
</body>
</html>