From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lionel Landwerlin Subject: Re: [PATCH i-g-t v7] lib/igt_kms: Add COMMIT_ATOMIC to igt_display_commit2() Date: Tue, 8 Mar 2016 16:11:18 +0000 Message-ID: <56DEF9A6.90808@gmail.com> References: <1457345934-28943-1-git-send-email-pratik.vishwakarma@intel.com> <56DD78FE.2090000@gmail.com> <56DEF62E.1080401@linux.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0271265438==" Return-path: Received: from mail-pf0-x230.google.com (mail-pf0-x230.google.com [IPv6:2607:f8b0:400e:c00::230]) by gabe.freedesktop.org (Postfix) with ESMTPS id C2C566E110 for ; Tue, 8 Mar 2016 16:11:22 +0000 (UTC) Received: by mail-pf0-x230.google.com with SMTP id 124so16007203pfg.0 for ; Tue, 08 Mar 2016 08:11:22 -0800 (PST) In-Reply-To: <56DEF62E.1080401@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Maarten Lankhorst , Pratik Vishwakarma , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============0271265438== Content-Type: multipart/alternative; boundary="------------080207070904080506020407" This is a multi-part message in MIME format. --------------080207070904080506020407 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hi Maarten, Yeah that would be much simpler I think. Damien also looked at this patch over my shoulder and had additional comments. So more or less proxying : - Not sure why we're exposing the new enums and the igt_atomic_popuplate_* macros in igt_kms.h. After all we're not using them anywhere outside igt_kms.c. - 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. Cheers, - Lionel On 08/03/16 15:56, Maarten Lankhorst wrote: > Op 07-03-16 om 13:50 schreef Lionel Landwerlin: >> Hi Pratik, >> >> I'm really looking forward to get this merged. >> Just a few comments on the plane commit part below. > 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; I missed that, but I think we need rotation_changed = false; here too. > - > - return; > } > > > --------------080207070904080506020407 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
Hi Maarten,

Yeah that would be much simpler I think.

Damien also looked at this patch over my shoulder and had additional comments.
So more or less proxying :

- Not sure why we're exposing the new enums and the igt_atomic_popuplate_* macros in igt_kms.h.
=C2=A0 After all we're not using them anywhere outside igt_kms.c.
- 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.

Cheers,

-
Lionel

On 08/03/16 15:56, Maarten Lankhorst wrote:
Op 07-03-16 om 13:50 schreef Lionel Landwerlin:

Hi Pratik,

I'm really looking forward to get this merged.
Just a few comments on the plane commit part below.

Would the following diff to the patch satisfy you? It would clean up thin=
gs 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,
=20
 	igt_display_t *display =3D 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;
=20
 	igt_assert(plane->drm_plane);
=20
@@ -1552,54 +1544,30 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plan=
e, igt_output_t *output,
 	fb_id =3D igt_plane_get_fb_id(plane);
 	crtc_id =3D output->config.crtc->crtc_id;
=20
-	if ((plane->fb_changed || plane->size_changed) && fb_id =3D=
=3D 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 =3D IGT_FIXED(plane->fb->src_x, 0); /* src_x */
-		src_y =3D IGT_FIXED(plane->fb->src_y, 0); /* src_y */
-		src_w =3D IGT_FIXED(plane->fb->src_w, 0); /* src_w */
-		src_h =3D IGT_FIXED(plane->fb->src_h, 0); /* src_h */
-		crtc_x =3D plane->crtc_x;
-		crtc_y =3D plane->crtc_y;
-		crtc_w =3D plane->crtc_w;
-		crtc_h =3D plane->crtc_h;
-
-		LOG(display,
-		    "%s: populating plane data: %s.%d, fb %u, src =3D (%d, %d) "
-		    "%ux%u dst =3D (%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 =3D (%d, %d) "
+	    "%ux%u dst =3D (%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 ? c=
rtc_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 =3D IGT_FIXED(plane->fb->src_x, 0); /* src_x */
+		uint32_t src_y =3D IGT_FIXED(plane->fb->src_y, 0); /* src_y */
+		uint32_t src_w =3D IGT_FIXED(plane->fb->src_w, 0); /* src_w */
+		uint32_t src_h =3D IGT_FIXED(plane->fb->src_h, 0); /* src_h */
+		int32_t crtc_x =3D plane->crtc_x;
+		int32_t crtc_y =3D plane->crtc_y;
+		uint32_t crtc_w =3D plane->crtc_w;
+		uint32_t crtc_h =3D plane->crtc_h;
=20
 		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 *plan=
e, 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);
-
 	}
=20
+	if (plane->rotation_changed)
+		igt_atomic_populate_plane_req(req, plane,
+			IGT_PLANE_ROTATION, plane->rotation);
+
 	plane->fb_changed =3D false;
 	plane->position_changed =3D false;
 	plane->size_changed =3D false;

I missed that, but I think we need rotation_changed =3D false; here too.

-
-	return;
 }
=20
=20

--------------080207070904080506020407-- --===============0271265438== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0271265438==--