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@xxxxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
