From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH 4/6] drm/vc4: Rework the async update logic Date: Mon, 19 Nov 2018 20:44:21 -0800 Message-ID: <87ftvwqrx6.fsf@anholt.net> References: <20181115103721.25601-1-boris.brezillon@bootlin.com> <20181115103721.25601-5-boris.brezillon@bootlin.com> <87zhua12yg.fsf@anholt.net> <20181115222105.23b02a96@bbrezillon> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1226613315==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 4C9886E30E for ; Tue, 20 Nov 2018 04:44:24 +0000 (UTC) In-Reply-To: <20181115222105.23b02a96@bbrezillon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Boris Brezillon Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1226613315== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Boris Brezillon writes: > On Thu, 15 Nov 2018 12:49:11 -0800 > Eric Anholt wrote: > >> Boris Brezillon writes: >>=20 >> > vc4_plane_atomic_async_check() was only based on the >> > state->{crtc,src}_{w,h} which was fine since scaling was not allowed on >> > the cursor plane. >> > >> > We are about to change that to properly support underscan, and, in ord= er >> > to make the async check more reliable, we call vc4_plane_mode_set() >> > from there and check that only the pos0, pos2 and ptr0 entries in the >> > dlist have changed. >> > >> > In vc4_plane_atomic_async_update(), we no longer call >> > vc4_plane_atomic_check() since vc4_plane_mode_set() has already been >> > called in vc4_plane_atomic_async_check(), and we don't need to allocate >> > a new LBM region (we reuse the one from the current state). >> > >> > Note that we now have to manually update each field of the current >> > plane state since it's no longer updated in place (not sure we have >> > to sync all of them, but it's harmless if we do). >> > We also drop the vc4_plane_async_set_fb() call (ptr0 dlist entry has >> > been properly updated in vc4_plane_mode_set()) >> > >> > Signed-off-by: Boris Brezillon >> > --- >> > drivers/gpu/drm/vc4/vc4_plane.c | 87 +++++++++++++++++++++++++-------- >> > 1 file changed, 66 insertions(+), 21 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4= _plane.c >> > index 09c7478b095b..31c7b63dd723 100644 >> > --- a/drivers/gpu/drm/vc4/vc4_plane.c >> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c >> > @@ -895,30 +895,50 @@ static void vc4_plane_atomic_async_update(struct= drm_plane *plane, >> > { >> > struct vc4_plane_state *vc4_state, *new_vc4_state; >> >=20=20 >> > - if (plane->state->fb !=3D state->fb) { >> > - vc4_plane_async_set_fb(plane, state->fb); >> > - drm_atomic_set_fb_for_plane(plane->state, state->fb); >> > - } >> > - >> > - /* Set the cursor's position on the screen. This is the >> > - * expected change from the drm_mode_cursor_universal() >> > - * helper. >> > - */ >> > + drm_atomic_set_fb_for_plane(plane->state, state->fb); >> > plane->state->crtc_x =3D state->crtc_x; >> > plane->state->crtc_y =3D state->crtc_y; >> > - >> > - /* Allow changing the start position within the cursor BO, if >> > - * that matters. >> > - */ >> > + plane->state->crtc_w =3D state->crtc_w; >> > + plane->state->crtc_h =3D state->crtc_h; >> > plane->state->src_x =3D state->src_x; >> > plane->state->src_y =3D state->src_y; >> > - >> > - /* Update the display list based on the new crtc_x/y. */ >> > - vc4_plane_atomic_check(plane, state); >> > + plane->state->src_w =3D state->src_w; >> > + plane->state->src_h =3D state->src_h; >> > + plane->state->src_h =3D state->src_h; >> > + plane->state->alpha =3D state->alpha; >> > + plane->state->pixel_blend_mode =3D state->pixel_blend_mode; >> > + plane->state->rotation =3D state->rotation; >> > + plane->state->zpos =3D state->zpos; >> > + plane->state->normalized_zpos =3D state->normalized_zpos; >> > + plane->state->color_encoding =3D state->color_encoding; >> > + plane->state->color_range =3D state->color_range; >> > + plane->state->src =3D state->src; >> > + plane->state->dst =3D state->dst; >> > + plane->state->visible =3D state->visible; >> >=20=20 >> > new_vc4_state =3D to_vc4_plane_state(state); >> > vc4_state =3D to_vc4_plane_state(plane->state); >> >=20=20 >> > + vc4_state->crtc_x =3D new_vc4_state->crtc_x; >> > + vc4_state->crtc_y =3D new_vc4_state->crtc_y; >> > + vc4_state->crtc_h =3D new_vc4_state->crtc_h; >> > + vc4_state->crtc_w =3D new_vc4_state->crtc_w; >> > + vc4_state->src_x =3D new_vc4_state->src_x; >> > + vc4_state->src_y =3D new_vc4_state->src_y; >> > + memcpy(vc4_state->src_w, new_vc4_state->src_w, >> > + sizeof(vc4_state->src_w)); >> > + memcpy(vc4_state->src_h, new_vc4_state->src_h, >> > + sizeof(vc4_state->src_h)); >> > + memcpy(vc4_state->x_scaling, new_vc4_state->x_scaling, >> > + sizeof(vc4_state->x_scaling)); >> > + memcpy(vc4_state->y_scaling, new_vc4_state->y_scaling, >> > + sizeof(vc4_state->y_scaling)); >> > + vc4_state->is_unity =3D new_vc4_state->is_unity; >> > + vc4_state->is_yuv =3D new_vc4_state->is_yuv; >> > + memcpy(vc4_state->offsets, new_vc4_state->offsets, >> > + sizeof(vc4_state->offsets)); >> > + vc4_state->needs_bg_fill =3D new_vc4_state->needs_bg_fill;=20=20 >>=20 >> This copying feels like a maintenance nightmare to me -- nothing's going >> to be testing async updates of each random bit of state, so if something >> new could change while passing atomic_async_check(), we're going to get >> it wrong. > > Yeah, I don't like it either. I'd definitely prefer if states could be > swapped somehow, but then you have the problem of migrating some of the > resources from the old state to the new one (the most obvious one being > the LBM drm_mm_node object which is already inserted in a list, but > I'm pretty we have the same issue with other fields too). > >>=20 >> Any ideas for what we can do to handle that? > > Nope. I think I'm less concerned than I was the first time around. Realistically, potential new fields won't be changing anyway. If they did, they'd have an effect on the dlists other than position/pointer, and that would make them fail the async check. So, while I have some reservations, I'm also unhappy with my solution too, and I think yours has benefits that outweigh the cost of the code in question here. Reviewed-by: Eric Anholt --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlvzkSUACgkQtdYpNtH8 nuifdBAAkxTvXPIT/GybNuILDd0Xi4s9gQJig4DpJfPnhquVCOXvr4ntzVUrVpKs ZorHBJOGKR/gjWfACDyOvK6lkWP6YYulFU3yUE9Fmrn+kivUc+Fqa6LBSCOFig8G 0KNDZyTnYmrMUFQoga5qULCfiOvEGlU+ICznJy5St72/edAvfND686dMkwIOQgLy 0eSd8qkli43BPApk50xgYRRXaAvDW1PgdFk6/gK3PIeT099slgUP6wcopuvGbkGI inpxvEmpYL2UmtNExirlyHTEM7lqaqyCpLo5KDOnCNJ5/I8eykQGnqE/bRxu92aW yZVpP7oewDcL0lk13zv1w286x2LaP79zdQi+c3AGh4T5rBFed2wmdQZ3blmUfzt8 e7O6wGE3fPZy6Mk7r8jv+t19VrqLDEKRwGJsLysmtJSk22OfCWWHwCV5APwRfj+5 CIUReTgcXWXnHYmE1sICzaugAetjztOZ9HsqH6GjvaZzkqrHAysdvpUCjkZqtU6/ SvLQh8RFC67oJRiegHAuh9/WTREWIaQj++zOuQsvfSh3kWKbB8kAMWdiYJb71Q4y AOgvi78RQ3pRB92EFwoaU0Uq3IlnZF1Rm8aDW4qGcB61A6dtb/fPs7drvP6rsUgo jPQE4+Wwukwb25hNhOf/sy8sbUTilaqyPh5kQipxo1TxZoW2BNo= =g6+i -----END PGP SIGNATURE----- --=-=-=-- --===============1226613315== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1226613315==--