| Message ID | 1500302187-6464-4-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 17, 2017 at 08:06:24PM +0530, Shashank Sharma wrote: > To get HDMI YCBCR420 output, the PIPEMISC register should be > programmed to: > - Generate YCBCR output (bit 11) > - In case of YCBCR420 outputs, it should be programmed in full > blend mode to use the scaler in 5x3 ratio (bits 26 and 27) > > This patch: > - Adds definition of these bits. > - Programs PIPEMISC for YCBCR420 outputs. > - Adds readouts to compare HW and SW states. > > V2: rebase > V3: rebase > V4: rebase > V5: added r-b from Ander > V6: Handle only YCBCR420 outputs (ville) > V7: rebase > V8: Addressed review comments from Ville > - Add readouts for state->ycbcr420 and 420 pixel_clock. > - Handle warning due to mismatch in clock for ycbcr420 clock. > - Rename PIPEMISC macros to match the Bspec. > - Add a debug print stating if YCBCR 4:2:0 output enabled. > Added r-b from Ville > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> > Reviewed-by: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 3 ++ > drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++-- > 2 files changed, 55 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c712d01..6dfcdd3 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -5227,6 +5227,9 @@ enum { > > #define _PIPE_MISC_A 0x70030 > #define _PIPE_MISC_B 0x71030 > +#define PIPEMISC_YUV420_ENABLE (1<<27) > +#define PIPEMISC_YUV420_MODE_BLEND (1<<26) > +#define PIPEMISC_OUTPUT_YUV (1<<11) Missing the rename here requested by Ville. > #define PIPEMISC_DITHER_BPC_MASK (7<<5) > #define PIPEMISC_DITHER_8_BPC (0<<5) > #define PIPEMISC_DITHER_10_BPC (1<<5) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index d78f1c2..788502a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6216,7 +6216,6 @@ static uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config) > * We only use IF-ID interlacing. If we ever use > * PF-ID we'll need to adjust the pixel_rate here. > */ > - Extra w/s change. > if (pipe_config->pch_pfit.enabled) { > uint64_t pipe_w, pipe_h, pfit_w, pfit_h; > uint32_t pfit_size = pipe_config->pch_pfit.size; > @@ -8076,6 +8075,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_crtc_state *config = intel_crtc->config; > > if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { > u32 val = 0; > @@ -8101,6 +8101,12 @@ 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_YUV | > + PIPEMISC_YUV420_ENABLE | > + PIPEMISC_YUV420_MODE_BLEND; > + } > + > I915_WRITE(PIPEMISC(intel_crtc->pipe), val); > } > } > @@ -9170,6 +9176,10 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); > } > > + if (IS_GEMINILAKE(dev_priv)) > + pipe_config->ycbcr420 = I915_READ(PIPEMISC(crtc->pipe)) & > + PIPEMISC_YUV420_ENABLE; > + > power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe); > if (intel_display_power_get_if_enabled(dev_priv, power_domain)) { > power_domain_mask |= BIT_ULL(power_domain); > @@ -11377,6 +11387,9 @@ 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); > @@ -11704,6 +11717,22 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) > } > } > > +static bool intel_420_clock_check(int clock1, int clock2) > +{ > + int diff; > + > + if (clock1 == clock2 * 2) > + return true; > + > + clock2 *= 2; > + diff = abs(clock1 - clock2); > + > + if (((((diff + clock1 + clock2) * 100)) / (clock1 + clock2)) < 105) > + return true; > + > + return false; > +} > + > static bool intel_fuzzy_clock_check(int clock1, int clock2) > { > int diff; > @@ -11832,6 +11861,18 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > ret = false; \ > } > > +#define PIPE_CONF_CHECK_CLOCK_420(name) \ > + do { \ > + if (!intel_420_clock_check(current_config->name, \ > + pipe_config->name)) { \ > + pipe_config_err(adjust, __stringify(name), \ > + "(expected %i, found %i)\n", \ > + current_config->name, \ > + pipe_config->name); \ > + ret = false; \ > + } \ > + } while (0) > + > #define PIPE_CONF_CHECK_M_N(name) \ > if (!intel_compare_link_m_n(¤t_config->name, \ > &pipe_config->name,\ > @@ -11983,7 +12024,10 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > } > > PIPE_CONF_CHECK_I(scaler_state.scaler_id); > - PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); > + if (current_config->ycbcr420) > + PIPE_CONF_CHECK_CLOCK_420(pixel_rate); > + else > + PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); > } > > /* BDW+ don't expose a synchronous way to read the state */ > @@ -12009,7 +12053,11 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) > PIPE_CONF_CHECK_I(pipe_bpp); > > - PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); > + /* YCBCR 420 pixel clock is half of the actual mode */ > + if (current_config->ycbcr420) > + PIPE_CONF_CHECK_CLOCK_420(base.adjusted_mode.crtc_clock); > + else > + PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); > PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock); > > #undef PIPE_CONF_CHECK_X > @@ -12018,6 +12066,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > #undef PIPE_CONF_CHECK_FLAGS > #undef PIPE_CONF_CHECK_CLOCK_FUZZY > #undef PIPE_CONF_QUIRK > +#undef PIPE_CONF_CHECK_CLOCK_420 We don't need to adjust things during compare. The dotclock is 2x the port clock in case of YUV420, so all what's needed is if (pipe_config->ycbcr420) dotclock = pipe_config->port_clock * 2; in ddi_get_clock() as Ville said. Also, right now if I boot with these patches all 4k YUV420 modes will be pruned on my monitor. This is caused by the monitor reporting a 300MHz max tmds clock (which is the port clock limit) and the driver comparing this against the dotclock which is double of this. So I needed if (drm_mode_is_420_only(&connector->display_info, mode)) clock /= 2; in intel_hdmi_mode_valid() to preserve these modes. Not sure why this didn't come up earlier. With that I see the YUV420 modes, but the initial modeset for the fb console will result in the gray screen and some jitter. A following modeset with the same mode will fix things; this is I think what Ville also saw. Still trying to find the reason for this. --Imre > > return ret; > } > -- > 2.7.4 >
Regards Shashank On 7/18/2017 11:42 PM, Imre Deak wrote: > On Mon, Jul 17, 2017 at 08:06:24PM +0530, Shashank Sharma wrote: >> To get HDMI YCBCR420 output, the PIPEMISC register should be >> programmed to: >> - Generate YCBCR output (bit 11) >> - In case of YCBCR420 outputs, it should be programmed in full >> blend mode to use the scaler in 5x3 ratio (bits 26 and 27) >> >> This patch: >> - Adds definition of these bits. >> - Programs PIPEMISC for YCBCR420 outputs. >> - Adds readouts to compare HW and SW states. >> >> V2: rebase >> V3: rebase >> V4: rebase >> V5: added r-b from Ander >> V6: Handle only YCBCR420 outputs (ville) >> V7: rebase >> V8: Addressed review comments from Ville >> - Add readouts for state->ycbcr420 and 420 pixel_clock. >> - Handle warning due to mismatch in clock for ycbcr420 clock. >> - Rename PIPEMISC macros to match the Bspec. >> - Add a debug print stating if YCBCR 4:2:0 output enabled. >> Added r-b from Ville >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> >> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> >> Reviewed-by: Ville Syrjala <ville.syrjala@linux.intel.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 3 ++ >> drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++-- >> 2 files changed, 55 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index c712d01..6dfcdd3 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -5227,6 +5227,9 @@ enum { >> >> #define _PIPE_MISC_A 0x70030 >> #define _PIPE_MISC_B 0x71030 >> +#define PIPEMISC_YUV420_ENABLE (1<<27) >> +#define PIPEMISC_YUV420_MODE_BLEND (1<<26) >> +#define PIPEMISC_OUTPUT_YUV (1<<11) > Missing the rename here requested by Ville. Ah, I thought it was more strict on changing YCBCR->YUV, will get this done. >> #define PIPEMISC_DITHER_BPC_MASK (7<<5) >> #define PIPEMISC_DITHER_8_BPC (0<<5) >> #define PIPEMISC_DITHER_10_BPC (1<<5) >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index d78f1c2..788502a 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6216,7 +6216,6 @@ static uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config) >> * We only use IF-ID interlacing. If we ever use >> * PF-ID we'll need to adjust the pixel_rate here. >> */ >> - > Extra w/s change. Got it. > >> if (pipe_config->pch_pfit.enabled) { >> uint64_t pipe_w, pipe_h, pfit_w, pfit_h; >> uint32_t pfit_size = pipe_config->pch_pfit.size; >> @@ -8076,6 +8075,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc->dev); >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> + struct intel_crtc_state *config = intel_crtc->config; >> >> if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { >> u32 val = 0; >> @@ -8101,6 +8101,12 @@ 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_YUV | >> + PIPEMISC_YUV420_ENABLE | >> + PIPEMISC_YUV420_MODE_BLEND; >> + } >> + >> I915_WRITE(PIPEMISC(intel_crtc->pipe), val); >> } >> } >> @@ -9170,6 +9176,10 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, >> pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); >> } >> >> + if (IS_GEMINILAKE(dev_priv)) >> + pipe_config->ycbcr420 = I915_READ(PIPEMISC(crtc->pipe)) & >> + PIPEMISC_YUV420_ENABLE; >> + >> power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe); >> if (intel_display_power_get_if_enabled(dev_priv, power_domain)) { >> power_domain_mask |= BIT_ULL(power_domain); >> @@ -11377,6 +11387,9 @@ 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); >> @@ -11704,6 +11717,22 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) >> } >> } >> >> +static bool intel_420_clock_check(int clock1, int clock2) >> +{ >> + int diff; >> + >> + if (clock1 == clock2 * 2) >> + return true; >> + >> + clock2 *= 2; >> + diff = abs(clock1 - clock2); >> + >> + if (((((diff + clock1 + clock2) * 100)) / (clock1 + clock2)) < 105) >> + return true; >> + >> + return false; >> +} >> + >> static bool intel_fuzzy_clock_check(int clock1, int clock2) >> { >> int diff; >> @@ -11832,6 +11861,18 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, >> ret = false; \ >> } >> >> +#define PIPE_CONF_CHECK_CLOCK_420(name) \ >> + do { \ >> + if (!intel_420_clock_check(current_config->name, \ >> + pipe_config->name)) { \ >> + pipe_config_err(adjust, __stringify(name), \ >> + "(expected %i, found %i)\n", \ >> + current_config->name, \ >> + pipe_config->name); \ >> + ret = false; \ >> + } \ >> + } while (0) >> + >> #define PIPE_CONF_CHECK_M_N(name) \ >> if (!intel_compare_link_m_n(¤t_config->name, \ >> &pipe_config->name,\ >> @@ -11983,7 +12024,10 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, >> } >> >> PIPE_CONF_CHECK_I(scaler_state.scaler_id); >> - PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); >> + if (current_config->ycbcr420) >> + PIPE_CONF_CHECK_CLOCK_420(pixel_rate); >> + else >> + PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); >> } >> >> /* BDW+ don't expose a synchronous way to read the state */ >> @@ -12009,7 +12053,11 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, >> if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) >> PIPE_CONF_CHECK_I(pipe_bpp); >> >> - PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); >> + /* YCBCR 420 pixel clock is half of the actual mode */ >> + if (current_config->ycbcr420) >> + PIPE_CONF_CHECK_CLOCK_420(base.adjusted_mode.crtc_clock); >> + else >> + PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); >> PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock); >> >> #undef PIPE_CONF_CHECK_X >> @@ -12018,6 +12066,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, >> #undef PIPE_CONF_CHECK_FLAGS >> #undef PIPE_CONF_CHECK_CLOCK_FUZZY >> #undef PIPE_CONF_QUIRK >> +#undef PIPE_CONF_CHECK_CLOCK_420 > We don't need to adjust things during compare. The dotclock is 2x > the port clock in case of YUV420, so all what's needed is > > if (pipe_config->ycbcr420) > dotclock = pipe_config->port_clock * 2; > > in ddi_get_clock() as Ville said. Got it, > Also, right now if I boot with these patches all 4k YUV420 modes will be > pruned on my monitor. This is caused by the monitor reporting a 300MHz > max tmds clock (which is the port clock limit) and the driver comparing > this against the dotclock which is double of this. So I needed > > if (drm_mode_is_420_only(&connector->display_info, mode)) > clock /= 2; > > in intel_hdmi_mode_valid() to preserve these modes. Not sure why this > didn't come up earlier. Because, when a monitor declares max clock on 300Mhz, it doesn't contain a mode in its EDID which needs clock > 300Mhz, but YCBCR_420 introduces a corner case here, its possible if all the modes with clock > 300Mhz are YCBCR420_only modes. Looks like you got hold of one such monitor, which is HDMI 2.0 but has all its 4k@60 modes as YCBCR420_only, and hence they fit into 300Mhz limit. In any case, its not illegal, and should have been taken care of in code, so thanks for pointing this out. Can you please share the monitor details ? I will see if I have one here in Bangalore. > > With that I see the YUV420 modes, but the initial modeset for the fb > console will result in the gray screen and some jitter. A following > modeset with the same mode will fix things; this is I think what Ville > also saw. Still trying to find the reason for this. Can you also please check if this is specific to one particular monitor ? I still can't reproduce this issue with my GLK + ACER/SAMSUNG monitors. - Shashank > > --Imre > >> >> return ret; >> } >> -- >> 2.7.4 >>
On Wed, Jul 19, 2017 at 12:46:37AM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 7/18/2017 11:42 PM, Imre Deak wrote: > > On Mon, Jul 17, 2017 at 08:06:24PM +0530, Shashank Sharma wrote: > > > To get HDMI YCBCR420 output, the PIPEMISC register should be > > > programmed to: > > > - Generate YCBCR output (bit 11) > > > - In case of YCBCR420 outputs, it should be programmed in full > > > blend mode to use the scaler in 5x3 ratio (bits 26 and 27) > > > > > > This patch: > > > - Adds definition of these bits. > > > - Programs PIPEMISC for YCBCR420 outputs. > > > - Adds readouts to compare HW and SW states. > > > > > > V2: rebase > > > V3: rebase > > > V4: rebase > > > V5: added r-b from Ander > > > V6: Handle only YCBCR420 outputs (ville) > > > V7: rebase > > > V8: Addressed review comments from Ville > > > - Add readouts for state->ycbcr420 and 420 pixel_clock. > > > - Handle warning due to mismatch in clock for ycbcr420 clock. > > > - Rename PIPEMISC macros to match the Bspec. > > > - Add a debug print stating if YCBCR 4:2:0 output enabled. > > > Added r-b from Ville > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> > > > Reviewed-by: Ville Syrjala <ville.syrjala@linux.intel.com> > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 3 ++ > > > drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++-- > > > 2 files changed, 55 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index c712d01..6dfcdd3 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -5227,6 +5227,9 @@ enum { > > > #define _PIPE_MISC_A 0x70030 > > > #define _PIPE_MISC_B 0x71030 > > > +#define PIPEMISC_YUV420_ENABLE (1<<27) > > > +#define PIPEMISC_YUV420_MODE_BLEND (1<<26) > > > +#define PIPEMISC_OUTPUT_YUV (1<<11) > > Missing the rename here requested by Ville. > Ah, I thought it was more strict on changing YCBCR->YUV, will get this done. > > > #define PIPEMISC_DITHER_BPC_MASK (7<<5) > > > #define PIPEMISC_DITHER_8_BPC (0<<5) > > > #define PIPEMISC_DITHER_10_BPC (1<<5) > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index d78f1c2..788502a 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -6216,7 +6216,6 @@ static uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config) > > > * We only use IF-ID interlacing. If we ever use > > > * PF-ID we'll need to adjust the pixel_rate here. > > > */ > > > - > > Extra w/s change. > Got it. > > > > > if (pipe_config->pch_pfit.enabled) { > > > uint64_t pipe_w, pipe_h, pfit_w, pfit_h; > > > uint32_t pfit_size = pipe_config->pch_pfit.size; > > > @@ -8076,6 +8075,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) > > > { > > > struct drm_i915_private *dev_priv = to_i915(crtc->dev); > > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + struct intel_crtc_state *config = intel_crtc->config; > > > if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { > > > u32 val = 0; > > > @@ -8101,6 +8101,12 @@ 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_YUV | > > > + PIPEMISC_YUV420_ENABLE | > > > + PIPEMISC_YUV420_MODE_BLEND; > > > + } > > > + > > > I915_WRITE(PIPEMISC(intel_crtc->pipe), val); > > > } > > > } > > > @@ -9170,6 +9176,10 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > > > pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); > > > } > > > + if (IS_GEMINILAKE(dev_priv)) > > > + pipe_config->ycbcr420 = I915_READ(PIPEMISC(crtc->pipe)) & > > > + PIPEMISC_YUV420_ENABLE; > > > + > > > power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe); > > > if (intel_display_power_get_if_enabled(dev_priv, power_domain)) { > > > power_domain_mask |= BIT_ULL(power_domain); > > > @@ -11377,6 +11387,9 @@ 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); > > > @@ -11704,6 +11717,22 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) > > > } > > > } > > > +static bool intel_420_clock_check(int clock1, int clock2) > > > +{ > > > + int diff; > > > + > > > + if (clock1 == clock2 * 2) > > > + return true; > > > + > > > + clock2 *= 2; > > > + diff = abs(clock1 - clock2); > > > + > > > + if (((((diff + clock1 + clock2) * 100)) / (clock1 + clock2)) < 105) > > > + return true; > > > + > > > + return false; > > > +} > > > + > > > static bool intel_fuzzy_clock_check(int clock1, int clock2) > > > { > > > int diff; > > > @@ -11832,6 +11861,18 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > > > ret = false; \ > > > } > > > +#define PIPE_CONF_CHECK_CLOCK_420(name) \ > > > + do { \ > > > + if (!intel_420_clock_check(current_config->name, \ > > > + pipe_config->name)) { \ > > > + pipe_config_err(adjust, __stringify(name), \ > > > + "(expected %i, found %i)\n", \ > > > + current_config->name, \ > > > + pipe_config->name); \ > > > + ret = false; \ > > > + } \ > > > + } while (0) > > > + > > > #define PIPE_CONF_CHECK_M_N(name) \ > > > if (!intel_compare_link_m_n(¤t_config->name, \ > > > &pipe_config->name,\ > > > @@ -11983,7 +12024,10 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > > > } > > > PIPE_CONF_CHECK_I(scaler_state.scaler_id); > > > - PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); > > > + if (current_config->ycbcr420) > > > + PIPE_CONF_CHECK_CLOCK_420(pixel_rate); > > > + else > > > + PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); > > > } > > > /* BDW+ don't expose a synchronous way to read the state */ > > > @@ -12009,7 +12053,11 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > > > if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) > > > PIPE_CONF_CHECK_I(pipe_bpp); > > > - PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); > > > + /* YCBCR 420 pixel clock is half of the actual mode */ > > > + if (current_config->ycbcr420) > > > + PIPE_CONF_CHECK_CLOCK_420(base.adjusted_mode.crtc_clock); > > > + else > > > + PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); > > > PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock); > > > #undef PIPE_CONF_CHECK_X > > > @@ -12018,6 +12066,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > > > #undef PIPE_CONF_CHECK_FLAGS > > > #undef PIPE_CONF_CHECK_CLOCK_FUZZY > > > #undef PIPE_CONF_QUIRK > > > +#undef PIPE_CONF_CHECK_CLOCK_420 > > We don't need to adjust things during compare. The dotclock is 2x > > the port clock in case of YUV420, so all what's needed is > > > > if (pipe_config->ycbcr420) > > dotclock = pipe_config->port_clock * 2; > > > > in ddi_get_clock() as Ville said. > Got it, > > Also, right now if I boot with these patches all 4k YUV420 modes will be > > pruned on my monitor. This is caused by the monitor reporting a 300MHz > > max tmds clock (which is the port clock limit) and the driver comparing > > this against the dotclock which is double of this. So I needed > > > > if (drm_mode_is_420_only(&connector->display_info, mode)) > > clock /= 2; > > > > in intel_hdmi_mode_valid() to preserve these modes. Not sure why this > > didn't come up earlier. > Because, when a monitor declares max clock on 300Mhz, it doesn't contain a > mode in its EDID which needs clock > 300Mhz, but YCBCR_420 introduces a > corner case here, its possible if all the modes with clock > 300Mhz are > YCBCR420_only modes. Well, Ville was using the same montior. In any case looks like we just need the above adjustment in intel_hdmi_mode_valid() then. > Looks like you got hold of one such monitor, which is HDMI 2.0 but has all > its 4k@60 modes as YCBCR420_only, and hence they fit into 300Mhz limit. In > any case, its not illegal, and should have been taken care of in code, so > thanks for pointing this out. > Can you please share the monitor details ? I will see if I have one here in > Bangalore. It's a Samsung monitor with 4096x2160@60Hz and 3840x2400@60Hz modes. The dotclock for these is ~600MHz while the portclock is half of this. They are also YUV420 only modes, so yes, that is where the 300MHz limit comes from. > > > > With that I see the YUV420 modes, but the initial modeset for the fb > > console will result in the gray screen and some jitter. A following > > modeset with the same mode will fix things; this is I think what Ville > > also saw. Still trying to find the reason for this. > Can you also please check if this is specific to one particular monitor ? I > still can't reproduce this issue with my GLK + ACER/SAMSUNG monitors. Not sure if I have any other monitor with YUV420 formats, I will do some more debugging tomorrow. --Imre > - Shashank > > > > --Imre > > > > > return ret; > > > } > > > -- > > > 2.7.4 > > > >
On Mon, Jul 17, 2017 at 08:06:24PM +0530, Shashank Sharma wrote: > To get HDMI YCBCR420 output, the PIPEMISC register should be > programmed to: > - Generate YCBCR output (bit 11) > - In case of YCBCR420 outputs, it should be programmed in full > blend mode to use the scaler in 5x3 ratio (bits 26 and 27) > > This patch: > - Adds definition of these bits. > - Programs PIPEMISC for YCBCR420 outputs. > - Adds readouts to compare HW and SW states. > > V2: rebase > V3: rebase > V4: rebase > V5: added r-b from Ander > V6: Handle only YCBCR420 outputs (ville) > V7: rebase > V8: Addressed review comments from Ville > - Add readouts for state->ycbcr420 and 420 pixel_clock. > - Handle warning due to mismatch in clock for ycbcr420 clock. > - Rename PIPEMISC macros to match the Bspec. > - Add a debug print stating if YCBCR 4:2:0 output enabled. > Added r-b from Ville > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> > Reviewed-by: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 3 ++ > drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++-- > 2 files changed, 55 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c712d01..6dfcdd3 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -5227,6 +5227,9 @@ enum { > > #define _PIPE_MISC_A 0x70030 > #define _PIPE_MISC_B 0x71030 > +#define PIPEMISC_YUV420_ENABLE (1<<27) > +#define PIPEMISC_YUV420_MODE_BLEND (1<<26) > +#define PIPEMISC_OUTPUT_YUV (1<<11) > #define PIPEMISC_DITHER_BPC_MASK (7<<5) > #define PIPEMISC_DITHER_8_BPC (0<<5) > #define PIPEMISC_DITHER_10_BPC (1<<5) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index d78f1c2..788502a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6216,7 +6216,6 @@ static uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config) > * We only use IF-ID interlacing. If we ever use > * PF-ID we'll need to adjust the pixel_rate here. > */ > - > if (pipe_config->pch_pfit.enabled) { > uint64_t pipe_w, pipe_h, pfit_w, pfit_h; > uint32_t pfit_size = pipe_config->pch_pfit.size; > @@ -8076,6 +8075,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_crtc_state *config = intel_crtc->config; > > if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { > u32 val = 0; > @@ -8101,6 +8101,12 @@ 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_YUV | > + PIPEMISC_YUV420_ENABLE | > + PIPEMISC_YUV420_MODE_BLEND; > + } > + > I915_WRITE(PIPEMISC(intel_crtc->pipe), val); > } > } > @@ -9170,6 +9176,10 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); > } > > + if (IS_GEMINILAKE(dev_priv)) > + pipe_config->ycbcr420 = I915_READ(PIPEMISC(crtc->pipe)) & > + PIPEMISC_YUV420_ENABLE; > + Besides my previous comments on this patch: I don't see any problem of handling the other platforms too as Ville suggested, even if only YUV420 is supported atm and even if only on GLK. It gives fastboot a chance to work on those platforms too and additional debug info if they happen to boot with a YUV420 mode. Also would be good to print at least a note for modes that we don't support or are invalid: if (IS_BDW || GEN >= 9) { tmp = READ(PIPEMISC); if (IS_GLK || GEN >= 10) { crtc_state->ycbcr420 = tmp & YUV420_ENABLE; if (crtc_state->ycbcr420 != !!(tmp & OUTPUT_COLORSPACE_YUV) || crtc_state->ycbcr420 != !!(tmp & YUV420_MODE_FULL_BLEND)) DRM_DEBUG_KMS("Unsupported/invalid YCbCr 420 mode (%08x)\n", tmp); } else if (tmp & OUTPUT_COLORSPACE_YUV) { DRM_DEBUG_KMS("Unsupported YCbCr mode\n"); } } > power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe); > if (intel_display_power_get_if_enabled(dev_priv, power_domain)) { > power_domain_mask |= BIT_ULL(power_domain); > @@ -11377,6 +11387,9 @@ 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); > @@ -11704,6 +11717,22 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) > } > } > > +static bool intel_420_clock_check(int clock1, int clock2) > +{ > + int diff; > + > + if (clock1 == clock2 * 2) > + return true; > + > + clock2 *= 2; > + diff = abs(clock1 - clock2); > + > + if (((((diff + clock1 + clock2) * 100)) / (clock1 + clock2)) < 105) > + return true; > + > + return false; > +} > + > static bool intel_fuzzy_clock_check(int clock1, int clock2) > { > int diff; > @@ -11832,6 +11861,18 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > ret = false; \ > } > > +#define PIPE_CONF_CHECK_CLOCK_420(name) \ > + do { \ > + if (!intel_420_clock_check(current_config->name, \ > + pipe_config->name)) { \ > + pipe_config_err(adjust, __stringify(name), \ > + "(expected %i, found %i)\n", \ > + current_config->name, \ > + pipe_config->name); \ > + ret = false; \ > + } \ > + } while (0) > + > #define PIPE_CONF_CHECK_M_N(name) \ > if (!intel_compare_link_m_n(¤t_config->name, \ > &pipe_config->name,\ > @@ -11983,7 +12024,10 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > } > > PIPE_CONF_CHECK_I(scaler_state.scaler_id); > - PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); > + if (current_config->ycbcr420) > + PIPE_CONF_CHECK_CLOCK_420(pixel_rate); > + else > + PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); > } > > /* BDW+ don't expose a synchronous way to read the state */ > @@ -12009,7 +12053,11 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) > PIPE_CONF_CHECK_I(pipe_bpp); > > - PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); > + /* YCBCR 420 pixel clock is half of the actual mode */ > + if (current_config->ycbcr420) > + PIPE_CONF_CHECK_CLOCK_420(base.adjusted_mode.crtc_clock); > + else > + PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); > PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock); > > #undef PIPE_CONF_CHECK_X > @@ -12018,6 +12066,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, > #undef PIPE_CONF_CHECK_FLAGS > #undef PIPE_CONF_CHECK_CLOCK_FUZZY > #undef PIPE_CONF_QUIRK > +#undef PIPE_CONF_CHECK_CLOCK_420 > > return ret; > } > -- > 2.7.4 >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c712d01..6dfcdd3 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5227,6 +5227,9 @@ enum { #define _PIPE_MISC_A 0x70030 #define _PIPE_MISC_B 0x71030 +#define PIPEMISC_YUV420_ENABLE (1<<27) +#define PIPEMISC_YUV420_MODE_BLEND (1<<26) +#define PIPEMISC_OUTPUT_YUV (1<<11) #define PIPEMISC_DITHER_BPC_MASK (7<<5) #define PIPEMISC_DITHER_8_BPC (0<<5) #define PIPEMISC_DITHER_10_BPC (1<<5) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d78f1c2..788502a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6216,7 +6216,6 @@ static uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config) * We only use IF-ID interlacing. If we ever use * PF-ID we'll need to adjust the pixel_rate here. */ - if (pipe_config->pch_pfit.enabled) { uint64_t pipe_w, pipe_h, pfit_w, pfit_h; uint32_t pfit_size = pipe_config->pch_pfit.size; @@ -8076,6 +8075,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->dev); struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_crtc_state *config = intel_crtc->config; if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { u32 val = 0; @@ -8101,6 +8101,12 @@ 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_YUV | + PIPEMISC_YUV420_ENABLE | + PIPEMISC_YUV420_MODE_BLEND; + } + I915_WRITE(PIPEMISC(intel_crtc->pipe), val); } } @@ -9170,6 +9176,10 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); } + if (IS_GEMINILAKE(dev_priv)) + pipe_config->ycbcr420 = I915_READ(PIPEMISC(crtc->pipe)) & + PIPEMISC_YUV420_ENABLE; + power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe); if (intel_display_power_get_if_enabled(dev_priv, power_domain)) { power_domain_mask |= BIT_ULL(power_domain); @@ -11377,6 +11387,9 @@ 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); @@ -11704,6 +11717,22 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) } } +static bool intel_420_clock_check(int clock1, int clock2) +{ + int diff; + + if (clock1 == clock2 * 2) + return true; + + clock2 *= 2; + diff = abs(clock1 - clock2); + + if (((((diff + clock1 + clock2) * 100)) / (clock1 + clock2)) < 105) + return true; + + return false; +} + static bool intel_fuzzy_clock_check(int clock1, int clock2) { int diff; @@ -11832,6 +11861,18 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, ret = false; \ } +#define PIPE_CONF_CHECK_CLOCK_420(name) \ + do { \ + if (!intel_420_clock_check(current_config->name, \ + pipe_config->name)) { \ + pipe_config_err(adjust, __stringify(name), \ + "(expected %i, found %i)\n", \ + current_config->name, \ + pipe_config->name); \ + ret = false; \ + } \ + } while (0) + #define PIPE_CONF_CHECK_M_N(name) \ if (!intel_compare_link_m_n(¤t_config->name, \ &pipe_config->name,\ @@ -11983,7 +12024,10 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, } PIPE_CONF_CHECK_I(scaler_state.scaler_id); - PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); + if (current_config->ycbcr420) + PIPE_CONF_CHECK_CLOCK_420(pixel_rate); + else + PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); } /* BDW+ don't expose a synchronous way to read the state */ @@ -12009,7 +12053,11 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) PIPE_CONF_CHECK_I(pipe_bpp); - PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); + /* YCBCR 420 pixel clock is half of the actual mode */ + if (current_config->ycbcr420) + PIPE_CONF_CHECK_CLOCK_420(base.adjusted_mode.crtc_clock); + else + PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock); #undef PIPE_CONF_CHECK_X @@ -12018,6 +12066,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, #undef PIPE_CONF_CHECK_FLAGS #undef PIPE_CONF_CHECK_CLOCK_FUZZY #undef PIPE_CONF_QUIRK +#undef PIPE_CONF_CHECK_CLOCK_420 return ret; }
To get HDMI YCBCR420 output, the PIPEMISC register should be programmed to: - Generate YCBCR output (bit 11) - In case of YCBCR420 outputs, it should be programmed in full blend mode to use the scaler in 5x3 ratio (bits 26 and 27) This patch: - Adds definition of these bits. - Programs PIPEMISC for YCBCR420 outputs. - Adds readouts to compare HW and SW states. V2: rebase V3: rebase V4: rebase V5: added r-b from Ander V6: Handle only YCBCR420 outputs (ville) V7: rebase V8: Addressed review comments from Ville - Add readouts for state->ycbcr420 and 420 pixel_clock. - Handle warning due to mismatch in clock for ycbcr420 clock. - Rename PIPEMISC macros to match the Bspec. - Add a debug print stating if YCBCR 4:2:0 output enabled. Added r-b from Ville Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> Reviewed-by: Ville Syrjala <ville.syrjala@linux.intel.com> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 3 ++ drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 3 deletions(-)