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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 12, 2018 at 11:21:56PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 10/12/2018 7:28 PM, Ville Syrjälä wrote:
> > On Fri, Oct 12, 2018 at 11:53:14AM +0530, Shashank Sharma wrote:
> >> From: Shashank Sharma <shashank.sharma@xxxxxxxxxxxxxxx>
> >>
> >> LSPCON chips can generate YCBCR 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
> >>
> >> Whereas for YCBCR 4:4:4 outputs, the source must:
> >> - send YCBCR 4:4:4 signals to LSPCON
> >> - program color space as 4:4:4 in AVI infoframes
> >>
> >> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
> >> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
> >> information indicates LSPCON FW to start scaling down from YCBCR
> >> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
> >> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
> >>
> >> V2: rebase
> >> V3: Addressed review comments from Ville
> >>      - add enum crtc_output_format instead of bool ycbcr420
> >>      - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
> >>        cases in this way we will have YCBCR 4:4:4 framework ready (except
> >>        the ABI part)
> >> V4: Added r-b from Maarten (for v3)
> >>      Addressed review comments from Ville:
> >>      - Do not add a non-atomic state variable to determine lspcon output.
> >>        Instead add bool in CRTC state to indicate lspcon based scaling.
> >> V5: Addressed review comments from Ville:
> >>      - Change the state bool name from external scaling to something more
> >>        relavent.
> >>      - Keep the info and adjusted_mode structures const.
> >>      - use crtc_state instead of pipe_config.
> >>      - Push all the config change into lspcon_ycbcr420_config function.
> >> V6: Rebase, small changes to accommodate changes in patch 2.
> >> V7: Fixed checkpatch warnings for alignment
> >> V8: Rebase
> >>
> >>      PS: Ignored following warnings to match the current formatting:
> >>      drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
> >>       -:53: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> >>      #53: FILE: drivers/gpu/drm/i915/i915_reg.h:8721:
> >>      +#define  TRANS_MSA_SAMPLING_444        (2<<1)
> >>                                            ^
> >>      -:54: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> >>      #54: FILE: drivers/gpu/drm/i915/i915_reg.h:8722:
> >>      +#define  TRANS_MSA_CLRSP_YCBCR         (2<<3)
> >> V9: Rebase
> >> V10: Rebase
> >> V11: Rebase
> >>
> >> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> >> ---
> >>   drivers/gpu/drm/i915/i915_reg.h      |  2 ++
> >>   drivers/gpu/drm/i915/intel_ddi.c     |  7 +++++++
> >>   drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++
> >>   drivers/gpu/drm/i915/intel_dp.c      |  4 ++++
> >>   drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
> >>   drivers/gpu/drm/i915/intel_lspcon.c  | 26 ++++++++++++++++++++++++++
> >>   6 files changed, 56 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index 2078541..1e13e51 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -9229,6 +9229,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 be21131..186111a 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -1784,6 +1784,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
> >>   		break;
> >>   	}
> >>   
> >> +	/*
> >> +	 * 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. The output colorspace encoding is BT601.
> >> +	 */
> >> +	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
> >> +		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 3317c6c..41abd03 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -7797,6 +7797,8 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc,
> >>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>   	enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB;
> >>   
> >> +	pipe_config->lspcon_downsampling = false;
> >> +
> >>   	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
> >>   		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
> >>   
> >> @@ -7814,6 +7816,16 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc,
> >>   				else
> >>   					output = INTEL_OUTPUT_FORMAT_YCBCR420;
> >>   			} else {
> >> +				/*
> >> +				 * Currently there is no interface defined to
> >> +				 * check user preference between RGB/YCBCR444
> >> +				 * or YCBCR420. So the only possible case for
> >> +				 * YCBCR444 usage is driving YCBCR420 output
> >> +				 * with LSPCON, when pipe is configured for
> >> +				 * YCBCR444 output and LSPCON takes care of
> >> +				 * downsampling it.
> >> +				 */
> >> +				pipe_config->lspcon_downsampling = true;
> >>   				output = INTEL_OUTPUT_FORMAT_YCBCR444;
> >>   			}
> >>   		}
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index c3f6330..1156d59 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -2074,6 +2074,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >>   	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >>   	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >>   	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >> +	struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
> >>   	enum port port = encoder->port;
> >>   	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
> >>   	struct intel_connector *intel_connector = intel_dp->attached_connector;
> >> @@ -2086,6 +2087,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >>   		pipe_config->has_pch_encoder = true;
> >>   
> >>   	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> >> +	if (lspcon->active)
> >> +		lspcon_ycbcr420_config(&intel_connector->base, pipe_config);
> >> +
> >>   	pipe_config->has_drrs = false;
> >>   	if (IS_G4X(dev_priv) || port == PORT_A)
> >>   		pipe_config->has_audio = false;
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index b8e5f95..95e252e 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -908,6 +908,9 @@ struct intel_crtc_state {
> >>   
> >>   	/* Output format RGB/YCBCR etc */
> >>   	enum intel_output_format output_format;
> >> +
> >> +	/* Output down scaling is done in LSPCON device */
> >> +	bool lspcon_downsampling;
> >>   };
> >>   
> >>   struct intel_crtc {
> >> @@ -2225,6 +2228,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder,
> >>   			   const struct drm_connector_state *conn_state);
> >>   bool lspcon_infoframe_enabled(struct intel_encoder *encoder,
> >>   			      const struct intel_crtc_state *pipe_config);
> >> +void lspcon_ycbcr420_config(struct drm_connector *connector,
> >> +			    struct intel_crtc_state *crtc_state);
> >>   
> >>   /* intel_pipe_crc.c */
> >>   #ifdef CONFIG_DEBUG_FS
> >> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> >> index 829c40a..fff32b3 100644
> >> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> >> @@ -180,6 +180,21 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
> >>   	return true;
> >>   }
> >>   
> >> +void lspcon_ycbcr420_config(struct drm_connector *connector,
> >> +			    struct intel_crtc_state *crtc_state)
> >> +{
> >> +	const struct drm_display_info *info = &connector->display_info;
> >> +	const struct drm_display_mode *adjusted_mode =
> >> +					&crtc_state->base.adjusted_mode;
> >> +
> >> +	if (drm_mode_is_420_only(info, adjusted_mode) &&
> >> +	    connector->ycbcr_420_allowed) {
> >> +		crtc_state->port_clock /= 2;
> > This looks bogus. We're talking about DP here so we should not be
> > frobbing port_clock. And since we output 4:4:4 from the port
> > anyway we don't even have to take 4:2:0 into consideration
> > when calculating port_clock.
> I agree on this.
> > Ah, this guy is called before we even calculate port_clock.
> > That explains why this didn't cause any problems. So this is
> > just dead code here.
> >
> > And looks like the port_clock adjustment in intel_hdmi_ycbcr420_config()
> > also dead code since the caller overwrites it there as well.
> I am not sure if that's the case for Native HDMI 2.0.
> In intel_hdmi_420_config we are updating the config->port_clock, which 
> is the same thing being updated for 8/12/16 BPC deep color too, just 
> after this function.
> I have tested the output with HDMI 2.0 analyzer, which reported right 
> pixel clock (297Mhz). Now, as any of the port clock calculations do not 
> get information
> about HDMI output type, there is no other way they will calculate the 
> right pixel clock (594/2) for 4:2:0 outputs. So I believe that code is 
> active.

It effectively does this:

1. port_clock = who knows
2. port_clock /= 2;
3. if (12bpc)
   	port_clock = clock_12bpc;
   else if (10bpc)
   	port_clock = clock_10bpc;
   else
   	port_clock = clock_8bpc;

Ie. step 2. is dead code.

> 
> - Shashank
> >> +		crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444;
> >> +		crtc_state->lspcon_downsampling = true;
> >> +	}
> >> +}
> >> +
> >>   static bool lspcon_probe(struct intel_lspcon *lspcon)
> >>   {
> >>   	int retry;
> >> @@ -464,6 +479,15 @@ void lspcon_set_infoframes(struct intel_encoder *encoder,
> >>   		return;
> >>   	}
> >>   
> >> +	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
> >> +		if (crtc_state->lspcon_downsampling)
> >> +			frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> >> +		else
> >> +			frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
> >> +	} else {
> >> +		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> >> +	}
> >> +
> >>   	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
> >>   					   crtc_state->limited_color_range ?
> >>   					   HDMI_QUANTIZATION_RANGE_LIMITED :
> >> @@ -517,6 +541,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");
> >> @@ -541,6 +566,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >>   		return false;
> >>   	}
> >>   
> >> +	connector->ycbcr_420_allowed = true;
> >>   	lspcon->active = true;
> >>   	DRM_DEBUG_KMS("Success: LSPCON init\n");
> >>   	return true;
> >> -- 
> >> 2.7.4

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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux