| Message ID | 1515145535-11228-2-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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) {
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(-)