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

Brian Starkey [off-list ref] writes:

Hi Boris,

Sorry lost track of this thread.


On Tue, Jun 06, 2017 at 09:13:00PM +0200, Boris Brezillon wrote:

quoted

Hi Brian,

Le Mon, 5 Jun 2017 12:25:50 +0100,
Brian Starkey [off-list ref] a écrit :

quoted

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.

quoted

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

[...]

quoted

quoted

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

quoted

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.

In our case, there's no vrefresh rate, though.  Just completion.

I've been trying to come up with a usecase for vblank events on
writeack, and the best I have is: to enable VNC-style capture of a
complete hardware rendering stack, we could run modesetting against the
writeback connector and do one-shot writebacks when damage comes in.
You would want GL apps to be throttled to the frame capture rate, so X
needs to implement waits for at least a swapinterval of 1 (I don't see
how greater than 1 would make any sense)

However, given that you'd be triggering writebacks on damage, and using
the fence to trigger the next wait for damage and writeback already, I
think you'd use that set of code for Present/DRI2 swapinterval support,
not the current vblank path.

My current inclination would be to throw -EINVAL for userspace vblank
requests on writeback conncetor pipes.