Hi Eric,
On Fri, 29 Jun 2018 13:35:04 -0700
Eric Anholt <eric@xxxxxxxxxx> wrote:
> Boris Brezillon <boris.brezillon@xxxxxxxxxxx> writes:
>
> > From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> >
> > The transposer block is providing support for mem-to-mem composition,
> > which is exposed as a drm_writeback connector in DRM.
> >
> > Add a driver to support this feature.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
>
> > +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;
> > + u32 dsp3_mux;
> > +
> > + /*
> > + * 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 prevent disable the
>
> s/prevent //
>
> > + * FIFO2 -> pixelvalve1 route.
> > + */
> > + if (vc4_state->feed_txp)
> > + dsp3_mux = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX);
> > + else
> > + dsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
> > +
> > + /* Reconfigure the DSP3 mux if required. */
> > + dispctrl = HVS_READ(SCALER_DISPCTRL);
> > + if ((dispctrl & SCALER_DISPCTRL_DSP3_MUX_MASK) != dsp3_mux) {
> > + dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;
> > + HVS_WRITE(SCALER_DISPCTRL, dispctrl | dsp3_mux);
> > + }
>
> This is fine, but you could also skip the matching mux check here -- the
> read is the expensive part.
>
> > + }
> > +
> > + if (!vc4_state->feed_txp)
> > + vc4_crtc_config_pv(crtc);
> >
> > HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel),
> > SCALER_DISPBKGND_AUTOHS |
> > @@ -499,6 +539,13 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
> > }
> > }
> >
> > +void vc4_crtc_txp_armed(struct drm_crtc_state *state)
> > +{
> > + struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
> > +
> > + vc4_state->txp_armed = true;
> > +}
> > +
> > static void vc4_crtc_update_dlist(struct drm_crtc *crtc)
> > {
> > struct drm_device *dev = crtc->dev;
> > @@ -514,8 +561,11 @@ static void vc4_crtc_update_dlist(struct drm_crtc *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;
> > +
> > + if (!vc4_state->feed_txp || vc4_state->txp_armed) {
> > + vc4_crtc->event = crtc->state->event;
> > + crtc->state->event = NULL;
> > + }
> >
> > HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel),
> > vc4_state->mm.start);
> > @@ -533,8 +583,8 @@ static void vc4_crtc_atomic_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);
> >
> > @@ -546,15 +596,21 @@ static void vc4_crtc_atomic_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
>
> "transposer block"? composer block made me think HVS.
>
> > + * should not be enabled.
> > + */
> > + if (!vc4_state->feed_txp)
> > + CRTC_WRITE(PV_V_CONTROL,
> > + CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
> > }
> >
> > static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc,
> > @@ -579,8 +635,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.)
> > @@ -600,6 +658,22 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
> > if (ret)
> > return ret;
> >
> > + state->no_vblank = false;
> > + for_each_new_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 the HVS FIFO.
> > + */
> > + if (conn->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) {
> > + state->no_vblank = true;
> > + vc4_state->feed_txp = true;
> > + }
> > +
> > + break;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -713,7 +787,8 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc)
> >
> > 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)) ||
> > + vc4_state->feed_txp)) {
>
> Can vc4_crtc->event even be set if vc4_state->feed_txp?
>
> > diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
> > index d6864fa4bd14..744a689751f0 100644
> > --- a/drivers/gpu/drm/vc4/vc4_regs.h
> > +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> > @@ -330,6 +330,7 @@
> > #define SCALER_DISPCTRL0 0x00000040
> > # define SCALER_DISPCTRLX_ENABLE BIT(31)
> > # define SCALER_DISPCTRLX_RESET BIT(30)
> > +
> > /* Generates a single frame when VSTART is seen and stops at the last
> > * pixel read from the FIFO.
> > */
>
> stray hunk?
>
> > +static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
> > + struct drm_connector_state *conn_state)
> > +{
> > + struct vc4_txp *txp = connector_to_vc4_txp(conn);
> > + struct drm_gem_cma_object *gem;
> > + struct drm_display_mode *mode;
> > + struct drm_framebuffer *fb;
> > + u32 ctrl = TXP_GO | TXP_VSTART_AT_EOF | TXP_EI;
> > +
> > + if (WARN_ON(!conn_state->writeback_job ||
> > + !conn_state->writeback_job->fb))
> > + return;
> > +
> > + mode = &conn_state->crtc->state->adjusted_mode;
> > + fb = conn_state->writeback_job->fb;
> > +
> > + switch (fb->format->format) {
> > + case DRM_FORMAT_ARGB8888:
> > + ctrl |= TXP_ALPHA_ENABLE;
>
> Optional suggestion: Have the txp_formats[] table be a struct with these
> register values in it.
>
> All my feedback seems really minor, so with whatever components you like
> integrated, feel free to add:
Will fix all the things you pointed in your review.
Thanks,
Boris
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
