From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH RESEND 1/6] drm/atomic: initial support for asynchronous plane update Date: Wed, 31 May 2017 17:47:31 -0700 Message-ID: <87fufkcz4s.fsf@eliezer.anholt.net> References: <20170525195144.18134-1-gustavo@padovan.org> <20170525195144.18134-2-gustavo@padovan.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2110698834==" Return-path: In-Reply-To: <20170525195144.18134-2-gustavo@padovan.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Gustavo Padovan , dri-devel@lists.freedesktop.org Cc: Gustavo Padovan , intel-gfx@lists.freedesktop.org, Daniel Vetter List-Id: intel-gfx@lists.freedesktop.org --===============2110698834== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Gustavo Padovan writes: > From: Gustavo Padovan > > In some cases, like cursor updates, it is interesting to update the > plane in an asynchronous fashion to avoid big delays. The current queued > update could be still waiting for a fence to signal and thus block any > subsequent update until its scan out. In cases like this if we update the > cursor synchronously through the atomic API it will cause significant > delays that would even be noticed by the final user. > > This patch creates a fast path to jump ahead the current queued state and > do single planes updates without going through all atomic steps in > drm_atomic_helper_commit(). We take this path for legacy cursor updates. > > For now only single plane updates are supported, but we plan to support > multiple planes updates and async PageFlips through this interface as well > in the near future. Sorry for taking so long to do some review for this. > --- > drivers/gpu/drm/drm_atomic.c | 65 ++++++++++++++++++++++++++= ++++++ > drivers/gpu/drm/drm_atomic_helper.c | 35 +++++++++++++++++ > include/drm/drm_atomic.h | 2 + > include/drm/drm_atomic_helper.h | 2 + > include/drm/drm_modeset_helper_vtables.h | 48 +++++++++++++++++++++++ > 5 files changed, 152 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index be62774..2a0c297 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -631,6 +631,68 @@ static int drm_atomic_crtc_check(struct drm_crtc *cr= tc, > return 0; > } >=20=20 > +static bool drm_atomic_async_check(struct drm_atomic_state *state) > +{ > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > + struct drm_crtc_commit *commit; > + struct drm_plane *__plane, *plane =3D NULL; > + struct drm_plane_state *__plane_state, *plane_state =3D NULL; > + const struct drm_plane_helper_funcs *funcs; > + int i, j, n_planes =3D 0; > + > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > + return false; > + } > + > + for_each_new_plane_in_state(state, __plane, __plane_state, i) { > + n_planes++; > + plane =3D __plane; > + plane_state =3D __plane_state; > + } > + > + /* FIXME: we support only single plane updates for now */ > + if (!plane || n_planes !=3D 1) > + return false; > + > + if (!plane_state->crtc) > + return false; > + > + funcs =3D plane->helper_private; > + if (!funcs->atomic_async_update) > + return false; > + > + if (plane_state->fence) > + return false; > + Could you add a comment here like: /* Don't do an async update if there is an outstanding commit modifying * the plane. This prevents our async update's changes from getting * overridden by a previous synchronous update's state. */ (assuming I understand its intent correctly) I don't understand KMS locking enough to say if this loop is actually safely guaranteeing that. If you're convinced of that, then with my little cleanups this patch will be: Acked-by: Eric Anholt > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > + if (plane->crtc !=3D crtc) > + continue; > + > + spin_lock(&crtc->commit_lock); > + commit =3D list_first_entry_or_null(&crtc->commit_list, > + struct drm_crtc_commit, > + commit_entry); > + if (!commit) { > + spin_unlock(&crtc->commit_lock); > + continue; > + } > + spin_unlock(&crtc->commit_lock); > + > + if (!crtc->state->state) > + continue; > + > + for_each_plane_in_state(crtc->state->state, __plane, > + __plane_state, j) { > + if (__plane =3D=3D plane) > + return false; > + } > + } > + > + return !funcs->atomic_async_check(plane, plane_state); > +} > + > static void drm_atomic_crtc_print_state(struct drm_printer *p, > const struct drm_crtc_state *state) > { > @@ -1591,6 +1653,9 @@ int drm_atomic_check_only(struct drm_atomic_state *= state) > } > } >=20=20 > + if (state->legacy_cursor_update) > + state->async_update =3D drm_atomic_async_check(state); > + > return ret; > } > EXPORT_SYMBOL(drm_atomic_check_only); > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_at= omic_helper.c > index 5a3c9c0..a8efdfe 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1235,6 +1235,30 @@ static void commit_work(struct work_struct *work) > } >=20=20 > /** > + * drm_atomic_helper_async_commit - commit state asynchronously > + * > + * This function commits a state asynchronously, i.e., not vblank > + * synchronized. It should be used on a state only when > + * drm_atomic_async_check() succeeds. Async commits are not supposed to = swap > + * the states like normal sync commits, but just do in-place changes on = the > + * current state. > + */ > +void drm_atomic_helper_async_commit(struct drm_device *dev, > + struct drm_atomic_state *state) > +{ > + struct drm_plane *plane; > + struct drm_plane_state *plane_state; > + const struct drm_plane_helper_funcs *funcs; > + int i; > + > + for_each_new_plane_in_state(state, plane, plane_state, i) { > + funcs =3D plane->helper_private; > + funcs->atomic_async_update(plane, plane_state); > + } > +} > +EXPORT_SYMBOL(drm_atomic_helper_async_commit); > + > +/** > * drm_atomic_helper_commit - commit validated state object > * @dev: DRM device > * @state: the driver state object > @@ -1258,6 +1282,17 @@ int drm_atomic_helper_commit(struct drm_device *de= v, > { > int ret; >=20=20 > + if (state->async_update) { > + ret =3D drm_atomic_helper_prepare_planes(dev, state); > + if (ret) > + return ret; > + > + drm_atomic_helper_async_commit(dev, state); > + drm_atomic_helper_cleanup_planes(dev, state); > + > + return 0; > + } > + > ret =3D drm_atomic_helper_setup_commit(state, nonblock); > if (ret) > return ret; > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index 788daf7..70ca279 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -160,6 +160,7 @@ struct __drm_connnectors_state { > * @dev: parent DRM device > * @allow_modeset: allow full modeset > * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics > + * @async_update: hint for asynchronous plane update > * @planes: pointer to array of structures with per-plane data > * @crtcs: pointer to array of CRTC pointers > * @num_connector: size of the @connectors and @connector_states arrays > @@ -172,6 +173,7 @@ struct drm_atomic_state { > struct drm_device *dev; > bool allow_modeset : 1; > bool legacy_cursor_update : 1; > + bool async_update : 1; > struct __drm_planes_state *planes; > struct __drm_crtcs_state *crtcs; > int num_connector; > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_hel= per.h > index f0a8678..8571b51 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -44,6 +44,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_st= ate *state); > int drm_atomic_helper_commit(struct drm_device *dev, > struct drm_atomic_state *state, > bool nonblock); > +void drm_atomic_helper_async_commit(struct drm_device *dev, > + struct drm_atomic_state *state); >=20=20 > int drm_atomic_helper_wait_for_fences(struct drm_device *dev, > struct drm_atomic_state *state, > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_m= odeset_helper_vtables.h > index c01c328..7964a78 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -1048,6 +1048,54 @@ struct drm_plane_helper_funcs { > */ > void (*atomic_disable)(struct drm_plane *plane, > struct drm_plane_state *old_state); > + > + /** > + * @atomic_async_check: > + * > + * Drivers should set this function pointer to check if the plane state > + * can be updated in a async fashion. Here async means "not vblank > + * synchronized". > + * > + * This hook is called by drm_atomic_async_check() to establish if a > + * given update can be committed asynchronously, that is, if it can > + * jump ahead the state currently queued for update. > + * > + * RETURNS: > + * > + * Return 0 on success and any error returned indicates that the update > + * can not be applied in asynchronous manner. > + */ > + int (*atomic_async_check)(struct drm_plane *plane, > + struct drm_plane_state *state); > + > + /** > + * @atomic_async_update: > + * > + * Drivers should set this function pointer to perform asynchronous > + * updates of planes, that is, jump ahead the currently queued "jump ahead of the" > + * state for and update the plane. Here async means "not vblank s/for // > + * synchronized". > + * > + * This hook is called by drm_atomic_helper_async_commit(). > + * > + * An async update will happen on legacy cursor updates. > + * > + * Note that unlike &drm_plane_helper_funcs.atomic_update this hook > + * takes the new &drm_plane_state as parameter. When doing async_update > + * drivers shouldn't replace the &drm_plane_state but update the > + * current one with the new plane configurations in the new > + * plane_state. Also maybe document that this won't be called if there is an outstanding commit modifying the plane? > + * > + * FIXME: > + * - It only works for single plane updates > + * - Async Pageflips are not supported yet > + * - Some hm might still scan out the old buffer until the next "Some hw"? > + * vblank, however we let go of the fb references as soon as > + * we run this hook. For now drivers must implement their own workers > + * for defering if needed, until a common solution is created. > + */ "deferring" > + void (*atomic_async_update)(struct drm_plane *plane, > + struct drm_plane_state *new_state); Is there a simple list of what parts of drm_plane_state may change in this hook? I noticed that vc4 wasn't checking that the format doesn't change, for example. I'm also not sure if we're guaranteed that the plane-is-enabled state isn't changing, either. If it's "everything", we'll need to improve the vc4 async check before we expose this under anything but the cursor ioctl. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlkvZCMACgkQtdYpNtH8 nuhbpQ/+LBKSJ+3FKEot9tXruP9TxTv1YsznWJkA4En8hEcAoHw7k82e9D+0bA1O QRLxnMbLZHLFOScJKUUIiWj239rgwwocZ3CkyKpS1Yj7cXeg6idLXfaLjtHFcTsb cWIYbGma4HqV4Al+lqICh+4AjVMfHoEVMl4Dx64jYHSdBSQsvOr4sC6nE2H2SZWv ZtZZZKUji/73nnnY+M1klc57wzBXphqm8gdr2Do7Eo0OMi+7oAOtGnIdGrdLkYZD b8NAS/z5uX7n/OSQprL+jE2AeyYM4uWimP3CSh3pSE6BaYOoX64/sgQEZicXyB7y lCw0Av3lNAMr9mFo2oEdHikJT0viJ45U/g12TC3ZvuWymDvO9qt32vxER/m1HjFu V1bvjGdqlVQYtZPre/J3wgjKNGCHLDnO0tUj7C0IZw+/BGbYhOks+TB2jC9hm8gy XV2kQZK0pxYAcjGCpk0s32ZBsG2Rxch+nCGCb4G7h6TLJcvGC5LpYc1rcK/sZOnp CvY6hOwafv5Ff8tiVvyMkrnEuR5W2qh7OIYaQGGBNFTECRbyFxgsynJGVJ1ZAil0 Jaf6hN7QcDABHrkMghzl7SPp+klDz35bjGuL2b51byD95IM29NdqaSiAQQ1PnMnH QU31gGLZuNLaicd12x9vfub6ZGhVclMvbx8JUVIKM4uTtS4tSyA= =HPGK -----END PGP SIGNATURE----- --=-=-=-- --===============2110698834== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============2110698834==--