Re: [PATCH 5/5] drm/i915: Add YCBCR 4:2:0 support for LSPCON

["Sharma, Shashank" <shashank.sharma@xxxxxxxxx>]

Thanks for the review Maarten,

My comments inline.

Regards
Shashank
On 12/19/2017 12:53 AM, Maarten Lankhorst wrote:
> Op 14-11-17 om 16:17 schreef Shashank Sharma:
> > LSPCON chips are capable of generating YCBCR 4:2:0 outputs, if asked
> > nicely :). In order to generate YCBCR 4:2:0 outputs, a source must:
> > - send YCBCR 4:4:4 signals to LSPCON
> > - program color space as 4:2:0 in AVI infoframes
> >
> > This will indicate LSPCON FW to start scaling down from YCBCR 4:4:4
> > and generate YCBCR 4:2:0 output. Unlike HDMI 2.0 native case, as the
> > scaling is done by LSPCON device, we need not to reserve a scaler for
> > 4:2:0 outputs.
> >
> > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h      |  2 ++
> >   drivers/gpu/drm/i915/intel_ddi.c     |  8 ++++++++
> >   drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
> >   drivers/gpu/drm/i915/intel_dp.c      |  7 ++++++-
> >   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >   drivers/gpu/drm/i915/intel_lspcon.c  | 22 ++++++++++++++++++++++
> >   6 files changed, 51 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f0f8f60..ea6ef5e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8495,6 +8495,8 @@ enum skl_power_gate {
> >   #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC)
> >   
> >   #define  TRANS_MSA_SYNC_CLK		(1<<0)
> > +#define  TRANS_MSA_SAMPLING_444        (2<<1)
> > +#define  TRANS_MSA_CLRSP_YCBCR		(2<<3)
> >   #define  TRANS_MSA_6_BPC		(0<<5)
> >   #define  TRANS_MSA_8_BPC		(1<<5)
> >   #define  TRANS_MSA_10_BPC		(2<<5)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 9e2ab02..3fd839d 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1499,6 +1499,14 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
> >   		break;
> >   	}
> >   
> > +	/*
> > +	 * To get YCBCR 420 output from LSPCON, we should send YCBCR 444
> > +	 * signals. And as per DP 1.2 spec section 2.3.4.3 while sending
> > +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
> > +	 * colorspace information.
> > +	 */
> > +	if (crtc_state->lspcon_active && crtc_state->ycbcr420)
> > +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
> >   	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
> >   }
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 737de25..787119b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4638,7 +4638,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
> >   	 */
> >   	need_scaling = src_w != dst_w || src_h != dst_h;
> >   
> > -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> > +	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX &&
> > +		!crtc_state->lspcon_active)
> >   		need_scaling = true;
> >   
> >   	/*
> > @@ -8133,9 +8134,15 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
> >   			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
> >   
> >   		if (config->ycbcr420) {
> > -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
> > -				PIPEMISC_YUV420_ENABLE |
> > -				PIPEMISC_YUV420_MODE_FULL_BLEND;
> > +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> > +			/*
> > +			 * LSPCON doesn't need scaling/blending to be done in
> > +			 * pipe. It just needs YCBCR444 input and proper AVI
> > +			 * infoframes for 4:2:0 output enabling.
> > +			 */
> > +			if (!config->lspcon_active)
> > +				val |= PIPEMISC_YUV420_ENABLE |
> > +				       PIPEMISC_YUV420_MODE_FULL_BLEND;
> >   		}
> >   
> >   		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index fa9e5e6..1d4b669 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1657,8 +1657,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >   						    intel_dp->max_link_rate);
> >   
> >   	/* LSPCON needs special handling to drive YCBCR420 outputs */
> > -	if (lspcon->active)
> > +	if (lspcon->active) {
> > +		struct drm_connector *connector = &intel_connector->base;
> > +
> >   		pipe_config->lspcon_active = true;
> > +		pipe_config->ycbcr420 = lspcon_ycbcr420_config(connector,
> > +							       pipe_config);
> > +	}
> >   
> >   	/* No common link rates between source and sink */
> >   	WARN_ON(common_len <= 0);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a468dd6..f271967 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
> >   			   const struct drm_connector_state *conn_state);
> >   bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
> >   			      const struct intel_crtc_state *pipe_config);
> > +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> > +			     struct intel_crtc_state *config);
> >   
> >   /* intel_pipe_crc.c */
> >   int intel_pipe_crc_create(struct drm_minor *minor);
> > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> > index 77f0687..ca77f50 100644
> > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > @@ -180,6 +180,26 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
> >   	return true;
> >   }
> >   
> > +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> > +			     struct intel_crtc_state *config)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> > +
> > +	if (drm_mode_is_420_only(info, mode)) {
> > +
> > +		if (!connector->ycbcr_420_allowed) {
> > +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
> > +			return false;
> > +		}
> Seems to be always true if lspcon->active is true?
Actually you are correct, if HAS_LSPCON && lspcon->active, ycbcr_420 
will be always allowed. But as we added this flag specifically to 
control 4:2:0 support from connector, for HDMI 2.0 native controllers, I 
though it would be more readable to use this flag. Do you think so ?
> > +		config->port_clock /= 2;
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >   static bool lspcon_probe(struct intel_lspcon *lspcon)
> >   {
> >   	int retry;
> > @@ -516,6 +536,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >   	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
> >   	struct drm_device *dev = intel_dig_port->base.base.dev;
> >   	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_connector *connector = &dp->attached_connector->base;
> >   
> >   	if (!HAS_LSPCON(dev_priv)) {
> >   		DRM_ERROR("LSPCON is not supported on this platform\n");
> > @@ -540,6 +561,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >   		return false;
> >   	}
> >   
> > +	connector->ycbcr_420_allowed = true;
> Is this flag different from lspcon->active?
Yes, when we designed HDMI 2.0 native controller from GLK and plus 
platforms, we added this connector flag, so that, for the platforms 
which do not support 4:2:0 outputs, 4:2:0 only modes will be rejected 
from EDID's list of modes. This is the place where connector allows the 
4:2:0 capability, based on the platform, whereas is_lspcon is fro the 
encoder and state.
> >   	lspcon->active = true;
> >   	DRM_DEBUG_KMS("Success: LSPCON init\n");
> >   	return true;
> Comments from Ville still apply too. The pipe is set to ycbcr mode, so it makes more sense to have a ycbcr444 flag or something, and a separate flag to downconvert lspcon.
>
> It would be really nice if we could set the ycbcr444 flag in hw readout, and intel_pipe_config_compare. This way we know the sw mode matches the hw mode. :)
We will eventually need the right colorspace information while 
programming the AVI IF.
Actually we are trying to accommodate LSPCON into HDMI 2.0 framework as 
such. If you analyze the native 4:2:0 framework, there also we need 
scaling down, for which we use PIPEMISC register. So the requirement for 
GLK would be:
- one flag like YCBCR444 for pipe config (say state->444) to set CSC
- one flag in state to let modeset know that we have to program PIPEMISC 
and reserve a pipe_scaler (say state->420)
- one flag to set avi_if colorspace to ycbcr420 (again state->420, here 
we can't use 444, as we need accurate colorspace)

Now, in case of LSPCON,
- I still need state->444 to set CSC
- In modeset, I don't need to program PIPEMISC and scalar, as scaling 
happens in LSPCON internally so I need to know if this is LSPCON (hence 
state->lspcon)
- One flag to set appropriate avi_infoframes (so state->420)

So I realized even if I use state->444, I still need a state->420 flags 
to set correct colorspace in the avi infoframes, so there is no benefit 
of making this change.

Please let me know your opinion.

- Shashank
> ~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx