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

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 7545 bytes --]

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:

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel