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=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 AE02CC43603 for ; Fri, 13 Dec 2019 00:52:23 +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 86D8F206E2 for ; Fri, 13 Dec 2019 00:52:23 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 86D8F206E2 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 1C33A6E250; Fri, 13 Dec 2019 00:52:23 +0000 (UTC) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by gabe.freedesktop.org (Postfix) with ESMTPS id D0B5D6E250 for ; Fri, 13 Dec 2019 00:52:21 +0000 (UTC) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Dec 2019 16:52:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,307,1571727600"; d="scan'208";a="208289189" Received: from labuser-z97x-ud5h.jf.intel.com (HELO intel.com) ([10.54.75.49]) by orsmga008.jf.intel.com with ESMTP; 12 Dec 2019 16:52:21 -0800 Date: Thu, 12 Dec 2019 16:53:43 -0800 From: Manasi Navare To: Jani Nikula Message-ID: <20191213005342.GD24342@intel.com> References: <01bcddcdf397b1c8eb859ed18ebe023fb64383d9.1576081155.git.jani.nikula@intel.com> <20191211220837.GE12192@intel.com> <87h8262eqv.fsf@intel.com> <20191212220510.GC24342@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20191212220510.GC24342@intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) 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 Thu, Dec 12, 2019 at 02:05:10PM -0800, Manasi Navare wrote: > On Thu, Dec 12, 2019 at 09:18:48AM +0200, Jani Nikula wrote: > > 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. > > Okay, it will work for Pipe A agreed but IMO still broken for Pipe D that also has DSC now. > Are you looking at fixing this for all pipes? > > Manasi Actually I just did the register offsets calculations for DSC0_PPS and DSC1_PPS registers on TGL: And yes it does still work to give correct offsets for Pipe A-D but at some point we should rewrite it to look less of magic Manasi > > > > > > > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx