From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, UNWANTED_LANGUAGE_BODY autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 236FCC2D0C8 for ; Thu, 12 Dec 2019 07:18:53 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 022DA2173E for ; Thu, 12 Dec 2019 07:18:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 022DA2173E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 981AC6EC63; Thu, 12 Dec 2019 07:18:52 +0000 (UTC) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 701C26EC63 for ; Thu, 12 Dec 2019 07:18:51 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Dec 2019 23:18:50 -0800 X-IronPort-AV: E=Sophos;i="5.69,305,1571727600"; d="scan'208";a="207976446" Received: from lenovo-x280.ger.corp.intel.com (HELO localhost) ([10.252.35.33]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Dec 2019 23:18:48 -0800 From: Jani Nikula To: Manasi Navare In-Reply-To: <20191211220837.GE12192@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <01bcddcdf397b1c8eb859ed18ebe023fb64383d9.1576081155.git.jani.nikula@intel.com> <20191211220837.GE12192@intel.com> Date: Thu, 12 Dec 2019 09:18:48 +0200 Message-ID: <87h8262eqv.fsf@intel.com> MIME-Version: 1.0 Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/dsc: fix DSC register selection for ICL DSI transcoders X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Wed, 11 Dec 2019, Manasi Navare wrote: > On Wed, Dec 11, 2019 at 06:23:46PM +0200, Jani Nikula wrote: >> ICL eDP and DSI transcoders have a DSC engine separate from the >> pipe. Abstract the register selection and fix it for ICL. >> >> Add a warning for pipe A DSC on ICL; it does not exist. >> >> Cc: Manasi Navare >> Cc: Vandita Kulkarni >> Signed-off-by: Jani Nikula >> --- >> drivers/gpu/drm/i915/display/intel_vdsc.c | 58 +++++++++++++++-------- >> 1 file changed, 38 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c >> index ed9048140937..e6f60be9ee84 100644 >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c >> @@ -353,6 +353,26 @@ bool intel_dsc_source_support(struct intel_encoder *encoder, >> return false; >> } >> >> +static bool is_pipe_dsc(const struct intel_crtc_state *crtc_state) >> +{ >> + const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> + const struct drm_i915_private *i915 = to_i915(crtc->base.dev); >> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; >> + >> + if (INTEL_GEN(i915) >= 12) >> + return true; >> + >> + if (cpu_transcoder == TRANSCODER_EDP || >> + cpu_transcoder == TRANSCODER_DSI_0 || >> + cpu_transcoder == TRANSCODER_DSI_1) >> + return false; >> + >> + /* There's no pipe A DSC engine on ICL */ >> + WARN_ON(crtc->pipe == PIPE_A); >> + >> + return true; >> +} >> + > > So for >=Gen12, it will always go to the else part in PPS configure and use > ICL_DSC0_PICTURE_PARAMETER_SET_1 register per pipe. FWIW, this was already the case for gen 12+, because there's no eDP transcoder. > Right now this only calculates register addresses for PIPE_B and PIPE_C for ICL but > no register defs for DSC_PPS for GEN >=12 > > As far as the selection logic this patch looks good and will work for <12 GEN , but it will > currently fail for DSC on >=12 > > Lucas, Jose - Is thsi something anyone's looking at adding? Oh, I failed to mention how subtle this is. It does work fine on all pipes on gen 12+. It's just that with pipe A, the index becomes (PIPE_A - PIPE_B) = -1, and it extrapolates the correct register offset, the other direction than usually. You see, gen 12 only has pipe DSC, with a DSC engine added to pipe A too, and no eDP/DSI specific DSC like ICL. Sure, the register definitions need an update, it's ugly and misleading, but it's not really broken for gen 12 pipe A. :) BR, Jani. > > Regards > Manasi > >> int intel_dsc_compute_params(struct intel_encoder *encoder, >> struct intel_crtc_state *pipe_config) >> { >> @@ -471,7 +491,6 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> const struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; >> enum pipe pipe = crtc->pipe; >> - enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; >> u32 pps_val = 0; >> u32 rc_buf_thresh_dword[4]; >> u32 rc_range_params_dword[8]; >> @@ -492,7 +511,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> if (vdsc_cfg->vbr_enable) >> pps_val |= DSC_VBR_ENABLE; >> DRM_INFO("PPS0 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_0, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -511,7 +530,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> pps_val = 0; >> pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); >> DRM_INFO("PPS1 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_1, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -531,7 +550,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> pps_val |= DSC_PIC_HEIGHT(vdsc_cfg->pic_height) | >> DSC_PIC_WIDTH(vdsc_cfg->pic_width / num_vdsc_instances); >> DRM_INFO("PPS2 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_2, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -551,7 +570,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> pps_val |= DSC_SLICE_HEIGHT(vdsc_cfg->slice_height) | >> DSC_SLICE_WIDTH(vdsc_cfg->slice_width); >> DRM_INFO("PPS3 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_3, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -571,7 +590,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> pps_val |= DSC_INITIAL_XMIT_DELAY(vdsc_cfg->initial_xmit_delay) | >> DSC_INITIAL_DEC_DELAY(vdsc_cfg->initial_dec_delay); >> DRM_INFO("PPS4 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_4, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -591,7 +610,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> pps_val |= DSC_SCALE_INC_INT(vdsc_cfg->scale_increment_interval) | >> DSC_SCALE_DEC_INT(vdsc_cfg->scale_decrement_interval); >> DRM_INFO("PPS5 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_5, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -613,7 +632,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> DSC_FLATNESS_MIN_QP(vdsc_cfg->flatness_min_qp) | >> DSC_FLATNESS_MAX_QP(vdsc_cfg->flatness_max_qp); >> DRM_INFO("PPS6 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_6, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -633,7 +652,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> pps_val |= DSC_SLICE_BPG_OFFSET(vdsc_cfg->slice_bpg_offset) | >> DSC_NFL_BPG_OFFSET(vdsc_cfg->nfl_bpg_offset); >> DRM_INFO("PPS7 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_7, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -653,7 +672,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> pps_val |= DSC_FINAL_OFFSET(vdsc_cfg->final_offset) | >> DSC_INITIAL_OFFSET(vdsc_cfg->initial_offset); >> DRM_INFO("PPS8 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_8, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -673,7 +692,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> pps_val |= DSC_RC_MODEL_SIZE(DSC_RC_MODEL_SIZE_CONST) | >> DSC_RC_EDGE_FACTOR(DSC_RC_EDGE_FACTOR_CONST); >> DRM_INFO("PPS9 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_9, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -695,7 +714,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> DSC_RC_TARGET_OFF_HIGH(DSC_RC_TGT_OFFSET_HI_CONST) | >> DSC_RC_TARGET_OFF_LOW(DSC_RC_TGT_OFFSET_LO_CONST); >> DRM_INFO("PPS10 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_10, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -718,7 +737,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> DSC_SLICE_ROW_PER_FRAME(vdsc_cfg->pic_height / >> vdsc_cfg->slice_height); >> DRM_INFO("PPS16 = 0x%08x\n", pps_val); >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_PICTURE_PARAMETER_SET_16, pps_val); >> /* >> * If 2 VDSC instances are needed, configure PPS for second >> @@ -742,7 +761,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> DRM_INFO(" RC_BUF_THRESH%d = 0x%08x\n", i, >> rc_buf_thresh_dword[i / 4]); >> } >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_RC_BUF_THRESH_0, rc_buf_thresh_dword[0]); >> I915_WRITE(DSCA_RC_BUF_THRESH_0_UDW, rc_buf_thresh_dword[1]); >> I915_WRITE(DSCA_RC_BUF_THRESH_1, rc_buf_thresh_dword[2]); >> @@ -791,7 +810,7 @@ static void intel_dsc_pps_configure(struct intel_encoder *encoder, >> DRM_INFO(" RC_RANGE_PARAM_%d = 0x%08x\n", i, >> rc_range_params_dword[i / 2]); >> } >> - if (cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> I915_WRITE(DSCA_RC_RANGE_PARAMETERS_0, >> rc_range_params_dword[0]); >> I915_WRITE(DSCA_RC_RANGE_PARAMETERS_0_UDW, >> @@ -870,7 +889,6 @@ void intel_dsc_get_config(struct intel_encoder *encoder, >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; >> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> - enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; >> enum pipe pipe = crtc->pipe; >> enum intel_display_power_domain power_domain; >> intel_wakeref_t wakeref; >> @@ -885,7 +903,7 @@ void intel_dsc_get_config(struct intel_encoder *encoder, >> if (!wakeref) >> return; >> >> - if (crtc_state->cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> dss_ctl1 = I915_READ(DSS_CTL1); >> dss_ctl2 = I915_READ(DSS_CTL2); >> } else { >> @@ -903,7 +921,7 @@ void intel_dsc_get_config(struct intel_encoder *encoder, >> /* FIXME: add more state readout as needed */ >> >> /* PPS1 */ >> - if (cpu_transcoder == TRANSCODER_EDP) >> + if (!is_pipe_dsc(crtc_state)) >> val = I915_READ(DSCA_PICTURE_PARAMETER_SET_1); >> else >> val = I915_READ(ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe)); >> @@ -975,7 +993,7 @@ void intel_dsc_enable(struct intel_encoder *encoder, >> else >> intel_dsc_dp_pps_write(encoder, crtc_state); >> >> - if (crtc_state->cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(crtc_state)) { >> dss_ctl1_reg = DSS_CTL1; >> dss_ctl2_reg = DSS_CTL2; >> } else { >> @@ -1002,7 +1020,7 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state) >> if (!old_crtc_state->dsc.compression_enable) >> return; >> >> - if (old_crtc_state->cpu_transcoder == TRANSCODER_EDP) { >> + if (!is_pipe_dsc(old_crtc_state)) { >> dss_ctl1_reg = DSS_CTL1; >> dss_ctl2_reg = DSS_CTL2; >> } else { >> -- >> 2.20.1 >> -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx