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

[Brian Starkey]

On Tue, Jun 13, 2017 at 10:32:23AM -0700, Eric Anholt wrote:
>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.

I'm not sure how you'd plumb that in, but the behaviour sounds OK to
me. We can write-back at the same time as scanout to the display from
the same CRTC, so we'd not want to return -EINVAL in that case.

-Brian