Re: [Intel-gfx] [PATCH v5 5/9] drm/i915/crc: Make IPS workaround generic
On Thu, 2019-03-07 at 12:18 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2019-03-05 at 22:47 -0800, José 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.
> >
> > v2: Renaming and parameter changes to the functions that prepares
> > the
> > commit (Ville)
> >
> > Cc: Dhinakaran Pandiyan
> > Cc: Ville Syrjälä
> > Reviewed-by: Rodrigo Vivi
> > Signed-off-by: José 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 | 47 +++
> >
> > 3 files changed, 29 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 9312b3f35eb0..b3a5d8462251 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;
> >
> > - 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;
> >
> > /* IPS should be fine as long as at least one plane is enabled.
> > */
> > @@ -11687,7 +11693,7 @@ clear_intel_crtc_state(struct
> > intel_crtc_state *crtc_state)
> > saved_state->shared_dpll = crtc_state->shared_dpll;
> > saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > saved_state->pch_pfit.force_thru = crtc_state-
> > > pch_pfit.force_thru;
> > - saved_state->ips_force_disable = crtc_state->ips_force_disable;
> > + saved_state->crc_enabled = crtc_state->crc_enabled;
> > if (IS_G4X(dev_priv) ||
> > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > saved_state->wm = 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;
> >
> > bool ips_enabled;
> > - bool ips_force_disable;
> > +
> > + bool crc_enabled;
> >
> > bool enable_fbc;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index 53d4ec68d3c4..af64597c5c6e 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum
> > intel_pipe_crc_source *source,
> > return 0;
> > }
> >
> > -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > - bool enable)
> > +static void
> > +intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool
> > enable)
> > {
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > struct drm_device *dev = _priv->drm;
> We could eliminate this local.
Done
>
> > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> > PIPE_A);
> > struct intel_crtc_state *pipe_config;
> > struct drm_atomic_state *state;
> > struct drm_modeset_acquire_ctx ctx;
> > - int ret = 0;
> > + int ret;
> >
> > drm_modeset_acquire_init(, 0);
> >
> > @@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct
> > drm_i915_private *dev_priv,
> > goto put_state;
> > }
> >
> > - 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 = enable;
> > - }
> > + pipe_config->crc_enabled = enable;
> >
> > - if (IS_HASWELL(dev_priv)) {
> > + if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
> > pipe_config->pch_pfit.force_thru = enable;
> > if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > pipe_config->pch_pfit.enabled != enable)
> > @@ -343,8 +335,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,
> >
Re: [Intel-gfx] [PATCH v5 5/9] drm/i915/crc: Make IPS workaround generic
On Tue, 2019-03-05 at 22:47 -0800, José 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.
>
> v2: Renaming and parameter changes to the functions that prepares the
> commit (Ville)
>
> Cc: Dhinakaran Pandiyan
> Cc: Ville Syrjälä
> Reviewed-by: Rodrigo Vivi
> Signed-off-by: José 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 | 47 +++
>
> 3 files changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 9312b3f35eb0..b3a5d8462251 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;
>
> - 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;
>
> /* IPS should be fine as long as at least one plane is enabled.
> */
> @@ -11687,7 +11693,7 @@ clear_intel_crtc_state(struct
> intel_crtc_state *crtc_state)
> saved_state->shared_dpll = crtc_state->shared_dpll;
> saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> saved_state->pch_pfit.force_thru = crtc_state-
> >pch_pfit.force_thru;
> - saved_state->ips_force_disable = crtc_state->ips_force_disable;
> + saved_state->crc_enabled = crtc_state->crc_enabled;
> if (IS_G4X(dev_priv) ||
> IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> saved_state->wm = 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;
>
> bool ips_enabled;
> - bool ips_force_disable;
> +
> + bool crc_enabled;
>
> bool enable_fbc;
>
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 53d4ec68d3c4..af64597c5c6e 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum
> intel_pipe_crc_source *source,
> return 0;
> }
>
> -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> - bool enable)
> +static void
> +intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool
> enable)
> {
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> struct drm_device *dev = _priv->drm;
We could eliminate this local.
> - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> PIPE_A);
> struct intel_crtc_state *pipe_config;
> struct drm_atomic_state *state;
> struct drm_modeset_acquire_ctx ctx;
> - int ret = 0;
> + int ret;
>
> drm_modeset_acquire_init(, 0);
>
> @@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct
> drm_i915_private *dev_priv,
> goto put_state;
> }
>
> - 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 = enable;
> - }
> + pipe_config->crc_enabled = enable;
>
> - if (IS_HASWELL(dev_priv)) {
> + if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
> pipe_config->pch_pfit.force_thru = enable;
> if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> pipe_config->pch_pfit.enabled != enable)
> @@ -343,8 +335,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 ==
[Intel-gfx] [PATCH v5 5/9] drm/i915/crc: Make IPS workaround generic
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.
v2: Renaming and parameter changes to the functions that prepares the
commit (Ville)
Cc: Dhinakaran Pandiyan
Cc: Ville Syrjälä
Reviewed-by: Rodrigo Vivi
Signed-off-by: José 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 | 47 +++
3 files changed, 29 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 9312b3f35eb0..b3a5d8462251 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;
- 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;
/* IPS should be fine as long as at least one plane is enabled. */
@@ -11687,7 +11693,7 @@ clear_intel_crtc_state(struct intel_crtc_state
*crtc_state)
saved_state->shared_dpll = crtc_state->shared_dpll;
saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
saved_state->pch_pfit.force_thru = crtc_state->pch_pfit.force_thru;
- saved_state->ips_force_disable = crtc_state->ips_force_disable;
+ saved_state->crc_enabled = crtc_state->crc_enabled;
if (IS_G4X(dev_priv) ||
IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
saved_state->wm = 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;
bool ips_enabled;
- bool ips_force_disable;
+
+ bool crc_enabled;
bool enable_fbc;
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 53d4ec68d3c4..af64597c5c6e 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum
intel_pipe_crc_source *source,
return 0;
}
-static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
- bool enable)
+static void
+intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable)
{
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
struct drm_device *dev = _priv->drm;
- struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
struct intel_crtc_state *pipe_config;
struct drm_atomic_state *state;
struct drm_modeset_acquire_ctx ctx;
- int ret = 0;
+ int ret;
drm_modeset_acquire_init(, 0);
@@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private
*dev_priv,
goto put_state;
}
- 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 = enable;
- }
+ pipe_config->crc_enabled = enable;
- if (IS_HASWELL(dev_priv)) {
+ if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
pipe_config->pch_pfit.force_thru = enable;
if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
pipe_config->pch_pfit.enabled != enable)
@@ -343,8 +335,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 == INTEL_PIPE_CRC_SOURCE_AUTO)
*source = INTEL_PIPE_CRC_SOURCE_PIPE;
@@ -357,10 +348,6 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private
*dev_priv,
*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
