Re: [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Brian,

Le Mon, 5 Jun 2017 12:25:50 +0100,
Brian Starkey <brian.starkey@arm.com> a écrit :

> Hi Boris,
> 
> I can't speak for the HW-specific details, but the writeback part
> looks pretty good (and familiar ;-) to me. There certainly seems to be
> some scope for code-sharing there, but I think it's fine not to do it
> yet.

Agreed.

> 
> I've a further query below about the handling of CRTC events.
> 

[...]

> >+
> >+void vc4_txp_atomic_commit(struct drm_device *dev,
> >+			   struct drm_atomic_state *old_state)
> >+{
> >+	struct vc4_dev *vc4 = to_vc4_dev(dev);
> >+	struct vc4_txp *txp = vc4->txp;
> >+	struct drm_connector_state *conn_state = txp->connector.base.state;
> >+	struct drm_display_mode *mode;
> >+	struct drm_gem_cma_object *gem;
> >+	struct drm_framebuffer *fb;
> >+	u32 ctrl = TXP_GO | TXP_EI;
> >+
> >+	/* Writeback connector is disabled, nothing to do. */
> >+	if (!conn_state->crtc)
> >+		return;
> >+
> >+	/* Writeback connector is enabled, but has no FB assigned to it. Fake a
> >+	 * vblank event to complete ->flip_done.
> >+	 */
> >+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> >+		vc4_crtc_eof_event(conn_state->crtc);  
> 
> I'm not sure about hiding away the one-shot thing like this. If the
> CRTC remains "active" from the API point of view, I'd expect it to be
> able to keep generating VBLANK events.
> 
> I don't know how to do if, but if there were some notion of
> "auto-disabling" CRTCs then this quirk would go away, and you'd also
> be able to enforce that the CRTC can't be enabled without a writeback
> framebuffer.

I don't have a strong opinion on "auto-disabling CRTC" vs "fake VBLANK
events". Note that I'm already faking a VBLANK event when activating
writeback, because there's no such concept at the HVS/TXP level. We
do have EOF (End Of Frame) and writeback done events, but these are
quite independent from the VBLANK events generated by the pixelvalve
block (the block responsible for generating the HSYNC/VSYNC signals).

> 
> I'm also not sure how (if?) this works today with a CRTC driving a DSI
> command-mode panel. Does the CRTC keep generating VBLANKs even when
> there are no updates?

I don't know. Is this mandatory to have VBLANK events? I mean, the
core is using VBLANK events to detect when page flips have been done,
that is, when old framebuffers are unused and new ones started to be
fetched by the CRTC, but on some HW, VBLANK is not the only way to
detect such situations. The question is, are there other situations
where VBLANK events are required, or is there an implicit rule stating
that a CRTC has to generate VBLANK events at a vrefresh rate even when
the display is actually not updated when nothing changes?

> 
> >+		return;
> >+	}
> >+
> >+	fb = conn_state->writeback_job->fb;
> >+
> >+	switch (fb->format->format) {
> >+	case DRM_FORMAT_ARGB8888:
> >+		ctrl |= TXP_ALPHA_ENABLE;
> >+	case DRM_FORMAT_XRGB8888:
> >+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_ARGB8888, TXP_FORMAT) |
> >+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
> >+		break;
> >+
> >+	case DRM_FORMAT_ABGR8888:
> >+		ctrl |= TXP_ALPHA_ENABLE;
> >+	case DRM_FORMAT_XBGR8888:
> >+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_ABGR8888, TXP_FORMAT) |
> >+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
> >+		break;
> >+
> >+	case DRM_FORMAT_RGBA8888:
> >+		ctrl |= TXP_ALPHA_ENABLE;
> >+	case DRM_FORMAT_RGBX8888:
> >+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_RGBA8888, TXP_FORMAT) |
> >+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
> >+		break;
> >+
> >+	case DRM_FORMAT_BGRA8888:
> >+		ctrl |= TXP_ALPHA_ENABLE;
> >+	case DRM_FORMAT_BGRX8888:
> >+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_BGRA8888, TXP_FORMAT) |
> >+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
> >+		break;
> >+
> >+	case DRM_FORMAT_BGR888:
> >+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_BGR888, TXP_FORMAT) |
> >+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
> >+		break;
> >+
> >+	case DRM_FORMAT_RGB888:
> >+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_RGB888, TXP_FORMAT) |
> >+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
> >+		break;
> >+	default:
> >+		WARN_ON(1);
> >+		return;
> >+	}
> >+
> >+	if (!(ctrl & TXP_ALPHA_ENABLE))
> >+		ctrl |= TXP_ALPHA_INVERT;
> >+
> >+	mode = &conn_state->crtc->state->adjusted_mode;
> >+	gem = drm_fb_cma_get_gem_obj(fb, 0);
> >+	TXP_WRITE(TXP_DST_PTR, gem->paddr + fb->offsets[0]);
> >+	TXP_WRITE(TXP_DST_PITCH, fb->pitches[0]);
> >+	TXP_WRITE(TXP_DIM,
> >+		  VC4_SET_FIELD(mode->hdisplay, TXP_WIDTH) |
> >+		  VC4_SET_FIELD(mode->vdisplay, TXP_HEIGHT));
> >+
> >+	TXP_WRITE(TXP_DST_CTRL, ctrl);
> >+
> >+	drm_writeback_queue_job(&txp->connector, conn_state->writeback_job);
> >+	conn_state->writeback_job = NULL;
> >+}
> >+
> >+bool vc4_is_txp_encoder(struct drm_device *dev, struct drm_encoder *encoder)
> >+{
> >+	struct vc4_dev *vc4 = to_vc4_dev(dev);
> >+
> >+	return encoder == &vc4->txp->connector.encoder;
> >+}
> >+
> >+static const struct drm_connector_helper_funcs vc4_txp_connector_helper_funcs = {
> >+	.get_modes = vc4_txp_connector_get_modes,
> >+	.mode_valid = vc4_txp_connector_mode_valid,
> >+	.atomic_check = vc4_txp_connector_atomic_check,  
> 
> huh. This hook didn't exist when I did Mali-DP. I wonder if we should
> switch Mali-DP to it too. Do you know if the semantics are any
> different from the encoder atomic_check?

It seems that connector->atomic_check() is called even when the
CRTC -> encoder -> connector routing did not change if at least one of
the property attached to the connector was changed, which is a good fit
for writeback connectors ;-).
Also, by using connector->atomic_check() I get rid of the
dummy encoder_helper_funcs object.

Thanks for the review,

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