From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Starkey Subject: Re: [PATCH 5/7] drm/vc4: Add support for the TXP (transposer) block Date: Tue, 13 Jun 2017 11:34:04 +0100 Message-ID: <20170613103403.GA3989@e106950-lin.cambridge.arm.com> References: <1496392332-8722-1-git-send-email-boris.brezillon@free-electrons.com> <1496392332-8722-6-git-send-email-boris.brezillon@free-electrons.com> <20170605112549.GA32480@e106950-lin.cambridge.arm.com> <20170606211300.61e93ec1@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <20170606211300.61e93ec1@bbrezillon> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Boris Brezillon Cc: David Airlie , Daniel Vetter , dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, Eric Anholt , Sean Paul , Gerd Hoffmann , Mark Yao , Shawn Guo , Florian Fainelli , Ray Jui , Scott Branden , bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org, Stephen Warren , Lee Jones , linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Eben Upton List-Id: devicetree@vger.kernel.org Hi Boris, Sorry lost track of this thread. On Tue, Jun 06, 2017 at 09:13:00PM +0200, Boris Brezillon wrote: >Hi Brian, > >Le Mon, 5 Jun 2017 12:25:50 +0100, >Brian Starkey a écrit : > >> Hi Boris, >> >> I can't speak for the HW-specific details, but the writeback part >> looks pretty good (and familiar ;-) to me. There certainly seems to be >> some scope for code-sharing there, but I think it's fine not to do it >> yet. > >Agreed. > >> >> I've a further query below about the handling of CRTC events. >> > >[...] > >> >+ >> >+void vc4_txp_atomic_commit(struct drm_device *dev, >> >+ struct drm_atomic_state *old_state) >> >+{ >> >+ struct vc4_dev *vc4 = to_vc4_dev(dev); >> >+ struct vc4_txp *txp = vc4->txp; >> >+ struct drm_connector_state *conn_state = txp->connector.base.state; >> >+ struct drm_display_mode *mode; >> >+ struct drm_gem_cma_object *gem; >> >+ struct drm_framebuffer *fb; >> >+ u32 ctrl = TXP_GO | TXP_EI; >> >+ >> >+ /* Writeback connector is disabled, nothing to do. */ >> >+ if (!conn_state->crtc) >> >+ return; >> >+ >> >+ /* Writeback connector is enabled, but has no FB assigned to it. Fake a >> >+ * vblank event to complete ->flip_done. >> >+ */ >> >+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb) { >> >+ vc4_crtc_eof_event(conn_state->crtc); >> >> I'm not sure about hiding away the one-shot thing like this. If the >> CRTC remains "active" from the API point of view, I'd expect it to be >> able to keep generating VBLANK events. >> >> I don't know how to do if, but if there were some notion of >> "auto-disabling" CRTCs then this quirk would go away, and you'd also >> be able to enforce that the CRTC can't be enabled without a writeback >> framebuffer. > >I don't have a strong opinion on "auto-disabling CRTC" vs "fake VBLANK >events". Note that I'm already faking a VBLANK event when activating >writeback, because there's no such concept at the HVS/TXP level. We >do have EOF (End Of Frame) and writeback done events, but these are >quite independent from the VBLANK events generated by the pixelvalve >block (the block responsible for generating the HSYNC/VSYNC signals). > >> >> I'm also not sure how (if?) this works today with a CRTC driving a DSI >> command-mode panel. Does the CRTC keep generating VBLANKs even when >> there are no updates? > >I don't know. Is this mandatory to have VBLANK events? I mean, the >core is using VBLANK events to detect when page flips have been done, >that is, when old framebuffers are unused and new ones started to be >fetched by the CRTC, but on some HW, VBLANK is not the only way to >detect such situations. The question is, are there other situations >where VBLANK events are required, or is there an implicit rule stating >that a CRTC has to generate VBLANK events at a vrefresh rate even when >the display is actually not updated when nothing changes? I'm not sure how widely relied upon it is, but I'd expect that there's a requirement to make sure DRM_IOCTL_WAIT_VBLANK works. I _think_ that means the CRTC should generate events at vrefresh rate if there's a wait request outstanding that depends on that. 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