From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mayuresh Gharpure Subject: Re: [PATCH i-g-t v7] lib/igt_kms: Add COMMIT_ATOMIC to igt_display_commit2() Date: Wed, 9 Mar 2016 19:55:27 +0530 Message-ID: <56E03257.8080608@intel.com> References: <1457345934-28943-1-git-send-email-pratik.vishwakarma@intel.com> <56DD78FE.2090000@gmail.com> <56DEF62E.1080401@linux.intel.com> <56DEF9A6.90808@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1616554150==" Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTP id 41A6A6E13F for ; Wed, 9 Mar 2016 14:25:34 +0000 (UTC) In-Reply-To: <56DEF9A6.90808@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Lionel Landwerlin , 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. --===============1616554150== Content-Type: multipart/alternative; boundary="------------070803060704040404030407" This is a multi-part message in MIME format. --------------070803060704040404030407 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hi Lionel, I've incorporated your comments using the diff patch submitted by Maarten and submitted the changes https://patchwork.freedesktop.org/series/4274/ Also, please find my comments inline Regards, Mayuresh On 3/8/2016 9:41 PM, Lionel Landwerlin wrote: > 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. 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. These were added in initial implementation by Marius. > > - 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. 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 > > 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; >> } >> >> >> > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx --------------070803060704040404030407 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Lionel,

I've incorporated your comments using the diff patch submitted by Maarten and submitted the changes

https://patchwork.freedesktop.org/series/4274/ =

Also, please find my comments inline

Regards,
Mayuresh

On 3/8/2016 9:41 PM, Lionel Landwerlin wrote:
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.=
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.

These were added in initial implementation by Marius.


- 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.
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

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,
=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





_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinf=
o/intel-gfx

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