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]

 

Regards

Shashank


On 10/12/2018 11:39 PM, Ville Syrjälä wrote:

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;
Here, in intel_hdmi_compute_ycbcr_config() function, we are additionally doing this also: 

clock_8bpc /= 2;
clock_10bpc /= 2;
clock_12bpc /= 2;


3. if (12bpc)
    	port_clock = clock_12bpc;
    else if (10bpc)
    	port_clock = clock_10bpc;
    else
    	port_clock = clock_8bpc;
This means effectively the selected clock is /2, so the code is not dead (thankfully :-)) 
- Shashank


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


_______________________________________________
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