[Intel-gfx] [PATCH v4 3/6] drm/i915: prepare pipe for YCBCR420 output
Sharma, Shashank
shashank.sharma at intel.com
Tue Jul 18 19:16:37 UTC 2017
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 at linux.intel.com>
>> Cc: Ander Conselvan de Oliveira <conselvan2 at gmail.com>
>> Cc: Daniel Vetter <daniel.vetter at intel.com>
>>
>> Reviewed-by: Ander Conselvan de Oliveira <conselvan2 at gmail.com>
>> Reviewed-by: Ville Syrjala <ville.syrjala at linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma at 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 at 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
>>
More information about the Intel-gfx
mailing list