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.
> >
> >> +
> >> 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.
> >> + 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.
> - 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.h
> >> index 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
