From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Starkey Subject: Re: [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block Date: Wed, 14 Jun 2017 09:23:33 +0100 Message-ID: <20170614082332.GA16457@e106950-lin.cambridge.arm.com> References: <1496392332-8722-1-git-send-email-boris.brezillon@free-electrons.com> <1496392332-8722-6-git-send-email-boris.brezillon@free-electrons.com> <20170605112549.GA32480@e106950-lin.cambridge.arm.com> <20170606211300.61e93ec1@bbrezillon> <20170613103403.GA3989@e106950-lin.cambridge.arm.com> <87efunaj4o.fsf@eliezer.anholt.net> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <87efunaj4o.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Eric Anholt Cc: Boris Brezillon , David Airlie , Daniel Vetter , dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, Sean Paul , Gerd Hoffmann , Mark Yao , Shawn Guo , Florian Fainelli , Ray Jui , Scott Branden , bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org, Stephen Warren , Lee Jones , linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , devicetree-u79uwXL29TZNg+MwTxZMZA@public.gmane.org List-Id: devicetree@vger.kernel.org On Tue, Jun 13, 2017 at 10:32:23AM -0700, Eric Anholt wrote: >Brian Starkey 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 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 -- 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