Regards Shashank On 2/2/2018 7:30 PM, Ville Syrjälä wrote:
On Fri, Feb 02, 2018 at 11:38:07AM +0530, Sharma, Shashank wrote:Thanks for the comments, mine inline. Regards Shashank On 2/2/2018 12:39 AM, Ville Syrjälä wrote:On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote:From: "Sharma, Shashank" <shashank.sharma@xxxxxxxxx> 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. V4: Added r-b from Maarten (on v3) Addressed review comments from Ville: - Change the enum name to intel_output_format from crtc_output_format. - Start the enum value (INVALID) from 0 instaed of 1. - Set the crtc's output_format to RGB in encoder's compute_config. Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> --- drivers/gpu/drm/i915/intel_color.c | 2 +- drivers/gpu/drm/i915/intel_ddi.c | 4 ++- 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, 61 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 e51559b..d565e28 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)@@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder, if (port == PORT_A) pipe_config->cpu_transcoder = TRANSCODER_EDP;+ pipe_config->output_format = CRTC_OUTPUT_RGB;We seem to be missing this for most platforms/encoder types.I thought this would be required only for DDI interfaces, do you suggest we should set this per encoder basis ?Yes, I'd like to see every encoder set it. Otherwise our state dumps will contain invalid information.
Got it.
+ if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI)) ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state); else diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 83de43c..877336d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4650,7 +4650,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;/*@@ -6362,7 +6363,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 @@ -8192,10 +8194,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;Why two |= ?Wanna make it clear that first set is for all YCBCR outputs(420 and 444) whereas other two are explicitly for 420.Fair enough.}I915_WRITE(PIPEMISC(intel_crtc->pipe), val);@@ -9171,6 +9173,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 intel_output_format format) +{ + if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420) + format = CRTC_OUTPUT_INVALID;BTW >= ARRAY_SIZE(format_str) would seem like a better check here.
Yep, agree.
+ return output_format_str[format]; +} + static bool haswell_get_pipe_config(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { @@ -9211,19 +9226,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 intel_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));Useless debug print.Its a _kms only, but can be removed if you think so.Yes, we can't have everyone pollute the dmesg with their favorite detail of the day. That'll lead to the whole thing being mostly noise. I think the important stuff we want to print is mostly to do with error conditions and the modeset sequencing. Detais about the crtc/plane/etc. states should be covered by the state dumps.
Ok, it will be gone. - Shashank
- Shashank}power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);@@ -10578,6 +10602,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); @@ -10587,9 +10614,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); @@ -11163,6 +11187,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)) @@ -11171,7 +11196,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.hindex 3cee54b..35be78e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -687,6 +687,12 @@ struct intel_crtc_wm_state { bool need_postvbl_update; };+enum intel_output_format {+ CRTC_OUTPUT_INVALID, + CRTC_OUTPUT_RGB, + CRTC_OUTPUT_YCBCR420, +}; + struct intel_crtc_state { struct drm_crtc_state base;@@ -873,8 +879,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 intel_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 978f22b..6700ed6 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -479,7 +479,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; @@ -1615,7 +1615,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))@@ -1650,7 +1650,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 e702a64..17025bc 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
