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

[Brian Starkey <brian.starkey-5wv7dgnIgG8@public.gmane.org>]

Hi Boris,

Sorry lost track of this thread.


On Tue, Jun 06, 2017 at 09:13:00PM +0200, Boris Brezillon wrote:
>Hi Brian,
>
>Le Mon, 5 Jun 2017 12:25:50 +0100,
>Brian Starkey <brian.starkey-5wv7dgnIgG8@public.gmane.org> 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?

I'm not sure how widely relied upon it is, but I'd expect that there's
a requirement to make sure DRM_IOCTL_WAIT_VBLANK works. I _think_ that
means the CRTC should generate events at vrefresh rate if there's a
wait request outstanding that depends on that.

It's not exactly documented though.

>
>>
>> >+		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.

Agree, sounds like a very good fit.

Cheers,
-Brian

>
>Thanks for the review,
>
>Boris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html