* [PATCH 0/7] drm/vc4: Add writeback support to the VC4 driver @ 2017-06-02 8:32 Boris Brezillon 2017-06-02 8:32 ` [PATCH 1/7] drm: Add drm_atomic_helper_wait_for_flip_done() Boris Brezillon [not found] ` <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 0 siblings, 2 replies; 26+ messages in thread From: Boris Brezillon @ 2017-06-02 8:32 UTC (permalink / raw) To: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Boris Brezillon, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey Hello, This series adds a driver for the TXP (transposer) block which is providing writeback support. Note that adding support for this IP required reworking a bit how the VC4 driver handles commits (vc4_kms.c). I also had to support a new use case in vc4_crtc.c because the TXP IP is completely bypassing the pixelvalve element (it's taking its data directly from the HVS FIFO). First patch is adding a new helper to the core to wait for all pageflips to be done. It can be used as a replacement for drm_atomic_helper_wait_for_vblanks() when your HW is only generating a single frame (AKA oneshot mode), which implies a single VBLANK event. In this situation, if drm_atomic_helper_wait_for_vblanks() is called after the VBLANK event has been sent, we face a timeout because vblank counter (used to detect when vblanks are happening) is never updated again. Daniel, I'm not sure this is exactly what you had in mind when you proposed this approach, so feel free to comment on this patch. Patches 2 to 4 are modifying the current logic to make it work with the TXP block. Patches 5 and 6 are adding code to support the TXP and documenting the bindings. And the last patch is defining the TXP block in bcm283x.dtsi. This series depends on Brian Starkey/Liviu Dudau patchset [1] and has been tested on a RPi2 with the kms_writeback tests provided here [2]. Regards, Boris [1]https://lists.freedesktop.org/archives/dri-devel/2017-May/141796.html [2]https://github.com/dliviu/intel-gpu-tools/tree/master Boris Brezillon (7): drm: Add drm_atomic_helper_wait_for_flip_done() drm/vc4: Fix vblank handling drm/vc4: Mimic drm_atomic_helper_commit() behavior drm/vc4: Use drm_atomic_helper_wait_for_flip_done() drm/vc4: Add support for the TXP (transposer) block dt-bindings: Document the VC4 TXP module nodes. ARM: dts: bcm283x: Add Transposer block .../devicetree/bindings/display/brcm,bcm-vc4.txt | 12 + arch/arm/boot/dts/bcm283x.dtsi | 6 + drivers/gpu/drm/drm_atomic_helper.c | 30 ++ drivers/gpu/drm/vc4/Makefile | 1 + drivers/gpu/drm/vc4/vc4_crtc.c | 206 +++++++-- drivers/gpu/drm/vc4/vc4_debugfs.c | 1 + drivers/gpu/drm/vc4/vc4_drv.c | 1 + drivers/gpu/drm/vc4/vc4_drv.h | 10 + drivers/gpu/drm/vc4/vc4_kms.c | 47 +- drivers/gpu/drm/vc4/vc4_regs.h | 1 + drivers/gpu/drm/vc4/vc4_txp.c | 509 +++++++++++++++++++++ include/drm/drm_atomic_helper.h | 3 + 12 files changed, 755 insertions(+), 72 deletions(-) create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c -- 2.7.4 -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/7] drm: Add drm_atomic_helper_wait_for_flip_done() 2017-06-02 8:32 [PATCH 0/7] drm/vc4: Add writeback support to the VC4 driver Boris Brezillon @ 2017-06-02 8:32 ` Boris Brezillon [not found] ` <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 1 sibling, 0 replies; 26+ messages in thread From: Boris Brezillon @ 2017-06-02 8:32 UTC (permalink / raw) To: David Airlie, Daniel Vetter, dri-devel, Boris Brezillon, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo Cc: Mark Rutland, devicetree, Ian Campbell, Florian Fainelli, Pawel Moll, Scott Branden, Stephen Warren, Ray Jui, Eben Upton, Lee Jones, Liviu Dudau, Rob Herring, bcm-kernel-feedback-list, linux-rpi-kernel, Kumar Gala, Hollingworth, Gordon, Cobley, Dom Add an helper to wait for all page flips of an atomic state to be done. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 3 +++ 2 files changed, 33 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fa2d8d7a602a..11f2d644771b 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1166,6 +1166,36 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks); /** + * drm_atomic_helper_wait_for_flip_done - wait for all page flips to be done + * @state: atomic state object + * + * Helper to, after atomic commit, wait for page flips on all effected + * crtcs (ie. before cleaning up old framebuffers using + * drm_atomic_helper_cleanup_planes()). + */ +void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev, + struct drm_atomic_state *state) +{ + struct drm_crtc_state *crtc_state; + struct drm_crtc *crtc; + int i; + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + struct drm_crtc_commit *commit = state->crtcs[i].commit; + int ret; + + if (!commit) + continue; + + ret = wait_for_completion_timeout(&commit->flip_done, 10 * HZ); + if (ret == 0) + DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", + crtc->base.id, crtc->name); + } +} +EXPORT_SYMBOL(drm_atomic_helper_wait_for_flip_done); + +/** * drm_atomic_helper_commit_tail - commit atomic update to hardware * @old_state: atomic state object with old state structures * diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index f0a8678ae98e..7192e7ac117f 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -52,6 +52,9 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev, void drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, struct drm_atomic_state *old_state); +void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev, + struct drm_atomic_state *state); + void drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, struct drm_atomic_state *old_state); -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 26+ messages in thread
[parent not found: <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* [PATCH 2/7] drm/vc4: Fix vblank handling [not found] ` <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2017-06-02 8:32 ` Boris Brezillon [not found] ` <1496392332-8722-3-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-02 8:32 ` [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior Boris Brezillon ` (4 subsequent siblings) 5 siblings, 1 reply; 26+ messages in thread From: Boris Brezillon @ 2017-06-02 8:32 UTC (permalink / raw) To: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Boris Brezillon, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey There are two problems related to VBLANK handling in the current CRTC driver: * VBLANK events are missed when the CRTC is being disabled because the driver does not wait till the end of the frame before stopping the HVS and PV blocks. In this case, we should explicitly issue a VBLANK event if there's one waiting * when we are enabling a CRTC, drm_crtc_vblank_get() is called before drm_crtc_vblank_on(), which is not supposed to happen (hence the WARN_ON() in the code). To solve the problem, we delay the 'update display list' operation after the CRTC is actually enabled. Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --- drivers/gpu/drm/vc4/vc4_crtc.c | 80 ++++++++++++++++++++++++++++++------------ 1 file changed, 57 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index d86c8cce3182..df1d81533076 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -528,6 +528,47 @@ static void vc4_crtc_disable(struct drm_crtc *crtc) WARN_ON_ONCE((HVS_READ(SCALER_DISPSTATX(chan)) & (SCALER_DISPSTATX_FULL | SCALER_DISPSTATX_EMPTY)) != SCALER_DISPSTATX_EMPTY); + + /* + * Make sure we issue a vblank event after disabling the CRTC if + * someone was waiting it. + */ + if (crtc->state->event) { + unsigned long flags; + + spin_lock_irqsave(&dev->event_lock, flags); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + crtc->state->event = NULL; + spin_unlock_irqrestore(&dev->event_lock, flags); + } +} + +static void vc4_crtc_update_dlist(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + struct vc4_dev *vc4 = to_vc4_dev(dev); + struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); + struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); + + if (crtc->state->event) { + unsigned long flags; + + crtc->state->event->pipe = drm_crtc_index(crtc); + + WARN_ON(drm_crtc_vblank_get(crtc) != 0); + + spin_lock_irqsave(&dev->event_lock, flags); + vc4_crtc->event = crtc->state->event; + crtc->state->event = NULL; + + HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel), + vc4_state->mm.start); + + spin_unlock_irqrestore(&dev->event_lock, flags); + } else { + HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel), + vc4_state->mm.start); + } } static void vc4_crtc_enable(struct drm_crtc *crtc) @@ -540,6 +581,12 @@ static void vc4_crtc_enable(struct drm_crtc *crtc) require_hvs_enabled(dev); + /* Enable vblank irq handling before crtc is started otherwise + * drm_crtc_get_vblank() fails in vc4_crtc_update_dlist(). + */ + drm_crtc_vblank_on(crtc); + vc4_crtc_update_dlist(crtc); + /* Turn on the scaler, which will wait for vstart to start * compositing. */ @@ -551,9 +598,6 @@ static void vc4_crtc_enable(struct drm_crtc *crtc) /* Turn on the pixel valve, which will emit the vstart signal. */ CRTC_WRITE(PV_V_CONTROL, CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN); - - /* Enable vblank irq handling after crtc is started. */ - drm_crtc_vblank_on(crtc); } static bool vc4_crtc_mode_fixup(struct drm_crtc *crtc, @@ -608,7 +652,6 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc, { struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); - struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); struct drm_plane *plane; bool debug_dump_regs = false; @@ -630,25 +673,16 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc, WARN_ON_ONCE(dlist_next - dlist_start != vc4_state->mm.size); - if (crtc->state->event) { - unsigned long flags; - - crtc->state->event->pipe = drm_crtc_index(crtc); - - WARN_ON(drm_crtc_vblank_get(crtc) != 0); - - spin_lock_irqsave(&dev->event_lock, flags); - vc4_crtc->event = crtc->state->event; - crtc->state->event = NULL; - - HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel), - vc4_state->mm.start); - - spin_unlock_irqrestore(&dev->event_lock, flags); - } else { - HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel), - vc4_state->mm.start); - } + /* + * Only update DISPLIST if the CRTC was already running and is not + * being disabled. + * vc4_crtc_enable() takes care of updating the dlist just after + * re-enabling VBLANK interrupts and before enabling the engine. + * If the CRTC is being disabled, there's no point in updating this + * information. + */ + if (crtc->state->active && old_state->active) + vc4_crtc_update_dlist(crtc); if (debug_dump_regs) { DRM_INFO("CRTC %d HVS after:\n", drm_crtc_index(crtc)); -- 2.7.4 -- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
[parent not found: <1496392332-8722-3-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 2/7] drm/vc4: Fix vblank handling [not found] ` <1496392332-8722-3-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2017-06-15 23:30 ` Eric Anholt 2017-06-16 7:47 ` Boris Brezillon 0 siblings, 1 reply; 26+ messages in thread From: Eric Anholt @ 2017-06-15 23:30 UTC (permalink / raw) To: Boris Brezillon, David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey [-- Attachment #1: Type: text/plain, Size: 1042 bytes --] Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > There are two problems related to VBLANK handling in the current CRTC > driver: > > * VBLANK events are missed when the CRTC is being disabled because the > driver does not wait till the end of the frame before stopping the > HVS and PV blocks. In this case, we should explicitly issue a VBLANK > event if there's one waiting > * when we are enabling a CRTC, drm_crtc_vblank_get() is called before > drm_crtc_vblank_on(), which is not supposed to happen (hence the > WARN_ON() in the code). To solve the problem, we delay the 'update > display list' operation after the CRTC is actually enabled. Does drm_crtc_vblank_get() actually fail when drm_crtc_vblank_on() hasn't been called? The code is a bit twisty, but it looked like we would track the get refcount and then turn on the interrupt once drm_crtc_vblank_on() was called. The first change of the two seems straightforward and good, though (and possible to separate, right?). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/7] drm/vc4: Fix vblank handling 2017-06-15 23:30 ` Eric Anholt @ 2017-06-16 7:47 ` Boris Brezillon 0 siblings, 0 replies; 26+ messages in thread From: Boris Brezillon @ 2017-06-16 7:47 UTC (permalink / raw) To: Eric Anholt Cc: Mark Rutland, Lee Jones, Liviu Dudau, dri-devel, Gerd Hoffmann, Florian Fainelli, Eben Upton, bcm-kernel-feedback-list, devicetree, Pawel Moll, Stephen Warren, Ray Jui, Rob Herring, linux-rpi-kernel, Hollingworth, Gordon, Cobley, Dom, Ian Campbell, Scott Branden, Kumar Gala, Shawn Guo On Thu, 15 Jun 2017 16:30:58 -0700 Eric Anholt <eric@anholt.net> wrote: > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > > > There are two problems related to VBLANK handling in the current CRTC > > driver: > > > > * VBLANK events are missed when the CRTC is being disabled because the > > driver does not wait till the end of the frame before stopping the > > HVS and PV blocks. In this case, we should explicitly issue a VBLANK > > event if there's one waiting > > * when we are enabling a CRTC, drm_crtc_vblank_get() is called before > > drm_crtc_vblank_on(), which is not supposed to happen (hence the > > WARN_ON() in the code). To solve the problem, we delay the 'update > > display list' operation after the CRTC is actually enabled. > > Does drm_crtc_vblank_get() actually fail when drm_crtc_vblank_on() > hasn't been called? The code is a bit twisty, but it looked like we > would track the get refcount and then turn on the interrupt once > drm_crtc_vblank_on() was called. I thought I had faced the WARN_ON() backtrace while developing the TXP block, but I can't reproduce it (even after reverting these changes), so it was probably caused by something else. > > The first change of the two seems straightforward and good, though (and > possible to separate, right?). I'll prepare a new patch containing only the first change. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior [not found] ` <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-02 8:32 ` [PATCH 2/7] drm/vc4: Fix vblank handling Boris Brezillon @ 2017-06-02 8:32 ` Boris Brezillon [not found] ` <1496392332-8722-4-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-02 8:32 ` [PATCH 4/7] drm/vc4: Use drm_atomic_helper_wait_for_flip_done() Boris Brezillon ` (3 subsequent siblings) 5 siblings, 1 reply; 26+ messages in thread From: Boris Brezillon @ 2017-06-02 8:32 UTC (permalink / raw) To: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Boris Brezillon, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey The VC4 KMS driver is implementing its own ->atomic_commit() but there are a few generic helpers we can use instead of open-coding the logic. Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --- drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index ad7925a9e0ea..f229abc0991b 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) struct drm_device *dev = state->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); + drm_atomic_helper_wait_for_fences(dev, state, false); + + drm_atomic_helper_wait_for_dependencies(state); + drm_atomic_helper_commit_modeset_disables(dev, state); drm_atomic_helper_commit_planes(dev, state, 0); @@ -57,10 +61,14 @@ vc4_atomic_complete_commit(struct vc4_commit *c) */ state->legacy_cursor_update = false; + drm_atomic_helper_commit_hw_done(state); + drm_atomic_helper_wait_for_vblanks(dev, state); drm_atomic_helper_cleanup_planes(dev, state); + drm_atomic_helper_commit_cleanup_done(state); + drm_atomic_state_put(state); up(&vc4->async_modeset); @@ -117,32 +125,10 @@ static int vc4_atomic_commit(struct drm_device *dev, if (!c) return -ENOMEM; - /* Make sure that any outstanding modesets have finished. */ - if (nonblock) { - struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; - unsigned long flags; - bool busy = false; - - /* - * If there's an undispatched event to send then we're - * obviously still busy. If there isn't, then we can - * unconditionally wait for the semaphore because it - * shouldn't be contended (for long). - * - * This is to prevent a race where queuing a new flip - * from userspace immediately on receipt of an event - * beats our clean-up and returns EBUSY. - */ - spin_lock_irqsave(&dev->event_lock, flags); - for_each_crtc_in_state(state, crtc, crtc_state, i) - busy |= vc4_event_pending(crtc); - spin_unlock_irqrestore(&dev->event_lock, flags); - if (busy) { - kfree(c); - return -EBUSY; - } - } + ret = drm_atomic_helper_setup_commit(state, nonblock); + if (ret) + return ret; + ret = down_interruptible(&vc4->async_modeset); if (ret) { kfree(c); -- 2.7.4 -- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
[parent not found: <1496392332-8722-4-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior [not found] ` <1496392332-8722-4-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2017-06-06 20:27 ` Eric Anholt [not found] ` <87k24orheq.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 0 siblings, 1 reply; 26+ messages in thread From: Eric Anholt @ 2017-06-06 20:27 UTC (permalink / raw) To: Boris Brezillon, David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey [-- Attachment #1: Type: text/plain, Size: 3061 bytes --] Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > The VC4 KMS driver is implementing its own ->atomic_commit() but there > are a few generic helpers we can use instead of open-coding the logic. > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> > --- > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- > 1 file changed, 12 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index ad7925a9e0ea..f229abc0991b 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > struct drm_device *dev = state->dev; > struct vc4_dev *vc4 = to_vc4_dev(dev); > > + drm_atomic_helper_wait_for_fences(dev, state, false); > + > + drm_atomic_helper_wait_for_dependencies(state); With this wait_for_fences() addition and the reservation stuff that landed, I think we can rip out the "seqno cb" in vc4, and just use drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you see anything missing, with that? > + > drm_atomic_helper_commit_modeset_disables(dev, state); > > drm_atomic_helper_commit_planes(dev, state, 0); > @@ -57,10 +61,14 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > */ > state->legacy_cursor_update = false; > > + drm_atomic_helper_commit_hw_done(state); > + > drm_atomic_helper_wait_for_vblanks(dev, state); > > drm_atomic_helper_cleanup_planes(dev, state); > > + drm_atomic_helper_commit_cleanup_done(state); > + > drm_atomic_state_put(state); > > up(&vc4->async_modeset); > @@ -117,32 +125,10 @@ static int vc4_atomic_commit(struct drm_device *dev, > if (!c) > return -ENOMEM; > > - /* Make sure that any outstanding modesets have finished. */ > - if (nonblock) { > - struct drm_crtc *crtc; > - struct drm_crtc_state *crtc_state; > - unsigned long flags; > - bool busy = false; > - > - /* > - * If there's an undispatched event to send then we're > - * obviously still busy. If there isn't, then we can > - * unconditionally wait for the semaphore because it > - * shouldn't be contended (for long). > - * > - * This is to prevent a race where queuing a new flip > - * from userspace immediately on receipt of an event > - * beats our clean-up and returns EBUSY. > - */ > - spin_lock_irqsave(&dev->event_lock, flags); > - for_each_crtc_in_state(state, crtc, crtc_state, i) > - busy |= vc4_event_pending(crtc); > - spin_unlock_irqrestore(&dev->event_lock, flags); > - if (busy) { > - kfree(c); > - return -EBUSY; > - } > - } > + ret = drm_atomic_helper_setup_commit(state, nonblock); > + if (ret) > + return ret; > + Looks like vc4_event_pending() should be garbage-collected with this commit. > ret = down_interruptible(&vc4->async_modeset); > if (ret) { > kfree(c); > -- > 2.7.4 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
[parent not found: <87k24orheq.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org>]
* Re: [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior [not found] ` <87k24orheq.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> @ 2017-06-06 20:48 ` Boris Brezillon 2017-06-07 17:46 ` Eric Anholt 2017-06-13 10:10 ` Boris Brezillon 1 sibling, 1 reply; 26+ messages in thread From: Boris Brezillon @ 2017-06-06 20:48 UTC (permalink / raw) To: Eric Anholt Cc: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingwort On Tue, 06 Jun 2017 13:27:09 -0700 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote: > Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > > > The VC4 KMS driver is implementing its own ->atomic_commit() but there > > are a few generic helpers we can use instead of open-coding the logic. > > > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> > > --- > > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- > > 1 file changed, 12 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > > index ad7925a9e0ea..f229abc0991b 100644 > > --- a/drivers/gpu/drm/vc4/vc4_kms.c > > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > > struct drm_device *dev = state->dev; > > struct vc4_dev *vc4 = to_vc4_dev(dev); > > > > + drm_atomic_helper_wait_for_fences(dev, state, false); > > + > > + drm_atomic_helper_wait_for_dependencies(state); > > With this wait_for_fences() addition and the reservation stuff that > landed, I think we can rip out the "seqno cb" in vc4, and just use > drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you > see anything missing, with that? I can't tell. I haven't dig enough to understand what this seqno cb was used for :-), but Daniel was suggesting the same thing. I'll try to better understand what seqno cb does and if it's all safe to get rid of it and use the standard helpers. > > > + > > drm_atomic_helper_commit_modeset_disables(dev, state); > > > > drm_atomic_helper_commit_planes(dev, state, 0); > > @@ -57,10 +61,14 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > > */ > > state->legacy_cursor_update = false; > > > > + drm_atomic_helper_commit_hw_done(state); > > + > > drm_atomic_helper_wait_for_vblanks(dev, state); > > > > drm_atomic_helper_cleanup_planes(dev, state); > > > > + drm_atomic_helper_commit_cleanup_done(state); > > + > > drm_atomic_state_put(state); > > > > up(&vc4->async_modeset); > > @@ -117,32 +125,10 @@ static int vc4_atomic_commit(struct drm_device *dev, > > if (!c) > > return -ENOMEM; > > > > - /* Make sure that any outstanding modesets have finished. */ > > - if (nonblock) { > > - struct drm_crtc *crtc; > > - struct drm_crtc_state *crtc_state; > > - unsigned long flags; > > - bool busy = false; > > - > > - /* > > - * If there's an undispatched event to send then we're > > - * obviously still busy. If there isn't, then we can > > - * unconditionally wait for the semaphore because it > > - * shouldn't be contended (for long). > > - * > > - * This is to prevent a race where queuing a new flip > > - * from userspace immediately on receipt of an event > > - * beats our clean-up and returns EBUSY. > > - */ > > - spin_lock_irqsave(&dev->event_lock, flags); > > - for_each_crtc_in_state(state, crtc, crtc_state, i) > > - busy |= vc4_event_pending(crtc); > > - spin_unlock_irqrestore(&dev->event_lock, flags); > > - if (busy) { > > - kfree(c); > > - return -EBUSY; > > - } > > - } > > + ret = drm_atomic_helper_setup_commit(state, nonblock); > > + if (ret) > > + return ret; > > + > > Looks like vc4_event_pending() should be garbage-collected with this > commit. Indeed. Thanks for the review. -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior 2017-06-06 20:48 ` Boris Brezillon @ 2017-06-07 17:46 ` Eric Anholt [not found] ` <87o9tzg06z.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 0 siblings, 1 reply; 26+ messages in thread From: Eric Anholt @ 2017-06-07 17:46 UTC (permalink / raw) To: Boris Brezillon Cc: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingwort [-- Attachment #1: Type: text/plain, Size: 1978 bytes --] Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > On Tue, 06 Jun 2017 13:27:09 -0700 > Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote: > >> Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: >> >> > The VC4 KMS driver is implementing its own ->atomic_commit() but there >> > are a few generic helpers we can use instead of open-coding the logic. >> > >> > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> >> > --- >> > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- >> > 1 file changed, 12 insertions(+), 26 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c >> > index ad7925a9e0ea..f229abc0991b 100644 >> > --- a/drivers/gpu/drm/vc4/vc4_kms.c >> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c >> > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) >> > struct drm_device *dev = state->dev; >> > struct vc4_dev *vc4 = to_vc4_dev(dev); >> > >> > + drm_atomic_helper_wait_for_fences(dev, state, false); >> > + >> > + drm_atomic_helper_wait_for_dependencies(state); >> >> With this wait_for_fences() addition and the reservation stuff that >> landed, I think we can rip out the "seqno cb" in vc4, and just use >> drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you >> see anything missing, with that? > > I can't tell. I haven't dig enough to understand what this seqno cb was > used for :-), but Daniel was suggesting the same thing. I'll try to > better understand what seqno cb does and if it's all safe to get rid of > it and use the standard helpers. The seqno cb was the thing for stalling the modeset until V3D was done rendering to the planes. The wait_for_fences() does the same thing using generic dmabuf reservations, so the seqno cb isn't needed any more. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
[parent not found: <87o9tzg06z.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org>]
* Re: [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior [not found] ` <87o9tzg06z.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> @ 2017-06-15 23:33 ` Eric Anholt [not found] ` <87d1a4u8qw.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 0 siblings, 1 reply; 26+ messages in thread From: Eric Anholt @ 2017-06-15 23:33 UTC (permalink / raw) To: Boris Brezillon Cc: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingwort [-- Attachment #1: Type: text/plain, Size: 2342 bytes --] Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> writes: > [ Unknown signature status ] > Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > >> On Tue, 06 Jun 2017 13:27:09 -0700 >> Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote: >> >>> Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: >>> >>> > The VC4 KMS driver is implementing its own ->atomic_commit() but there >>> > are a few generic helpers we can use instead of open-coding the logic. >>> > >>> > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> >>> > --- >>> > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- >>> > 1 file changed, 12 insertions(+), 26 deletions(-) >>> > >>> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c >>> > index ad7925a9e0ea..f229abc0991b 100644 >>> > --- a/drivers/gpu/drm/vc4/vc4_kms.c >>> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c >>> > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) >>> > struct drm_device *dev = state->dev; >>> > struct vc4_dev *vc4 = to_vc4_dev(dev); >>> > >>> > + drm_atomic_helper_wait_for_fences(dev, state, false); >>> > + >>> > + drm_atomic_helper_wait_for_dependencies(state); >>> >>> With this wait_for_fences() addition and the reservation stuff that >>> landed, I think we can rip out the "seqno cb" in vc4, and just use >>> drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you >>> see anything missing, with that? >> >> I can't tell. I haven't dig enough to understand what this seqno cb was >> used for :-), but Daniel was suggesting the same thing. I'll try to >> better understand what seqno cb does and if it's all safe to get rid of >> it and use the standard helpers. > > The seqno cb was the thing for stalling the modeset until V3D was done > rendering to the planes. The wait_for_fences() does the same thing > using generic dmabuf reservations, so the seqno cb isn't needed any > more. Oh, we've got another user of the seqno cb (async flips), anyway, so we can't delete it quite yet. I've pushed this patch since it's a clear improvement (less code, waits for fences from other devices if any). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
[parent not found: <87d1a4u8qw.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org>]
* Re: [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior [not found] ` <87d1a4u8qw.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> @ 2017-06-16 7:19 ` Boris Brezillon 2017-06-21 17:25 ` Eric Anholt 0 siblings, 1 reply; 26+ messages in thread From: Boris Brezillon @ 2017-06-16 7:19 UTC (permalink / raw) To: Eric Anholt Cc: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingwort On Thu, 15 Jun 2017 16:33:11 -0700 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote: > Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> writes: > > > [ Unknown signature status ] > > Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > > > >> On Tue, 06 Jun 2017 13:27:09 -0700 > >> Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote: > >> > >>> Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > >>> > >>> > The VC4 KMS driver is implementing its own ->atomic_commit() but there > >>> > are a few generic helpers we can use instead of open-coding the logic. > >>> > > >>> > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> > >>> > --- > >>> > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- > >>> > 1 file changed, 12 insertions(+), 26 deletions(-) > >>> > > >>> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > >>> > index ad7925a9e0ea..f229abc0991b 100644 > >>> > --- a/drivers/gpu/drm/vc4/vc4_kms.c > >>> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > >>> > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > >>> > struct drm_device *dev = state->dev; > >>> > struct vc4_dev *vc4 = to_vc4_dev(dev); > >>> > > >>> > + drm_atomic_helper_wait_for_fences(dev, state, false); > >>> > + > >>> > + drm_atomic_helper_wait_for_dependencies(state); > >>> > >>> With this wait_for_fences() addition and the reservation stuff that > >>> landed, I think we can rip out the "seqno cb" in vc4, and just use > >>> drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you > >>> see anything missing, with that? > >> > >> I can't tell. I haven't dig enough to understand what this seqno cb was > >> used for :-), but Daniel was suggesting the same thing. I'll try to > >> better understand what seqno cb does and if it's all safe to get rid of > >> it and use the standard helpers. > > > > The seqno cb was the thing for stalling the modeset until V3D was done > > rendering to the planes. The wait_for_fences() does the same thing > > using generic dmabuf reservations, so the seqno cb isn't needed any > > more. > > Oh, we've got another user of the seqno cb (async flips), anyway, so we > can't delete it quite yet. I've pushed this patch since it's a clear > improvement (less code, waits for fences from other devices if any). Ok. Did you fix the leak before applying? -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior 2017-06-16 7:19 ` Boris Brezillon @ 2017-06-21 17:25 ` Eric Anholt 0 siblings, 0 replies; 26+ messages in thread From: Eric Anholt @ 2017-06-21 17:25 UTC (permalink / raw) To: Boris Brezillon Cc: Mark Rutland, Lee Jones, Liviu Dudau, dri-devel, Gerd Hoffmann, Florian Fainelli, Eben Upton, bcm-kernel-feedback-list, devicetree, Pawel Moll, Stephen Warren, Ray Jui, Rob Herring, linux-rpi-kernel, Hollingworth, Gordon, Cobley, Dom, Ian Campbell, Scott Branden, Kumar Gala, Shawn Guo [-- Attachment #1.1: Type: text/plain, Size: 2615 bytes --] Boris Brezillon <boris.brezillon@free-electrons.com> writes: > On Thu, 15 Jun 2017 16:33:11 -0700 > Eric Anholt <eric@anholt.net> wrote: > >> Eric Anholt <eric@anholt.net> writes: >> >> > [ Unknown signature status ] >> > Boris Brezillon <boris.brezillon@free-electrons.com> writes: >> > >> >> On Tue, 06 Jun 2017 13:27:09 -0700 >> >> Eric Anholt <eric@anholt.net> wrote: >> >> >> >>> Boris Brezillon <boris.brezillon@free-electrons.com> writes: >> >>> >> >>> > The VC4 KMS driver is implementing its own ->atomic_commit() but there >> >>> > are a few generic helpers we can use instead of open-coding the logic. >> >>> > >> >>> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >> >>> > --- >> >>> > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- >> >>> > 1 file changed, 12 insertions(+), 26 deletions(-) >> >>> > >> >>> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c >> >>> > index ad7925a9e0ea..f229abc0991b 100644 >> >>> > --- a/drivers/gpu/drm/vc4/vc4_kms.c >> >>> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c >> >>> > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) >> >>> > struct drm_device *dev = state->dev; >> >>> > struct vc4_dev *vc4 = to_vc4_dev(dev); >> >>> > >> >>> > + drm_atomic_helper_wait_for_fences(dev, state, false); >> >>> > + >> >>> > + drm_atomic_helper_wait_for_dependencies(state); >> >>> >> >>> With this wait_for_fences() addition and the reservation stuff that >> >>> landed, I think we can rip out the "seqno cb" in vc4, and just use >> >>> drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you >> >>> see anything missing, with that? >> >> >> >> I can't tell. I haven't dig enough to understand what this seqno cb was >> >> used for :-), but Daniel was suggesting the same thing. I'll try to >> >> better understand what seqno cb does and if it's all safe to get rid of >> >> it and use the standard helpers. >> > >> > The seqno cb was the thing for stalling the modeset until V3D was done >> > rendering to the planes. The wait_for_fences() does the same thing >> > using generic dmabuf reservations, so the seqno cb isn't needed any >> > more. >> >> Oh, we've got another user of the seqno cb (async flips), anyway, so we >> can't delete it quite yet. I've pushed this patch since it's a clear >> improvement (less code, waits for fences from other devices if any). > > Ok. Did you fix the leak before applying? I don't think I see discussion of a leak. What are you referring to? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior [not found] ` <87k24orheq.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 2017-06-06 20:48 ` Boris Brezillon @ 2017-06-13 10:10 ` Boris Brezillon 1 sibling, 0 replies; 26+ messages in thread From: Boris Brezillon @ 2017-06-13 10:10 UTC (permalink / raw) To: Eric Anholt Cc: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingwort +Gustavo On Tue, 06 Jun 2017 13:27:09 -0700 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote: > Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > > > The VC4 KMS driver is implementing its own ->atomic_commit() but there > > are a few generic helpers we can use instead of open-coding the logic. > > > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> > > --- > > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- > > 1 file changed, 12 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > > index ad7925a9e0ea..f229abc0991b 100644 > > --- a/drivers/gpu/drm/vc4/vc4_kms.c > > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > > struct drm_device *dev = state->dev; > > struct vc4_dev *vc4 = to_vc4_dev(dev); > > > > + drm_atomic_helper_wait_for_fences(dev, state, false); > > + > > + drm_atomic_helper_wait_for_dependencies(state); > > With this wait_for_fences() addition and the reservation stuff that > landed, I think we can rip out the "seqno cb" in vc4, and just use > drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you > see anything missing, with that? We can probably get rid of "seqno cb", but I'm not sure we have everything in place to use drm_atomic_helper_commit_tail() and drm_atomic_helper_commit() yet. What about async page flips? The async_modeset semaphore is taken in vc4_atomic_commit() to prevent races with async page flips. Can we safely get rid of this? Would Gustavo's work on async plane update help us get rid of this semaphore? Also note that patch 4 replaces the call to drm_atomic_helper_wait_for_vblanks() by one to drm_atomic_helper_wait_for_flip_done(), and we haven't decided yet if this is something we want to make generic. -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/7] drm/vc4: Use drm_atomic_helper_wait_for_flip_done() [not found] ` <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-02 8:32 ` [PATCH 2/7] drm/vc4: Fix vblank handling Boris Brezillon 2017-06-02 8:32 ` [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior Boris Brezillon @ 2017-06-02 8:32 ` Boris Brezillon [not found] ` <1496392332-8722-5-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-02 8:32 ` [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block Boris Brezillon ` (2 subsequent siblings) 5 siblings, 1 reply; 26+ messages in thread From: Boris Brezillon @ 2017-06-02 8:32 UTC (permalink / raw) To: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Boris Brezillon, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey Replace the drm_atomic_helper_wait_for_vblanks() with a call to drm_atomic_helper_wait_for_flip_done(). This allows better detection of page flip done events which what we are really waiting for in vc4_atomic_complete_commit(). With this approach, we also addresse the 'missed single vblank event' problem that can arise when the CRTC is configured in oneshot mode (only a single frame is generated and the CRTC is immediately paused after this frame). Note that this oneshot mode will be used for the writeback connector feature. Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --- drivers/gpu/drm/vc4/vc4_kms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index f229abc0991b..86a60e9b623d 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -63,7 +63,7 @@ vc4_atomic_complete_commit(struct vc4_commit *c) drm_atomic_helper_commit_hw_done(state); - drm_atomic_helper_wait_for_vblanks(dev, state); + drm_atomic_helper_wait_for_flip_done(dev, state); drm_atomic_helper_cleanup_planes(dev, state); -- 2.7.4 -- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
[parent not found: <1496392332-8722-5-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 4/7] drm/vc4: Use drm_atomic_helper_wait_for_flip_done() [not found] ` <1496392332-8722-5-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2017-06-06 20:24 ` Eric Anholt [not found] ` <87lgp4rhj2.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 0 siblings, 1 reply; 26+ messages in thread From: Eric Anholt @ 2017-06-06 20:24 UTC (permalink / raw) To: Boris Brezillon, David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey [-- Attachment #1: Type: text/plain, Size: 896 bytes --] Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > Replace the drm_atomic_helper_wait_for_vblanks() with a call to > drm_atomic_helper_wait_for_flip_done(). This allows better detection of > page flip done events which what we are really waiting for in > vc4_atomic_complete_commit(). > > With this approach, we also addresse the 'missed single vblank event' > problem that can arise when the CRTC is configured in oneshot mode > (only a single frame is generated and the CRTC is immediately paused > after this frame). Note that this oneshot mode will be used for the > writeback connector feature. Should we just use drm_atomic_helper_commit_tail() and make this change in the core helper, instead? Actually, I'm confused. drm_atomic_helper_commit_cleanup_done() seems to be waiting for the flip_done on the crtc, already. What's the difference? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
[parent not found: <87lgp4rhj2.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org>]
* Re: [PATCH 4/7] drm/vc4: Use drm_atomic_helper_wait_for_flip_done() [not found] ` <87lgp4rhj2.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> @ 2017-06-06 20:59 ` Boris Brezillon 0 siblings, 0 replies; 26+ messages in thread From: Boris Brezillon @ 2017-06-06 20:59 UTC (permalink / raw) To: Eric Anholt Cc: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingwort On Tue, 06 Jun 2017 13:24:33 -0700 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote: > Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > > > Replace the drm_atomic_helper_wait_for_vblanks() with a call to > > drm_atomic_helper_wait_for_flip_done(). This allows better detection of > > page flip done events which what we are really waiting for in > > vc4_atomic_complete_commit(). > > > > With this approach, we also addresse the 'missed single vblank event' > > problem that can arise when the CRTC is configured in oneshot mode > > (only a single frame is generated and the CRTC is immediately paused > > after this frame). Note that this oneshot mode will be used for the > > writeback connector feature. > > Should we just use drm_atomic_helper_commit_tail() and make this change > in the core helper, instead? Hm, not sure changing the default behavior is such a good idea. I don't want to break other drivers. > > Actually, I'm confused. drm_atomic_helper_commit_cleanup_done() seems > to be waiting for the flip_done on the crtc, already. What's the > difference? Actually, drm_atomic_helper_wait_for_flip_done() is called just before drm_atomic_helper_cleanup_planes() which in turn is called before drm_atomic_helper_commit_cleanup_done(). My understanding was that it was unsafe to call plane->cleanup_fb() on FBs that are still in use, and the only thing guaranteeing that FBs are not used anymore is the flip_done event. Maybe I'm wrong, and FB refcounting is enough to make sure resources are preserved until page flip is actually done. -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block [not found] ` <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> ` (2 preceding siblings ...) 2017-06-02 8:32 ` [PATCH 4/7] drm/vc4: Use drm_atomic_helper_wait_for_flip_done() Boris Brezillon @ 2017-06-02 8:32 ` Boris Brezillon [not found] ` <1496392332-8722-6-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-02 8:32 ` [PATCH 6/7] dt-bindings: Document the VC4 TXP module nodes Boris Brezillon 2017-06-02 8:32 ` [PATCH 7/7] ARM: dts: bcm283x: Add Transposer block Boris Brezillon 5 siblings, 1 reply; 26+ messages in thread From: Boris Brezillon @ 2017-06-02 8:32 UTC (permalink / raw) To: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Boris Brezillon, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey The TXP block is providing writeback support. Add a driver to expose this feature. Supporting this engine requires reworking the CRTC because it's not using the usual 'HVS -> PV -> VC4 Encoder' scheme. Instead, the TXP block is directly connected to HVS FIFO2, and the PV is completely bypassed. In order to make things work, we have to detect when the TXP is enabled, avoid enabling the PV when this is the case, and generate fake VBLANK events when the TXP is done writing the frame back to memory. Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --- drivers/gpu/drm/vc4/Makefile | 1 + drivers/gpu/drm/vc4/vc4_crtc.c | 126 ++++++++-- drivers/gpu/drm/vc4/vc4_debugfs.c | 1 + drivers/gpu/drm/vc4/vc4_drv.c | 1 + drivers/gpu/drm/vc4/vc4_drv.h | 10 + drivers/gpu/drm/vc4/vc4_kms.c | 7 + drivers/gpu/drm/vc4/vc4_regs.h | 1 + drivers/gpu/drm/vc4/vc4_txp.c | 509 ++++++++++++++++++++++++++++++++++++++ 8 files changed, 634 insertions(+), 22 deletions(-) create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c diff --git a/drivers/gpu/drm/vc4/Makefile b/drivers/gpu/drm/vc4/Makefile index 61f45d122bd0..660b3363400f 100644 --- a/drivers/gpu/drm/vc4/Makefile +++ b/drivers/gpu/drm/vc4/Makefile @@ -18,6 +18,7 @@ vc4-y := \ vc4_plane.o \ vc4_render_cl.o \ vc4_trace_points.o \ + vc4_txp.o \ vc4_v3d.o \ vc4_validate.o \ vc4_validate_shaders.o diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index df1d81533076..564fd795803d 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -66,6 +66,7 @@ struct vc4_crtc_state { struct drm_crtc_state base; /* Dlist area for this CRTC configuration. */ struct drm_mm_node mm; + bool feed_txp; }; static inline struct vc4_crtc * @@ -369,10 +370,8 @@ static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc) return NULL; } -static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) +static void vc4_crtc_config_pv(struct drm_crtc *crtc) { - struct drm_device *dev = crtc->dev; - struct vc4_dev *vc4 = to_vc4_dev(dev); struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc); struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); @@ -383,12 +382,6 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) bool is_dsi = (vc4_encoder->type == VC4_ENCODER_TYPE_DSI0 || vc4_encoder->type == VC4_ENCODER_TYPE_DSI1); u32 format = is_dsi ? PV_CONTROL_FORMAT_DSIV_24 : PV_CONTROL_FORMAT_24; - bool debug_dump_regs = false; - - if (debug_dump_regs) { - DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc)); - vc4_crtc_dump_regs(vc4_crtc); - } /* Reset the PV fifo. */ CRTC_WRITE(PV_CONTROL, 0); @@ -464,6 +457,51 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) PV_CONTROL_CLK_SELECT) | PV_CONTROL_FIFO_CLR | PV_CONTROL_EN); +} + +static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + struct vc4_dev *vc4 = to_vc4_dev(dev); + struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); + struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); + struct drm_display_mode *mode = &crtc->state->adjusted_mode; + bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE; + bool debug_dump_regs = false; + + if (debug_dump_regs) { + DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc)); + vc4_crtc_dump_regs(vc4_crtc); + } + + if (vc4_crtc->channel == 2) { + u32 dispctrl, olddispctrl; + + olddispctrl = HVS_READ(SCALER_DISPCTRL); + dispctrl = olddispctrl & ~SCALER_DISPCTRL_DSP3_MUX_MASK; + + /* SCALER_DISPCTRL_DSP3 = X, where X < 2 means 'connect DSP3 to + * FIFO X'. + * SCALER_DISPCTRL_DSP3 = 3 means 'disable DSP 3'. + * + * DSP3 is connected to FIFO2 unless the transposer is + * enabled. In this case, FIFO 2 is directly accessed by the + * TXP IP, and we need to disable the FIFO2 -> pixelvalve1 + * route. + */ + if (vc4_state->feed_txp) + dispctrl |= VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX); + else + dispctrl |= VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX); + + /* Reconfigure the DSP3 mux if required. */ + if (olddispctrl != dispctrl) + HVS_WRITE(SCALER_DISPCTRL, dispctrl); + } + + /* Only configure the pixelvalve if we're not feeding the TXP block. */ + if (!vc4_state->feed_txp) + vc4_crtc_config_pv(crtc); HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel), SCALER_DISPBKGND_AUTOHS | @@ -576,8 +614,8 @@ static void vc4_crtc_enable(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); - struct drm_crtc_state *state = crtc->state; - struct drm_display_mode *mode = &state->adjusted_mode; + struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); + struct drm_display_mode *mode = &crtc->state->adjusted_mode; require_hvs_enabled(dev); @@ -589,15 +627,21 @@ static void vc4_crtc_enable(struct drm_crtc *crtc) /* Turn on the scaler, which will wait for vstart to start * compositing. + * When feeding the transposer, we should operate in oneshot + * mode. */ HVS_WRITE(SCALER_DISPCTRLX(vc4_crtc->channel), VC4_SET_FIELD(mode->hdisplay, SCALER_DISPCTRLX_WIDTH) | VC4_SET_FIELD(mode->vdisplay, SCALER_DISPCTRLX_HEIGHT) | - SCALER_DISPCTRLX_ENABLE); + SCALER_DISPCTRLX_ENABLE | + (vc4_state->feed_txp ? SCALER_DISPCTRLX_ONESHOT : 0)); - /* Turn on the pixel valve, which will emit the vstart signal. */ - CRTC_WRITE(PV_V_CONTROL, - CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN); + /* When feeding the composer block the pixelvalve is unneeded and + * should not be enabled. + */ + if (!vc4_state->feed_txp) + CRTC_WRITE(PV_V_CONTROL, + CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN); } static bool vc4_crtc_mode_fixup(struct drm_crtc *crtc, @@ -623,8 +667,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, struct drm_plane *plane; unsigned long flags; const struct drm_plane_state *plane_state; + struct drm_connector *conn; + struct drm_connector_state *conn_state; u32 dlist_count = 0; - int ret; + int ret, i; /* The pixelvalve can only feed one encoder (and encoders are * 1:1 with connectors.) @@ -644,6 +690,19 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, if (ret) return ret; + for_each_connector_in_state(state->state, conn, conn_state, i) { + if (conn_state->crtc != crtc) + continue; + + /* The writeback connector is implemented using the transposer + * block which is directly taking its data from HVS FIFO2. + */ + if (conn->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) + vc4_state->feed_txp = true; + + break; + } + return 0; } @@ -722,10 +781,14 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc) struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); u32 chan = vc4_crtc->channel; unsigned long flags; + u32 dispstat_mode; + dispstat_mode = VC4_GET_FIELD(HVS_READ(SCALER_DISPSTATX(chan)), + SCALER_DISPSTATX_MODE); spin_lock_irqsave(&dev->event_lock, flags); if (vc4_crtc->event && - (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)))) { + (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)) || + dispstat_mode == SCALER_DISPSTATX_MODE_DISABLED)) { drm_crtc_send_vblank_event(crtc, vc4_crtc->event); vc4_crtc->event = NULL; drm_crtc_vblank_put(crtc); @@ -733,6 +796,15 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc) spin_unlock_irqrestore(&dev->event_lock, flags); } +void vc4_crtc_eof_event(struct drm_crtc *crtc) +{ + struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); + + vc4_crtc->t_vblank = ktime_get(); + drm_crtc_handle_vblank(&vc4_crtc->base); + vc4_crtc_handle_page_flip(vc4_crtc); +} + static irqreturn_t vc4_crtc_irq_handler(int irq, void *data) { struct vc4_crtc *vc4_crtc = data; @@ -740,11 +812,8 @@ static irqreturn_t vc4_crtc_irq_handler(int irq, void *data) irqreturn_t ret = IRQ_NONE; if (stat & PV_INT_VFP_START) { - vc4_crtc->t_vblank = ktime_get(); CRTC_WRITE(PV_INTSTAT, PV_INT_VFP_START); - drm_crtc_handle_vblank(&vc4_crtc->base); - vc4_crtc_handle_page_flip(vc4_crtc); - ret = IRQ_HANDLED; + vc4_crtc_eof_event(&vc4_crtc->base); } return ret; @@ -956,9 +1025,22 @@ static void vc4_set_crtc_possible_masks(struct drm_device *drm, struct drm_encoder *encoder; drm_for_each_encoder(encoder, drm) { - struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder); + struct vc4_encoder *vc4_encoder; int i; + /* + * TXP is not a regular VC4 encoder, and has to be treated + * differently. + */ + if (vc4_is_txp_encoder(drm, encoder)) { + /* HVS FIFO2 can feed the TXP IP. */ + if (crtc_data->hvs_channel == 2) + encoder->possible_crtcs |= drm_crtc_mask(crtc); + + continue; + } + + vc4_encoder = to_vc4_encoder(encoder); for (i = 0; i < ARRAY_SIZE(crtc_data->encoder_types); i++) { if (vc4_encoder->type == encoder_types[i]) { vc4_encoder->clock_select = i; diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c index 5db06bdb5f27..7a0003de71ab 100644 --- a/drivers/gpu/drm/vc4/vc4_debugfs.c +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c @@ -21,6 +21,7 @@ static const struct drm_info_list vc4_debugfs_list[] = { {"dsi1_regs", vc4_dsi_debugfs_regs, 0, (void *)(uintptr_t)1}, {"hdmi_regs", vc4_hdmi_debugfs_regs, 0}, {"vec_regs", vc4_vec_debugfs_regs, 0}, + {"txp_regs", vc4_txp_debugfs_regs, 0}, {"hvs_regs", vc4_hvs_debugfs_regs, 0}, {"crtc0_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)0}, {"crtc1_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)1}, diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 61e674baf3a6..f1af50886d9d 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -309,6 +309,7 @@ static struct platform_driver *const component_drivers[] = { &vc4_vec_driver, &vc4_dpi_driver, &vc4_dsi_driver, + &vc4_txp_driver, &vc4_hvs_driver, &vc4_crtc_driver, &vc4_v3d_driver, diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 1999bbb073ec..efe8d34593fd 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -20,6 +20,7 @@ struct vc4_dev { struct vc4_dpi *dpi; struct vc4_dsi *dsi1; struct vc4_vec *vec; + struct vc4_txp *txp; struct drm_fbdev_cma *fbdev; @@ -453,6 +454,7 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id, int vc4_crtc_get_vblank_timestamp(struct drm_device *dev, unsigned int crtc_id, int *max_error, struct timeval *vblank_time, unsigned flags); +void vc4_crtc_eof_event(struct drm_crtc *crtc); /* vc4_debugfs.c */ int vc4_debugfs_init(struct drm_minor *minor); @@ -495,6 +497,14 @@ int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused); extern struct platform_driver vc4_vec_driver; int vc4_vec_debugfs_regs(struct seq_file *m, void *unused); +/* vc4_txp.c */ +extern struct platform_driver vc4_txp_driver; +int vc4_txp_debugfs_regs(struct seq_file *m, void *unused); +void vc4_txp_atomic_commit(struct drm_device *dev, + struct drm_atomic_state *old_state); +bool vc4_is_txp_encoder(struct drm_device *dev, + struct drm_encoder *encoder); + /* vc4_irq.c */ irqreturn_t vc4_irq(int irq, void *arg); void vc4_irq_preinstall(struct drm_device *dev); diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 86a60e9b623d..8e2cbe5a93d3 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -52,6 +52,13 @@ vc4_atomic_complete_commit(struct vc4_commit *c) drm_atomic_helper_commit_modeset_enables(dev, state); + /* + * Keep vc4_txp_atomic_commit() after + * drm_atomic_helper_commit_modeset_enables() because the TXP block has + * to be enabled after the HVS channel. + */ + vc4_txp_atomic_commit(dev, state); + /* Make sure that drm_atomic_helper_wait_for_vblanks() * actually waits for vblank. If we're doing a full atomic * modeset (as opposed to a vc4_update_plane() short circuit), diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h index 932093936178..ab6287336140 100644 --- a/drivers/gpu/drm/vc4/vc4_regs.h +++ b/drivers/gpu/drm/vc4/vc4_regs.h @@ -359,6 +359,7 @@ #define SCALER_DISPCTRL0 0x00000040 # define SCALER_DISPCTRLX_ENABLE BIT(31) # define SCALER_DISPCTRLX_RESET BIT(30) +# define SCALER_DISPCTRLX_ONESHOT BIT(29) # define SCALER_DISPCTRLX_WIDTH_MASK VC4_MASK(23, 12) # define SCALER_DISPCTRLX_WIDTH_SHIFT 12 # define SCALER_DISPCTRLX_HEIGHT_MASK VC4_MASK(11, 0) diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c new file mode 100644 index 000000000000..1886e031bef3 --- /dev/null +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -0,0 +1,509 @@ +/* + * Copyright © 2017 Broadcom + * + * Author: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_fb_cma_helper.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_edid.h> +#include <drm/drm_panel.h> +#include <drm/drm_writeback.h> +#include <linux/clk.h> +#include <linux/component.h> +#include <linux/of_graph.h> +#include <linux/of_platform.h> +#include <linux/pm_runtime.h> + +#include "vc4_drv.h" +#include "vc4_regs.h" + +/* Base address of the output. Raster formats must be 4-byte aligned, + * T and LT must be 16-byte aligned or maybe utile-aligned (docs are + * inconsistent, but probably utile). + */ +#define TXP_DST_PTR 0x00 + +/* Pitch in bytes for raster images, 16-byte aligned. For tiled, it's + * the width in tiles. + */ +#define TXP_DST_PITCH 0x04 +/* For T-tiled imgaes, DST_PITCH should be the number of tiles wide, + * shifted up. + */ +# define TXP_T_TILE_WIDTH_SHIFT 7 +/* For LT-tiled images, DST_PITCH should be the number of utiles wide, + * shifted up. + */ +# define TXP_LT_TILE_WIDTH_SHIFT 4 + +/* Pre-rotation width/height of the image. Must match HVS config. + * + * If TFORMAT and 32-bit, limit is 1920 for 32-bit and 3840 to 16-bit + * and width/height must be tile or utile-aligned as appropriate. If + * transposing (rotating), width is limited to 1920. + * + * Height is limited to various numbers between 4088 and 4095. I'd + * just use 4088 to be safe. + */ +#define TXP_DIM 0x08 +# define TXP_HEIGHT_SHIFT 16 +# define TXP_HEIGHT_MASK GENMASK(31, 16) +# define TXP_WIDTH_SHIFT 0 +# define TXP_WIDTH_MASK GENMASK(15, 0) + +#define TXP_DST_CTRL 0x0c +/* These bits are set to 0x54 */ +#define TXP_PILOT_SHIFT 24 +#define TXP_PILOT_MASK GENMASK(31, 24) +/* Bits 22-23 are set to 0x01 */ +#define TXP_VERSION_SHIFT 22 +#define TXP_VERSION_MASK GENMASK(23, 22) + +/* Powers down the internal memory. */ +# define TXP_POWERDOWN BIT(21) + +/* Enables storing the alpha component in 8888/4444, instead of + * filling with ~ALPHA_INVERT. + */ +# define TXP_ALPHA_ENABLE BIT(20) + +/* 4 bits, each enables stores for a channel in each set of 4 bytes. + * Set to 0xf for normal operation. + */ +# define TXP_BYTE_ENABLE_SHIFT 16 +# define TXP_BYTE_ENABLE_MASK GENMASK(19, 16) + +/* Debug: Generate VSTART again at EOF. */ +# define TXP_VSTART_AT_EOF BIT(15) + +/* Debug: Terminate the current frame immediately. Stops AXI + * writes. + */ +# define TXP_ABORT BIT(14) + +# define TXP_DITHER BIT(13) + +/* Inverts alpha if TXP_ALPHA_ENABLE, chooses fill value for + * !TXP_ALPHA_ENABLE. + */ +# define TXP_ALPHA_INVERT BIT(12) + +/* Note: I've listed the channels here in high bit (in byte 3/2/1) to + * low bit (in byte 0) order. + */ +# define TXP_FORMAT_SHIFT 8 +# define TXP_FORMAT_MASK GENMASK(11, 8) +# define TXP_FORMAT_ABGR4444 0 +# define TXP_FORMAT_ARGB4444 1 +# define TXP_FORMAT_BGRA4444 2 +# define TXP_FORMAT_RGBA4444 3 +# define TXP_FORMAT_BGR565 6 +# define TXP_FORMAT_RGB565 7 +/* 888s are non-rotated, raster-only */ +# define TXP_FORMAT_BGR888 8 +# define TXP_FORMAT_RGB888 9 +# define TXP_FORMAT_ABGR8888 12 +# define TXP_FORMAT_ARGB8888 13 +# define TXP_FORMAT_BGRA8888 14 +# define TXP_FORMAT_RGBA8888 15 + +/* If TFORMAT is set, generates LT instead of T format. */ +# define TXP_LINEAR_UTILE BIT(7) + +/* Rotate output by 90 degrees. */ +# define TXP_TRANSPOSE BIT(6) + +/* Generate a tiled format for V3D. */ +# define TXP_TFORMAT BIT(5) + +/* Generates some undefined test mode output. */ +# define TXP_TEST_MODE BIT(4) + +/* Request odd field from HVS. */ +# define TXP_FIELD BIT(3) + +/* Raise interrupt when idle. */ +# define TXP_EI BIT(2) + +/* Set when generating a frame, clears when idle. */ +# define TXP_BUSY BIT(1) + +/* Starts a frame. Self-clearing. */ +# define TXP_GO BIT(0) + +/* Number of lines received and committed to memory. */ +#define TXP_PROGRESS 0x10 + +#define TXP_READ(offset) readl(txp->regs + (offset)) +#define TXP_WRITE(offset, val) writel(val, txp->regs + (offset)) + +struct vc4_txp { + struct platform_device *pdev; + + struct drm_writeback_connector connector; + + void __iomem *regs; +}; + +static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn) +{ + return container_of(conn, struct vc4_txp, connector.base); +} + +#define TXP_REG(reg) { reg, #reg } +static const struct { + u32 reg; + const char *name; +} txp_regs[] = { + TXP_REG(TXP_DST_PTR), + TXP_REG(TXP_DST_PITCH), + TXP_REG(TXP_DIM), + TXP_REG(TXP_DST_CTRL), + TXP_REG(TXP_PROGRESS), +}; + +#ifdef CONFIG_DEBUG_FS +int vc4_txp_debugfs_regs(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = (struct drm_info_node *)m->private; + struct drm_device *dev = node->minor->dev; + struct vc4_dev *vc4 = to_vc4_dev(dev); + struct vc4_txp *txp = vc4->txp; + int i; + + if (!txp) + return 0; + + for (i = 0; i < ARRAY_SIZE(txp_regs); i++) { + seq_printf(m, "%s (0x%04x): 0x%08x\n", + txp_regs[i].name, txp_regs[i].reg, + TXP_READ(txp_regs[i].reg)); + } + + return 0; +} +#endif + +static int vc4_txp_connector_get_modes(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + + return drm_add_modes_noedid(connector, dev->mode_config.max_width, + dev->mode_config.max_height); +} + +static enum drm_mode_status +vc4_txp_connector_mode_valid(struct drm_connector *connector, + struct drm_display_mode *mode) +{ + struct drm_device *dev = connector->dev; + struct drm_mode_config *mode_config = &dev->mode_config; + int w = mode->hdisplay, h = mode->vdisplay; + + if (w < mode_config->min_width || w > mode_config->max_width) + return MODE_BAD_HVALUE; + + if (h < mode_config->min_height || h > mode_config->max_height) + return MODE_BAD_VVALUE; + + return MODE_OK; +} + +static const u32 vc4_txp_formats[] = { + DRM_FORMAT_RGB888, + DRM_FORMAT_BGR888, + DRM_FORMAT_XRGB8888, + DRM_FORMAT_XBGR8888, + DRM_FORMAT_ARGB8888, + DRM_FORMAT_ABGR8888, + DRM_FORMAT_RGBX8888, + DRM_FORMAT_BGRX8888, + DRM_FORMAT_RGBA8888, + DRM_FORMAT_BGRA8888, +}; + +static int +vc4_txp_connector_atomic_check(struct drm_connector *connector, + struct drm_connector_state *conn_state) +{ + struct drm_crtc_state *crtc_state; + struct drm_gem_cma_object *gem; + struct drm_framebuffer *fb; + int i; + + if (!conn_state->crtc) + return 0; + + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) + return 0; + + crtc_state = drm_atomic_get_crtc_state(conn_state->state, + conn_state->crtc); + fb = conn_state->writeback_job->fb; + if (fb->width != crtc_state->mode.hdisplay || + fb->height != crtc_state->mode.vdisplay) { + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", + fb->width, fb->height); + return -EINVAL; + } + + for (i = 0; i < ARRAY_SIZE(vc4_txp_formats); i++) { + if (fb->format->format == vc4_txp_formats[i]) + break; + } + + if (i == ARRAY_SIZE(vc4_txp_formats)) + return -EINVAL; + + gem = drm_fb_cma_get_gem_obj(fb, 0); + + /* Pitch must be aligned on 16 bytes. */ + if (fb->pitches[0] & GENMASK(3, 0)) + return -EINVAL; + + return 0; +} + +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); + 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, +}; + +static enum drm_connector_status +vc4_txp_connector_detect(struct drm_connector *connector, bool force) +{ + return connector_status_disconnected; +} + +static void vc4_txp_connector_destroy(struct drm_connector *connector) +{ + drm_connector_unregister(connector); + drm_connector_cleanup(connector); +} + +static const struct drm_connector_funcs vc4_txp_connector_funcs = { + .dpms = drm_atomic_helper_connector_dpms, + .detect = vc4_txp_connector_detect, + .fill_modes = drm_helper_probe_single_connector_modes, + .set_property = drm_atomic_helper_connector_set_property, + .destroy = vc4_txp_connector_destroy, + .reset = drm_atomic_helper_connector_reset, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; + +static const struct drm_encoder_funcs vc4_txp_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + +static irqreturn_t vc4_txp_interrupt(int irq, void *data) +{ + struct vc4_txp *txp = data; + struct drm_crtc *crtc = txp->connector.base.state->crtc; + + TXP_WRITE(TXP_DST_CTRL, 0); + vc4_crtc_eof_event(crtc); + drm_writeback_signal_completion(&txp->connector, 0); + + return IRQ_HANDLED; +} + +static int vc4_txp_bind(struct device *dev, struct device *master, void *data) +{ + struct platform_device *pdev = to_platform_device(dev); + struct drm_device *drm = dev_get_drvdata(master); + struct vc4_dev *vc4 = to_vc4_dev(drm); + struct vc4_txp *txp; + int ret, irq; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + txp = devm_kzalloc(dev, sizeof(*txp), GFP_KERNEL); + if (!txp) + return -ENOMEM; + + txp->pdev = pdev; + + txp->regs = vc4_ioremap_regs(pdev, 0); + if (IS_ERR(txp->regs)) + return PTR_ERR(txp->regs); + + drm_connector_helper_add(&txp->connector.base, + &vc4_txp_connector_helper_funcs); + ret = drm_writeback_connector_init(drm, &txp->connector, + &vc4_txp_connector_funcs, + NULL, + vc4_txp_formats, + ARRAY_SIZE(vc4_txp_formats)); + if (ret) + return ret; + + ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0, + dev_name(dev), txp); + if (ret) + return ret; + + dev_set_drvdata(dev, txp); + vc4->txp = txp; + + return 0; +} + +static void vc4_txp_unbind(struct device *dev, struct device *master, + void *data) +{ + struct drm_device *drm = dev_get_drvdata(master); + struct vc4_dev *vc4 = to_vc4_dev(drm); + struct vc4_txp *txp = dev_get_drvdata(dev); + + vc4_txp_connector_destroy(&txp->connector.base); + + vc4->txp = NULL; +} + +static const struct component_ops vc4_txp_ops = { + .bind = vc4_txp_bind, + .unbind = vc4_txp_unbind, +}; + +static int vc4_txp_dev_probe(struct platform_device *pdev) +{ + return component_add(&pdev->dev, &vc4_txp_ops); +} + +static int vc4_txp_dev_remove(struct platform_device *pdev) +{ + component_del(&pdev->dev, &vc4_txp_ops); + return 0; +} + +static const struct of_device_id vc4_txp_dt_match[] = { + { .compatible = "brcm,bcm2835-txp" }, + { /* sentinel */ }, +}; + +struct platform_driver vc4_txp_driver = { + .probe = vc4_txp_dev_probe, + .remove = vc4_txp_dev_remove, + .driver = { + .name = "vc4_txp", + .of_match_table = vc4_txp_dt_match, + }, +}; -- 2.7.4 -- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
[parent not found: <1496392332-8722-6-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block [not found] ` <1496392332-8722-6-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2017-06-05 11:25 ` Brian Starkey 2017-06-06 19:13 ` Boris Brezillon 0 siblings, 1 reply; 26+ messages in thread From: Brian Starkey @ 2017-06-05 11:25 UTC (permalink / raw) To: Boris Brezillon Cc: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton 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. I've a further query below about the handling of CRTC events. On Fri, Jun 02, 2017 at 10:32:10AM +0200, Boris Brezillon wrote: >The TXP block is providing writeback support. Add a driver to expose this >feature. > >Supporting this engine requires reworking the CRTC because it's not using >the usual 'HVS -> PV -> VC4 Encoder' scheme. Instead, the TXP block is >directly connected to HVS FIFO2, and the PV is completely bypassed. > >In order to make things work, we have to detect when the TXP is enabled, >avoid enabling the PV when this is the case, and generate fake VBLANK >events when the TXP is done writing the frame back to memory. > >Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> >--- [snip] >diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c >new file mode 100644 >index 000000000000..1886e031bef3 >--- /dev/null >+++ b/drivers/gpu/drm/vc4/vc4_txp.c >@@ -0,0 +1,509 @@ >+/* >+ * Copyright © 2017 Broadcom >+ * >+ * Author: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> >+ * >+ * Permission is hereby granted, free of charge, to any person obtaining a >+ * copy of this software and associated documentation files (the "Software"), >+ * to deal in the Software without restriction, including without limitation >+ * the rights to use, copy, modify, merge, publish, distribute, sublicense, >+ * and/or sell copies of the Software, and to permit persons to whom the >+ * Software is furnished to do so, subject to the following conditions: >+ * >+ * The above copyright notice and this permission notice (including the next >+ * paragraph) shall be included in all copies or substantial portions of the >+ * Software. >+ * >+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS >+ * IN THE SOFTWARE. >+ */ >+ >+#include <drm/drm_atomic.h> >+#include <drm/drm_atomic_helper.h> >+#include <drm/drm_fb_cma_helper.h> >+#include <drm/drm_crtc_helper.h> >+#include <drm/drm_edid.h> >+#include <drm/drm_panel.h> >+#include <drm/drm_writeback.h> >+#include <linux/clk.h> >+#include <linux/component.h> >+#include <linux/of_graph.h> >+#include <linux/of_platform.h> >+#include <linux/pm_runtime.h> >+ >+#include "vc4_drv.h" >+#include "vc4_regs.h" >+ >+/* Base address of the output. Raster formats must be 4-byte aligned, >+ * T and LT must be 16-byte aligned or maybe utile-aligned (docs are >+ * inconsistent, but probably utile). >+ */ >+#define TXP_DST_PTR 0x00 >+ >+/* Pitch in bytes for raster images, 16-byte aligned. For tiled, it's >+ * the width in tiles. >+ */ >+#define TXP_DST_PITCH 0x04 >+/* For T-tiled imgaes, DST_PITCH should be the number of tiles wide, >+ * shifted up. >+ */ >+# define TXP_T_TILE_WIDTH_SHIFT 7 >+/* For LT-tiled images, DST_PITCH should be the number of utiles wide, >+ * shifted up. >+ */ >+# define TXP_LT_TILE_WIDTH_SHIFT 4 >+ >+/* Pre-rotation width/height of the image. Must match HVS config. >+ * >+ * If TFORMAT and 32-bit, limit is 1920 for 32-bit and 3840 to 16-bit >+ * and width/height must be tile or utile-aligned as appropriate. If >+ * transposing (rotating), width is limited to 1920. >+ * >+ * Height is limited to various numbers between 4088 and 4095. I'd >+ * just use 4088 to be safe. >+ */ >+#define TXP_DIM 0x08 >+# define TXP_HEIGHT_SHIFT 16 >+# define TXP_HEIGHT_MASK GENMASK(31, 16) >+# define TXP_WIDTH_SHIFT 0 >+# define TXP_WIDTH_MASK GENMASK(15, 0) >+ >+#define TXP_DST_CTRL 0x0c >+/* These bits are set to 0x54 */ >+#define TXP_PILOT_SHIFT 24 >+#define TXP_PILOT_MASK GENMASK(31, 24) >+/* Bits 22-23 are set to 0x01 */ >+#define TXP_VERSION_SHIFT 22 >+#define TXP_VERSION_MASK GENMASK(23, 22) >+ >+/* Powers down the internal memory. */ >+# define TXP_POWERDOWN BIT(21) >+ >+/* Enables storing the alpha component in 8888/4444, instead of >+ * filling with ~ALPHA_INVERT. >+ */ >+# define TXP_ALPHA_ENABLE BIT(20) >+ >+/* 4 bits, each enables stores for a channel in each set of 4 bytes. >+ * Set to 0xf for normal operation. >+ */ >+# define TXP_BYTE_ENABLE_SHIFT 16 >+# define TXP_BYTE_ENABLE_MASK GENMASK(19, 16) >+ >+/* Debug: Generate VSTART again at EOF. */ >+# define TXP_VSTART_AT_EOF BIT(15) >+ >+/* Debug: Terminate the current frame immediately. Stops AXI >+ * writes. >+ */ >+# define TXP_ABORT BIT(14) >+ >+# define TXP_DITHER BIT(13) >+ >+/* Inverts alpha if TXP_ALPHA_ENABLE, chooses fill value for >+ * !TXP_ALPHA_ENABLE. >+ */ >+# define TXP_ALPHA_INVERT BIT(12) >+ >+/* Note: I've listed the channels here in high bit (in byte 3/2/1) to >+ * low bit (in byte 0) order. >+ */ >+# define TXP_FORMAT_SHIFT 8 >+# define TXP_FORMAT_MASK GENMASK(11, 8) >+# define TXP_FORMAT_ABGR4444 0 >+# define TXP_FORMAT_ARGB4444 1 >+# define TXP_FORMAT_BGRA4444 2 >+# define TXP_FORMAT_RGBA4444 3 >+# define TXP_FORMAT_BGR565 6 >+# define TXP_FORMAT_RGB565 7 >+/* 888s are non-rotated, raster-only */ >+# define TXP_FORMAT_BGR888 8 >+# define TXP_FORMAT_RGB888 9 >+# define TXP_FORMAT_ABGR8888 12 >+# define TXP_FORMAT_ARGB8888 13 >+# define TXP_FORMAT_BGRA8888 14 >+# define TXP_FORMAT_RGBA8888 15 >+ >+/* If TFORMAT is set, generates LT instead of T format. */ >+# define TXP_LINEAR_UTILE BIT(7) >+ >+/* Rotate output by 90 degrees. */ >+# define TXP_TRANSPOSE BIT(6) >+ >+/* Generate a tiled format for V3D. */ >+# define TXP_TFORMAT BIT(5) >+ >+/* Generates some undefined test mode output. */ >+# define TXP_TEST_MODE BIT(4) >+ >+/* Request odd field from HVS. */ >+# define TXP_FIELD BIT(3) >+ >+/* Raise interrupt when idle. */ >+# define TXP_EI BIT(2) >+ >+/* Set when generating a frame, clears when idle. */ >+# define TXP_BUSY BIT(1) >+ >+/* Starts a frame. Self-clearing. */ >+# define TXP_GO BIT(0) >+ >+/* Number of lines received and committed to memory. */ >+#define TXP_PROGRESS 0x10 >+ >+#define TXP_READ(offset) readl(txp->regs + (offset)) >+#define TXP_WRITE(offset, val) writel(val, txp->regs + (offset)) >+ >+struct vc4_txp { >+ struct platform_device *pdev; >+ >+ struct drm_writeback_connector connector; >+ >+ void __iomem *regs; >+}; >+ >+static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn) >+{ >+ return container_of(conn, struct vc4_txp, connector.base); >+} >+ >+#define TXP_REG(reg) { reg, #reg } >+static const struct { >+ u32 reg; >+ const char *name; >+} txp_regs[] = { >+ TXP_REG(TXP_DST_PTR), >+ TXP_REG(TXP_DST_PITCH), >+ TXP_REG(TXP_DIM), >+ TXP_REG(TXP_DST_CTRL), >+ TXP_REG(TXP_PROGRESS), >+}; >+ >+#ifdef CONFIG_DEBUG_FS >+int vc4_txp_debugfs_regs(struct seq_file *m, void *unused) >+{ >+ struct drm_info_node *node = (struct drm_info_node *)m->private; >+ struct drm_device *dev = node->minor->dev; >+ struct vc4_dev *vc4 = to_vc4_dev(dev); >+ struct vc4_txp *txp = vc4->txp; >+ int i; >+ >+ if (!txp) >+ return 0; >+ >+ for (i = 0; i < ARRAY_SIZE(txp_regs); i++) { >+ seq_printf(m, "%s (0x%04x): 0x%08x\n", >+ txp_regs[i].name, txp_regs[i].reg, >+ TXP_READ(txp_regs[i].reg)); >+ } >+ >+ return 0; >+} >+#endif >+ >+static int vc4_txp_connector_get_modes(struct drm_connector *connector) >+{ >+ struct drm_device *dev = connector->dev; >+ >+ return drm_add_modes_noedid(connector, dev->mode_config.max_width, >+ dev->mode_config.max_height); >+} >+ >+static enum drm_mode_status >+vc4_txp_connector_mode_valid(struct drm_connector *connector, >+ struct drm_display_mode *mode) >+{ >+ struct drm_device *dev = connector->dev; >+ struct drm_mode_config *mode_config = &dev->mode_config; >+ int w = mode->hdisplay, h = mode->vdisplay; >+ >+ if (w < mode_config->min_width || w > mode_config->max_width) >+ return MODE_BAD_HVALUE; >+ >+ if (h < mode_config->min_height || h > mode_config->max_height) >+ return MODE_BAD_VVALUE; >+ >+ return MODE_OK; >+} >+ >+static const u32 vc4_txp_formats[] = { >+ DRM_FORMAT_RGB888, >+ DRM_FORMAT_BGR888, >+ DRM_FORMAT_XRGB8888, >+ DRM_FORMAT_XBGR8888, >+ DRM_FORMAT_ARGB8888, >+ DRM_FORMAT_ABGR8888, >+ DRM_FORMAT_RGBX8888, >+ DRM_FORMAT_BGRX8888, >+ DRM_FORMAT_RGBA8888, >+ DRM_FORMAT_BGRA8888, >+}; >+ >+static int >+vc4_txp_connector_atomic_check(struct drm_connector *connector, >+ struct drm_connector_state *conn_state) >+{ >+ struct drm_crtc_state *crtc_state; >+ struct drm_gem_cma_object *gem; >+ struct drm_framebuffer *fb; >+ int i; >+ >+ if (!conn_state->crtc) >+ return 0; >+ >+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb) >+ return 0; >+ >+ crtc_state = drm_atomic_get_crtc_state(conn_state->state, >+ conn_state->crtc); >+ fb = conn_state->writeback_job->fb; >+ if (fb->width != crtc_state->mode.hdisplay || >+ fb->height != crtc_state->mode.vdisplay) { >+ DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", >+ fb->width, fb->height); >+ return -EINVAL; >+ } >+ >+ for (i = 0; i < ARRAY_SIZE(vc4_txp_formats); i++) { >+ if (fb->format->format == vc4_txp_formats[i]) >+ break; >+ } >+ >+ if (i == ARRAY_SIZE(vc4_txp_formats)) >+ return -EINVAL; >+ >+ gem = drm_fb_cma_get_gem_obj(fb, 0); >+ >+ /* Pitch must be aligned on 16 bytes. */ >+ if (fb->pitches[0] & GENMASK(3, 0)) >+ return -EINVAL; >+ >+ return 0; >+} >+ >+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'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? >+ 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? Cheers, -Brian >+}; >+ >+static enum drm_connector_status >+vc4_txp_connector_detect(struct drm_connector *connector, bool force) >+{ >+ return connector_status_disconnected; >+} >+ >+static void vc4_txp_connector_destroy(struct drm_connector *connector) >+{ >+ drm_connector_unregister(connector); >+ drm_connector_cleanup(connector); >+} >+ >+static const struct drm_connector_funcs vc4_txp_connector_funcs = { >+ .dpms = drm_atomic_helper_connector_dpms, >+ .detect = vc4_txp_connector_detect, >+ .fill_modes = drm_helper_probe_single_connector_modes, >+ .set_property = drm_atomic_helper_connector_set_property, >+ .destroy = vc4_txp_connector_destroy, >+ .reset = drm_atomic_helper_connector_reset, >+ .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >+ .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >+}; >+ >+static const struct drm_encoder_funcs vc4_txp_encoder_funcs = { >+ .destroy = drm_encoder_cleanup, >+}; >+ >+static irqreturn_t vc4_txp_interrupt(int irq, void *data) >+{ >+ struct vc4_txp *txp = data; >+ struct drm_crtc *crtc = txp->connector.base.state->crtc; >+ >+ TXP_WRITE(TXP_DST_CTRL, 0); >+ vc4_crtc_eof_event(crtc); >+ drm_writeback_signal_completion(&txp->connector, 0); >+ >+ return IRQ_HANDLED; >+} >+ >+static int vc4_txp_bind(struct device *dev, struct device *master, void *data) >+{ >+ struct platform_device *pdev = to_platform_device(dev); >+ struct drm_device *drm = dev_get_drvdata(master); >+ struct vc4_dev *vc4 = to_vc4_dev(drm); >+ struct vc4_txp *txp; >+ int ret, irq; >+ >+ irq = platform_get_irq(pdev, 0); >+ if (irq < 0) >+ return irq; >+ >+ txp = devm_kzalloc(dev, sizeof(*txp), GFP_KERNEL); >+ if (!txp) >+ return -ENOMEM; >+ >+ txp->pdev = pdev; >+ >+ txp->regs = vc4_ioremap_regs(pdev, 0); >+ if (IS_ERR(txp->regs)) >+ return PTR_ERR(txp->regs); >+ >+ drm_connector_helper_add(&txp->connector.base, >+ &vc4_txp_connector_helper_funcs); >+ ret = drm_writeback_connector_init(drm, &txp->connector, >+ &vc4_txp_connector_funcs, >+ NULL, >+ vc4_txp_formats, >+ ARRAY_SIZE(vc4_txp_formats)); >+ if (ret) >+ return ret; >+ >+ ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0, >+ dev_name(dev), txp); >+ if (ret) >+ return ret; >+ >+ dev_set_drvdata(dev, txp); >+ vc4->txp = txp; >+ >+ return 0; >+} >+ >+static void vc4_txp_unbind(struct device *dev, struct device *master, >+ void *data) >+{ >+ struct drm_device *drm = dev_get_drvdata(master); >+ struct vc4_dev *vc4 = to_vc4_dev(drm); >+ struct vc4_txp *txp = dev_get_drvdata(dev); >+ >+ vc4_txp_connector_destroy(&txp->connector.base); >+ >+ vc4->txp = NULL; >+} >+ >+static const struct component_ops vc4_txp_ops = { >+ .bind = vc4_txp_bind, >+ .unbind = vc4_txp_unbind, >+}; >+ >+static int vc4_txp_dev_probe(struct platform_device *pdev) >+{ >+ return component_add(&pdev->dev, &vc4_txp_ops); >+} >+ >+static int vc4_txp_dev_remove(struct platform_device *pdev) >+{ >+ component_del(&pdev->dev, &vc4_txp_ops); >+ return 0; >+} >+ >+static const struct of_device_id vc4_txp_dt_match[] = { >+ { .compatible = "brcm,bcm2835-txp" }, >+ { /* sentinel */ }, >+}; >+ >+struct platform_driver vc4_txp_driver = { >+ .probe = vc4_txp_dev_probe, >+ .remove = vc4_txp_dev_remove, >+ .driver = { >+ .name = "vc4_txp", >+ .of_match_table = vc4_txp_dt_match, >+ }, >+}; >-- >2.7.4 > -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block 2017-06-05 11:25 ` Brian Starkey @ 2017-06-06 19:13 ` Boris Brezillon 2017-06-13 10:34 ` Brian Starkey 0 siblings, 1 reply; 26+ messages in thread From: Boris Brezillon @ 2017-06-06 19:13 UTC (permalink / raw) To: Brian Starkey Cc: Mark Rutland, Lee Jones, Liviu Dudau, dri-devel, Gerd Hoffmann, Florian Fainelli, Eben Upton, bcm-kernel-feedback-list, devicetree, Pawel Moll, Stephen Warren, Ray Jui, Rob Herring, linux-rpi-kernel, Hollingworth, Gordon, Cobley, Dom, Ian Campbell, Scott Branden, Kumar Gala, Shawn Guo Hi Brian, Le Mon, 5 Jun 2017 12:25:50 +0100, Brian Starkey <brian.starkey@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? > > >+ 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. Thanks for the review, Boris _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block 2017-06-06 19:13 ` Boris Brezillon @ 2017-06-13 10:34 ` Brian Starkey 2017-06-13 17:32 ` Eric Anholt 0 siblings, 1 reply; 26+ messages in thread From: Brian Starkey @ 2017-06-13 10:34 UTC (permalink / raw) To: Boris Brezillon Cc: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block 2017-06-13 10:34 ` Brian Starkey @ 2017-06-13 17:32 ` Eric Anholt [not found] ` <87efunaj4o.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 0 siblings, 1 reply; 26+ messages in thread From: Eric Anholt @ 2017-06-13 17:32 UTC (permalink / raw) To: Brian Starkey, Boris Brezillon Cc: Mark Rutland, Lee Jones, Liviu Dudau, dri-devel, Gerd Hoffmann, Florian Fainelli, Eben Upton, bcm-kernel-feedback-list, devicetree, Pawel Moll, Stephen Warren, Ray Jui, Rob Herring, linux-rpi-kernel, Hollingworth, Gordon, Cobley, Dom, Ian Campbell, Scott Branden, Kumar Gala, Shawn Guo [-- Attachment #1.1: Type: text/plain, Size: 4271 bytes --] Brian Starkey <brian.starkey@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@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. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
[parent not found: <87efunaj4o.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org>]
* Re: [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block [not found] ` <87efunaj4o.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> @ 2017-06-14 8:23 ` Brian Starkey 0 siblings, 0 replies; 26+ messages in thread From: Brian Starkey @ 2017-06-14 8:23 UTC (permalink / raw) To: Eric Anholt Cc: Boris Brezillon, David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TZNg+MwTxZMZA On Tue, Jun 13, 2017 at 10:32:23AM -0700, Eric Anholt wrote: >Brian Starkey <brian.starkey-5wv7dgnIgG8@public.gmane.org> 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-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. > >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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 6/7] dt-bindings: Document the VC4 TXP module nodes. [not found] ` <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> ` (3 preceding siblings ...) 2017-06-02 8:32 ` [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block Boris Brezillon @ 2017-06-02 8:32 ` Boris Brezillon [not found] ` <1496392332-8722-7-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-02 8:32 ` [PATCH 7/7] ARM: dts: bcm283x: Add Transposer block Boris Brezillon 5 siblings, 1 reply; 26+ messages in thread From: Boris Brezillon @ 2017-06-02 8:32 UTC (permalink / raw) To: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Boris Brezillon, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey These are part of the vc4 display pipeline. Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --- Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt index ca02d3e4db91..aeed89cc7f05 100644 --- a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt +++ b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt @@ -20,6 +20,12 @@ Required properties for HVS: - interrupts: The interrupt number See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt +Required properties for TXP: +- compatible: Should be "brcm,bcm2835-txp" +- reg: Physical base address and length of the TXP's registers +- interrupts: The interrupt number + See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt + Required properties for HDMI - compatible: Should be "brcm,bcm2835-hdmi" - reg: Physical base address and length of the two register ranges @@ -86,6 +92,12 @@ hvs@7e400000 { interrupts = <2 1>; }; +txp@7e004000 { + compatible = "brcm,bcm2835-txp"; + reg = <0x7e004000 0x20>; + interrupts = <1 11>; +}; + hdmi: hdmi@7e902000 { compatible = "brcm,bcm2835-hdmi"; reg = <0x7e902000 0x600>, -- 2.7.4 -- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
[parent not found: <1496392332-8722-7-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 6/7] dt-bindings: Document the VC4 TXP module nodes. [not found] ` <1496392332-8722-7-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2017-06-06 20:30 ` Eric Anholt 2017-06-07 22:21 ` Rob Herring 1 sibling, 0 replies; 26+ messages in thread From: Eric Anholt @ 2017-06-06 20:30 UTC (permalink / raw) To: Boris Brezillon, David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey [-- Attachment #1: Type: text/plain, Size: 331 bytes --] Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes: > These are part of the vc4 display pipeline. > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> Patch 6-7 are: Reviewed-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/7] dt-bindings: Document the VC4 TXP module nodes. [not found] ` <1496392332-8722-7-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-06 20:30 ` Eric Anholt @ 2017-06-07 22:21 ` Rob Herring 1 sibling, 0 replies; 26+ messages in thread From: Rob Herring @ 2017-06-07 22:21 UTC (permalink / raw) To: Boris Brezillon Cc: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth On Fri, Jun 02, 2017 at 10:32:11AM +0200, Boris Brezillon wrote: > These are part of the vc4 display pipeline. > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> > --- > Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt | 12 ++++++++++++ > 1 file changed, 12 insertions(+) Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 7/7] ARM: dts: bcm283x: Add Transposer block [not found] ` <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> ` (4 preceding siblings ...) 2017-06-02 8:32 ` [PATCH 6/7] dt-bindings: Document the VC4 TXP module nodes Boris Brezillon @ 2017-06-02 8:32 ` Boris Brezillon 5 siblings, 0 replies; 26+ messages in thread From: Boris Brezillon @ 2017-06-02 8:32 UTC (permalink / raw) To: David Airlie, Daniel Vetter, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Boris Brezillon, Eric Anholt, Sean Paul, Gerd Hoffmann, Mark Yao, Shawn Guo Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Stephen Warren, Lee Jones, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, Eben Upton, Hollingworth, Gordon, Cobley, Dom, Liviu Dudau, Brian Starkey The transposer block is allowing one to write the result of the VC4 composition back to memory. Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --- arch/arm/boot/dts/bcm283x.dtsi | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi index 35cea3fcaf5c..d5b11483556c 100644 --- a/arch/arm/boot/dts/bcm283x.dtsi +++ b/arch/arm/boot/dts/bcm283x.dtsi @@ -35,6 +35,12 @@ clock-frequency = <1000000>; }; + txp@7e004000 { + compatible = "brcm,bcm2835-txp"; + reg = <0x7e004000 0x20>; + interrupts = <1 11>; + }; + dma: dma@7e007000 { compatible = "brcm,bcm2835-dma"; reg = <0x7e007000 0xf00>; -- 2.7.4 -- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-06-21 17:25 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-02 8:32 [PATCH 0/7] drm/vc4: Add writeback support to the VC4 driver Boris Brezillon 2017-06-02 8:32 ` [PATCH 1/7] drm: Add drm_atomic_helper_wait_for_flip_done() Boris Brezillon [not found] ` <1496392332-8722-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-02 8:32 ` [PATCH 2/7] drm/vc4: Fix vblank handling Boris Brezillon [not found] ` <1496392332-8722-3-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-15 23:30 ` Eric Anholt 2017-06-16 7:47 ` Boris Brezillon 2017-06-02 8:32 ` [PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior Boris Brezillon [not found] ` <1496392332-8722-4-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-06 20:27 ` Eric Anholt [not found] ` <87k24orheq.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 2017-06-06 20:48 ` Boris Brezillon 2017-06-07 17:46 ` Eric Anholt [not found] ` <87o9tzg06z.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 2017-06-15 23:33 ` Eric Anholt [not found] ` <87d1a4u8qw.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 2017-06-16 7:19 ` Boris Brezillon 2017-06-21 17:25 ` Eric Anholt 2017-06-13 10:10 ` Boris Brezillon 2017-06-02 8:32 ` [PATCH 4/7] drm/vc4: Use drm_atomic_helper_wait_for_flip_done() Boris Brezillon [not found] ` <1496392332-8722-5-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-06 20:24 ` Eric Anholt [not found] ` <87lgp4rhj2.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 2017-06-06 20:59 ` Boris Brezillon 2017-06-02 8:32 ` [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block Boris Brezillon [not found] ` <1496392332-8722-6-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-05 11:25 ` Brian Starkey 2017-06-06 19:13 ` Boris Brezillon 2017-06-13 10:34 ` Brian Starkey 2017-06-13 17:32 ` Eric Anholt [not found] ` <87efunaj4o.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> 2017-06-14 8:23 ` Brian Starkey 2017-06-02 8:32 ` [PATCH 6/7] dt-bindings: Document the VC4 TXP module nodes Boris Brezillon [not found] ` <1496392332-8722-7-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-06 20:30 ` Eric Anholt 2017-06-07 22:21 ` Rob Herring 2017-06-02 8:32 ` [PATCH 7/7] ARM: dts: bcm283x: Add Transposer block Boris Brezillon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.