<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi Lionel,<br>
    <br>
    I've incorporated your comments using the diff patch submitted by
    Maarten and submitted the changes<br>
    <br>
    <a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/series/4274/">https://patchwork.freedesktop.org/series/4274/</a> <br>
    <br>
    Also, please find my comments inline<br>
    <br>
    Regards,<br>
    Mayuresh<br>
    <br>
    <div class="moz-cite-prefix">On 3/8/2016 9:41 PM, Lionel Landwerlin
      wrote:<br>
    </div>
    <blockquote cite="mid:56DEF9A6.90808@gmail.com" type="cite">
      <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
      <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>
      </div>
    </blockquote>
    Macros are just to improve readability. In case some new feature
    needs to be added, we can just add an enum and use the macro to
    populate it in call to IOCTL.<br>
    <br>
    These were added in initial implementation by Marius. <br>
    <br>
    <blockquote cite="mid:56DEF9A6.90808@gmail.com" type="cite">
      <div class="moz-cite-prefix"> <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>
      </div>
    </blockquote>
    These are used by the COMMIT_LEGACY and COMMIT_UNIVERSAL style
    calls. So keeping it as is, as changing this may need a change in
    many existing tests<br>
    <blockquote cite="mid:56DEF9A6.90808@gmail.com" type="cite">
      <div class="moz-cite-prefix"> <br>
        Cheers,<br>
        <br>
        -<br>
        Lionel <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>
      <br>
      <fieldset class="mimeAttachmentHeader"></fieldset>
      <br>
      <pre wrap="">_______________________________________________
Intel-gfx mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a>
<a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a>
</pre>
    </blockquote>
    <br>
  </body>
</html>