From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Souza, Jose" Subject: Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic Date: Fri, 1 Mar 2019 18:29:15 +0000 Message-ID: References: <20190228013259.30026-1-jose.souza@intel.com> <20190228013259.30026-4-jose.souza@intel.com> <20190228165648.GM20097@intel.com> <7cf45d6fa43e67e3a81c396b199a74b512e3efb8.camel@intel.com> <20190301133550.GP20097@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0407201522==" Return-path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4BE9A6E31E for ; Fri, 1 Mar 2019 18:29:17 +0000 (UTC) In-Reply-To: <20190301133550.GP20097@intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "ville.syrjala@linux.intel.com" Cc: "intel-gfx@lists.freedesktop.org" , "Pandiyan, Dhinakaran" List-Id: intel-gfx@lists.freedesktop.org --===============0407201522== Content-Language: en-US Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-GFp1vM8TPguzL34ZVrrM" --=-GFp1vM8TPguzL34ZVrrM Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2019-03-01 at 15:35 +0200, Ville Syrj=C3=A4l=C3=A4 wrote: > On Thu, Feb 28, 2019 at 11:26:57PM +0000, Souza, Jose wrote: > > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrj=C3=A4l=C3=A4 wrote: > > > On Wed, Feb 27, 2019 at 05:32:57PM -0800, Jos=C3=A9 Roberto de Souza > > > wrote: > > > > Other features like PSR2 also needs to be disabled while > > > > getting > > > > CRC > > > > so lets rename ips_force_disable to crc_enabled, drop all this > > > > checks > > > > for pipe A and HSW and BDW and make it generic and > > > > hsw_compute_ips_config() will take care of all the checks > > > > removed > > > > from here. > > > >=20 > > > > Cc: Dhinakaran Pandiyan > > > > Cc: Ville Syrj=C3=A4l=C3=A4 > > > > Signed-off-by: Jos=C3=A9 Roberto de Souza > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 10 +++++-- > > > > drivers/gpu/drm/i915/intel_drv.h | 3 +- > > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++-------- > > > > ---- > > > > ------ > > > > 3 files changed, 24 insertions(+), 31 deletions(-) > > > >=20 > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index 816e8f124b3b..328967c642b3 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -6751,7 +6751,13 @@ static bool > > > > hsw_compute_ips_config(struct > > > > intel_crtc_state *crtc_state) > > > > if (!hsw_crtc_state_ips_capable(crtc_state)) > > > > return false; > > > > =20 > > > > - if (crtc_state->ips_force_disable) > > > > + /* > > > > + * When IPS gets enabled, the pipe CRC changes. Since > > > > IPS gets > > > > + * enabled and disabled dynamically based on package C > > > > states, > > > > + * user space can't make reliable use of the CRCs, so > > > > let's > > > > just > > > > + * completely disable it. > > > > + */ > > > > + if (crtc_state->crc_enabled) > > > > return false; > > >=20 > > > Hmm. I was wondering how we even manage to pass the state checker > > > with > > > the current code. But apparently we don't have state checking for > > > IPS. > > > I would suggest moving this into hsw_compute_ips_config() and > > > then > > > adding the state checker (for HSW only though since BDW can't do > > > the > > > readout). > > >=20 > > > > =20 > > > > /* IPS should be fine as long as at least one plane is > > > > enabled. > > > > */ > > > > @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct > > > > intel_crtc_state *crtc_state) > > > > saved_state->shared_dpll =3D crtc_state->shared_dpll; > > > > saved_state->dpll_hw_state =3D crtc_state->dpll_hw_state; > > > > saved_state->pch_pfit.force_thru =3D crtc_state- > > > > > pch_pfit.force_thru; > > > > - saved_state->ips_force_disable =3D crtc_state- > > > > >ips_force_disable; > > > > + saved_state->crc_enabled =3D crtc_state->crc_enabled; > > > > if (IS_G4X(dev_priv) || > > > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > saved_state->wm =3D crtc_state->wm; > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index 5412373e2f98..2be64529e4a2 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -999,7 +999,8 @@ struct intel_crtc_state { > > > > struct intel_link_m_n fdi_m_n; > > > > =20 > > > > bool ips_enabled; > > > > - bool ips_force_disable; > > > > + > > > > + bool crc_enabled; > > > > =20 > > > > bool enable_fbc; > > > > =20 > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > index 53d4ec68d3c4..f6d0b2aaffe2 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > @@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum > > > > intel_pipe_crc_source *source, > > > > return 0; > > > > } > > > > =20 > > > > -static void hsw_pipe_A_crc_wa(struct drm_i915_private > > > > *dev_priv, > > > > - bool enable) > > > > +static void > > > > +intel_crtc_crc_prepare(struct drm_i915_private *dev_priv, > > > > struct > > > > drm_crtc *crtc, > > >=20 > > > Just pass in the intel_crtc > >=20 > > Okay > >=20 > > > > + bool enable) > > > > { > > > > struct drm_device *dev =3D &dev_priv->drm; > > > > - struct intel_crtc *crtc =3D > > > > intel_get_crtc_for_pipe(dev_priv, > > > > PIPE_A); > > > > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > > >=20 > > > and then we don't have to have an ugly name for this. > > >=20 > > > Also pasing in dev_priv is redundant when you're already passing > > > in > > > the > > > crtc. > > >=20 > >=20 > > okay > >=20 > > > The function name isn't super descriptive. It makes me think > > > we're > > > preparing for CRC capture, when in fact it just adds/removes the > > > w/as. > >=20 > > What about: intel_crtc_crc_workarounds_setup()? >=20 > Or _setup_workarounds() so that it reads more naturally? >=20 > > > > struct intel_crtc_state *pipe_config; > > > > struct drm_atomic_state *state; > > > > struct drm_modeset_acquire_ctx ctx; > > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct > > > > drm_i915_private *dev_priv, > > > > state->acquire_ctx =3D &ctx; > > > > =20 > > > > retry: > > > > - pipe_config =3D intel_atomic_get_crtc_state(state, crtc); > > > > + pipe_config =3D intel_atomic_get_crtc_state(state, > > > > intel_crtc); > > > > if (IS_ERR(pipe_config)) { > > > > ret =3D PTR_ERR(pipe_config); > > > > goto put_state; > > > > } > > > > =20 > > > > - if (HAS_IPS(dev_priv)) { > > > > - /* > > > > - * When IPS gets enabled, the pipe CRC changes. > > > > Since > > > > IPS gets > > > > - * enabled and disabled dynamically based on > > > > package C > > > > states, > > > > - * user space can't make reliable use of the > > > > CRCs, so > > > > let's just > > > > - * completely disable it. > > > > - */ > > > > - pipe_config->ips_force_disable =3D enable; > > > > - } > > > > + pipe_config->crc_enabled =3D enable; > > > > =20 > > > > - if (IS_HASWELL(dev_priv)) { > > > > + if (IS_HASWELL(dev_priv) && intel_crtc->pipe =3D=3D PIPE_A) > > > > { > > > > pipe_config->pch_pfit.force_thru =3D enable; > > > > if (pipe_config->cpu_transcoder =3D=3D > > > > TRANSCODER_EDP && > > > > pipe_config->pch_pfit.enabled !=3D enable) > > > > @@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct > > > > drm_i915_private *dev_priv, > > > > static int ivb_pipe_crc_ctl_reg(struct drm_i915_private > > > > *dev_priv, > > > > enum pipe pipe, > > > > enum intel_pipe_crc_source > > > > *source, > > > > - u32 *val, > > > > - bool set_wa) > > > > + u32 *val) > > > > { > > > > if (*source =3D=3D INTEL_PIPE_CRC_SOURCE_AUTO) > > > > *source =3D INTEL_PIPE_CRC_SOURCE_PIPE; > > > > @@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct > > > > drm_i915_private *dev_priv, > > > > *val =3D PIPE_CRC_ENABLE | > > > > PIPE_CRC_SOURCE_SPRITE_IVB; > > > > break; > > > > case INTEL_PIPE_CRC_SOURCE_PIPE: > > > > - if (set_wa && (IS_HASWELL(dev_priv) || > > > > - IS_BROADWELL(dev_priv)) && pipe =3D=3D PIPE_A) > > > > - hsw_pipe_A_crc_wa(dev_priv, true); > > > > - > > > > *val =3D PIPE_CRC_ENABLE | > > > > PIPE_CRC_SOURCE_PF_IVB; > > > > break; > > > > case INTEL_PIPE_CRC_SOURCE_NONE: > > > > @@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct > > > > drm_i915_private *dev_priv, > > > > =20 > > > > static int get_new_crc_ctl_reg(struct drm_i915_private > > > > *dev_priv, > > > > enum pipe pipe, > > > > - enum intel_pipe_crc_source > > > > *source, u32 > > > > *val, > > > > - bool set_wa) > > > > + enum intel_pipe_crc_source > > > > *source, u32 > > > > *val) > > > > { > > > > if (IS_GEN(dev_priv, 2)) > > > > return i8xx_pipe_crc_ctl_reg(source, val); > > > > @@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct > > > > drm_i915_private *dev_priv, > > > > else if (IS_GEN_RANGE(dev_priv, 5, 6)) > > > > return ilk_pipe_crc_ctl_reg(source, val); > > > > else if (INTEL_GEN(dev_priv) < 9) > > > > - return ivb_pipe_crc_ctl_reg(dev_priv, pipe, > > > > source, > > > > val, set_wa); > > > > + return ivb_pipe_crc_ctl_reg(dev_priv, pipe, > > > > source, > > > > val); > > > > else > > > > return skl_pipe_crc_ctl_reg(dev_priv, pipe, > > > > source, > > > > val); > > > > } > > > > @@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct > > > > drm_crtc > > > > *crtc, const char *source_name) > > > > return -EIO; > > > > } > > > > =20 > > > > - ret =3D get_new_crc_ctl_reg(dev_priv, crtc->index, > > > > &source, &val, > > > > true); > > > > + intel_crtc_crc_prepare(dev_priv, crtc, source !=3D > > > > INTEL_PIPE_CRC_SOURCE_NONE); > > > > + > > >=20 > > > Aren't we potentially corrupting the last CRC(s) by turning off > > > the > > > w/as > > > before disbling CRC capture? > >=20 > > It will only be NULL when uses closes the file descriptor to CRC > > data, > > so even if its corrupted user will never get those CRCs. >=20 > Userspace may not get it but the tracepoint will still see it. Which > may > cause confusion. Okay changing to: if (source !=3D INTEL_PIPE_CRC_SOURCE_NONE) intel_crtc_crc_setup_workarounds() ret =3D get_new_crc_ctl_reg() ... if (source =3D=3D INTEL_PIPE_CRC_SOURCE_NONE) intel_crtc_crc_setup_workarounds() >=20 > > > > + ret =3D get_new_crc_ctl_reg(dev_priv, crtc->index, > > > > &source, > > > > &val); > > > > if (ret !=3D 0) > > > > goto out; > > > > =20 > > > > @@ -629,9 +618,6 @@ int intel_crtc_set_crc_source(struct > > > > drm_crtc > > > > *crtc, const char *source_name) > > > > if (!source) { > > > > if (IS_VALLEYVIEW(dev_priv) || > > > > IS_CHERRYVIEW(dev_priv)) > > > > vlv_undo_pipe_scramble_reset(dev_priv, > > > > crtc- > > > > > index); > > > > - else if ((IS_HASWELL(dev_priv) || > > > > - IS_BROADWELL(dev_priv)) && crtc- > > > > >index =3D=3D > > > > PIPE_A) > > > > - hsw_pipe_A_crc_wa(dev_priv, false); > > > > } > > > > =20 > > > > pipe_crc->skipped =3D 0; > > > > @@ -652,7 +638,7 @@ void intel_crtc_enable_pipe_crc(struct > > > > intel_crtc *intel_crtc) > > > > if (!crtc->crc.opened) > > > > return; > > > > =20 > > > > - if (get_new_crc_ctl_reg(dev_priv, crtc->index, > > > > &pipe_crc- > > > > > source, &val, false) < 0) > > > > + if (get_new_crc_ctl_reg(dev_priv, crtc->index, > > > > &pipe_crc- > > > > > source, &val) < 0) > > > > return; > > > > =20 > > > > /* Don't need pipe_crc->lock here, IRQs are not > > > > generated. */ > > > > --=20 > > > > 2.21.0 >=20 >=20 --=-GFp1vM8TPguzL34ZVrrM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEVNG051EijGa0MiaQVenbO/mOWkkFAlx5efkACgkQVenbO/mO WklvcQf8DfEAisxaiER1oLr8TkTw+dZGUID0T0a436cQ6Nlo3W8UI0eBNxJER1lC jIwjpep/7gAgOiUF2tbQCsubtZ8Zf6OOq/P9IgiNfEwr/s1URu8K5Kf5mzDCCmQn +gJwQSTo+emIGmNEHE85j4F2eErCggPkXgcAasdD9EoQnxCCBY0/nZKmxrmSTmG1 I0P2NfiGQWQRiwBfzHs4zwXwHrK5Z3JMQL0NcIlq8MdolGm+Q4pY7/hae2JFG3HO qY1FcVPGmPdkdMicx8fru7+bAkJxMF6gOUWRB9xDP4QZQbDkySeyshJLPhd5nR4w cfX+W3Qm2QET9fqgExt5+7ENrc8zjg== =YHVe -----END PGP SIGNATURE----- --=-GFp1vM8TPguzL34ZVrrM-- --===============0407201522== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4 --===============0407201522==--