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

From: Boris Brezillon <boris.brezillon@bootlin.com>
To: Eric Anholt <eric@anholt.net>
Cc: David Airlie <airlied@linux.ie>,
	Boris Brezillon <boris.brezillon@free-electrons.com>,
	Liviu Dudau <Liviu.Dudau@arm.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 7/8] drm/vc4: Add support for the transposer block
Date: Mon, 2 Jul 2018 12:19:33 +0200	[thread overview]
Message-ID: <20180702121933.22fb322b@bbrezillon> (raw)
In-Reply-To: <87d0w9nxfr.fsf@anholt.net>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel