From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4E99789137 for ; Tue, 9 Apr 2019 01:41:10 +0000 (UTC) From: "Souza, Jose" Date: Tue, 9 Apr 2019 01:41:08 +0000 Message-ID: <01b88dc4ae786c993ef5052eaec297eb8096fa1d.camel@intel.com> References: <20190405233118.23045-1-jose.souza@intel.com> <408a528b534a3017f0a2d32561cd7cea0bb50441.camel@intel.com> In-Reply-To: <408a528b534a3017f0a2d32561cd7cea0bb50441.camel@intel.com> Content-Language: en-US MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============2124720090==" Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: "igt-dev@lists.freedesktop.org" , "Lisovskiy, Stanislav" List-ID: --===============2124720090== Content-Language: en-US Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-+CtEjFwKmJNOCPX3/kwd" --=-+CtEjFwKmJNOCPX3/kwd Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2019-04-08 at 09:38 +0100, Lisovskiy, Stanislav wrote: > On Fri, 2019-04-05 at 16:31 -0700, Jos=C3=A9 Roberto de Souza wrote: > > Cc: Stanislav Lisovskiy > > Signed-off-by: Jos=C3=A9 Roberto de Souza > > --- > > tests/kms_atomic_transition.c | 217 ++++++++++++++++++++---------- > > ---- > > 1 file changed, 130 insertions(+), 87 deletions(-) > >=20 > > diff --git a/tests/kms_atomic_transition.c > > b/tests/kms_atomic_transition.c > > index 18f73317..9988f303 100644 > > --- a/tests/kms_atomic_transition.c > > +++ b/tests/kms_atomic_transition.c > > @@ -40,9 +40,18 @@ > > #define DRM_CAP_CURSOR_HEIGHT 0x9 > > #endif > > =20 > > +enum group_type { > > + GROUP_TYPE_NONE =3D 0, > > + GROUP_TYPE_PRIMARY =3D 1 << 0, > > + GROUP_TYPE_CURSOR =3D 1 << 1, > > + GROUP_TYPE_OVERLAY =3D 1 << 2, > > + GROUP_TYPE_OVERLAY2 =3D 1 << 3 > > +}; > > + > > struct plane_parms { > > struct igt_fb *fb; > > - uint32_t width, height, mask; > > + uint32_t width, height; > > + enum group_type mask; > > }; > > =20 > > /* globals for fence support */ > > @@ -203,12 +212,11 @@ static void setup_parms(igt_display_t > > *display, > > enum pipe pipe, > > unsigned *iter_max) > > { > > uint64_t cursor_width, cursor_height; > > - unsigned sprite_width, sprite_height, prev_w, prev_h; > > - bool max_sprite_width, max_sprite_height, alpha =3D true; > > - uint32_t n_planes =3D display->pipes[pipe].n_planes; > > - uint32_t n_overlays =3D 0, overlays[n_planes]; > > + uint32_t sprite_width, sprite_height, prev_w, prev_h; > > igt_plane_t *plane; > > - uint32_t iter_mask =3D 3; > > + uint32_t iter_mask =3D 0, max_overlays, n_overlays =3D 0; > > + bool alpha =3D true; > > + int ret; > > =20 > > do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH, > > &cursor_width)); > > if (cursor_width >=3D mode->hdisplay) > > @@ -225,123 +233,156 @@ static void setup_parms(igt_display_t > > *display, enum pipe pipe, > > parms[i].fb =3D primary_fb; > > parms[i].width =3D mode->hdisplay; > > parms[i].height =3D mode->vdisplay; > > - parms[i].mask =3D 1 << 0; > > + parms[i].mask =3D GROUP_TYPE_PRIMARY; > > + iter_mask |=3D GROUP_TYPE_PRIMARY; > > } else if (plane->type =3D=3D DRM_PLANE_TYPE_CURSOR) { > > parms[i].fb =3D argb_fb; > > parms[i].width =3D cursor_width; > > parms[i].height =3D cursor_height; > > - parms[i].mask =3D 1 << 1; > > + parms[i].mask =3D GROUP_TYPE_CURSOR; > > + iter_mask |=3D GROUP_TYPE_CURSOR; > > } else { > > parms[i].fb =3D sprite_fb; > > - parms[i].mask =3D 1 << 2; > > - > > - iter_mask |=3D 1 << 2; > > - > > - overlays[n_overlays++] =3D i; > > + parms[i].mask =3D GROUP_TYPE_OVERLAY; > > + iter_mask |=3D GROUP_TYPE_OVERLAY; > > + n_overlays++; > > + if (alpha) > > + alpha =3D igt_plane_has_format_mod(plane, > > + DRM_FO > > RMAT_ARGB8888, > > + LOCAL_ > > DRM_FORMAT_MOD_NONE); > > } > > } > > =20 > > - if (n_overlays >=3D 2) { > > - uint32_t i; > > + prev_w =3D sprite_width =3D cursor_width; > > + prev_h =3D sprite_height =3D cursor_height; > > + > > + igt_create_fb(display->drm_fd, cursor_width, cursor_height, > > + DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE, > > argb_fb); > > + > > + igt_create_fb(display->drm_fd, sprite_width, sprite_height, > > + alpha ? DRM_FORMAT_ARGB8888 : > > DRM_FORMAT_XRGB8888, > > + LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb); > > + > > + max_overlays =3D n_overlays; > > +retry_bw: > > + /* Limit the number of planes */ > > + while (max_overlays < n_overlays) { > > + int i =3D hars_petruska_f54_1_random_unsafe_max(display- > > > pipes[pipe].n_planes); > > =20 > > /* > > - * Create 2 groups for overlays, make sure 1 plane is > > put > > - * in each then spread the rest out. > > + * Always keep primary and cursor and discard already > > + * removed planes > > */ > > - iter_mask |=3D 1 << 3; > > - parms[overlays[n_overlays - 1]].mask =3D 1 << 3; > > + if (parms[i].mask !=3D GROUP_TYPE_OVERLAY) > > + continue; > > =20 > > - for (i =3D 1; i < n_overlays - 1; i++) { > > - int val =3D > > hars_petruska_f54_1_random_unsafe_max(2); > > + parms[i].mask =3D GROUP_TYPE_NONE; >=20 > You were arguing that parms[i].mask can't be 0. Now what if you call > now wm_setup_plane with it as a parameter? :) There is no problem in have mask =3D 0, the problem is say that is mask =3D 0 in the commit message but that don't happen. >=20 > > + n_overlays--; > > + } > > =20 > > - parms[overlays[i]].mask =3D 1 << (2 + val); > > - } > > + /* > > + * Check if there is enough bandwidth for the current number of > > planes. > > + * As the plane size and position is not taken into account we > > can do > > + * this earlier. > > + */ > > + set_sprite_wh(display, pipe, parms, sprite_fb, alpha, > > sprite_width, > > + sprite_height); > > + wm_setup_plane(display, pipe, iter_mask, parms, false); > > + > > + /* It should be able to handle at least primary and cursor */ > > + if (!max_overlays) { > > + iter_mask &=3D ~GROUP_TYPE_OVERLAY; > > + *iter_max =3D iter_mask + 1; > > + return; > > } >=20 > You could just take care about this organizing cycle above properly. > Moreover you could bail out, checking that much earlier, thus > reducing > the amount of code executed. The calls to set_sprite_wh() and wm_setup_plane() in !max_overlays conditions takes care of free the framebuffers. >=20 > > =20 > > - igt_create_fb(display->drm_fd, cursor_width, cursor_height, > > - DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE, > > argb_fb); > > + ret =3D igt_display_try_commit_atomic(display, > > DRM_MODE_ATOMIC_TEST_ONLY | > > + DRM_MODE_ATOMIC_ALLOW_MODES > > ET, > > + NULL); > > + /* > > + * Could mean other errors but this is also the error returned > > when > > + * there is not enough bandwidth for all the planes > > + */ > > + if (ret =3D=3D -EINVAL) { > > + max_overlays--; > > + goto retry_bw; > > + } >=20 > There was a macro for that, in fact I have agreed with Daniel, so=20 > that we'll have a macro here to see if there is a "correct" return > value or if we need to fail. What if ret is ENOSPACE? Or something > else? We had a bug about this. Now you are going to make it fail > somewhere else again.=09 Yeah it should return a error for anything !=3D 0 after check for -EINVAL. >=20 > Also=20 >=20 > > =20 > > - igt_create_fb(display->drm_fd, cursor_width, cursor_height, > > - DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE, > > sprite_fb); > > + igt_assert_f(!ret, "Error %i not expected by try_commit()\n", > > ret); > > =20 > > - *iter_max =3D iter_mask + 1; > > - if (!n_overlays) > > - return; > > + /* So it have enough bandwidth for n_overlays planes */ > > =20 > > /* > > - * Pre gen9 not all sizes are supported, find the biggest > > possible > > - * size that can be enabled on all sprite planes. > > + * Create 2 groups for overlays, make sure 1 plane is put in > > each then > > + * spread the rest out. > > */ > > -retry: > > - prev_w =3D sprite_width =3D cursor_width; > > - prev_h =3D sprite_height =3D cursor_height; > > + iter_mask &=3D ~GROUP_TYPE_OVERLAY; > > + for_each_plane_on_pipe(display, pipe, plane) { > > + int i =3D plane->index; > > =20 > > - max_sprite_width =3D (sprite_width =3D=3D mode->hdisplay); > > - max_sprite_height =3D (sprite_height =3D=3D mode->vdisplay); > > + if (parms[i].mask !=3D GROUP_TYPE_OVERLAY) > > + continue; > > =20 > > - while (1) { > > - int ret; > > + /* First overlay plane will be overlay group 1 */ > > + if (!(iter_mask & GROUP_TYPE_OVERLAY)) { > > + iter_mask |=3D GROUP_TYPE_OVERLAY; > > + continue; > > + } > > =20 > > - set_sprite_wh(display, pipe, parms, sprite_fb, > > - alpha, sprite_width, sprite_height); > > + /* Second overlay plane will be overlay group 1 */ > > + if (!(iter_mask & GROUP_TYPE_OVERLAY2)) { > > + iter_mask |=3D GROUP_TYPE_OVERLAY2; > > + parms[i].mask =3D GROUP_TYPE_OVERLAY2; > > + continue; > > + } > > =20 >=20 > Why it is so complex? Previously we were setting it only once. > Now you have two more conditions, checking and setting it second > time, > introducing more entropy. Here it is only randomizing the groups once, after the number of planes that fits in BW is founds. >=20 > Moreover this is very unreadable and hard to check which errors > it can bring.=20 >=20 > > - wm_setup_plane(display, pipe, (1 << n_planes) - 1, > > parms, false); > > - ret =3D igt_display_try_commit_atomic(display, > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > - igt_assert(!is_atomic_check_failure_errno(ret)); > > + /* Sort the group of the rest of overlay planes */ > > + if (hars_petruska_f54_1_random_unsafe_max(2)) > > + parms[i].mask =3D GROUP_TYPE_OVERLAY2; > > + } > > =20 > > - if (is_atomic_check_plane_size_errno(ret)) { > > - if (cursor_width =3D=3D sprite_width && > > - cursor_height =3D=3D sprite_height) { > > - igt_assert_f(alpha, > > - "Cannot configure the > > test with all sprite planes enabled\n"); > > - > > - /* retry once with XRGB format. */ > > - alpha =3D false; > > - goto retry; > > - } > > + *iter_max =3D iter_mask + 1; > > =20 > > + /* > > + * Pre gen9 not all sizes are supported, find the biggest > > possible > > + * size that can be enabled on all sprite planes. > > + */ > > + while (1) { > > + /* Size limit reached */ > > + if (is_atomic_check_plane_size_errno(ret)) { > > sprite_width =3D prev_w; > > sprite_height =3D prev_h; > > - > > - if (max_sprite_width && max_sprite_height) { > > - set_sprite_wh(display, pipe, parms, > > sprite_fb, > > - alpha, sprite_width, > > sprite_height); > > - break; > > - } > > - > > - if (!max_sprite_width) > > - max_sprite_width =3D true; > > - else > > - max_sprite_height =3D true; > > - } else { > > - prev_w =3D sprite_width; > > - prev_h =3D sprite_height; > > + break; > > } > > =20 > > - if (!max_sprite_width) { > > - sprite_width *=3D 2; > > + /* Commit is valid and it reached max size, use this > > size */ > > + if (sprite_width =3D=3D mode->hdisplay || > > + sprite_height =3D=3D mode->vdisplay) > > + break; > > =20 > > - if (sprite_width >=3D mode->hdisplay) { > > - max_sprite_width =3D true; > > + prev_w =3D sprite_width; > > + prev_h =3D sprite_height; > > =20 > > - sprite_width =3D mode->hdisplay; > > - } > > - } else if (!max_sprite_height) { > > - sprite_height *=3D 2; > > + sprite_width *=3D 2; > > + if (sprite_width >=3D mode->hdisplay) > > + sprite_width =3D mode->hdisplay; > > =20 > > - if (sprite_height >=3D mode->vdisplay) { > > - max_sprite_height =3D true; > > + sprite_height *=3D 2; > > + if (sprite_height >=3D mode->vdisplay) > > + sprite_height =3D mode->vdisplay; > > =20 > > - sprite_height =3D mode->vdisplay; > > - } > > - } else > > - /* Max sized sprites for all! */ > > - break; > > + set_sprite_wh(display, pipe, parms, sprite_fb, > > sprite_width, > > + sprite_height, alpha); > > + wm_setup_plane(display, pipe, iter_mask, parms, false); > > + ret =3D igt_display_try_commit_atomic(display, > > + DRM_MODE_ATOMIC_TES > > T_ONLY | > > + DRM_MODE_ATOMIC_ALL > > OW_MODESET, > > + NULL); > > } > > =20 > > - igt_info("Running test on pipe %s with resolution %dx%d and > > sprite size %dx%d alpha %i\n", > > + igt_info("Running test on pipe %s with resolution %dx%d, sprite > > size %dx%d, alpha %i and %i overlay planes\n", > > kmstest_pipe_name(pipe), mode->hdisplay, mode- > > > vdisplay, > > - sprite_width, sprite_height, alpha); > > + sprite_width, sprite_height, alpha, n_overlays); > > } > > =20 > > static void prepare_fencing(igt_display_t *display, enum pipe > > pipe) > > @@ -519,8 +560,10 @@ run_transition_test(igt_display_t *display, > > enum > > pipe pipe, igt_output_t *output > > } > > =20 > > /* force planes to be part of commit */ > > - for_each_plane_on_pipe(display, pipe, plane) > > - igt_plane_set_position(plane, 0, 0); > > + for_each_plane_on_pipe(display, pipe, plane) { > > + if (parms[plane->index].mask !=3D > > GROUP_TYPE_NONE) > > + igt_plane_set_position(plane, 0, 0); > > + } > > =20 > > igt_display_commit2(display, COMMIT_ATOMIC); > > =20 --=-+CtEjFwKmJNOCPX3/kwd Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEVNG051EijGa0MiaQVenbO/mOWkkFAlyr+DMACgkQVenbO/mO WkkqbQgAo44qyFO4xHuoq/cY1fk0LmOzqUb8JbHLYb1OfPHqLE2yq5iopJJqcBFJ 8iZOXSA2CE1rM1XsnAjgd2NOiCF0D9F9p+jluWRFegpwso7tVovPueHCQq9XkDkR eM1YbA0xPzKVMHiT1O+JKDA4X3up3wlSVLyIwQooDTf0c8dkV1Yf6jNqjFrZ1STi pUmlweKlJtNEHAW660WkYuI6/ilArhzYjfOEgkKMc4zHkjiitc38KKuZZFSVgY8d VEFtjFU5VUfnFgB/TX0wdJG+h/dnfG56gVe+2AG4ohO/wcLvpaJuAB+oBUkIGSSi gDRJ7WBzN1Vkv36oVgFkNKkMqna1Xw== =HMEF -----END PGP SIGNATURE----- --=-+CtEjFwKmJNOCPX3/kwd-- --===============2124720090== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KaWd0LWRldiBt YWlsaW5nIGxpc3QKaWd0LWRldkBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pZ3QtZGV2 --===============2124720090==--