From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Shashank Sharma <shashank.sharma@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0
Date: Wed, 17 Jan 2018 20:27:22 +0200 [thread overview]
Message-ID: <20180117182722.GP10981@intel.com> (raw)
In-Reply-To: <1515145535-11228-2-git-send-email-shashank.sharma@intel.com>
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
next prev parent reply other threads:[~2018-01-17 18:27 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
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ä [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180117182722.GP10981@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=shashank.sharma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.