From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH v2 7/8] drm/vc4: Add support for the transposer block Date: Fri, 29 Jun 2018 13:35:04 -0700 Message-ID: <87d0w9nxfr.fsf@anholt.net> References: <20180629111722.20299-1-boris.brezillon@bootlin.com> <20180629111722.20299-8-boris.brezillon@bootlin.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1655891270==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 6DB7A6F1AD for ; Fri, 29 Jun 2018 20:35:07 +0000 (UTC) In-Reply-To: <20180629111722.20299-8-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" To: Boris Brezillon , David Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org Cc: Boris Brezillon , Liviu Dudau List-Id: dri-devel@lists.freedesktop.org --===============1655891270== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Boris Brezillon writes: > From: Boris Brezillon > > The transposer block is providing support for mem-to-mem composition, > which is exposed as a drm_writeback connector in DRM. > > Add a driver to support this feature. > > Signed-off-by: Boris Brezillon > +static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) > +{ > + struct drm_device *dev =3D crtc->dev; > + struct vc4_dev *vc4 =3D to_vc4_dev(dev); > + struct vc4_crtc *vc4_crtc =3D to_vc4_crtc(crtc); > + struct vc4_crtc_state *vc4_state =3D to_vc4_crtc_state(crtc->state); > + struct drm_display_mode *mode =3D &crtc->state->adjusted_mode; > + bool interlace =3D mode->flags & DRM_MODE_FLAG_INTERLACE; > + bool debug_dump_regs =3D false; > + > + if (debug_dump_regs) { > + DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc)); > + vc4_crtc_dump_regs(vc4_crtc); > + } > + > + if (vc4_crtc->channel =3D=3D 2) { > + u32 dispctrl; > + u32 dsp3_mux; > + > + /* > + * SCALER_DISPCTRL_DSP3 =3D X, where X < 2 means 'connect DSP3 to > + * FIFO X'. > + * SCALER_DISPCTRL_DSP3 =3D 3 means 'disable DSP 3'. > + * > + * DSP3 is connected to FIFO2 unless the transposer is > + * enabled. In this case, FIFO 2 is directly accessed by the > + * TXP IP, and we need to prevent disable the s/prevent // > + * FIFO2 -> pixelvalve1 route. > + */ > + if (vc4_state->feed_txp) > + dsp3_mux =3D VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX); > + else > + dsp3_mux =3D VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX); > + > + /* Reconfigure the DSP3 mux if required. */ > + dispctrl =3D HVS_READ(SCALER_DISPCTRL); > + if ((dispctrl & SCALER_DISPCTRL_DSP3_MUX_MASK) !=3D dsp3_mux) { > + dispctrl &=3D ~SCALER_DISPCTRL_DSP3_MUX_MASK; > + HVS_WRITE(SCALER_DISPCTRL, dispctrl | dsp3_mux); > + } This is fine, but you could also skip the matching mux check here -- the read is the expensive part. > + } > + > + if (!vc4_state->feed_txp) > + vc4_crtc_config_pv(crtc); >=20=20 > HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel), > SCALER_DISPBKGND_AUTOHS | > @@ -499,6 +539,13 @@ static void vc4_crtc_atomic_disable(struct drm_crtc = *crtc, > } > } >=20=20 > +void vc4_crtc_txp_armed(struct drm_crtc_state *state) > +{ > + struct vc4_crtc_state *vc4_state =3D to_vc4_crtc_state(state); > + > + vc4_state->txp_armed =3D true; > +} > + > static void vc4_crtc_update_dlist(struct drm_crtc *crtc) > { > struct drm_device *dev =3D crtc->dev; > @@ -514,8 +561,11 @@ static void vc4_crtc_update_dlist(struct drm_crtc *c= rtc) > WARN_ON(drm_crtc_vblank_get(crtc) !=3D 0); >=20=20 > spin_lock_irqsave(&dev->event_lock, flags); > - vc4_crtc->event =3D crtc->state->event; > - crtc->state->event =3D NULL; > + > + if (!vc4_state->feed_txp || vc4_state->txp_armed) { > + vc4_crtc->event =3D crtc->state->event; > + crtc->state->event =3D NULL; > + } >=20=20 > HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel), > vc4_state->mm.start); > @@ -533,8 +583,8 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *c= rtc, > struct drm_device *dev =3D crtc->dev; > struct vc4_dev *vc4 =3D to_vc4_dev(dev); > struct vc4_crtc *vc4_crtc =3D to_vc4_crtc(crtc); > - struct drm_crtc_state *state =3D crtc->state; > - struct drm_display_mode *mode =3D &state->adjusted_mode; > + struct vc4_crtc_state *vc4_state =3D to_vc4_crtc_state(crtc->state); > + struct drm_display_mode *mode =3D &crtc->state->adjusted_mode; >=20=20 > require_hvs_enabled(dev); >=20=20 > @@ -546,15 +596,21 @@ static void vc4_crtc_atomic_enable(struct drm_crtc = *crtc, >=20=20 > /* Turn on the scaler, which will wait for vstart to start > * compositing. > + * When feeding the transposer, we should operate in oneshot > + * mode. > */ > HVS_WRITE(SCALER_DISPCTRLX(vc4_crtc->channel), > VC4_SET_FIELD(mode->hdisplay, SCALER_DISPCTRLX_WIDTH) | > VC4_SET_FIELD(mode->vdisplay, SCALER_DISPCTRLX_HEIGHT) | > - SCALER_DISPCTRLX_ENABLE); > + SCALER_DISPCTRLX_ENABLE | > + (vc4_state->feed_txp ? SCALER_DISPCTRLX_ONESHOT : 0)); >=20=20 > - /* Turn on the pixel valve, which will emit the vstart signal. */ > - CRTC_WRITE(PV_V_CONTROL, > - CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN); > + /* When feeding the composer block the pixelvalve is unneeded and "transposer block"? composer block made me think HVS. > + * should not be enabled. > + */ > + if (!vc4_state->feed_txp) > + CRTC_WRITE(PV_V_CONTROL, > + CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN); > } >=20=20 > static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc, > @@ -579,8 +635,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *cr= tc, > struct drm_plane *plane; > unsigned long flags; > const struct drm_plane_state *plane_state; > + struct drm_connector *conn; > + struct drm_connector_state *conn_state; > u32 dlist_count =3D 0; > - int ret; > + int ret, i; >=20=20 > /* The pixelvalve can only feed one encoder (and encoders are > * 1:1 with connectors.) > @@ -600,6 +658,22 @@ static int vc4_crtc_atomic_check(struct drm_crtc *cr= tc, > if (ret) > return ret; >=20=20 > + state->no_vblank =3D false; > + for_each_new_connector_in_state(state->state, conn, conn_state, i) { > + if (conn_state->crtc !=3D crtc) > + continue; > + > + /* The writeback connector is implemented using the transposer > + * block which is directly taking its data from the HVS FIFO. > + */ > + if (conn->connector_type =3D=3D DRM_MODE_CONNECTOR_WRITEBACK) { > + state->no_vblank =3D true; > + vc4_state->feed_txp =3D true; > + } > + > + break; > + } > + > return 0; > } >=20=20 > @@ -713,7 +787,8 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc= *vc4_crtc) >=20=20 > spin_lock_irqsave(&dev->event_lock, flags); > if (vc4_crtc->event && > - (vc4_state->mm.start =3D=3D HVS_READ(SCALER_DISPLACTX(chan)))) { > + (vc4_state->mm.start =3D=3D HVS_READ(SCALER_DISPLACTX(chan)) || > + vc4_state->feed_txp)) { Can vc4_crtc->event even be set if vc4_state->feed_txp? > diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_reg= s.h > index d6864fa4bd14..744a689751f0 100644 > --- a/drivers/gpu/drm/vc4/vc4_regs.h > +++ b/drivers/gpu/drm/vc4/vc4_regs.h > @@ -330,6 +330,7 @@ > #define SCALER_DISPCTRL0 0x00000040 > # define SCALER_DISPCTRLX_ENABLE BIT(31) > # define SCALER_DISPCTRLX_RESET BIT(30) > + > /* Generates a single frame when VSTART is seen and stops at the last > * pixel read from the FIFO. > */ stray hunk? > +static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, > + struct drm_connector_state *conn_state) > +{ > + struct vc4_txp *txp =3D connector_to_vc4_txp(conn); > + struct drm_gem_cma_object *gem; > + struct drm_display_mode *mode; > + struct drm_framebuffer *fb; > + u32 ctrl =3D TXP_GO | TXP_VSTART_AT_EOF | TXP_EI; > + > + if (WARN_ON(!conn_state->writeback_job || > + !conn_state->writeback_job->fb)) > + return; > + > + mode =3D &conn_state->crtc->state->adjusted_mode; > + fb =3D conn_state->writeback_job->fb; > + > + switch (fb->format->format) { > + case DRM_FORMAT_ARGB8888: > + ctrl |=3D TXP_ALPHA_ENABLE; Optional suggestion: Have the txp_formats[] table be a struct with these register values in it. All my feedback seems really minor, so with whatever components you like integrated, feel free to add: Reviewed-by: Eric Anholt --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAls2l/gACgkQtdYpNtH8 nuh7hw//bjNHn+tYP4TTyTf9sLUcHWGgTCZugSR7/lYxKrscFK/iZ2EU/bXNw8DE LYLycEgdkzZ5qaeI+r7DEMn5IeBZchzDQlfJ64ZPCogMQHHb1LMKb6xfFu0cHAcN lB1oLakymfqH0GqXNFqme0CPJJjg2ks+uxbNgssIlXj3HbP+1eSOCC36n5ikvzRG ZkHAMViOw37wsAh9xE6QtHyWvaiAoZzzu5qHS8kt/+SumT1F/23Fq1rhVaN0LV7l e9amL9MS1zpGIb3ZUNJ04IRIFTLaNcx/n9OBCGCMfE5SsHaf2VHz2PjEdLhye1Yz NPCEfvz5qx1lksoFm/sIbQxdw2xVJOMsT0h47N2xJtbfHLtdeAKnhedAaQawrcfP T4jA3Ml9Wf54P2dHt7gi51nPDHUq1trxzS50YqC/wvXaIKzg21ECl/gTmJXb/YI9 g9tnhk3kUeDNAkd9q+B+tImXdQhWK9eNpadwhXHLNLcElctSKWbKq5bcD+1ROhNq sTHOMtWN7yAlHOgAKOsZqNOjXeVnKf42iCkiiPlqzmkq+rUHsHsNFzSVk/VJ9eqd eoDQW14xk8fFt+JXwyHuOBekvwyfL34qeBWGlcDTtAeGPJBAJm1XCzaGNULaQCyH RL8kNjqpBPv+O1wpyZzBAd3wuCn9X9aXRF1mqWFLd2zpWh+3zK8= =4gYu -----END PGP SIGNATURE----- --=-=-=-- --===============1655891270== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1655891270==--