* [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON
@ 2018-01-05 9:45 Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0 Shashank Sharma
` (7 more replies)
0 siblings, 8 replies; 22+ messages in thread
From: Shashank Sharma @ 2018-01-05 9:45 UTC (permalink / raw)
To: intel-gfx
This patch series adds YCBCR 4:2:0 output support for LSPCON displays.
In order to indicate the color format of output, to the LSPCON device,
a source has to set and send proper AVI infoframes to LSPCON. So this
patch series:
- first adds AVI infoframes support for LSPCON
- then adds YCBCR 4:2:0 output support for LSPCON
Previous versions of this series and its review can be found here:
https://patchwork.freedesktop.org/series/28536/
https://patchwork.freedesktop.org/series/33794/
In order to address review comment from V2, I have added 2 new patches
in this series, hence sending V3.
Shashank Sharma (7):
drm/i915: Add CRTC output format YCBCR 4:2:0
drm/i915: Add CRTC output format YCBCR 4:4:4
drm/i915: Check LSPCON vendor OUI
drm/i915: Add AVI infoframe support for LSPCON
drm/i915: Write AVI infoframes for MCA LSPCON
drm/i915: Write AVI infoframes for Parade LSPCON
drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
drivers/gpu/drm/i915/i915_reg.h | 2 +
drivers/gpu/drm/i915/intel_color.c | 3 +-
drivers/gpu/drm/i915/intel_ddi.c | 28 ++-
drivers/gpu/drm/i915/intel_display.c | 73 +++++---
drivers/gpu/drm/i915/intel_dp.c | 10 +
drivers/gpu/drm/i915/intel_drv.h | 37 +++-
drivers/gpu/drm/i915/intel_hdmi.c | 23 ++-
drivers/gpu/drm/i915/intel_lspcon.c | 343 +++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_panel.c | 2 +-
9 files changed, 469 insertions(+), 52 deletions(-)
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0
2018-01-05 9:45 [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON Shashank Sharma
@ 2018-01-05 9:45 ` Shashank Sharma
2018-01-05 11:21 ` Maarten Lankhorst
2018-01-17 18:27 ` Ville Syrjälä
2018-01-05 9:45 ` [PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4 Shashank Sharma
` (6 subsequent siblings)
7 siblings, 2 replies; 22+ messages in thread
From: Shashank Sharma @ 2018-01-05 9:45 UTC (permalink / raw)
To: intel-gfx
Currently, we are using a bool in CRTC state (state->ycbcr420),
to indicate modeset, that the output format is YCBCR 4:2:0. Now in
order to support other YCBCR formats, we will need more such flags.
The idea behind this patch is to replace this bool with an enum,
and plug this in in the existing YCBCR 4:2:0 framework in such a
way that this can be re-used for YCBCR 4:4:4 and other output formats too.
This patch adds a new enum for CRTC output formats, and then plugs it
in the existing modeset framework.
V3: Added this patch in the series, to address review comments from
second patchset.
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
drivers/gpu/drm/i915/intel_color.c | 2 +-
drivers/gpu/drm/i915/intel_ddi.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
drivers/gpu/drm/i915/intel_drv.h | 10 ++++--
drivers/gpu/drm/i915/intel_hdmi.c | 6 ++--
drivers/gpu/drm/i915/intel_panel.c | 2 +-
6 files changed, 59 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index aa66e95..99e32cb 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
uint16_t coeffs[9] = { 0, };
struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
- if (intel_crtc_state->ycbcr420) {
+ if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
i9xx_load_ycbcr_conversion_matrix(intel_crtc);
return;
} else if (crtc_state->ctm) {
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f51645a..84327e7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
else
dotclock = pipe_config->port_clock;
- if (pipe_config->ycbcr420)
+ if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
dotclock *= 2;
if (pipe_config->pixel_multiplier)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0cd3559..69b0aa3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4644,7 +4644,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->output_format == CRTC_OUTPUT_YCBCR420 &&
+ scaler_user == SKL_CRTC_INDEX)
need_scaling = true;
/*
@@ -6356,7 +6357,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
return -EINVAL;
}
- if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
+ if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
+ pipe_config->base.ctm) {
/*
* There is only one pipe CSC unit per pipe, and we need that
* for output conversion from RGB->YCBCR. So if CTM is already
@@ -8177,10 +8179,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
if (intel_crtc->config->dither)
val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
- if (config->ycbcr420) {
- val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
- PIPEMISC_YUV420_ENABLE |
- PIPEMISC_YUV420_MODE_FULL_BLEND;
+ if (config->output_format == CRTC_OUTPUT_YCBCR420) {
+ val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
+ val |= PIPEMISC_YUV420_ENABLE |
+ PIPEMISC_YUV420_MODE_FULL_BLEND;
}
I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
@@ -9156,6 +9158,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
}
}
+static const char * const output_format_str[] = {
+ "Invalid",
+ "RGB",
+ "YCBCR4:2:0",
+};
+
+static const char *output_formats(enum crtc_output_format format)
+{
+ if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
+ format = CRTC_OUTPUT_INVALID;
+ return output_format_str[format + 1];
+}
+
static bool haswell_get_pipe_config(struct intel_crtc *crtc,
struct intel_crtc_state *pipe_config)
{
@@ -9196,19 +9211,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
- bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
-
- if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
- bool blend_mode_420 = tmp &
- PIPEMISC_YUV420_MODE_FULL_BLEND;
+ enum crtc_output_format output_format = CRTC_OUTPUT_RGB;
+
+ if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
+ bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
+ bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
+
+ if (ycbcr420_enabled) {
+ /* We support 4:2:0 in full blend mode only */
+ if (!blend)
+ output_format = CRTC_OUTPUT_INVALID;
+ else if (!(IS_GEMINILAKE(dev_priv) ||
+ INTEL_GEN(dev_priv) >= 10))
+ output_format = CRTC_OUTPUT_INVALID;
+ else
+ output_format = CRTC_OUTPUT_YCBCR420;
+ }
- pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
- if (pipe_config->ycbcr420 != clrspace_yuv ||
- pipe_config->ycbcr420 != blend_mode_420)
- DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
- } else if (clrspace_yuv) {
- DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
}
+
+ pipe_config->output_format = output_format;
+ DRM_DEBUG_KMS("Output format %s\n",
+ output_formats(output_format));
}
power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
@@ -10558,6 +10582,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
buf, pipe_config->output_types);
+ DRM_DEBUG_KMS("output format: %s\n",
+ output_formats(pipe_config->output_format));
+
DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
transcoder_name(pipe_config->cpu_transcoder),
pipe_config->pipe_bpp, pipe_config->dither);
@@ -10567,9 +10594,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
pipe_config->fdi_lanes,
&pipe_config->fdi_m_n);
- if (pipe_config->ycbcr420)
- DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
-
if (intel_crtc_has_dp_encoder(pipe_config)) {
intel_dump_m_n_config(pipe_config, "dp m_n",
pipe_config->lane_count, &pipe_config->dp_m_n);
@@ -11143,6 +11167,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
PIPE_CONF_CHECK_I(pixel_multiplier);
+ PIPE_CONF_CHECK_I(output_format);
PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
@@ -11151,7 +11176,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
- PIPE_CONF_CHECK_BOOL(ycbcr420);
PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 30f791f..79662650 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -609,6 +609,12 @@ struct intel_crtc_wm_state {
bool need_postvbl_update;
};
+enum crtc_output_format {
+ CRTC_OUTPUT_INVALID = -1,
+ CRTC_OUTPUT_RGB,
+ CRTC_OUTPUT_YCBCR420,
+};
+
struct intel_crtc_state {
struct drm_crtc_state base;
@@ -795,8 +801,8 @@ struct intel_crtc_state {
/* HDMI High TMDS char rate ratio */
bool hdmi_high_tmds_clock_ratio;
- /* output format is YCBCR 4:2:0 */
- bool ycbcr420;
+ /* Output format RGB/YCBCR etc */
+ enum crtc_output_format output_format;
};
struct intel_crtc {
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index bced7b9..b266a7f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -478,7 +478,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
return;
}
- if (crtc_state->ycbcr420)
+ if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
else
frame.avi.colorspace = HDMI_COLORSPACE_RGB;
@@ -1372,7 +1372,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
if (connector_state->crtc != crtc_state->base.crtc)
continue;
- if (crtc_state->ycbcr420) {
+ if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
const struct drm_hdmi_info *hdmi = &info->hdmi;
if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
@@ -1407,7 +1407,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
config->port_clock /= 2;
*clock_12bpc /= 2;
*clock_8bpc /= 2;
- config->ycbcr420 = true;
+ config->output_format = CRTC_OUTPUT_YCBCR420;
/* YCBCR 420 output conversion needs a scaler */
if (skl_update_scaler_crtc(config)) {
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index fa6831f..c57819f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
/* Native modes don't need fitting */
if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
- !pipe_config->ycbcr420)
+ pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
goto done;
switch (fitting_mode) {
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4
2018-01-05 9:45 [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0 Shashank Sharma
@ 2018-01-05 9:45 ` Shashank Sharma
2018-01-17 18:27 ` Ville Syrjälä
2018-01-05 9:45 ` [PATCH v3 3/7] drm/i915: Check LSPCON vendor OUI Shashank Sharma
` (5 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Shashank Sharma @ 2018-01-05 9:45 UTC (permalink / raw)
To: intel-gfx
This patch adds support for YCBCR 4:4:4 CRTC output format.
To do this, this patch extends the existing YCBCR 4:2:0
framework by:
- Adding new parameter in for YCBCR 4:4:4 enum crtc_iutput_format.
- Adding case for YCBCR 4:4:4 in while setting AVI infoframes.
- Adding necessary checks in modeset sequence.
V3: Added this patch in the series
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
drivers/gpu/drm/i915/intel_color.c | 3 ++-
drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_hdmi.c | 2 ++
4 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 99e32cb..5b76de6 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -141,7 +141,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
uint16_t coeffs[9] = { 0, };
struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
- if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
+ if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420 ||
+ intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
i9xx_load_ycbcr_conversion_matrix(intel_crtc);
return;
} else if (crtc_state->ctm) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 69b0aa3..6ac5ca6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6357,8 +6357,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
return -EINVAL;
}
- if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
- pipe_config->base.ctm) {
+ if ((pipe_config->output_format == CRTC_OUTPUT_YCBCR420 ||
+ pipe_config->output_format == CRTC_OUTPUT_YCBCR444) &&
+ pipe_config->base.ctm) {
/*
* There is only one pipe CSC unit per pipe, and we need that
* for output conversion from RGB->YCBCR. So if CTM is already
@@ -8179,11 +8180,12 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
if (intel_crtc->config->dither)
val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
- if (config->output_format == CRTC_OUTPUT_YCBCR420) {
+ if (config->output_format > CRTC_OUTPUT_RGB)
val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
+
+ if (config->output_format == CRTC_OUTPUT_YCBCR420)
val |= PIPEMISC_YUV420_ENABLE |
PIPEMISC_YUV420_MODE_FULL_BLEND;
- }
I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
}
@@ -9161,6 +9163,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
static const char * const output_format_str[] = {
"Invalid",
"RGB",
+ "YCBCR4:4:4",
"YCBCR4:2:0",
};
@@ -9226,6 +9229,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
output_format = CRTC_OUTPUT_INVALID;
else
output_format = CRTC_OUTPUT_YCBCR420;
+ } else {
+ output_format = CRTC_OUTPUT_YCBCR444;
}
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 79662650..a393342 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -612,6 +612,7 @@ struct intel_crtc_wm_state {
enum crtc_output_format {
CRTC_OUTPUT_INVALID = -1,
CRTC_OUTPUT_RGB,
+ CRTC_OUTPUT_YCBCR444,
CRTC_OUTPUT_YCBCR420,
};
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index b266a7f..258bb51 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -480,6 +480,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
+ else if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
+ frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
else
frame.avi.colorspace = HDMI_COLORSPACE_RGB;
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 3/7] drm/i915: Check LSPCON vendor OUI
2018-01-05 9:45 [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0 Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4 Shashank Sharma
@ 2018-01-05 9:45 ` Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 4/7] drm/i915: Add AVI infoframe support for LSPCON Shashank Sharma
` (4 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Shashank Sharma @ 2018-01-05 9:45 UTC (permalink / raw)
To: intel-gfx
Intel LSPCON chip is provided by 2 vendors:
- Megachips America (MCA)
- Parade technologies (Parade tech)
Its important to know the vendor of this chip, as the address to
write AVI infoframes is different for those two.
This patch reads the vendor OUI signature, and marks into LSPCON
encoder structure for future usages.
This patch also does a small re-arrangement of the code, by moving
lspcon mode change into probe function.
V2: Use dp->desc for OUI detection, dont add a helper for this
(Ville)
V3: Rebase, Added r-b from Maarten
Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
drivers/gpu/drm/i915/intel_drv.h | 6 ++++
drivers/gpu/drm/i915/intel_lspcon.c | 69 +++++++++++++++++++++++++++++--------
2 files changed, 61 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a393342..ba6a599 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1055,9 +1055,15 @@ struct intel_dp {
struct intel_dp_compliance compliance;
};
+enum lspcon_vendor {
+ LSPCON_VENDOR_MCA,
+ LSPCON_VENDOR_PARADE
+};
+
struct intel_lspcon {
bool active;
enum drm_lspcon_mode mode;
+ enum lspcon_vendor vendor;
};
struct intel_digital_port {
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index dcbc786..946dfd0 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -27,6 +27,10 @@
#include <drm/drm_dp_dual_mode_helper.h>
#include "intel_drv.h"
+/* LSPCON OUI Vendor ID(signatures) */
+#define LSPCON_VENDOR_PARADE_OUI 0x001CF8
+#define LSPCON_VENDOR_MCA_OUI 0x0060AD
+
static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
{
struct intel_digital_port *dig_port =
@@ -50,6 +54,40 @@ static const char *lspcon_mode_name(enum drm_lspcon_mode mode)
}
}
+static bool lspcon_detect_vendor(struct intel_lspcon *lspcon)
+{
+ struct intel_dp *dp = lspcon_to_intel_dp(lspcon);
+ struct drm_dp_dpcd_ident *ident;
+ u32 vendor_oui;
+
+ if (drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd))) {
+ DRM_ERROR("Can't read description\n");
+ return false;
+ }
+
+ ident = &dp->desc.ident;
+ vendor_oui = (ident->oui[0] << 16) | (ident->oui[1] << 8) |
+ ident->oui[2];
+
+ switch (vendor_oui) {
+ case LSPCON_VENDOR_MCA_OUI:
+ lspcon->vendor = LSPCON_VENDOR_MCA;
+ DRM_DEBUG_KMS("Vendor: Mega Chips\n");
+ break;
+
+ case LSPCON_VENDOR_PARADE_OUI:
+ lspcon->vendor = LSPCON_VENDOR_PARADE;
+ DRM_DEBUG_KMS("Vendor: Parade Tech\n");
+ break;
+
+ default:
+ DRM_ERROR("Invalid/Unknown vendor OUI\n");
+ return false;
+ }
+
+ return true;
+}
+
static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
{
enum drm_lspcon_mode current_mode;
@@ -159,7 +197,18 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
/* Yay ... got a LSPCON device */
DRM_DEBUG_KMS("LSPCON detected\n");
lspcon->mode = lspcon_wait_mode(lspcon, expected_mode);
- lspcon->active = true;
+
+ /*
+ * In the SW state machine, lets Put LSPCON in PCON mode only.
+ * In this way, it will work with both HDMI 1.4 sinks as well as HDMI
+ * 2.0 sinks.
+ */
+ if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
+ if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
+ DRM_ERROR("LSPCON mode change to PCON failed\n");
+ return false;
+ }
+ }
return true;
}
@@ -231,25 +280,17 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
return false;
}
- /*
- * In the SW state machine, lets Put LSPCON in PCON mode only.
- * In this way, it will work with both HDMI 1.4 sinks as well as HDMI
- * 2.0 sinks.
- */
- if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
- if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
- DRM_ERROR("LSPCON mode change to PCON failed\n");
- return false;
- }
- }
-
if (!intel_dp_read_dpcd(dp)) {
DRM_ERROR("LSPCON DPCD read failed\n");
return false;
}
- drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd));
+ if (!lspcon_detect_vendor(lspcon)) {
+ DRM_ERROR("LSPCON vendor detection failed\n");
+ return false;
+ }
+ lspcon->active = true;
DRM_DEBUG_KMS("Success: LSPCON init\n");
return true;
}
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 4/7] drm/i915: Add AVI infoframe support for LSPCON
2018-01-05 9:45 [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON Shashank Sharma
` (2 preceding siblings ...)
2018-01-05 9:45 ` [PATCH v3 3/7] drm/i915: Check LSPCON vendor OUI Shashank Sharma
@ 2018-01-05 9:45 ` Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 5/7] drm/i915: Write AVI infoframes for MCA LSPCON Shashank Sharma
` (3 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Shashank Sharma @ 2018-01-05 9:45 UTC (permalink / raw)
To: intel-gfx; +Cc: Imre Deak
In order to pass AVI infoframes to LSPCON devices, a source has to
write them in a vendor recommended method and location.
This patch series:
- adds generic LSPCON infoframe setup functions.
- registers these functions into existing AVI infoframe framework.
- triggers these functions from modeset sequence.
Next patches in the series will add vendor specific code.
V2: Added new parameter to align with new definition of
drm_hdmi_avi_infoframe_quant_range
V3: Added r-b from Maarten (for V2)
Added new parameter output_format in struct lspcon to accommodate
Ville's review comments on last patch of the series
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 19 +++++++++++---
drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++-
drivers/gpu/drm/i915/intel_hdmi.c | 13 +++++++---
drivers/gpu/drm/i915/intel_lspcon.c | 49 +++++++++++++++++++++++++++++++++++++
4 files changed, 87 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 84327e7..7b89f2a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2238,10 +2238,22 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
- if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
+ if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
intel_ddi_pre_enable_hdmi(encoder, crtc_state, conn_state);
- else
+ } else {
+ struct intel_lspcon *lspcon =
+ enc_to_intel_lspcon(&encoder->base);
+
intel_ddi_pre_enable_dp(encoder, crtc_state, conn_state);
+ if (lspcon->active) {
+ struct intel_digital_port *dig_port =
+ enc_to_dig_port(&encoder->base);
+
+ dig_port->set_infoframes(&encoder->base,
+ crtc_state->has_infoframe,
+ crtc_state, conn_state);
+ }
+ }
}
static void intel_disable_ddi_buf(struct intel_encoder *encoder)
@@ -2892,8 +2904,6 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
intel_encoder->cloneable = 0;
- intel_infoframe_init(intel_dig_port);
-
if (init_dp) {
if (!intel_ddi_init_dp_connector(intel_dig_port))
goto err;
@@ -2923,6 +2933,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
port_name(port));
}
+ intel_infoframe_init(intel_dig_port);
return;
err:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ba6a599..f2e8752 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1064,6 +1064,7 @@ struct intel_lspcon {
bool active;
enum drm_lspcon_mode mode;
enum lspcon_vendor vendor;
+ enum crtc_output_format output_format;
};
struct intel_digital_port {
@@ -1189,6 +1190,12 @@ static inline struct intel_dp *enc_to_intel_dp(struct drm_encoder *encoder)
return &enc_to_dig_port(encoder)->dp;
}
+static inline struct intel_lspcon *
+enc_to_intel_lspcon(struct drm_encoder *encoder)
+{
+ return &enc_to_dig_port(encoder)->lspcon;
+}
+
static inline struct intel_digital_port *
dp_to_dig_port(struct intel_dp *intel_dp)
{
@@ -1703,7 +1710,6 @@ void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
void intel_infoframe_init(struct intel_digital_port *intel_dig_port);
-
/* intel_lvds.c */
void intel_lvds_init(struct drm_i915_private *dev_priv);
struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
@@ -2031,6 +2037,12 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state);
bool lspcon_init(struct intel_digital_port *intel_dig_port);
void lspcon_resume(struct intel_lspcon *lspcon);
void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
+void lspcon_set_infoframes(struct drm_encoder *encoder,
+ bool enable,
+ const struct intel_crtc_state *crtc_state,
+ const struct drm_connector_state *conn_state);
+bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
+ const struct intel_crtc_state *pipe_config);
/* intel_pipe_crc.c */
int intel_pipe_crc_create(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 258bb51..0609f11 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1999,9 +1999,16 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port)
intel_dig_port->set_infoframes = g4x_set_infoframes;
intel_dig_port->infoframe_enabled = g4x_infoframe_enabled;
} else if (HAS_DDI(dev_priv)) {
- intel_dig_port->write_infoframe = hsw_write_infoframe;
- intel_dig_port->set_infoframes = hsw_set_infoframes;
- intel_dig_port->infoframe_enabled = hsw_infoframe_enabled;
+ if (intel_dig_port->lspcon.active) {
+ intel_dig_port->set_infoframes = lspcon_set_infoframes;
+ intel_dig_port->infoframe_enabled =
+ lspcon_infoframe_enabled;
+ } else {
+ intel_dig_port->set_infoframes = hsw_set_infoframes;
+ intel_dig_port->infoframe_enabled =
+ hsw_infoframe_enabled;
+ intel_dig_port->write_infoframe = hsw_write_infoframe;
+ }
} else if (HAS_PCH_IBX(dev_priv)) {
intel_dig_port->write_infoframe = ibx_write_infoframe;
intel_dig_port->set_infoframes = ibx_set_infoframes;
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 946dfd0..32d4198 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -235,6 +235,55 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
}
+void lspcon_set_infoframes(struct drm_encoder *encoder,
+ bool enable,
+ const struct intel_crtc_state *crtc_state,
+ const struct drm_connector_state *conn_state)
+{
+ ssize_t ret;
+ union hdmi_infoframe frame;
+ uint8_t buf[VIDEO_DIP_DATA_SIZE];
+ struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+ struct intel_lspcon *lspcon = &dig_port->lspcon;
+ struct intel_dp *intel_dp = &dig_port->dp;
+ struct drm_connector *connector = &intel_dp->attached_connector->base;
+ const struct drm_display_mode *mode = &crtc_state->base.adjusted_mode;
+ bool is_hdmi2_sink = connector->display_info.hdmi.scdc.supported;
+
+ if (!lspcon->active) {
+ DRM_ERROR("Writing infoframes while LSPCON disabled ?\n");
+ return;
+ }
+
+ ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+ mode, is_hdmi2_sink);
+ if (ret < 0) {
+ DRM_ERROR("couldn't fill AVI infoframe\n");
+ return;
+ }
+
+ drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
+ crtc_state->limited_color_range ?
+ HDMI_QUANTIZATION_RANGE_LIMITED :
+ HDMI_QUANTIZATION_RANGE_FULL,
+ false, is_hdmi2_sink);
+
+ ret = hdmi_infoframe_pack(&frame, buf, sizeof(buf));
+ if (ret < 0) {
+ DRM_ERROR("Failed to pack AVI IF\n");
+ return;
+ }
+
+ dig_port->write_infoframe(encoder, crtc_state, HDMI_INFOFRAME_TYPE_AVI,
+ buf, ret);
+}
+
+bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
+ const struct intel_crtc_state *pipe_config)
+{
+ return enc_to_intel_lspcon(encoder)->active;
+}
+
void lspcon_resume(struct intel_lspcon *lspcon)
{
enum drm_lspcon_mode expected_mode;
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 5/7] drm/i915: Write AVI infoframes for MCA LSPCON
2018-01-05 9:45 [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON Shashank Sharma
` (3 preceding siblings ...)
2018-01-05 9:45 ` [PATCH v3 4/7] drm/i915: Add AVI infoframe support for LSPCON Shashank Sharma
@ 2018-01-05 9:45 ` Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 6/7] drm/i915: Write AVI infoframes for Parade LSPCON Shashank Sharma
` (2 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Shashank Sharma @ 2018-01-05 9:45 UTC (permalink / raw)
To: intel-gfx
As LSPCON is a DP branch device, LSPCON vendors define
specific methods to pass AVI infoframes to the the chip.
This patch adds:
- a generic wrapper function for writing AVI infoframes for
all LSPCON devices.
- a vendor specific function to wrire AVI infoframes into
MCA LSPCON devices.
V2: Rebase
V3: Added r-b from Maarten
Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
drivers/gpu/drm/i915/intel_drv.h | 4 ++
drivers/gpu/drm/i915/intel_hdmi.c | 2 +
drivers/gpu/drm/i915/intel_lspcon.c | 80 +++++++++++++++++++++++++++++++++++++
3 files changed, 86 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f2e8752..2de6b41 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2037,6 +2037,10 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state);
bool lspcon_init(struct intel_digital_port *intel_dig_port);
void lspcon_resume(struct intel_lspcon *lspcon);
void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
+void lspcon_write_infoframe(struct drm_encoder *encoder,
+ const struct intel_crtc_state *crtc_state,
+ enum hdmi_infoframe_type type,
+ const void *buf, ssize_t len);
void lspcon_set_infoframes(struct drm_encoder *encoder,
bool enable,
const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 0609f11..7b63889 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2000,6 +2000,8 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port)
intel_dig_port->infoframe_enabled = g4x_infoframe_enabled;
} else if (HAS_DDI(dev_priv)) {
if (intel_dig_port->lspcon.active) {
+ intel_dig_port->write_infoframe =
+ lspcon_write_infoframe;
intel_dig_port->set_infoframes = lspcon_set_infoframes;
intel_dig_port->infoframe_enabled =
lspcon_infoframe_enabled;
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 32d4198..0571108 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -31,6 +31,12 @@
#define LSPCON_VENDOR_PARADE_OUI 0x001CF8
#define LSPCON_VENDOR_MCA_OUI 0x0060AD
+/* AUX addresses to write MCA AVI IF */
+#define LSPCON_MCA_AVI_IF_WRITE_OFFSET 0x5C0
+#define LSPCON_MCA_AVI_IF_CTRL 0x5DF
+#define LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
+#define LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
+
static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
{
struct intel_digital_port *dig_port =
@@ -235,6 +241,80 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
}
+static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
+ const uint8_t *buffer, ssize_t len)
+{
+ int ret;
+ uint32_t val = 0;
+ uint16_t reg;
+ const uint8_t *data = buffer;
+
+ reg = LSPCON_MCA_AVI_IF_WRITE_OFFSET;
+ while (val < len) {
+ ret = drm_dp_dpcd_write(aux, reg, (void *)data, 1);
+ if (ret < 0) {
+ DRM_ERROR("DPCD write failed, add:0x%x\n", reg);
+ return false;
+ }
+ val++; reg++; data++;
+ }
+
+ val = 0;
+ reg = LSPCON_MCA_AVI_IF_CTRL;
+ ret = drm_dp_dpcd_read(aux, reg, &val, 1);
+ if (ret < 0) {
+ DRM_ERROR("DPCD read failed, address 0x%x\n", reg);
+ return false;
+ }
+
+ /* Indicate LSPCON chip about infoframe, clear bit 1 and set bit 0 */
+ val &= ~LSPCON_MCA_AVI_IF_HANDLED;
+ val |= LSPCON_MCA_AVI_IF_KICKOFF;
+
+ ret = drm_dp_dpcd_write(aux, reg, &val, 1);
+ if (ret < 0) {
+ DRM_ERROR("DPCD read failed, address 0x%x\n", reg);
+ return false;
+ }
+
+ val = 0;
+ ret = drm_dp_dpcd_read(aux, reg, &val, 1);
+ if (ret < 0) {
+ DRM_ERROR("DPCD read failed, address 0x%x\n", reg);
+ return false;
+ }
+
+ if (val == LSPCON_MCA_AVI_IF_HANDLED)
+ DRM_DEBUG_KMS("AVI IF handled by FW\n");
+
+ return true;
+}
+
+void lspcon_write_infoframe(struct drm_encoder *encoder,
+ const struct intel_crtc_state *crtc_state,
+ enum hdmi_infoframe_type type,
+ const void *frame, ssize_t len)
+{
+ bool ret = true;
+ struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+ struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
+
+ /* LSPCON only needs AVI IF */
+ if (type != HDMI_INFOFRAME_TYPE_AVI)
+ return;
+
+ if (lspcon->vendor == LSPCON_VENDOR_MCA)
+ ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
+ frame, len);
+ if (!ret) {
+ DRM_ERROR("Failed to write AVI infoframes\n");
+ return;
+ }
+
+ DRM_DEBUG_DRIVER("AVI infoframes updated successfully\n");
+}
+
+
void lspcon_set_infoframes(struct drm_encoder *encoder,
bool enable,
const struct intel_crtc_state *crtc_state,
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 6/7] drm/i915: Write AVI infoframes for Parade LSPCON
2018-01-05 9:45 [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON Shashank Sharma
` (4 preceding siblings ...)
2018-01-05 9:45 ` [PATCH v3 5/7] drm/i915: Write AVI infoframes for MCA LSPCON Shashank Sharma
@ 2018-01-05 9:45 ` Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON Shashank Sharma
2018-01-05 10:15 ` ✗ Fi.CI.BAT: warning for YCBCR 4:2:0/4:4:4 output " Patchwork
7 siblings, 0 replies; 22+ messages in thread
From: Shashank Sharma @ 2018-01-05 9:45 UTC (permalink / raw)
To: intel-gfx
Different LSPCON vendors specify their custom methods to pass
AVI infoframes to the LSPCON chip, so does Parade tech.
This patch adds functions to arrange and write AVI infoframes
into Parade LSPCON chips.
V2: rebase
V3: Added r-b from Maarten
Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
drivers/gpu/drm/i915/intel_lspcon.c | 119 +++++++++++++++++++++++++++++++++++-
1 file changed, 118 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 0571108..066ea91 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -37,6 +37,12 @@
#define LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
#define LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
+/* AUX addresses to write Parade AVI IF */
+#define LSPCON_PARADE_AVI_IF_WRITE_OFFSET 0x516
+#define LSPCON_PARADE_AVI_IF_CTRL 0x51E
+#define LSPCON_PARADE_AVI_IF_KICKOFF (1 << 7)
+#define LSPCON_PARADE_AVI_IF_DATA_SIZE 32
+
static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
{
struct intel_digital_port *dig_port =
@@ -241,6 +247,113 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
}
+static bool lspcon_parade_fw_ready(struct drm_dp_aux *aux)
+{
+ u8 avi_if_ctrl;
+ u8 retry;
+ ssize_t ret;
+
+ /* Check if LSPCON FW is ready for data */
+ for (retry = 0; retry < 5; retry++) {
+
+ if (retry)
+ usleep_range(200, 300);
+
+ ret = drm_dp_dpcd_read(aux, LSPCON_PARADE_AVI_IF_CTRL,
+ &avi_if_ctrl, 1);
+ if (ret < 0) {
+ DRM_ERROR("Failed to read AVI IF control\n");
+ return false;
+ }
+
+ if ((avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF) == 0)
+ return true;
+ }
+
+ DRM_ERROR("Parade FW not ready to accept AVI IF\n");
+ return false;
+}
+
+static bool _lspcon_parade_write_infoframe_blocks(struct drm_dp_aux *aux,
+ uint8_t *avi_buf)
+{
+ u8 avi_if_ctrl;
+ u8 block_count = 0;
+ u8 *data;
+ uint16_t reg;
+ ssize_t ret;
+
+ while (block_count < 4) {
+
+ if (!lspcon_parade_fw_ready(aux)) {
+ DRM_DEBUG_KMS("LSPCON FW not ready, block %d\n",
+ block_count);
+ return false;
+ }
+
+ reg = LSPCON_PARADE_AVI_IF_WRITE_OFFSET;
+ data = avi_buf + block_count * 8;
+ ret = drm_dp_dpcd_write(aux, reg, data, 8);
+ if (ret < 0) {
+ DRM_ERROR("Failed to write AVI IF block %d\n",
+ block_count);
+ return false;
+ }
+
+ /*
+ * Once a block of data is written, we have to inform the FW
+ * about this by writing into avi infoframe control register:
+ * - set the kickoff bit[7] to 1
+ * - write the block no. to bits[1:0]
+ */
+ reg = LSPCON_PARADE_AVI_IF_CTRL;
+ avi_if_ctrl = LSPCON_PARADE_AVI_IF_KICKOFF | block_count;
+ ret = drm_dp_dpcd_write(aux, reg, &avi_if_ctrl, 1);
+ if (ret < 0) {
+ DRM_ERROR("Failed to update (0x%x), block %d\n",
+ reg, block_count);
+ return false;
+ }
+
+ block_count++;
+ }
+
+ DRM_DEBUG_KMS("Wrote AVI IF blocks successfully\n");
+ return true;
+}
+
+static bool _lspcon_write_avi_infoframe_parade(struct drm_dp_aux *aux,
+ const uint8_t *frame,
+ ssize_t len)
+{
+ uint8_t avi_if[LSPCON_PARADE_AVI_IF_DATA_SIZE] = {1, };
+
+ /*
+ * Parade's frames contains 32 bytes of data, divided
+ * into 4 frames:
+ * Token byte (first byte of first frame, must be non-zero)
+ * HB0 to HB2 from AVI IF (3 bytes header)
+ * PB0 to PB27 from AVI IF (28 bytes data)
+ * So it should look like this
+ * first block: | <token> <HB0-HB2> <DB0-DB3> |
+ * next 3 blocks: |<DB4-DB11>|<DB12-DB19>|<DB20-DB28>|
+ */
+
+ if (len > LSPCON_PARADE_AVI_IF_DATA_SIZE - 1) {
+ DRM_ERROR("Invalid length of infoframes\n");
+ return false;
+ }
+
+ memcpy(&avi_if[1], frame, len);
+
+ if (!_lspcon_parade_write_infoframe_blocks(aux, avi_if)) {
+ DRM_DEBUG_KMS("Failed to write infoframe blocks\n");
+ return false;
+ }
+
+ return true;
+}
+
static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
const uint8_t *buffer, ssize_t len)
{
@@ -295,7 +408,7 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
enum hdmi_infoframe_type type,
const void *frame, ssize_t len)
{
- bool ret = true;
+ bool ret;
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
@@ -306,6 +419,10 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
if (lspcon->vendor == LSPCON_VENDOR_MCA)
ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
frame, len);
+ else
+ ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux,
+ frame, len);
+
if (!ret) {
DRM_ERROR("Failed to write AVI infoframes\n");
return;
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
2018-01-05 9:45 [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON Shashank Sharma
` (5 preceding siblings ...)
2018-01-05 9:45 ` [PATCH v3 6/7] drm/i915: Write AVI infoframes for Parade LSPCON Shashank Sharma
@ 2018-01-05 9:45 ` Shashank Sharma
2018-01-17 18:27 ` Ville Syrjälä
2018-01-05 10:15 ` ✗ Fi.CI.BAT: warning for YCBCR 4:2:0/4:4:4 output " Patchwork
7 siblings, 1 reply; 22+ messages in thread
From: Shashank Sharma @ 2018-01-05 9:45 UTC (permalink / raw)
To: intel-gfx
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)
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 2 ++
drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
drivers/gpu/drm/i915/intel_drv.h | 2 ++
drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
5 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 966e4df..45ee264 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8547,6 +8547,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 7b89f2a..7616f6f 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1499,6 +1499,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.
+ */
+ if (crtc_state->output_format == CRTC_OUTPUT_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_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 35c5299..3bf82ea 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1613,6 +1613,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;
@@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
pipe_config->has_pch_encoder = true;
+ if (lspcon->active) {
+ struct drm_connector *connector = &intel_connector->base;
+
+ if (lspcon_ycbcr420_config(connector, pipe_config)) {
+ pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
+ lspcon->output_format = CRTC_OUTPUT_YCBCR420;
+ }
+ }
+
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 2de6b41..5edba06 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 066ea91..cb88138 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -180,6 +180,25 @@ 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;
+ }
+
+ config->port_clock /= 2;
+ return true;
+ }
+
+ return false;
+}
+
static bool lspcon_probe(struct intel_lspcon *lspcon)
{
int retry;
@@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
return;
}
+ if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
+ frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
+ else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
+ 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 :
@@ -512,6 +538,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");
@@ -536,6 +563,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* ✗ Fi.CI.BAT: warning for YCBCR 4:2:0/4:4:4 output support for LSPCON
2018-01-05 9:45 [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON Shashank Sharma
` (6 preceding siblings ...)
2018-01-05 9:45 ` [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON Shashank Sharma
@ 2018-01-05 10:15 ` Patchwork
7 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2018-01-05 10:15 UTC (permalink / raw)
To: Shashank Sharma; +Cc: intel-gfx
== Series Details ==
Series: YCBCR 4:2:0/4:4:4 output support for LSPCON
URL : https://patchwork.freedesktop.org/series/36068/
State : warning
== Summary ==
Series 36068v1 YCBCR 4:2:0/4:4:4 output support for LSPCON
https://patchwork.freedesktop.org/api/1.0/series/36068/revisions/1/mbox/
Test debugfs_test:
Subgroup read_all_entries:
dmesg-warn -> PASS (fi-elk-e7500) fdo#103989 +2
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> DMESG-WARN (fi-kbl-7567u)
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
pass -> DMESG-WARN (fi-kbl-7567u)
pass -> DMESG-WARN (fi-kbl-r) fdo#104172 +1
Subgroup suspend-read-crc-pipe-b:
pass -> DMESG-WARN (fi-kbl-7567u)
Subgroup suspend-read-crc-pipe-c:
pass -> DMESG-WARN (fi-kbl-7567u)
Test kms_psr_sink_crc:
Subgroup psr_basic:
dmesg-warn -> PASS (fi-skl-6700hq) fdo#104260
fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172
fdo#104260 https://bugs.freedesktop.org/show_bug.cgi?id=104260
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:422s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:426s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:367s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:478s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:275s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:477s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:478s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:463s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:453s
fi-elk-e7500 total:224 pass:168 dwarn:10 dfail:0 fail:0 skip:45
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:508s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:392s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:405s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:411s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:449s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:408s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:463s
fi-kbl-7560u total:288 pass:268 dwarn:1 dfail:0 fail:0 skip:19 time:497s
fi-kbl-7567u total:288 pass:264 dwarn:4 dfail:0 fail:0 skip:20 time:452s
fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:500s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:572s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:429s
fi-skl-6600u total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:505s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:522s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:496s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:484s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:431s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:520s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:394s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:563s
fi-cnl-y total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:595s
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:467s
fi-gdg-551 failed to collect. IGT log at Patchwork_7616/fi-gdg-551/igt.log
914d61a8fb5fc53f6b0366167210468147495b3f drm-tip: 2018y-01m-05d-09h-12m-18s UTC integration manifest
7e873eeda3c9 drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
493d65ec6f3b drm/i915: Write AVI infoframes for Parade LSPCON
0681b480b514 drm/i915: Write AVI infoframes for MCA LSPCON
7dc583b6251a drm/i915: Add AVI infoframe support for LSPCON
3f657d86adf2 drm/i915: Check LSPCON vendor OUI
be479b386bb5 drm/i915: Add CRTC output format YCBCR 4:4:4
0344c5ac79d0 drm/i915: Add CRTC output format YCBCR 4:2:0
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7616/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0
2018-01-05 9:45 ` [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0 Shashank Sharma
@ 2018-01-05 11:21 ` Maarten Lankhorst
2018-01-17 18:27 ` Ville Syrjälä
1 sibling, 0 replies; 22+ messages in thread
From: Maarten Lankhorst @ 2018-01-05 11:21 UTC (permalink / raw)
To: Shashank Sharma, intel-gfx
Op 05-01-18 om 10:45 schreef Shashank Sharma:
> Currently, we are using a bool in CRTC state (state->ycbcr420),
> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
> order to support other YCBCR formats, we will need more such flags.
>
> The idea behind this patch is to replace this bool with an enum,
> and plug this in in the existing YCBCR 4:2:0 framework in such a
> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
>
> This patch adds a new enum for CRTC output formats, and then plugs it
> in the existing modeset framework.
>
> V3: Added this patch in the series, to address review comments from
> second patchset.
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Series looks good to me now, so feel free to add my r-b to where its missing. But you still need an ack from Ville I think. :)
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0
2018-01-05 9:45 ` [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0 Shashank Sharma
2018-01-05 11:21 ` Maarten Lankhorst
@ 2018-01-17 18:27 ` Ville Syrjälä
2018-01-18 6:21 ` Sharma, Shashank
1 sibling, 1 reply; 22+ messages in thread
From: Ville Syrjälä @ 2018-01-17 18:27 UTC (permalink / raw)
To: Shashank Sharma; +Cc: intel-gfx
On Fri, Jan 05, 2018 at 03:15:29PM +0530, Shashank Sharma wrote:
> Currently, we are using a bool in CRTC state (state->ycbcr420),
> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
> order to support other YCBCR formats, we will need more such flags.
>
> The idea behind this patch is to replace this bool with an enum,
> and plug this in in the existing YCBCR 4:2:0 framework in such a
> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
>
> This patch adds a new enum for CRTC output formats, and then plugs it
> in the existing modeset framework.
>
> V3: Added this patch in the series, to address review comments from
> second patchset.
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
> drivers/gpu/drm/i915/intel_color.c | 2 +-
> drivers/gpu/drm/i915/intel_ddi.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
> drivers/gpu/drm/i915/intel_drv.h | 10 ++++--
> drivers/gpu/drm/i915/intel_hdmi.c | 6 ++--
> drivers/gpu/drm/i915/intel_panel.c | 2 +-
> 6 files changed, 59 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index aa66e95..99e32cb 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
> uint16_t coeffs[9] = { 0, };
> struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>
> - if (intel_crtc_state->ycbcr420) {
> + if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
> i9xx_load_ycbcr_conversion_matrix(intel_crtc);
> return;
> } else if (crtc_state->ctm) {
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index f51645a..84327e7 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
> else
> dotclock = pipe_config->port_clock;
>
> - if (pipe_config->ycbcr420)
> + if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
> dotclock *= 2;
>
> if (pipe_config->pixel_multiplier)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0cd3559..69b0aa3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4644,7 +4644,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->output_format == CRTC_OUTPUT_YCBCR420 &&
> + scaler_user == SKL_CRTC_INDEX)
> need_scaling = true;
>
> /*
> @@ -6356,7 +6357,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> return -EINVAL;
> }
>
> - if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
> + if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
> + pipe_config->base.ctm) {
> /*
> * There is only one pipe CSC unit per pipe, and we need that
> * for output conversion from RGB->YCBCR. So if CTM is already
> @@ -8177,10 +8179,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
> if (intel_crtc->config->dither)
> val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>
> - if (config->ycbcr420) {
> - val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
> - PIPEMISC_YUV420_ENABLE |
> - PIPEMISC_YUV420_MODE_FULL_BLEND;
> + if (config->output_format == CRTC_OUTPUT_YCBCR420) {
> + val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> + val |= PIPEMISC_YUV420_ENABLE |
> + PIPEMISC_YUV420_MODE_FULL_BLEND;
> }
>
> I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> @@ -9156,6 +9158,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
> }
> }
>
> +static const char * const output_format_str[] = {
> + "Invalid",
> + "RGB",
> + "YCBCR4:2:0",
> +};
> +
> +static const char *output_formats(enum crtc_output_format format)
> +{
> + if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
> + format = CRTC_OUTPUT_INVALID;
> + return output_format_str[format + 1];
> +}
> +
> static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> struct intel_crtc_state *pipe_config)
> {
> @@ -9196,19 +9211,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>
> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
> u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
> - bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
> -
> - if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
> - bool blend_mode_420 = tmp &
> - PIPEMISC_YUV420_MODE_FULL_BLEND;
> + enum crtc_output_format output_format = CRTC_OUTPUT_RGB;
> +
> + if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
> + bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
> + bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
> +
> + if (ycbcr420_enabled) {
> + /* We support 4:2:0 in full blend mode only */
> + if (!blend)
> + output_format = CRTC_OUTPUT_INVALID;
Not sure that INVALID thing really provides any real value.
Just feels like it's making the code more convoluted for something that
should never really happen.
> + else if (!(IS_GEMINILAKE(dev_priv) ||
> + INTEL_GEN(dev_priv) >= 10))
> + output_format = CRTC_OUTPUT_INVALID;
> + else
> + output_format = CRTC_OUTPUT_YCBCR420;
> + }
>
> - pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
> - if (pipe_config->ycbcr420 != clrspace_yuv ||
> - pipe_config->ycbcr420 != blend_mode_420)
> - DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
> - } else if (clrspace_yuv) {
> - DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
> }
> +
> + pipe_config->output_format = output_format;
> + DRM_DEBUG_KMS("Output format %s\n",
> + output_formats(output_format));
> }
>
> power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> @@ -10558,6 +10582,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
> buf, pipe_config->output_types);
>
> + DRM_DEBUG_KMS("output format: %s\n",
> + output_formats(pipe_config->output_format));
> +
> DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
> transcoder_name(pipe_config->cpu_transcoder),
> pipe_config->pipe_bpp, pipe_config->dither);
> @@ -10567,9 +10594,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> pipe_config->fdi_lanes,
> &pipe_config->fdi_m_n);
>
> - if (pipe_config->ycbcr420)
> - DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
> -
> if (intel_crtc_has_dp_encoder(pipe_config)) {
> intel_dump_m_n_config(pipe_config, "dp m_n",
> pipe_config->lane_count, &pipe_config->dp_m_n);
> @@ -11143,6 +11167,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>
> PIPE_CONF_CHECK_I(pixel_multiplier);
> + PIPE_CONF_CHECK_I(output_format);
> PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
> if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
> IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> @@ -11151,7 +11176,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
> PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
> PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
> - PIPE_CONF_CHECK_BOOL(ycbcr420);
>
> PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 30f791f..79662650 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -609,6 +609,12 @@ struct intel_crtc_wm_state {
> bool need_postvbl_update;
> };
>
> +enum crtc_output_format {
intel_output_format or something would fit the normal namespacing
better.
> + CRTC_OUTPUT_INVALID = -1,
The -1 doesn't make much sense to me. If we want to keep this IMO
just let the enum start from 0. That will at least make it clear
if you entirely forgot to do readout.
> + CRTC_OUTPUT_RGB,
> + CRTC_OUTPUT_YCBCR420,
> +};
> +
> struct intel_crtc_state {
> struct drm_crtc_state base;
>
> @@ -795,8 +801,8 @@ struct intel_crtc_state {
> /* HDMI High TMDS char rate ratio */
> bool hdmi_high_tmds_clock_ratio;
>
> - /* output format is YCBCR 4:2:0 */
> - bool ycbcr420;
> + /* Output format RGB/YCBCR etc */
> + enum crtc_output_format output_format;
> };
>
> struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index bced7b9..b266a7f 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -478,7 +478,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> return;
> }
>
> - if (crtc_state->ycbcr420)
> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
> frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> else
> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> @@ -1372,7 +1372,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
> if (connector_state->crtc != crtc_state->base.crtc)
> continue;
>
> - if (crtc_state->ycbcr420) {
> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
> const struct drm_hdmi_info *hdmi = &info->hdmi;
>
> if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
> @@ -1407,7 +1407,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
> config->port_clock /= 2;
> *clock_12bpc /= 2;
> *clock_8bpc /= 2;
> - config->ycbcr420 = true;
> + config->output_format = CRTC_OUTPUT_YCBCR420;
>
> /* YCBCR 420 output conversion needs a scaler */
> if (skl_update_scaler_crtc(config)) {
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index fa6831f..c57819f 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
> /* Native modes don't need fitting */
> if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
> adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
> - !pipe_config->ycbcr420)
> + pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
> goto done;
>
> switch (fitting_mode) {
> --
> 2.7.4
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4
2018-01-05 9:45 ` [PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4 Shashank Sharma
@ 2018-01-17 18:27 ` Ville Syrjälä
2018-01-18 6:23 ` Sharma, Shashank
0 siblings, 1 reply; 22+ messages in thread
From: Ville Syrjälä @ 2018-01-17 18:27 UTC (permalink / raw)
To: Shashank Sharma; +Cc: intel-gfx
On Fri, Jan 05, 2018 at 03:15:30PM +0530, Shashank Sharma wrote:
> This patch adds support for YCBCR 4:4:4 CRTC output format.
> To do this, this patch extends the existing YCBCR 4:2:0
> framework by:
> - Adding new parameter in for YCBCR 4:4:4 enum crtc_iutput_format.
> - Adding case for YCBCR 4:4:4 in while setting AVI infoframes.
> - Adding necessary checks in modeset sequence.
>
> V3: Added this patch in the series
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
> drivers/gpu/drm/i915/intel_color.c | 3 ++-
> drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_hdmi.c | 2 ++
> 4 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 99e32cb..5b76de6 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -141,7 +141,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
> uint16_t coeffs[9] = { 0, };
> struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>
> - if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
> + if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420 ||
> + intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
> i9xx_load_ycbcr_conversion_matrix(intel_crtc);
> return;
> } else if (crtc_state->ctm) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 69b0aa3..6ac5ca6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6357,8 +6357,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> return -EINVAL;
> }
>
> - if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
> - pipe_config->base.ctm) {
> + if ((pipe_config->output_format == CRTC_OUTPUT_YCBCR420 ||
> + pipe_config->output_format == CRTC_OUTPUT_YCBCR444) &&
> + pipe_config->base.ctm) {
> /*
> * There is only one pipe CSC unit per pipe, and we need that
> * for output conversion from RGB->YCBCR. So if CTM is already
> @@ -8179,11 +8180,12 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
> if (intel_crtc->config->dither)
> val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>
> - if (config->output_format == CRTC_OUTPUT_YCBCR420) {
> + if (config->output_format > CRTC_OUTPUT_RGB)
The '>' looks weird. For best clarity I would just use
'x == YCBCR444 || x == YCBCR420'
> val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> +
> + if (config->output_format == CRTC_OUTPUT_YCBCR420)
> val |= PIPEMISC_YUV420_ENABLE |
> PIPEMISC_YUV420_MODE_FULL_BLEND;
> - }
>
> I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> }
> @@ -9161,6 +9163,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
> static const char * const output_format_str[] = {
> "Invalid",
> "RGB",
> + "YCBCR4:4:4",
> "YCBCR4:2:0",
> };
>
> @@ -9226,6 +9229,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> output_format = CRTC_OUTPUT_INVALID;
> else
> output_format = CRTC_OUTPUT_YCBCR420;
> + } else {
> + output_format = CRTC_OUTPUT_YCBCR444;
> }
>
> }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 79662650..a393342 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -612,6 +612,7 @@ struct intel_crtc_wm_state {
> enum crtc_output_format {
> CRTC_OUTPUT_INVALID = -1,
> CRTC_OUTPUT_RGB,
> + CRTC_OUTPUT_YCBCR444,
> CRTC_OUTPUT_YCBCR420,
> };
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index b266a7f..258bb51 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -480,6 +480,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>
> if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
> frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> + else if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
> + frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
> else
> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>
> --
> 2.7.4
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
2018-01-05 9:45 ` [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON Shashank Sharma
@ 2018-01-17 18:27 ` Ville Syrjälä
2018-01-18 6:27 ` Sharma, Shashank
0 siblings, 1 reply; 22+ messages in thread
From: Ville Syrjälä @ 2018-01-17 18:27 UTC (permalink / raw)
To: Shashank Sharma; +Cc: intel-gfx
On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
> 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)
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
> 5 files changed, 49 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 966e4df..45ee264 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8547,6 +8547,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 7b89f2a..7616f6f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1499,6 +1499,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.
> + */
> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
This fails to state that we're indicating BT.601 encoding. I think we
should spell that out explicitly.
> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 35c5299..3bf82ea 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1613,6 +1613,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;
> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
> pipe_config->has_pch_encoder = true;
>
> + if (lspcon->active) {
> + struct drm_connector *connector = &intel_connector->base;
> +
> + if (lspcon_ycbcr420_config(connector, pipe_config)) {
> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
I think I'd like to see all compute_config hooks explicitly set the
outout_format (to RGB usually obviously).
> + lspcon->output_format = CRTC_OUTPUT_YCBCR420;
You should not modify any non-atomic state like that in compute_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 2de6b41..5edba06 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 066ea91..cb88138 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -180,6 +180,25 @@ 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;
> + }
> +
> + config->port_clock /= 2;
> + return true;
> + }
> +
> + return false;
> +}
> +
> static bool lspcon_probe(struct intel_lspcon *lspcon)
> {
> int retry;
> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
> return;
> }
>
> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
> + 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 :
> @@ -512,6 +538,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");
> @@ -536,6 +563,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 OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0
2018-01-17 18:27 ` Ville Syrjälä
@ 2018-01-18 6:21 ` Sharma, Shashank
0 siblings, 0 replies; 22+ messages in thread
From: Sharma, Shashank @ 2018-01-18 6:21 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Regards
Shashank
On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> On Fri, Jan 05, 2018 at 03:15:29PM +0530, Shashank Sharma wrote:
>> Currently, we are using a bool in CRTC state (state->ycbcr420),
>> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
>> order to support other YCBCR formats, we will need more such flags.
>>
>> The idea behind this patch is to replace this bool with an enum,
>> and plug this in in the existing YCBCR 4:2:0 framework in such a
>> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
>>
>> This patch adds a new enum for CRTC output formats, and then plugs it
>> in the existing modeset framework.
>>
>> V3: Added this patch in the series, to address review comments from
>> second patchset.
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_color.c | 2 +-
>> drivers/gpu/drm/i915/intel_ddi.c | 2 +-
>> drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
>> drivers/gpu/drm/i915/intel_drv.h | 10 ++++--
>> drivers/gpu/drm/i915/intel_hdmi.c | 6 ++--
>> drivers/gpu/drm/i915/intel_panel.c | 2 +-
>> 6 files changed, 59 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index aa66e95..99e32cb 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>> uint16_t coeffs[9] = { 0, };
>> struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>>
>> - if (intel_crtc_state->ycbcr420) {
>> + if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>> i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>> return;
>> } else if (crtc_state->ctm) {
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index f51645a..84327e7 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
>> else
>> dotclock = pipe_config->port_clock;
>>
>> - if (pipe_config->ycbcr420)
>> + if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
>> dotclock *= 2;
>>
>> if (pipe_config->pixel_multiplier)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 0cd3559..69b0aa3 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4644,7 +4644,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->output_format == CRTC_OUTPUT_YCBCR420 &&
>> + scaler_user == SKL_CRTC_INDEX)
>> need_scaling = true;
>>
>> /*
>> @@ -6356,7 +6357,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>> return -EINVAL;
>> }
>>
>> - if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
>> + if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
>> + pipe_config->base.ctm) {
>> /*
>> * There is only one pipe CSC unit per pipe, and we need that
>> * for output conversion from RGB->YCBCR. So if CTM is already
>> @@ -8177,10 +8179,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>> if (intel_crtc->config->dither)
>> val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>>
>> - if (config->ycbcr420) {
>> - val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
>> - PIPEMISC_YUV420_ENABLE |
>> - PIPEMISC_YUV420_MODE_FULL_BLEND;
>> + if (config->output_format == CRTC_OUTPUT_YCBCR420) {
>> + val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> + val |= PIPEMISC_YUV420_ENABLE |
>> + PIPEMISC_YUV420_MODE_FULL_BLEND;
>> }
>>
>> I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>> @@ -9156,6 +9158,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>> }
>> }
>>
>> +static const char * const output_format_str[] = {
>> + "Invalid",
>> + "RGB",
>> + "YCBCR4:2:0",
>> +};
>> +
>> +static const char *output_formats(enum crtc_output_format format)
>> +{
>> + if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
>> + format = CRTC_OUTPUT_INVALID;
>> + return output_format_str[format + 1];
>> +}
>> +
>> static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>> struct intel_crtc_state *pipe_config)
>> {
>> @@ -9196,19 +9211,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>
>> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>> u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>> - bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> -
>> - if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
>> - bool blend_mode_420 = tmp &
>> - PIPEMISC_YUV420_MODE_FULL_BLEND;
>> + enum crtc_output_format output_format = CRTC_OUTPUT_RGB;
>> +
>> + if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
>> + bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
>> + bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +
>> + if (ycbcr420_enabled) {
>> + /* We support 4:2:0 in full blend mode only */
>> + if (!blend)
>> + output_format = CRTC_OUTPUT_INVALID;
> Not sure that INVALID thing really provides any real value.
> Just feels like it's making the code more convoluted for something that
> should never really happen.
The check is directly adapted from the previous version of the code,
where we were still checking if
the registers are programmed properly, CRTC_OUTPUT_INVALID helps to
print the error messages and
fits into the existing framework, so I thought there is no harm in
keeping it.
>> + else if (!(IS_GEMINILAKE(dev_priv) ||
>> + INTEL_GEN(dev_priv) >= 10))
>> + output_format = CRTC_OUTPUT_INVALID;
>> + else
>> + output_format = CRTC_OUTPUT_YCBCR420;
>> + }
>>
>> - pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
>> - if (pipe_config->ycbcr420 != clrspace_yuv ||
>> - pipe_config->ycbcr420 != blend_mode_420)
>> - DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
>> - } else if (clrspace_yuv) {
>> - DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
>> }
>> +
>> + pipe_config->output_format = output_format;
>> + DRM_DEBUG_KMS("Output format %s\n",
>> + output_formats(output_format));
>> }
>>
>> power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>> @@ -10558,6 +10582,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>> DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
>> buf, pipe_config->output_types);
>>
>> + DRM_DEBUG_KMS("output format: %s\n",
>> + output_formats(pipe_config->output_format));
>> +
>> DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
>> transcoder_name(pipe_config->cpu_transcoder),
>> pipe_config->pipe_bpp, pipe_config->dither);
>> @@ -10567,9 +10594,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>> pipe_config->fdi_lanes,
>> &pipe_config->fdi_m_n);
>>
>> - if (pipe_config->ycbcr420)
>> - DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
>> -
>> if (intel_crtc_has_dp_encoder(pipe_config)) {
>> intel_dump_m_n_config(pipe_config, "dp m_n",
>> pipe_config->lane_count, &pipe_config->dp_m_n);
>> @@ -11143,6 +11167,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>> PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>>
>> PIPE_CONF_CHECK_I(pixel_multiplier);
>> + PIPE_CONF_CHECK_I(output_format);
>> PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
>> if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>> IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> @@ -11151,7 +11176,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>> PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
>> PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
>> PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
>> - PIPE_CONF_CHECK_BOOL(ycbcr420);
>>
>> PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 30f791f..79662650 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -609,6 +609,12 @@ struct intel_crtc_wm_state {
>> bool need_postvbl_update;
>> };
>>
>> +enum crtc_output_format {
> intel_output_format or something would fit the normal namespacing
> better.
Agree, will change it accordingly.
>
>> + CRTC_OUTPUT_INVALID = -1,
> The -1 doesn't make much sense to me. If we want to keep this IMO
> just let the enum start from 0. That will at least make it clear
> if you entirely forgot to do readout.
The reason why I kept INVALID as -1, is to make sure RGB starts at 0. So
that we need not to
explicitly set the CRTC_OUTPUT_FORMAT = RGB (which is also one of your
comment in the
series). This will allow least modifications in the existing code.
May be I can keep it in the end of the enum, but that would be moving as
and when we add
new formats. What do you suggest ?
- Shashank
>> + CRTC_OUTPUT_RGB,
>> + CRTC_OUTPUT_YCBCR420,
>> +};
>> +
>> struct intel_crtc_state {
>> struct drm_crtc_state base;
>>
>> @@ -795,8 +801,8 @@ struct intel_crtc_state {
>> /* HDMI High TMDS char rate ratio */
>> bool hdmi_high_tmds_clock_ratio;
>>
>> - /* output format is YCBCR 4:2:0 */
>> - bool ycbcr420;
>> + /* Output format RGB/YCBCR etc */
>> + enum crtc_output_format output_format;
>> };
>>
>> struct intel_crtc {
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index bced7b9..b266a7f 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -478,7 +478,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>> return;
>> }
>>
>> - if (crtc_state->ycbcr420)
>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>> frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>> else
>> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>> @@ -1372,7 +1372,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
>> if (connector_state->crtc != crtc_state->base.crtc)
>> continue;
>>
>> - if (crtc_state->ycbcr420) {
>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>> const struct drm_hdmi_info *hdmi = &info->hdmi;
>>
>> if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
>> @@ -1407,7 +1407,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>> config->port_clock /= 2;
>> *clock_12bpc /= 2;
>> *clock_8bpc /= 2;
>> - config->ycbcr420 = true;
>> + config->output_format = CRTC_OUTPUT_YCBCR420;
>>
>> /* YCBCR 420 output conversion needs a scaler */
>> if (skl_update_scaler_crtc(config)) {
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index fa6831f..c57819f 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
>> /* Native modes don't need fitting */
>> if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
>> adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
>> - !pipe_config->ycbcr420)
>> + pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
>> goto done;
>>
>> switch (fitting_mode) {
>> --
>> 2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4
2018-01-17 18:27 ` Ville Syrjälä
@ 2018-01-18 6:23 ` Sharma, Shashank
0 siblings, 0 replies; 22+ messages in thread
From: Sharma, Shashank @ 2018-01-18 6:23 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Regards
Shashank
On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> On Fri, Jan 05, 2018 at 03:15:30PM +0530, Shashank Sharma wrote:
>> This patch adds support for YCBCR 4:4:4 CRTC output format.
>> To do this, this patch extends the existing YCBCR 4:2:0
>> framework by:
>> - Adding new parameter in for YCBCR 4:4:4 enum crtc_iutput_format.
>> - Adding case for YCBCR 4:4:4 in while setting AVI infoframes.
>> - Adding necessary checks in modeset sequence.
>>
>> V3: Added this patch in the series
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_color.c | 3 ++-
>> drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>> drivers/gpu/drm/i915/intel_hdmi.c | 2 ++
>> 4 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index 99e32cb..5b76de6 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -141,7 +141,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>> uint16_t coeffs[9] = { 0, };
>> struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>>
>> - if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>> + if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420 ||
>> + intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
>> i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>> return;
>> } else if (crtc_state->ctm) {
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 69b0aa3..6ac5ca6 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6357,8 +6357,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>> return -EINVAL;
>> }
>>
>> - if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
>> - pipe_config->base.ctm) {
>> + if ((pipe_config->output_format == CRTC_OUTPUT_YCBCR420 ||
>> + pipe_config->output_format == CRTC_OUTPUT_YCBCR444) &&
>> + pipe_config->base.ctm) {
>> /*
>> * There is only one pipe CSC unit per pipe, and we need that
>> * for output conversion from RGB->YCBCR. So if CTM is already
>> @@ -8179,11 +8180,12 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>> if (intel_crtc->config->dither)
>> val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>>
>> - if (config->output_format == CRTC_OUTPUT_YCBCR420) {
>> + if (config->output_format > CRTC_OUTPUT_RGB)
> The '>' looks weird. For best clarity I would just use
> 'x == YCBCR444 || x == YCBCR420'
I knew I would get this comment, still gave a try :-), was just trying
to reduce one if condition, and dint want
to add (x == YCBCR444 || x == YCBCR420) to all the places. Will do the
changes.
- Shashank
>> val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> +
>> + if (config->output_format == CRTC_OUTPUT_YCBCR420)
>> val |= PIPEMISC_YUV420_ENABLE |
>> PIPEMISC_YUV420_MODE_FULL_BLEND;
>> - }
>>
>> I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>> }
>> @@ -9161,6 +9163,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>> static const char * const output_format_str[] = {
>> "Invalid",
>> "RGB",
>> + "YCBCR4:4:4",
>> "YCBCR4:2:0",
>> };
>>
>> @@ -9226,6 +9229,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>> output_format = CRTC_OUTPUT_INVALID;
>> else
>> output_format = CRTC_OUTPUT_YCBCR420;
>> + } else {
>> + output_format = CRTC_OUTPUT_YCBCR444;
>> }
>>
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 79662650..a393342 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -612,6 +612,7 @@ struct intel_crtc_wm_state {
>> enum crtc_output_format {
>> CRTC_OUTPUT_INVALID = -1,
>> CRTC_OUTPUT_RGB,
>> + CRTC_OUTPUT_YCBCR444,
>> CRTC_OUTPUT_YCBCR420,
>> };
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index b266a7f..258bb51 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -480,6 +480,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>>
>> if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>> frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>> + else if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
>> else
>> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>
>> --
>> 2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
2018-01-17 18:27 ` Ville Syrjälä
@ 2018-01-18 6:27 ` Sharma, Shashank
2018-01-18 9:30 ` Maarten Lankhorst
2018-01-18 14:03 ` Ville Syrjälä
0 siblings, 2 replies; 22+ messages in thread
From: Sharma, Shashank @ 2018-01-18 6:27 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Regards
Shashank
On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>> 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)
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 2 ++
>> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>> 5 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 966e4df..45ee264 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1499,6 +1499,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.
>> + */
>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
> This fails to state that we're indicating BT.601 encoding. I think we
> should spell that out explicitly.
Agree, I will add this in comments.
>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 35c5299..3bf82ea 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1613,6 +1613,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;
>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>> pipe_config->has_pch_encoder = true;
>>
>> + if (lspcon->active) {
>> + struct drm_connector *connector = &intel_connector->base;
>> +
>> + if (lspcon_ycbcr420_config(connector, pipe_config)) {
>> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
> I think I'd like to see all compute_config hooks explicitly set the
> outout_format (to RGB usually obviously).
That's the one I was talking about in previous patch. If we keep
output_format_RGB = 0, we need
not to do this, as reset of the pipe_config will automatically make it RGB.
>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420;
> You should not modify any non-atomic state like that in compute_config.
Please help me to understand this better, Can you elaborate a bit more on:
- Why is this a non-atomic state ?
- What is the right place we should modify it ?
- Shashank
>> + }
>> + }
>> +
>> 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 2de6b41..5edba06 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 066ea91..cb88138 100644
>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -180,6 +180,25 @@ 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;
>> + }
>> +
>> + config->port_clock /= 2;
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> static bool lspcon_probe(struct intel_lspcon *lspcon)
>> {
>> int retry;
>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>> return;
>> }
>>
>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>> + 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 :
>> @@ -512,6 +538,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");
>> @@ -536,6 +563,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
2018-01-18 6:27 ` Sharma, Shashank
@ 2018-01-18 9:30 ` Maarten Lankhorst
2018-01-18 16:16 ` Sharma, Shashank
2018-01-18 14:03 ` Ville Syrjälä
1 sibling, 1 reply; 22+ messages in thread
From: Maarten Lankhorst @ 2018-01-18 9:30 UTC (permalink / raw)
To: Sharma, Shashank, Ville Syrjälä; +Cc: intel-gfx
Op 18-01-18 om 07:27 schreef Sharma, Shashank:
> Regards
>
> Shashank
>
>
> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>>> 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)
>>>
>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++
>>> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>>> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
>>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>> 5 files changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 966e4df..45ee264 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -1499,6 +1499,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.
>>> + */
>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>> This fails to state that we're indicating BT.601 encoding. I think we
>> should spell that out explicitly.
> Agree, I will add this in comments.
>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>> }
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 35c5299..3bf82ea 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -1613,6 +1613,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;
>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>> pipe_config->has_pch_encoder = true;
>>> + if (lspcon->active) {
>>> + struct drm_connector *connector = &intel_connector->base;
>>> +
>>> + if (lspcon_ycbcr420_config(connector, pipe_config)) {
>>> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
>> I think I'd like to see all compute_config hooks explicitly set the
>> outout_format (to RGB usually obviously).
> That's the one I was talking about in previous patch. If we keep output_format_RGB = 0, we need
> not to do this, as reset of the pipe_config will automatically make it RGB.
>>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420;
>> You should not modify any non-atomic state like that in compute_config.
> Please help me to understand this better, Can you elaborate a bit more on:
> - Why is this a non-atomic state ?
Because lspcon->output_format is modified even if a commit is TEST_ONLY..
This will break if you do a nonblocking modeset vs TEST_ONLY, it could commit the wrong lspcon->output_format. :)
> - What is the right place we should modify it ?
intel_digital_connector_state probably.
~Maarten
>
> - Shashank
>>> + }
>>> + }
>>> +
>>> 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 2de6b41..5edba06 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 066ea91..cb88138 100644
>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>> @@ -180,6 +180,25 @@ 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;
>>> + }
>>> +
>>> + config->port_clock /= 2;
>>> + return true;
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> static bool lspcon_probe(struct intel_lspcon *lspcon)
>>> {
>>> int retry;
>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>> return;
>>> }
>>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>>> + 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 :
>>> @@ -512,6 +538,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");
>>> @@ -536,6 +563,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
2018-01-18 6:27 ` Sharma, Shashank
2018-01-18 9:30 ` Maarten Lankhorst
@ 2018-01-18 14:03 ` Ville Syrjälä
2018-01-18 15:30 ` Sharma, Shashank
1 sibling, 1 reply; 22+ messages in thread
From: Ville Syrjälä @ 2018-01-18 14:03 UTC (permalink / raw)
To: Sharma, Shashank; +Cc: intel-gfx
On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> > On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
> >> 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)
> >>
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> >> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
> >> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
> >> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> >> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
> >> 5 files changed, 49 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index 966e4df..45ee264 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -8547,6 +8547,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 7b89f2a..7616f6f 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -1499,6 +1499,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.
> >> + */
> >> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
> >> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
> > This fails to state that we're indicating BT.601 encoding. I think we
> > should spell that out explicitly.
> Agree, I will add this in comments.
> >> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
> >> }
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 35c5299..3bf82ea 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -1613,6 +1613,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;
> >> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
> >> pipe_config->has_pch_encoder = true;
> >>
> >> + if (lspcon->active) {
> >> + struct drm_connector *connector = &intel_connector->base;
> >> +
> >> + if (lspcon_ycbcr420_config(connector, pipe_config)) {
> >> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
> > I think I'd like to see all compute_config hooks explicitly set the
> > outout_format (to RGB usually obviously).
> That's the one I was talking about in previous patch. If we keep
> output_format_RGB = 0, we need
> not to do this, as reset of the pipe_config will automatically make it RGB.
IMO either we have INVALID=0 so that we use it to catch readout
fails, or we have no INVALID. Other options make little sense to me.
> >> + lspcon->output_format = CRTC_OUTPUT_YCBCR420;
> > You should not modify any non-atomic state like that in compute_config.
> Please help me to understand this better, Can you elaborate a bit more on:
> - Why is this a non-atomic state ?
> - What is the right place we should modify it ?
I don't think you need it at all. Just consult the crtc state when
needed.
>
> - Shashank
> >> + }
> >> + }
> >> +
> >> 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 2de6b41..5edba06 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 066ea91..cb88138 100644
> >> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> >> @@ -180,6 +180,25 @@ 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;
> >> + }
> >> +
> >> + config->port_clock /= 2;
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> +}
> >> +
> >> static bool lspcon_probe(struct intel_lspcon *lspcon)
> >> {
> >> int retry;
> >> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
> >> return;
> >> }
> >>
> >> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
> >> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> >> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
> >> + 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 :
> >> @@ -512,6 +538,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");
> >> @@ -536,6 +563,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 OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
2018-01-18 14:03 ` Ville Syrjälä
@ 2018-01-18 15:30 ` Sharma, Shashank
2018-01-18 15:35 ` Ville Syrjälä
0 siblings, 1 reply; 22+ messages in thread
From: Sharma, Shashank @ 2018-01-18 15:30 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Regards
Shashank
On 1/18/2018 7:33 PM, Ville Syrjälä wrote:
> On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
>>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>>>> 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)
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++
>>>> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>>>> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
>>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
>>>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>>> 5 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 966e4df..45ee264 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -1499,6 +1499,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.
>>>> + */
>>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>>> This fails to state that we're indicating BT.601 encoding. I think we
>>> should spell that out explicitly.
>> Agree, I will add this in comments.
>>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>>> }
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 35c5299..3bf82ea 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -1613,6 +1613,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;
>>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>>> pipe_config->has_pch_encoder = true;
>>>>
>>>> + if (lspcon->active) {
>>>> + struct drm_connector *connector = &intel_connector->base;
>>>> +
>>>> + if (lspcon_ycbcr420_config(connector, pipe_config)) {
>>>> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
>>> I think I'd like to see all compute_config hooks explicitly set the
>>> outout_format (to RGB usually obviously).
>> That's the one I was talking about in previous patch. If we keep
>> output_format_RGB = 0, we need
>> not to do this, as reset of the pipe_config will automatically make it RGB.
> IMO either we have INVALID=0 so that we use it to catch readout
> fails, or we have no INVALID. Other options make little sense to me.
Its a minor thing, and doesn't really matter. I can change this.
>>>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420;
>>> You should not modify any non-atomic state like that in compute_config.
>> Please help me to understand this better, Can you elaborate a bit more on:
>> - Why is this a non-atomic state ?
>> - What is the right place we should modify it ?
> I don't think you need it at all. Just consult the crtc state when
> needed.
I dont think CRTC state can cover all the cases, for example, what would
be do when we need
YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444
for LSPCON output
420, we can't handle this. This is equivalent to your previous
suggestion of adding 'scaling_reqd',
but I added in LPSCON (instead of CRTC state) as this information is
required only in case of LSPCON.
In all native cases, scaling would be always required for 4:2:0 outputs.
- Shashank
>> - Shashank
>>>> + }
>>>> + }
>>>> +
>>>> 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 2de6b41..5edba06 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 066ea91..cb88138 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>>> @@ -180,6 +180,25 @@ 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;
>>>> + }
>>>> +
>>>> + config->port_clock /= 2;
>>>> + return true;
>>>> + }
>>>> +
>>>> + return false;
>>>> +}
>>>> +
>>>> static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>> {
>>>> int retry;
>>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>> return;
>>>> }
>>>>
>>>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>>>> + 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 :
>>>> @@ -512,6 +538,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");
>>>> @@ -536,6 +563,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
2018-01-18 15:30 ` Sharma, Shashank
@ 2018-01-18 15:35 ` Ville Syrjälä
2018-01-18 15:52 ` Sharma, Shashank
0 siblings, 1 reply; 22+ messages in thread
From: Ville Syrjälä @ 2018-01-18 15:35 UTC (permalink / raw)
To: Sharma, Shashank; +Cc: intel-gfx
On Thu, Jan 18, 2018 at 09:00:50PM +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 1/18/2018 7:33 PM, Ville Syrjälä wrote:
> > On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> >>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
> >>>> 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)
> >>>>
> >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>> ---
> >>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> >>>> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
> >>>> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
> >>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> >>>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
> >>>> 5 files changed, 49 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>>> index 966e4df..45ee264 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> @@ -1499,6 +1499,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.
> >>>> + */
> >>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
> >>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
> >>> This fails to state that we're indicating BT.601 encoding. I think we
> >>> should spell that out explicitly.
> >> Agree, I will add this in comments.
> >>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
> >>>> }
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>> index 35c5299..3bf82ea 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>> @@ -1613,6 +1613,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;
> >>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >>>> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
> >>>> pipe_config->has_pch_encoder = true;
> >>>>
> >>>> + if (lspcon->active) {
> >>>> + struct drm_connector *connector = &intel_connector->base;
> >>>> +
> >>>> + if (lspcon_ycbcr420_config(connector, pipe_config)) {
> >>>> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
> >>> I think I'd like to see all compute_config hooks explicitly set the
> >>> outout_format (to RGB usually obviously).
> >> That's the one I was talking about in previous patch. If we keep
> >> output_format_RGB = 0, we need
> >> not to do this, as reset of the pipe_config will automatically make it RGB.
> > IMO either we have INVALID=0 so that we use it to catch readout
> > fails, or we have no INVALID. Other options make little sense to me.
> Its a minor thing, and doesn't really matter. I can change this.
> >>>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420;
> >>> You should not modify any non-atomic state like that in compute_config.
> >> Please help me to understand this better, Can you elaborate a bit more on:
> >> - Why is this a non-atomic state ?
> >> - What is the right place we should modify it ?
> > I don't think you need it at all. Just consult the crtc state when
> > needed.
> I dont think CRTC state can cover all the cases, for example, what would
> be do when we need
> YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444
> for LSPCON output
> 420, we can't handle this.
Like I said earlier, just add another thing to the state to indicate what
LSPCON should do with the 444 data. Either pass it through or downsample
it.
> This is equivalent to your previous
> suggestion of adding 'scaling_reqd',
> but I added in LPSCON (instead of CRTC state) as this information is
> required only in case of LSPCON.
But we have no atomic lspcon state. And probably not worth adding one
since there won't be much there. So just adding what's needed into the
crtc state seems like the best approach.
> In all native cases, scaling would be always required for 4:2:0 outputs.
>
> - Shashank
> >> - Shashank
> >>>> + }
> >>>> + }
> >>>> +
> >>>> 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 2de6b41..5edba06 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 066ea91..cb88138 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> >>>> @@ -180,6 +180,25 @@ 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;
> >>>> + }
> >>>> +
> >>>> + config->port_clock /= 2;
> >>>> + return true;
> >>>> + }
> >>>> +
> >>>> + return false;
> >>>> +}
> >>>> +
> >>>> static bool lspcon_probe(struct intel_lspcon *lspcon)
> >>>> {
> >>>> int retry;
> >>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
> >>>> return;
> >>>> }
> >>>>
> >>>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
> >>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> >>>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
> >>>> + 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 :
> >>>> @@ -512,6 +538,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");
> >>>> @@ -536,6 +563,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 OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
2018-01-18 15:35 ` Ville Syrjälä
@ 2018-01-18 15:52 ` Sharma, Shashank
0 siblings, 0 replies; 22+ messages in thread
From: Sharma, Shashank @ 2018-01-18 15:52 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Regards
Shashank
On 1/18/2018 9:05 PM, Ville Syrjälä wrote:
> On Thu, Jan 18, 2018 at 09:00:50PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/18/2018 7:33 PM, Ville Syrjälä wrote:
>>> On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
>>>>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>>>>>> 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)
>>>>>>
>>>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++
>>>>>> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>>>>>> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
>>>>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
>>>>>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>>>>> 5 files changed, 49 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>>> index 966e4df..45ee264 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>> @@ -1499,6 +1499,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.
>>>>>> + */
>>>>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>>>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>>>>> This fails to state that we're indicating BT.601 encoding. I think we
>>>>> should spell that out explicitly.
>>>> Agree, I will add this in comments.
>>>>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>>>>> }
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> index 35c5299..3bf82ea 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> @@ -1613,6 +1613,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;
>>>>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>>>> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>>>>> pipe_config->has_pch_encoder = true;
>>>>>>
>>>>>> + if (lspcon->active) {
>>>>>> + struct drm_connector *connector = &intel_connector->base;
>>>>>> +
>>>>>> + if (lspcon_ycbcr420_config(connector, pipe_config)) {
>>>>>> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
>>>>> I think I'd like to see all compute_config hooks explicitly set the
>>>>> outout_format (to RGB usually obviously).
>>>> That's the one I was talking about in previous patch. If we keep
>>>> output_format_RGB = 0, we need
>>>> not to do this, as reset of the pipe_config will automatically make it RGB.
>>> IMO either we have INVALID=0 so that we use it to catch readout
>>> fails, or we have no INVALID. Other options make little sense to me.
>> Its a minor thing, and doesn't really matter. I can change this.
>>>>>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420;
>>>>> You should not modify any non-atomic state like that in compute_config.
>>>> Please help me to understand this better, Can you elaborate a bit more on:
>>>> - Why is this a non-atomic state ?
>>>> - What is the right place we should modify it ?
>>> I don't think you need it at all. Just consult the crtc state when
>>> needed.
>> I dont think CRTC state can cover all the cases, for example, what would
>> be do when we need
>> YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444
>> for LSPCON output
>> 420, we can't handle this.
> Like I said earlier, just add another thing to the state to indicate what
> LSPCON should do with the 444 data. Either pass it through or downsample
> it.
>
>> This is equivalent to your previous
>> suggestion of adding 'scaling_reqd',
>> but I added in LPSCON (instead of CRTC state) as this information is
>> required only in case of LSPCON.
> But we have no atomic lspcon state. And probably not worth adding one
> since there won't be much there. So just adding what's needed into the
> crtc state seems like the best approach.
Ok, So I guess the suggestion to move this bool into CRTC state is to
maintain the atomic flow.
Sure, in that case I will add a new bool in crtc_state called
"output_scaling_reqd" or something
similar.
- Shashank
>> In all native cases, scaling would be always required for 4:2:0 outputs.
>>
>> - Shashank
>>>> - Shashank
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> 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 2de6b41..5edba06 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 066ea91..cb88138 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>>>>> @@ -180,6 +180,25 @@ 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;
>>>>>> + }
>>>>>> +
>>>>>> + config->port_clock /= 2;
>>>>>> + return true;
>>>>>> + }
>>>>>> +
>>>>>> + return false;
>>>>>> +}
>>>>>> +
>>>>>> static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>>>> {
>>>>>> int retry;
>>>>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>>>>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>>>>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>>>>>> + 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 :
>>>>>> @@ -512,6 +538,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");
>>>>>> @@ -536,6 +563,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
2018-01-18 9:30 ` Maarten Lankhorst
@ 2018-01-18 16:16 ` Sharma, Shashank
0 siblings, 0 replies; 22+ messages in thread
From: Sharma, Shashank @ 2018-01-18 16:16 UTC (permalink / raw)
To: Maarten Lankhorst, Ville Syrjälä; +Cc: intel-gfx
Regards
Shashank
On 1/18/2018 3:00 PM, Maarten Lankhorst wrote:
> Op 18-01-18 om 07:27 schreef Sharma, Shashank:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
>>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>>>> 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)
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++
>>>> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>>>> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
>>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
>>>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>>> 5 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 966e4df..45ee264 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -1499,6 +1499,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.
>>>> + */
>>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>>> This fails to state that we're indicating BT.601 encoding. I think we
>>> should spell that out explicitly.
>> Agree, I will add this in comments.
>>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>>> }
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 35c5299..3bf82ea 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -1613,6 +1613,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;
>>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>>> pipe_config->has_pch_encoder = true;
>>>> + if (lspcon->active) {
>>>> + struct drm_connector *connector = &intel_connector->base;
>>>> +
>>>> + if (lspcon_ycbcr420_config(connector, pipe_config)) {
>>>> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
>>> I think I'd like to see all compute_config hooks explicitly set the
>>> outout_format (to RGB usually obviously).
>> That's the one I was talking about in previous patch. If we keep output_format_RGB = 0, we need
>> not to do this, as reset of the pipe_config will automatically make it RGB.
>>>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420;
>>> You should not modify any non-atomic state like that in compute_config.
>> Please help me to understand this better, Can you elaborate a bit more on:
>> - Why is this a non-atomic state ?
> Because lspcon->output_format is modified even if a commit is TEST_ONLY..
>
> This will break if you do a nonblocking modeset vs TEST_ONLY, it could commit the wrong lspcon->output_format. :)
>> - What is the right place we should modify it ?
> intel_digital_connector_state probably.
>
> ~Maarten
Thanks for the explanation Maarten, this was very helpful :-).
- Shashank
>> - Shashank
>>>> + }
>>>> + }
>>>> +
>>>> 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 2de6b41..5edba06 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 066ea91..cb88138 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>>> @@ -180,6 +180,25 @@ 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;
>>>> + }
>>>> +
>>>> + config->port_clock /= 2;
>>>> + return true;
>>>> + }
>>>> +
>>>> + return false;
>>>> +}
>>>> +
>>>> static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>> {
>>>> int retry;
>>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>> return;
>>>> }
>>>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>>>> + 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 :
>>>> @@ -512,6 +538,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");
>>>> @@ -536,6 +563,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-01-18 16:16 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-05 9:45 [PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0 Shashank Sharma
2018-01-05 11:21 ` Maarten Lankhorst
2018-01-17 18:27 ` Ville Syrjälä
2018-01-18 6:21 ` Sharma, Shashank
2018-01-05 9:45 ` [PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4 Shashank Sharma
2018-01-17 18:27 ` Ville Syrjälä
2018-01-18 6:23 ` Sharma, Shashank
2018-01-05 9:45 ` [PATCH v3 3/7] drm/i915: Check LSPCON vendor OUI Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 4/7] drm/i915: Add AVI infoframe support for LSPCON Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 5/7] drm/i915: Write AVI infoframes for MCA LSPCON Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 6/7] drm/i915: Write AVI infoframes for Parade LSPCON Shashank Sharma
2018-01-05 9:45 ` [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON Shashank Sharma
2018-01-17 18:27 ` Ville Syrjälä
2018-01-18 6:27 ` Sharma, Shashank
2018-01-18 9:30 ` Maarten Lankhorst
2018-01-18 16:16 ` Sharma, Shashank
2018-01-18 14:03 ` Ville Syrjälä
2018-01-18 15:30 ` Sharma, Shashank
2018-01-18 15:35 ` Ville Syrjälä
2018-01-18 15:52 ` Sharma, Shashank
2018-01-05 10:15 ` ✗ Fi.CI.BAT: warning for YCBCR 4:2:0/4:4:4 output " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.