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

[Eric Anholt]

Brian Starkey <brian.starkey at arm.com> writes:

> 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 at 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?
>
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170613/f67196db/attachment.sig>