Re: [PATCH v2 7/8] drm/vc4: Add support for the transposer block

Hi Eric,

On Fri, 29 Jun 2018 13:35:04 -0700 Eric Anholt eric@anholt.net wrote:

Boris Brezillon boris.brezillon@bootlin.com writes:

...

From: Boris Brezillon boris.brezillon@free-electrons.com

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 boris.brezillon@free-electrons.com

...

+static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) +{

• struct drm_device *dev = crtc->dev;
• struct vc4_dev *vc4 = to_vc4_dev(dev);
• struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
• struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
• struct drm_display_mode *mode = &crtc->state->adjusted_mode;
• bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE;
• bool debug_dump_regs = 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 == 2) {

• u32 dispctrl;
  

• u32 dsp3_mux;
  

• /*
  

•  * SCALER_DISPCTRL_DSP3 = X, where X < 2 means 'connect DSP3 to
  

•  * FIFO X'.
  

•  * SCALER_DISPCTRL_DSP3 = 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 = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX);
  

• else
  

• 	dsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
  

• /* Reconfigure the DSP3 mux if required. */
  

• dispctrl = HVS_READ(SCALER_DISPCTRL);
  

• if ((dispctrl & SCALER_DISPCTRL_DSP3_MUX_MASK) != dsp3_mux) {
  

• 	dispctrl &= ~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);
  
  

  HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel), SCALER_DISPBKGND_AUTOHS |

@@ -499,6 +539,13 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc, } }

+void vc4_crtc_txp_armed(struct drm_crtc_state *state) +{

• struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
• vc4_state->txp_armed = true;

+}

static void vc4_crtc_update_dlist(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -514,8 +561,11 @@ static void vc4_crtc_update_dlist(struct drm_crtc *crtc) WARN_ON(drm_crtc_vblank_get(crtc) != 0);

spin_lock_irqsave(&dev->event_lock, flags);

• vc4_crtc->event = crtc->state->event;
  

• crtc->state->event = NULL;
  

• if (!vc4_state->feed_txp || vc4_state->txp_armed) {
  

• 	vc4_crtc->event = crtc->state->event;
  

• 	crtc->state->event = NULL;
  

• }
  
  

  HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel), vc4_state->mm.start);

@@ -533,8 +583,8 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);

• struct drm_crtc_state *state = crtc->state;
• struct drm_display_mode *mode = &state->adjusted_mode;

• struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);

• struct drm_display_mode *mode = &crtc->state->adjusted_mode;

  require_hvs_enabled(dev);

@@ -546,15 +596,21 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,

/* 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));
  
  

• /* 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);
  

}

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 *crtc, 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 = 0;

• int ret;

• int ret, i;

  /* 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 *crtc, if (ret) return ret;

• state->no_vblank = false;
• for_each_new_connector_in_state(state->state, conn, conn_state, i) {

• if (conn_state->crtc != 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 == DRM_MODE_CONNECTOR_WRITEBACK) {
  

• 	state->no_vblank = true;
  

• 	vc4_state->feed_txp = true;
  

• }
  

• break;
  

• }
• return 0;

}

@@ -713,7 +787,8 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc)

spin_lock_irqsave(&dev->event_lock, flags); if (vc4_crtc->event &&

•    (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)))) {
  

•    (vc4_state->mm.start == 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_regs.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 = connector_to_vc4_txp(conn);
• struct drm_gem_cma_object *gem;
• struct drm_display_mode *mode;
• struct drm_framebuffer *fb;
• u32 ctrl = TXP_GO | TXP_VSTART_AT_EOF | TXP_EI;
• if (WARN_ON(!conn_state->writeback_job ||

•     !conn_state->writeback_job->fb))
  

• return;
  

• mode = &conn_state->crtc->state->adjusted_mode;
• fb = conn_state->writeback_job->fb;
• switch (fb->format->format) {
• case DRM_FORMAT_ARGB8888:

• ctrl |= 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:

Will fix all the things you pointed in your review.

Thanks,

Boris