| Message ID | 20161214194621.16499-6-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 14, 2016 at 11:46:15AM -0800, Eric Anholt wrote: > We have to set a different pixel format, which tells the hardware to > use the pix_width field that's fed in sideband from the DSI encoder to > divide the "pixel" clock. > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 33 +++++++++++++++++++-------------- > drivers/gpu/drm/vc4/vc4_regs.h | 2 ++ > 2 files changed, 21 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > index a0fd3e66bc4b..cd070e0c79a6 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -349,38 +349,40 @@ static u32 vc4_get_fifo_full_level(u32 format) > } > > /* > - * Returns the clock select bit for the connector attached to the > - * CRTC. > + * Returns the encoder attached to the CRTC. > + * > + * VC4 can only scan out to one encoder at a time, while the DRM core > + * allows drivers to push pixels to more than one encoder from the > + * same CRTC. > */ > -static int vc4_get_clock_select(struct drm_crtc *crtc) > +static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc) > { > struct drm_connector *connector; > > drm_for_each_connector(connector, crtc->dev) { > if (connector->state->crtc == crtc) { > - struct drm_encoder *encoder = connector->encoder; > - struct vc4_encoder *vc4_encoder = > - to_vc4_encoder(encoder); > - > - return vc4_encoder->clock_select; > + return connector->encoder; > } > } > > - return -1; > + return NULL; > } > > 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 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); > struct drm_crtc_state *state = crtc->state; > struct drm_display_mode *mode = &state->adjusted_mode; > bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE; > u32 pixel_rep = (mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1; > - u32 format = PV_CONTROL_FORMAT_24; > + 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; > - int clock_select = vc4_get_clock_select(crtc); > > if (debug_dump_regs) { > DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc)); > @@ -436,17 +438,19 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) > */ > CRTC_WRITE(PV_V_CONTROL, > PV_VCONTROL_CONTINUOUS | > + (is_dsi ? PV_VCONTROL_DSI : 0) | > PV_VCONTROL_INTERLACE | > VC4_SET_FIELD(mode->htotal * pixel_rep / 2, > PV_VCONTROL_ODD_DELAY)); > CRTC_WRITE(PV_VSYNCD_EVEN, 0); > } else { > - CRTC_WRITE(PV_V_CONTROL, PV_VCONTROL_CONTINUOUS); > + CRTC_WRITE(PV_V_CONTROL, > + PV_VCONTROL_CONTINUOUS | > + (is_dsi ? PV_VCONTROL_DSI : 0)); > } > > CRTC_WRITE(PV_HACT_ACT, mode->hdisplay * pixel_rep); > > - > CRTC_WRITE(PV_CONTROL, > VC4_SET_FIELD(format, PV_CONTROL_FORMAT) | > VC4_SET_FIELD(vc4_get_fifo_full_level(format), > @@ -455,7 +459,8 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) > PV_CONTROL_CLR_AT_START | > PV_CONTROL_TRIGGER_UNDERFLOW | > PV_CONTROL_WAIT_HSTART | > - VC4_SET_FIELD(clock_select, PV_CONTROL_CLK_SELECT) | > + VC4_SET_FIELD(vc4_encoder->clock_select, > + PV_CONTROL_CLK_SELECT) | Hm, so the usual way we solve the "crtc needs information from the encoder problem" is to add bits to the crtc state, and then fill those out in the encoders ->atomic_check function. In your case ->clock_select and is_dsi. The benefit is mostly when you start doing hw readout (which is great even just to cross-check your modeset code), or when you need that information to check limits (which sooner or later tends to happen ime). Anyway, this works too, just an idea for the future. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > PV_CONTROL_FIFO_CLR | > PV_CONTROL_EN); > > diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h > index b3b297fba709..385405a2df05 100644 > --- a/drivers/gpu/drm/vc4/vc4_regs.h > +++ b/drivers/gpu/drm/vc4/vc4_regs.h > @@ -190,6 +190,8 @@ > # define PV_VCONTROL_ODD_DELAY_SHIFT 6 > # define PV_VCONTROL_ODD_FIRST BIT(5) > # define PV_VCONTROL_INTERLACE BIT(4) > +# define PV_VCONTROL_DSI BIT(3) > +# define PV_VCONTROL_COMMAND BIT(2) > # define PV_VCONTROL_CONTINUOUS BIT(1) > # define PV_VCONTROL_VIDEN BIT(0) > > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter <daniel@ffwll.ch> writes: > On Wed, Dec 14, 2016 at 11:46:15AM -0800, Eric Anholt wrote: >> We have to set a different pixel format, which tells the hardware to >> use the pix_width field that's fed in sideband from the DSI encoder to >> divide the "pixel" clock. >> >> Signed-off-by: Eric Anholt <eric@anholt.net> >> --- >> drivers/gpu/drm/vc4/vc4_crtc.c | 33 +++++++++++++++++++-------------- >> drivers/gpu/drm/vc4/vc4_regs.h | 2 ++ >> 2 files changed, 21 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c >> index a0fd3e66bc4b..cd070e0c79a6 100644 >> --- a/drivers/gpu/drm/vc4/vc4_crtc.c >> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c >> @@ -349,38 +349,40 @@ static u32 vc4_get_fifo_full_level(u32 format) >> } >> >> /* >> - * Returns the clock select bit for the connector attached to the >> - * CRTC. >> + * Returns the encoder attached to the CRTC. >> + * >> + * VC4 can only scan out to one encoder at a time, while the DRM core >> + * allows drivers to push pixels to more than one encoder from the >> + * same CRTC. >> */ >> -static int vc4_get_clock_select(struct drm_crtc *crtc) >> +static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc) >> { >> struct drm_connector *connector; >> >> drm_for_each_connector(connector, crtc->dev) { >> if (connector->state->crtc == crtc) { >> - struct drm_encoder *encoder = connector->encoder; >> - struct vc4_encoder *vc4_encoder = >> - to_vc4_encoder(encoder); >> - >> - return vc4_encoder->clock_select; >> + return connector->encoder; >> } >> } >> >> - return -1; >> + return NULL; >> } >> >> 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 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); >> struct drm_crtc_state *state = crtc->state; >> struct drm_display_mode *mode = &state->adjusted_mode; >> bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE; >> u32 pixel_rep = (mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1; >> - u32 format = PV_CONTROL_FORMAT_24; >> + 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; >> - int clock_select = vc4_get_clock_select(crtc); >> >> if (debug_dump_regs) { >> DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc)); >> @@ -436,17 +438,19 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) >> */ >> CRTC_WRITE(PV_V_CONTROL, >> PV_VCONTROL_CONTINUOUS | >> + (is_dsi ? PV_VCONTROL_DSI : 0) | >> PV_VCONTROL_INTERLACE | >> VC4_SET_FIELD(mode->htotal * pixel_rep / 2, >> PV_VCONTROL_ODD_DELAY)); >> CRTC_WRITE(PV_VSYNCD_EVEN, 0); >> } else { >> - CRTC_WRITE(PV_V_CONTROL, PV_VCONTROL_CONTINUOUS); >> + CRTC_WRITE(PV_V_CONTROL, >> + PV_VCONTROL_CONTINUOUS | >> + (is_dsi ? PV_VCONTROL_DSI : 0)); >> } >> >> CRTC_WRITE(PV_HACT_ACT, mode->hdisplay * pixel_rep); >> >> - >> CRTC_WRITE(PV_CONTROL, >> VC4_SET_FIELD(format, PV_CONTROL_FORMAT) | >> VC4_SET_FIELD(vc4_get_fifo_full_level(format), >> @@ -455,7 +459,8 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) >> PV_CONTROL_CLR_AT_START | >> PV_CONTROL_TRIGGER_UNDERFLOW | >> PV_CONTROL_WAIT_HSTART | >> - VC4_SET_FIELD(clock_select, PV_CONTROL_CLK_SELECT) | >> + VC4_SET_FIELD(vc4_encoder->clock_select, >> + PV_CONTROL_CLK_SELECT) | > > Hm, so the usual way we solve the "crtc needs information from the encoder > problem" is to add bits to the crtc state, and then fill those out in the > encoders ->atomic_check function. In your case ->clock_select and is_dsi. > > The benefit is mostly when you start doing hw readout (which is great even > just to cross-check your modeset code), or when you need that information > to check limits (which sooner or later tends to happen ime). > > Anyway, this works too, just an idea for the future. > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> I like the idea! I'll try following up with that.
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index a0fd3e66bc4b..cd070e0c79a6 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -349,38 +349,40 @@ static u32 vc4_get_fifo_full_level(u32 format) } /* - * Returns the clock select bit for the connector attached to the - * CRTC. + * Returns the encoder attached to the CRTC. + * + * VC4 can only scan out to one encoder at a time, while the DRM core + * allows drivers to push pixels to more than one encoder from the + * same CRTC. */ -static int vc4_get_clock_select(struct drm_crtc *crtc) +static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc) { struct drm_connector *connector; drm_for_each_connector(connector, crtc->dev) { if (connector->state->crtc == crtc) { - struct drm_encoder *encoder = connector->encoder; - struct vc4_encoder *vc4_encoder = - to_vc4_encoder(encoder); - - return vc4_encoder->clock_select; + return connector->encoder; } } - return -1; + return NULL; } 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 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); struct drm_crtc_state *state = crtc->state; struct drm_display_mode *mode = &state->adjusted_mode; bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE; u32 pixel_rep = (mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1; - u32 format = PV_CONTROL_FORMAT_24; + 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; - int clock_select = vc4_get_clock_select(crtc); if (debug_dump_regs) { DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc)); @@ -436,17 +438,19 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) */ CRTC_WRITE(PV_V_CONTROL, PV_VCONTROL_CONTINUOUS | + (is_dsi ? PV_VCONTROL_DSI : 0) | PV_VCONTROL_INTERLACE | VC4_SET_FIELD(mode->htotal * pixel_rep / 2, PV_VCONTROL_ODD_DELAY)); CRTC_WRITE(PV_VSYNCD_EVEN, 0); } else { - CRTC_WRITE(PV_V_CONTROL, PV_VCONTROL_CONTINUOUS); + CRTC_WRITE(PV_V_CONTROL, + PV_VCONTROL_CONTINUOUS | + (is_dsi ? PV_VCONTROL_DSI : 0)); } CRTC_WRITE(PV_HACT_ACT, mode->hdisplay * pixel_rep); - CRTC_WRITE(PV_CONTROL, VC4_SET_FIELD(format, PV_CONTROL_FORMAT) | VC4_SET_FIELD(vc4_get_fifo_full_level(format), @@ -455,7 +459,8 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) PV_CONTROL_CLR_AT_START | PV_CONTROL_TRIGGER_UNDERFLOW | PV_CONTROL_WAIT_HSTART | - VC4_SET_FIELD(clock_select, PV_CONTROL_CLK_SELECT) | + VC4_SET_FIELD(vc4_encoder->clock_select, + PV_CONTROL_CLK_SELECT) | PV_CONTROL_FIFO_CLR | PV_CONTROL_EN); diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h index b3b297fba709..385405a2df05 100644 --- a/drivers/gpu/drm/vc4/vc4_regs.h +++ b/drivers/gpu/drm/vc4/vc4_regs.h @@ -190,6 +190,8 @@ # define PV_VCONTROL_ODD_DELAY_SHIFT 6 # define PV_VCONTROL_ODD_FIRST BIT(5) # define PV_VCONTROL_INTERLACE BIT(4) +# define PV_VCONTROL_DSI BIT(3) +# define PV_VCONTROL_COMMAND BIT(2) # define PV_VCONTROL_CONTINUOUS BIT(1) # define PV_VCONTROL_VIDEN BIT(0)
We have to set a different pixel format, which tells the hardware to use the pix_width field that's fed in sideband from the DSI encoder to divide the "pixel" clock. Signed-off-by: Eric Anholt <eric@anholt.net> --- drivers/gpu/drm/vc4/vc4_crtc.c | 33 +++++++++++++++++++-------------- drivers/gpu/drm/vc4/vc4_regs.h | 2 ++ 2 files changed, 21 insertions(+), 14 deletions(-)