From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH 2/6] drm/vc4: Move LBM creation out of vc4_plane_mode_set() Date: Thu, 15 Nov 2018 12:39:42 -0800 Message-ID: <878t1u2hyp.fsf@anholt.net> References: <20181115103721.25601-1-boris.brezillon@bootlin.com> <20181115103721.25601-3-boris.brezillon@bootlin.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0072777483==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 4F7A16E5E6 for ; Thu, 15 Nov 2018 20:39:45 +0000 (UTC) In-Reply-To: <20181115103721.25601-3-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 --===============0072777483== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Boris Brezillon writes: > We are about to use vc4_plane_mode_set() in the async check path, and > async updates require that LBM size stay the same since they reuse the > LBM from the previous state. So we definitely don't want to allocate a > new LBM region that we know for sure will be free right away. > > Move the LBM allocation out of vc4_plane_mode_set() and call the new > function (vc4_plane_update_lbm()) from vc4_plane_atomic_check(). > > Signed-off-by: Boris Brezillon > --- > drivers/gpu/drm/vc4/vc4_drv.h | 1 + > drivers/gpu/drm/vc4/vc4_plane.c | 82 ++++++++++++++++++++++----------- > 2 files changed, 55 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index bd6ef1f31822..9ed05fb61eb6 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -338,6 +338,7 @@ struct vc4_plane_state { > u32 pos0_offset; > u32 pos2_offset; > u32 ptr0_offset; > + u32 lbm_offset; >=20=20 > /* Offset where the plane's dlist was last stored in the > * hardware at vc4_crtc_atomic_flush() time. > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_pl= ane.c > index f6e3e8d33115..392a51f2bf8f 100644 > --- a/drivers/gpu/drm/vc4/vc4_plane.c > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > @@ -452,6 +452,43 @@ static void vc4_write_scaling_parameters(struct drm_= plane_state *state, > } > } >=20=20 > +static int vc4_plane_update_lbm(struct drm_plane_state *state) > +{ > + struct vc4_dev *vc4 =3D to_vc4_dev(state->plane->dev); > + struct vc4_plane_state *vc4_state =3D to_vc4_plane_state(state); > + unsigned long irqflags; > + u32 lbm_size; > + > + lbm_size =3D vc4_lbm_size(state); > + if (!lbm_size) > + return 0; > + > + if (WARN_ON(!vc4_state->lbm_offset)) > + return -EINVAL; > + > + /* Allocate the LBM memory that the HVS will use for temporary > + * storage due to our scaling/format conversion. > + */ > + if (!vc4_state->lbm.allocated) { > + int ret; > + > + spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags); > + ret =3D drm_mm_insert_node_generic(&vc4->hvs->lbm_mm, > + &vc4_state->lbm, > + lbm_size, 32, 0, 0); > + spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags); > + > + if (ret) > + return ret; > + } else { > + WARN_ON_ONCE(lbm_size !=3D vc4_state->lbm.size); > + } > + > + vc4_state->dlist[vc4_state->lbm_offset] =3D vc4_state->lbm.start; > + > + return 0; > +} > + > /* Writes out a full display list for an active plane to the plane's > * private dlist state. > */ > @@ -469,31 +506,12 @@ static int vc4_plane_mode_set(struct drm_plane *pla= ne, > bool mix_plane_alpha; > bool covers_screen; > u32 scl0, scl1, pitch0; > - u32 lbm_size, tiling; > - unsigned long irqflags; > + u32 tiling; > u32 hvs_format =3D format->hvs; > int ret, i; >=20=20 > - ret =3D vc4_plane_setup_clipping_and_scaling(state); > - if (ret) > - return ret; > - > - /* Allocate the LBM memory that the HVS will use for temporary > - * storage due to our scaling/format conversion. > - */ > - lbm_size =3D vc4_lbm_size(state); > - if (lbm_size) { > - if (!vc4_state->lbm.allocated) { > - spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags); > - ret =3D drm_mm_insert_node_generic(&vc4->hvs->lbm_mm, > - &vc4_state->lbm, > - lbm_size, 32, 0, 0); > - spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags); > - } else { > - WARN_ON_ONCE(lbm_size !=3D vc4_state->lbm.size); > - } > - } >=20=20 > + ret =3D vc4_plane_setup_clipping_and_scaling(state); > if (ret) > return ret; >=20=20 > @@ -717,15 +735,18 @@ static int vc4_plane_mode_set(struct drm_plane *pla= ne, > vc4_dlist_write(vc4_state, SCALER_CSC2_ITR_R_601_5); > } >=20=20 > + vc4_state->lbm_offset =3D 0; > + > if (vc4_state->x_scaling[0] !=3D VC4_SCALING_NONE || > vc4_state->x_scaling[1] !=3D VC4_SCALING_NONE || > vc4_state->y_scaling[0] !=3D VC4_SCALING_NONE || > vc4_state->y_scaling[1] !=3D VC4_SCALING_NONE) { > - /* LBM Base Address. */ > + /* Reserve a slot for the LBM Base Address. The real value will > + * be set when calling vc4_plane_update_lbm(). > + */ > if (vc4_state->y_scaling[0] !=3D VC4_SCALING_NONE || > - vc4_state->y_scaling[1] !=3D VC4_SCALING_NONE) { > - vc4_dlist_write(vc4_state, vc4_state->lbm.start); > - } > + vc4_state->y_scaling[1] !=3D VC4_SCALING_NONE) > + vc4_state->lbm_offset =3D vc4_state->dlist_count++; >=20=20 > if (num_planes > 1) { > /* Emit Cb/Cr as channel 0 and Y as channel > @@ -785,13 +806,18 @@ static int vc4_plane_atomic_check(struct drm_plane = *plane, > struct drm_plane_state *state) > { > struct vc4_plane_state *vc4_state =3D to_vc4_plane_state(state); > + int ret; >=20=20 > vc4_state->dlist_count =3D 0; >=20=20 > - if (plane_enabled(state)) > - return vc4_plane_mode_set(plane, state); > - else > + if (!plane_enabled(state)) > return 0; > + > + ret =3D vc4_plane_mode_set(plane, state); > + if (ret) > + return ret; > + > + return vc4_plane_update_lbm(state); > } Couldn't you call update_lbm() (maybe called allocate_lbm()) first, and then leave the dlist setup as is without needing lbm_offset? --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlvt2Y4ACgkQtdYpNtH8 nugIeBAAoayRkcKuc0/LES/nb92q3X/wQ7/gTTA6cwPBdDWjXklCFpupwLwBwVxo viQ2ic54VJSbpq4cNBb4xgF4DPu6rdNs89qfuxE5nFdVI2FwBG9xymTjRk3JEe4V TfLdkLA+r1HUuq8HfqxSYgRIKJmdSIva2JPMu8PX+vvDeIgWNMw3amRufNKXdOVx 11UHcT15YvXpdWQMrW+dipYrrBkzXHd2O7A15hIxElwsv+eHPnMbyUYl255OKUwV 1j9Fa47dIRUuueg3W15GaX1f76vGuf4/Iv6fcn0mjxCBn0M0KuAYbIoDMRNpzxvg Leb9nJhC1AFePWLoQBSmgBFYmFX1u8E6ZTR3cZoLlo4oREktPsIeMeyupQIKiqAp yXdFHdQGs4nfzXt7cC+EuJGwVWZhgSd3GalBTJqPbvHcc4dgFGGobjAJM+Jb6qvV i9nKICm19nki3EuIyPIh9wdXfauQVb/MddPIfcCXC+ZJPbNP4mXDIdsLWWHCyOCn Ww4TKUjeQ9v55HegBjw0JloBEO2A1ikwydcODNj3lrOU/+mZVSAGfBDccVzHTQPX qzDUj+Kd0LZzL9h77D+YjMl5c0hAKW07E0AvFoJsy/QkwNo2CtPiYC05XdZppveA cv3TpeaptbyTu7tj1xxcbCV+JJ3Do++ITjEOKInvyscKGByfiyY= =X6oA -----END PGP SIGNATURE----- --=-=-=-- --===============0072777483== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0072777483==--