Re: [PATCH 05/11] drm/vc4: Add support for feeding DSI encoders from the pixel valve.

Eric Anholt
2017-01-31T19:54:04Z

[-- Attachment #1: Type: text/plain, Size: 4482 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]