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: Thu, 15 Nov 2018 12:49:11 -0800 Message-ID: <87zhua12yg.fsf@anholt.net> References: <20181115103721.25601-1-boris.brezillon@bootlin.com> <20181115103721.25601-5-boris.brezillon@bootlin.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2003036646==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 8E1006E670 for ; Thu, 15 Nov 2018 20:49:14 +0000 (UTC) In-Reply-To: <20181115103721.25601-5-boris.brezillon@bootlin.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Cc: Boris Brezillon , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============2003036646== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Boris Brezillon writes: > 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 order > 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_pl= ane.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 dr= m_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; 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. Any ideas for what we can do to handle that? > + > /* Update the current vc4_state pos0, pos2 and ptr0 dlist entries. */ > vc4_state->dlist[vc4_state->pos0_offset] =3D > new_vc4_state->dlist[vc4_state->pos0_offset]; > @@ -942,13 +962,38 @@ static void vc4_plane_atomic_async_update(struct dr= m_plane *plane, > static int vc4_plane_atomic_async_check(struct drm_plane *plane, > struct drm_plane_state *state) > { > - /* No configuring new scaling in the fast path. */ > - if (plane->state->crtc_w !=3D state->crtc_w || > - plane->state->crtc_h !=3D state->crtc_h || > - plane->state->src_w !=3D state->src_w || > - plane->state->src_h !=3D state->src_h) > + struct vc4_plane_state *old_vc4_state, *new_vc4_state; > + int ret; > + u32 i; > + > + ret =3D vc4_plane_mode_set(plane, state); > + if (ret) > + return ret; > + > + old_vc4_state =3D to_vc4_plane_state(plane->state); > + new_vc4_state =3D to_vc4_plane_state(state); > + if (old_vc4_state->dlist_count !=3D new_vc4_state->dlist_count || > + old_vc4_state->pos0_offset !=3D new_vc4_state->pos0_offset || > + old_vc4_state->pos2_offset !=3D new_vc4_state->pos2_offset || > + old_vc4_state->ptr0_offset !=3D new_vc4_state->ptr0_offset || > + vc4_lbm_size(plane->state) !=3D vc4_lbm_size(state)) > return -EINVAL; >=20=20 > + /* Only pos0, pos2 and ptr0 DWORDS can be updated in an async update > + * if anything else has changed, fallback to a sync update. > + */ > + for (i =3D 0; i < new_vc4_state->dlist_count; i++) { > + if (i =3D=3D new_vc4_state->pos0_offset || > + i =3D=3D new_vc4_state->pos2_offset || > + i =3D=3D new_vc4_state->ptr0_offset || > + (new_vc4_state->lbm_offset && > + i =3D=3D new_vc4_state->lbm_offset)) > + continue; > + > + if (new_vc4_state->dlist[i] !=3D old_vc4_state->dlist[i]) > + return -EINVAL; > + } > + I really like these new checks for whether we can do the async update, compared to my old ones! --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlvt28gACgkQtdYpNtH8 nui8dQ/+K1spJOe0nnOSrRzIbO1ca3cLdHXciM3Sh04mGBWVYbnvZbVb4BUMxU2u 4Pp55UbgoWbTugWCrJZH7XS3JYz/LLsEgSBMf1+kumHUhrJYWv0IvMefbygIpy7/ icMgyUoBVdWhtdZYThPkGbNyULbl3mnuBa0p8fwdob/5ofRkwzWsz03KfUOIOFWg befRypZBhVLtLgxXFYKVNKm7nHJM2dOcAipCFNME7eC5sODQeJdloxvNjTMiOTkc qH4jDQEePIgRWU1ukodvFmDPH3ol42r3ED0ZNhnWBQILvf2zhVJ0bwg0wDuCjhRU kSHTa0DQ+W8D9PS9LPnL+rXzaVF1zLQJf95RvB2YzM921WIu9Z8zoczMw3NPisJm lgmQshUOiLSBwlFjJRkjyRboOP4xHxvVZ1vV8eVEEIjDkUEoL4SzModdQL3r7mIo Cdjb9aKvdoA6DUJlJgFSBZYKAn7oheOLhXLMhOSSRkyLG7m6igslMNZyue9hju/G 3GcfqNd1Ue4shRWuD98KWQKBqScLRGnD3xGnmPXN9cqJg9n/5ZLj6v/0ffxOmNPN GTQAqt0bsx4IiZNtKemiMyntcUr4Krv4/HpDvW/lBDyqnC11m6elQ7ZwiZaKvcG7 SKtx8st2OLs4AQ33b8OVZI8F4v8XNwDpSOr9fh1XJTNn2fFQR1k= =7olJ -----END PGP SIGNATURE----- --=-=-=-- --===============2003036646== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============2003036646==--