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

[Eric Anholt <eric@anholt.net>]

[-- 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 --]