Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1500277 > unrolled thread

Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

Started byMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
First post2016-10-13 15:50 +0200
Last post2016-10-17 10:10 +0200
Articles 6 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant  watermark values Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2016-10-13 15:50 +0200
    Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of  redundant watermark values Paulo Zanoni <paulo.r.zanoni@intel.com> - 2016-10-13 22:10 +0200
      Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of  redundant watermark values Paulo Zanoni <paulo.r.zanoni@intel.com> - 2016-10-13 22:20 +0200
        Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of  redundant watermark values Lyude Paul <cpaul@redhat.com> - 2016-10-13 23:30 +0200
      Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant  watermark values Daniel Vetter <daniel@ffwll.ch> - 2016-10-17 08:30 +0200
        Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant  watermark values Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2016-10-17 10:10 +0200

#1500277 — Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

FromMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date2016-10-13 15:50 +0200
SubjectRe: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values
Message-ID<srOyt-49a-17@gated-at.bofh.it>
Op 08-10-16 om 02:11 schreef Lyude:
> Now that we've make skl_wm_levels make a little more sense, we can
> remove all of the redundant wm information. Up until now we'd been
> storing two copies of all of the skl watermarks: one being the
> skl_pipe_wm structs, the other being the global wm struct in
> drm_i915_private containing the raw register values. This is confusing
> and problematic, since it means we're prone to accidentally letting the
> two copies go out of sync. So, get rid of all of the functions
> responsible for computing the register values and just use a single
> helper, skl_write_wm_level(), to convert and write the new watermarks on
> the fly.
>
> Changes since v1:
> - Fixup skl_write_wm_level()
> - Fixup skl_wm_level_from_reg_val()
> - Don't forget to copy *active to intel_crtc->wm.active.skl
>
> Signed-off-by: Lyude <cpaul@redhat.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   2 -
>  drivers/gpu/drm/i915/intel_display.c |  14 ++-
>  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 204 ++++++++++++-----------------------
>  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
>  5 files changed, 90 insertions(+), 144 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0287c93..76583b2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation {
>  struct skl_wm_values {
>  	unsigned dirty_pipes;
>  	struct skl_ddb_allocation ddb;
> -	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> -	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
>  };
>  
>  struct skl_wm_level {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a71d05a..39400a0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
>  	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	const struct skl_plane_wm *p_wm =
> +		&crtc_state->wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -3414,7 +3416,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_y = src_y;
>  
>  	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> -		skl_write_plane_wm(intel_crtc, wm, 0);
> +		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
>  
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
> @@ -3448,6 +3450,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> +	const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  
>  	/*
> @@ -3455,7 +3459,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
>  	 * plane's visiblity isn't actually changing neither is its watermarks.
>  	 */
>  	if (!crtc->primary->state->visible)
> -		skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0);
> +		skl_write_plane_wm(intel_crtc, p_wm,
> +				   &dev_priv->wm.skl_results.ddb, 0);
>  
>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
> @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	const struct skl_plane_wm *p_wm =
> +		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
>  	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
> -		skl_write_cursor_wm(intel_crtc, wm);
> +		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
>  
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d684f4f..958dc72 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  				 struct intel_crtc *intel_crtc);
>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_wm_values *wm);
> +			 const struct skl_plane_wm *wm,
> +			 const struct skl_ddb_allocation *ddb);
>  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> -			const struct skl_wm_values *wm,
> +			const struct skl_plane_wm *wm,
> +			const struct skl_ddb_allocation *ddb,
>  			int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5dbaf12..5cb537c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_crtc *crtc;
> +	struct intel_crtc_state *cstate;
> +	struct skl_plane_wm *wm;
>  	enum pipe pipe;
>  	int level, plane;
>  
> @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
>  	/* Since we're now guaranteed to only have one active CRTC... */
>  	pipe = ffs(intel_state->active_crtcs) - 1;
>  	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +	cstate = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
Wrong cstate here, should be crtc->state since it's committed and crtc_state is the old state.

~Maarten

[toc] | [next] | [standalone]

#1500524 — Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

FromPaulo Zanoni <paulo.r.zanoni@intel.com>
Date2016-10-13 22:10 +0200
SubjectRe: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values
Message-ID<srUud-8eQ-1@gated-at.bofh.it>
In reply to#1500277
Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
> Op 08-10-16 om 02:11 schreef Lyude:
> > 
> > Now that we've make skl_wm_levels make a little more sense, we can
> > remove all of the redundant wm information. Up until now we'd been
> > storing two copies of all of the skl watermarks: one being the
> > skl_pipe_wm structs, the other being the global wm struct in
> > drm_i915_private containing the raw register values. This is
> > confusing
> > and problematic, since it means we're prone to accidentally letting
> > the
> > two copies go out of sync. So, get rid of all of the functions
> > responsible for computing the register values and just use a single
> > helper, skl_write_wm_level(), to convert and write the new
> > watermarks on
> > the fly.
> > 
> > Changes since v1:
> > - Fixup skl_write_wm_level()
> > - Fixup skl_wm_level_from_reg_val()
> > - Don't forget to copy *active to intel_crtc->wm.active.skl
> > 
> > Signed-off-by: Lyude <cpaul@redhat.com>
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |   2 -
> >  drivers/gpu/drm/i915/intel_display.c |  14 ++-
> >  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
> >  drivers/gpu/drm/i915/intel_pm.c      | 204 ++++++++++++-----------
> > ------------
> >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
> >  5 files changed, 90 insertions(+), 144 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 0287c93..76583b2 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation {
> >  struct skl_wm_values {
> >  	unsigned dirty_pipes;
> >  	struct skl_ddb_allocation ddb;
> > -	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> > -	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
> >  };
> >  
> >  struct skl_wm_level {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index a71d05a..39400a0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3378,6 +3378,8 @@ static void
> > skylake_update_primary_plane(struct drm_plane *plane,
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> > >base.crtc);
> >  	struct drm_framebuffer *fb = plane_state->base.fb;
> >  	const struct skl_wm_values *wm = &dev_priv-
> > >wm.skl_results;
> > +	const struct skl_plane_wm *p_wm =
> > +		&crtc_state->wm.skl.optimal.planes[0];
> >  	int pipe = intel_crtc->pipe;
> >  	u32 plane_ctl;
> >  	unsigned int rotation = plane_state->base.rotation;
> > @@ -3414,7 +3416,7 @@ static void
> > skylake_update_primary_plane(struct drm_plane *plane,
> >  	intel_crtc->adjusted_y = src_y;
> >  
> >  	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> > -		skl_write_plane_wm(intel_crtc, wm, 0);
> > +		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
> >  
> >  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> >  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
> > @@ -3448,6 +3450,8 @@ static void
> > skylake_disable_primary_plane(struct drm_plane *primary,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_crtc_state *cstate =
> > to_intel_crtc_state(crtc->state);
> > +	const struct skl_plane_wm *p_wm = &cstate-
> > >wm.skl.optimal.planes[0];
> >  	int pipe = intel_crtc->pipe;
> >  
> >  	/*
> > @@ -3455,7 +3459,8 @@ static void
> > skylake_disable_primary_plane(struct drm_plane *primary,
> >  	 * plane's visiblity isn't actually changing neither is
> > its watermarks.
> >  	 */
> >  	if (!crtc->primary->state->visible)
> > -		skl_write_plane_wm(intel_crtc, &dev_priv-
> > >wm.skl_results, 0);
> > +		skl_write_plane_wm(intel_crtc, p_wm,
> > +				   &dev_priv->wm.skl_results.ddb,
> > 0);
> >  
> >  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
> >  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
> > @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct
> > drm_crtc *crtc, u32 base,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_crtc_state *cstate =
> > to_intel_crtc_state(crtc->state);
> >  	const struct skl_wm_values *wm = &dev_priv-
> > >wm.skl_results;
> > +	const struct skl_plane_wm *p_wm =
> > +		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
> >  	int pipe = intel_crtc->pipe;
> >  	uint32_t cntl = 0;
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> > drm_crtc_mask(crtc))
> > -		skl_write_cursor_wm(intel_crtc, wm);
> > +		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> >  
> >  	if (plane_state && plane_state->base.visible) {
> >  		cntl = MCURSOR_GAMMA_ENABLE;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d684f4f..958dc72 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct
> > skl_ddb_allocation *old,
> >  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> >  				 struct intel_crtc *intel_crtc);
> >  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > -			 const struct skl_wm_values *wm);
> > +			 const struct skl_plane_wm *wm,
> > +			 const struct skl_ddb_allocation *ddb);
> >  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> > -			const struct skl_wm_values *wm,
> > +			const struct skl_plane_wm *wm,
> > +			const struct skl_ddb_allocation *ddb,
> >  			int plane);
> >  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> > *pipe_config);
> >  bool ilk_disable_lp_wm(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 5dbaf12..5cb537c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct
> > drm_atomic_state *state)
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> >  	struct drm_crtc *crtc;
> > +	struct intel_crtc_state *cstate;
> > +	struct skl_plane_wm *wm;
> >  	enum pipe pipe;
> >  	int level, plane;
> >  
> > @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
> > drm_atomic_state *state)
> >  	/* Since we're now guaranteed to only have one active
> > CRTC... */
> >  	pipe = ffs(intel_state->active_crtcs) - 1;
> >  	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > +	cstate = intel_atomic_get_crtc_state(state,
> > to_intel_crtc(crtc));
> Wrong cstate here, should be crtc->state since it's committed and
> crtc_state is the old state.

Ah... How many instances of the "you're looking at the wrong state
struct" have we spotted/fixed ever since the introduction of the atomic
code? Can't we try to do something more human-proof in order to avoid
these problems from happening again and again?

Like for example copying the old states to a specific structure like
drm_atomic_state->old_states_which_you_probably_shouldn_be_using, and
then setting the CRTC/connector/plane states in the drm_atomic_state
struct to NULL so bugs will be much easier to spot?

Anyway, with this problem fixed, feel free to add:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> ~Maarten

[toc] | [prev] | [next] | [standalone]

#1500536 — Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

FromPaulo Zanoni <paulo.r.zanoni@intel.com>
Date2016-10-13 22:20 +0200
SubjectRe: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values
Message-ID<srUDU-8id-25@gated-at.bofh.it>
In reply to#1500524
Em Qui, 2016-10-13 às 17:04 -0300, Paulo Zanoni escreveu:
> Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
> > 
> > Op 08-10-16 om 02:11 schreef Lyude:
> > > 
> > > 
> > > Now that we've make skl_wm_levels make a little more sense, we
> > > can
> > > remove all of the redundant wm information. Up until now we'd
> > > been
> > > storing two copies of all of the skl watermarks: one being the
> > > skl_pipe_wm structs, the other being the global wm struct in
> > > drm_i915_private containing the raw register values. This is
> > > confusing
> > > and problematic, since it means we're prone to accidentally
> > > letting
> > > the
> > > two copies go out of sync. So, get rid of all of the functions
> > > responsible for computing the register values and just use a
> > > single
> > > helper, skl_write_wm_level(), to convert and write the new
> > > watermarks on
> > > the fly.
> > > 
> > > Changes since v1:
> > > - Fixup skl_write_wm_level()
> > > - Fixup skl_wm_level_from_reg_val()
> > > - Don't forget to copy *active to intel_crtc->wm.active.skl
> > > 
> > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
> > > >
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |   2 -
> > >  drivers/gpu/drm/i915/intel_display.c |  14 ++-
> > >  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
> > >  drivers/gpu/drm/i915/intel_pm.c      | 204 ++++++++++++---------
> > > --
> > > ------------
> > >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
> > >  5 files changed, 90 insertions(+), 144 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0287c93..76583b2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation {
> > >  struct skl_wm_values {
> > >  	unsigned dirty_pipes;
> > >  	struct skl_ddb_allocation ddb;
> > > -	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> > > -	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
> > >  };
> > >  
> > >  struct skl_wm_level {
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index a71d05a..39400a0 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3378,6 +3378,8 @@ static void
> > > skylake_update_primary_plane(struct drm_plane *plane,
> > >  	struct intel_crtc *intel_crtc =
> > > to_intel_crtc(crtc_state-
> > > > 
> > > > base.crtc);
> > >  	struct drm_framebuffer *fb = plane_state->base.fb;
> > >  	const struct skl_wm_values *wm = &dev_priv-
> > > > 
> > > > wm.skl_results;
> > > +	const struct skl_plane_wm *p_wm =
> > > +		&crtc_state->wm.skl.optimal.planes[0];
> > >  	int pipe = intel_crtc->pipe;
> > >  	u32 plane_ctl;
> > >  	unsigned int rotation = plane_state->base.rotation;
> > > @@ -3414,7 +3416,7 @@ static void
> > > skylake_update_primary_plane(struct drm_plane *plane,
> > >  	intel_crtc->adjusted_y = src_y;
> > >  
> > >  	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> > > -		skl_write_plane_wm(intel_crtc, wm, 0);
> > > +		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb,
> > > 0);
> > >  
> > >  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> > >  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) |
> > > src_x);
> > > @@ -3448,6 +3450,8 @@ static void
> > > skylake_disable_primary_plane(struct drm_plane *primary,
> > >  	struct drm_device *dev = crtc->dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +	struct intel_crtc_state *cstate =
> > > to_intel_crtc_state(crtc->state);
> > > +	const struct skl_plane_wm *p_wm = &cstate-
> > > > 
> > > > wm.skl.optimal.planes[0];
> > >  	int pipe = intel_crtc->pipe;
> > >  
> > >  	/*
> > > @@ -3455,7 +3459,8 @@ static void
> > > skylake_disable_primary_plane(struct drm_plane *primary,
> > >  	 * plane's visiblity isn't actually changing neither is
> > > its watermarks.
> > >  	 */
> > >  	if (!crtc->primary->state->visible)
> > > -		skl_write_plane_wm(intel_crtc, &dev_priv-
> > > > 
> > > > wm.skl_results, 0);
> > > +		skl_write_plane_wm(intel_crtc, p_wm,
> > > +				   &dev_priv-
> > > >wm.skl_results.ddb,
> > > 0);
> > >  
> > >  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
> > >  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
> > > @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct
> > > drm_crtc *crtc, u32 base,
> > >  	struct drm_device *dev = crtc->dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +	struct intel_crtc_state *cstate =
> > > to_intel_crtc_state(crtc->state);
> > >  	const struct skl_wm_values *wm = &dev_priv-
> > > > 
> > > > wm.skl_results;
> > > +	const struct skl_plane_wm *p_wm =
> > > +		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
> > >  	int pipe = intel_crtc->pipe;
> > >  	uint32_t cntl = 0;
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> > > drm_crtc_mask(crtc))
> > > -		skl_write_cursor_wm(intel_crtc, wm);
> > > +		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> > >  
> > >  	if (plane_state && plane_state->base.visible) {
> > >  		cntl = MCURSOR_GAMMA_ENABLE;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index d684f4f..958dc72 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const
> > > struct
> > > skl_ddb_allocation *old,
> > >  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> > >  				 struct intel_crtc *intel_crtc);
> > >  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > > -			 const struct skl_wm_values *wm);
> > > +			 const struct skl_plane_wm *wm,
> > > +			 const struct skl_ddb_allocation *ddb);
> > >  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> > > -			const struct skl_wm_values *wm,
> > > +			const struct skl_plane_wm *wm,
> > > +			const struct skl_ddb_allocation *ddb,
> > >  			int plane);
> > >  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> > > *pipe_config);
> > >  bool ilk_disable_lp_wm(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 5dbaf12..5cb537c 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct
> > > drm_atomic_state *state)
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_atomic_state *intel_state =
> > > to_intel_atomic_state(state);
> > >  	struct drm_crtc *crtc;
> > > +	struct intel_crtc_state *cstate;
> > > +	struct skl_plane_wm *wm;
> > >  	enum pipe pipe;
> > >  	int level, plane;
> > >  
> > > @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
> > > drm_atomic_state *state)
> > >  	/* Since we're now guaranteed to only have one active
> > > CRTC... */
> > >  	pipe = ffs(intel_state->active_crtcs) - 1;
> > >  	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > > +	cstate = intel_atomic_get_crtc_state(state,
> > > to_intel_crtc(crtc));
> > Wrong cstate here, should be crtc->state since it's committed and
> > crtc_state is the old state.
> 
> Ah... How many instances of the "you're looking at the wrong state
> struct" have we spotted/fixed ever since the introduction of the
> atomic
> code? Can't we try to do something more human-proof in order to avoid
> these problems from happening again and again?
> 
> Like for example copying the old states to a specific structure like
> drm_atomic_state->old_states_which_you_probably_shouldn_be_using, and
> then setting the CRTC/connector/plane states in the drm_atomic_state
> struct to NULL so bugs will be much easier to spot?
> 
> Anyway, with this problem fixed, feel free to add:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Oh, I forgot to mention: this patch has a conflict with my
"unconditionally apply the memory workaround" patch, so we may need to
decide which one gets merged first. Of course I'd suggest my patch to
be merged first since I want it to land in stable, but I'm always
biased towards my patches. If you agree, I can even volunteer myself to
solve the conflicts of this patch later when applying.

> 
> > 
> > 
> > ~Maarten
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[toc] | [prev] | [next] | [standalone]

#1500588 — Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

FromLyude Paul <cpaul@redhat.com>
Date2016-10-13 23:30 +0200
SubjectRe: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values
Message-ID<srVJE-u5-21@gated-at.bofh.it>
In reply to#1500536
Your is SAGV related, and when we don't make the SAGV happy people's
machines usually hang. So I'm definitely for your patches getting
merged first


On Thu, 2016-10-13 at 17:07 -0300, Paulo Zanoni wrote:
> Em Qui, 2016-10-13 às 17:04 -0300, Paulo Zanoni escreveu:
> > 
> > Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
> > > 
> > > 
> > > Op 08-10-16 om 02:11 schreef Lyude:
> > > > 
> > > > 
> > > > 
> > > > Now that we've make skl_wm_levels make a little more sense, we
> > > > can
> > > > remove all of the redundant wm information. Up until now we'd
> > > > been
> > > > storing two copies of all of the skl watermarks: one being the
> > > > skl_pipe_wm structs, the other being the global wm struct in
> > > > drm_i915_private containing the raw register values. This is
> > > > confusing
> > > > and problematic, since it means we're prone to accidentally
> > > > letting
> > > > the
> > > > two copies go out of sync. So, get rid of all of the functions
> > > > responsible for computing the register values and just use a
> > > > single
> > > > helper, skl_write_wm_level(), to convert and write the new
> > > > watermarks on
> > > > the fly.
> > > > 
> > > > Changes since v1:
> > > > - Fixup skl_write_wm_level()
> > > > - Fixup skl_wm_level_from_reg_val()
> > > > - Don't forget to copy *active to intel_crtc->wm.active.skl
> > > > 
> > > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > > om
> > > > > 
> > > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h      |   2 -
> > > >  drivers/gpu/drm/i915/intel_display.c |  14 ++-
> > > >  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
> > > >  drivers/gpu/drm/i915/intel_pm.c      | 204 ++++++++++++-------
> > > > --
> > > > --
> > > > ------------
> > > >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
> > > >  5 files changed, 90 insertions(+), 144 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 0287c93..76583b2 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation {
> > > >  struct skl_wm_values {
> > > >  	unsigned dirty_pipes;
> > > >  	struct skl_ddb_allocation ddb;
> > > > -	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> > > > -	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
> > > >  };
> > > >  
> > > >  struct skl_wm_level {
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index a71d05a..39400a0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -3378,6 +3378,8 @@ static void
> > > > skylake_update_primary_plane(struct drm_plane *plane,
> > > >  	struct intel_crtc *intel_crtc =
> > > > to_intel_crtc(crtc_state-
> > > > > 
> > > > > 
> > > > > base.crtc);
> > > >  	struct drm_framebuffer *fb = plane_state->base.fb;
> > > >  	const struct skl_wm_values *wm = &dev_priv-
> > > > > 
> > > > > 
> > > > > wm.skl_results;
> > > > +	const struct skl_plane_wm *p_wm =
> > > > +		&crtc_state->wm.skl.optimal.planes[0];
> > > >  	int pipe = intel_crtc->pipe;
> > > >  	u32 plane_ctl;
> > > >  	unsigned int rotation = plane_state->base.rotation;
> > > > @@ -3414,7 +3416,7 @@ static void
> > > > skylake_update_primary_plane(struct drm_plane *plane,
> > > >  	intel_crtc->adjusted_y = src_y;
> > > >  
> > > >  	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc-
> > > > >base))
> > > > -		skl_write_plane_wm(intel_crtc, wm, 0);
> > > > +		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb,
> > > > 0);
> > > >  
> > > >  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> > > >  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) |
> > > > src_x);
> > > > @@ -3448,6 +3450,8 @@ static void
> > > > skylake_disable_primary_plane(struct drm_plane *primary,
> > > >  	struct drm_device *dev = crtc->dev;
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > +	struct intel_crtc_state *cstate =
> > > > to_intel_crtc_state(crtc->state);
> > > > +	const struct skl_plane_wm *p_wm = &cstate-
> > > > > 
> > > > > 
> > > > > wm.skl.optimal.planes[0];
> > > >  	int pipe = intel_crtc->pipe;
> > > >  
> > > >  	/*
> > > > @@ -3455,7 +3459,8 @@ static void
> > > > skylake_disable_primary_plane(struct drm_plane *primary,
> > > >  	 * plane's visiblity isn't actually changing neither
> > > > is
> > > > its watermarks.
> > > >  	 */
> > > >  	if (!crtc->primary->state->visible)
> > > > -		skl_write_plane_wm(intel_crtc, &dev_priv-
> > > > > 
> > > > > 
> > > > > wm.skl_results, 0);
> > > > +		skl_write_plane_wm(intel_crtc, p_wm,
> > > > +				   &dev_priv-
> > > > > 
> > > > > wm.skl_results.ddb,
> > > > 0);
> > > >  
> > > >  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
> > > >  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
> > > > @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct
> > > > drm_crtc *crtc, u32 base,
> > > >  	struct drm_device *dev = crtc->dev;
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > +	struct intel_crtc_state *cstate =
> > > > to_intel_crtc_state(crtc->state);
> > > >  	const struct skl_wm_values *wm = &dev_priv-
> > > > > 
> > > > > 
> > > > > wm.skl_results;
> > > > +	const struct skl_plane_wm *p_wm =
> > > > +		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
> > > >  	int pipe = intel_crtc->pipe;
> > > >  	uint32_t cntl = 0;
> > > >  
> > > >  	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> > > > drm_crtc_mask(crtc))
> > > > -		skl_write_cursor_wm(intel_crtc, wm);
> > > > +		skl_write_cursor_wm(intel_crtc, p_wm, &wm-
> > > > >ddb);
> > > >  
> > > >  	if (plane_state && plane_state->base.visible) {
> > > >  		cntl = MCURSOR_GAMMA_ENABLE;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index d684f4f..958dc72 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const
> > > > struct
> > > > skl_ddb_allocation *old,
> > > >  bool skl_ddb_allocation_overlaps(struct drm_atomic_state
> > > > *state,
> > > >  				 struct intel_crtc
> > > > *intel_crtc);
> > > >  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > > > -			 const struct skl_wm_values *wm);
> > > > +			 const struct skl_plane_wm *wm,
> > > > +			 const struct skl_ddb_allocation
> > > > *ddb);
> > > >  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> > > > -			const struct skl_wm_values *wm,
> > > > +			const struct skl_plane_wm *wm,
> > > > +			const struct skl_ddb_allocation *ddb,
> > > >  			int plane);
> > > >  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> > > > *pipe_config);
> > > >  bool ilk_disable_lp_wm(struct drm_device *dev);
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 5dbaf12..5cb537c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct
> > > > drm_atomic_state *state)
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  	struct intel_atomic_state *intel_state =
> > > > to_intel_atomic_state(state);
> > > >  	struct drm_crtc *crtc;
> > > > +	struct intel_crtc_state *cstate;
> > > > +	struct skl_plane_wm *wm;
> > > >  	enum pipe pipe;
> > > >  	int level, plane;
> > > >  
> > > > @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
> > > > drm_atomic_state *state)
> > > >  	/* Since we're now guaranteed to only have one active
> > > > CRTC... */
> > > >  	pipe = ffs(intel_state->active_crtcs) - 1;
> > > >  	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > > > +	cstate = intel_atomic_get_crtc_state(state,
> > > > to_intel_crtc(crtc));
> > > Wrong cstate here, should be crtc->state since it's committed and
> > > crtc_state is the old state.
> > 
> > Ah... How many instances of the "you're looking at the wrong state
> > struct" have we spotted/fixed ever since the introduction of the
> > atomic
> > code? Can't we try to do something more human-proof in order to
> > avoid
> > these problems from happening again and again?
> > 
> > Like for example copying the old states to a specific structure
> > like
> > drm_atomic_state->old_states_which_you_probably_shouldn_be_using,
> > and
> > then setting the CRTC/connector/plane states in the
> > drm_atomic_state
> > struct to NULL so bugs will be much easier to spot?
> > 
> > Anyway, with this problem fixed, feel free to add:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Oh, I forgot to mention: this patch has a conflict with my
> "unconditionally apply the memory workaround" patch, so we may need
> to
> decide which one gets merged first. Of course I'd suggest my patch to
> be merged first since I want it to land in stable, but I'm always
> biased towards my patches. If you agree, I can even volunteer myself
> to
> solve the conflicts of this patch later when applying.
> 
> > 
> > 
> > > 
> > > 
> > > 
> > > ~Maarten
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[toc] | [prev] | [next] | [standalone]

#1501623

FromDaniel Vetter <daniel@ffwll.ch>
Date2016-10-17 08:30 +0200
Message-ID<st9AR-8hu-3@gated-at.bofh.it>
In reply to#1500524
On Thu, Oct 13, 2016 at 05:04:03PM -0300, Paulo Zanoni wrote:
> Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
> > Op 08-10-16 om 02:11 schreef Lyude:
> > > 
> > > Now that we've make skl_wm_levels make a little more sense, we can
> > > remove all of the redundant wm information. Up until now we'd been
> > > storing two copies of all of the skl watermarks: one being the
> > > skl_pipe_wm structs, the other being the global wm struct in
> > > drm_i915_private containing the raw register values. This is
> > > confusing
> > > and problematic, since it means we're prone to accidentally letting
> > > the
> > > two copies go out of sync. So, get rid of all of the functions
> > > responsible for computing the register values and just use a single
> > > helper, skl_write_wm_level(), to convert and write the new
> > > watermarks on
> > > the fly.
> > > 
> > > Changes since v1:
> > > - Fixup skl_write_wm_level()
> > > - Fixup skl_wm_level_from_reg_val()
> > > - Don't forget to copy *active to intel_crtc->wm.active.skl
> > > 
> > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |   2 -
> > >  drivers/gpu/drm/i915/intel_display.c |  14 ++-
> > >  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
> > >  drivers/gpu/drm/i915/intel_pm.c      | 204 ++++++++++++-----------
> > > ------------
> > >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
> > >  5 files changed, 90 insertions(+), 144 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0287c93..76583b2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation {
> > >  struct skl_wm_values {
> > >  	unsigned dirty_pipes;
> > >  	struct skl_ddb_allocation ddb;
> > > -	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> > > -	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
> > >  };
> > >  
> > >  struct skl_wm_level {
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index a71d05a..39400a0 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3378,6 +3378,8 @@ static void
> > > skylake_update_primary_plane(struct drm_plane *plane,
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> > > >base.crtc);
> > >  	struct drm_framebuffer *fb = plane_state->base.fb;
> > >  	const struct skl_wm_values *wm = &dev_priv-
> > > >wm.skl_results;
> > > +	const struct skl_plane_wm *p_wm =
> > > +		&crtc_state->wm.skl.optimal.planes[0];
> > >  	int pipe = intel_crtc->pipe;
> > >  	u32 plane_ctl;
> > >  	unsigned int rotation = plane_state->base.rotation;
> > > @@ -3414,7 +3416,7 @@ static void
> > > skylake_update_primary_plane(struct drm_plane *plane,
> > >  	intel_crtc->adjusted_y = src_y;
> > >  
> > >  	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> > > -		skl_write_plane_wm(intel_crtc, wm, 0);
> > > +		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
> > >  
> > >  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> > >  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
> > > @@ -3448,6 +3450,8 @@ static void
> > > skylake_disable_primary_plane(struct drm_plane *primary,
> > >  	struct drm_device *dev = crtc->dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +	struct intel_crtc_state *cstate =
> > > to_intel_crtc_state(crtc->state);
> > > +	const struct skl_plane_wm *p_wm = &cstate-
> > > >wm.skl.optimal.planes[0];
> > >  	int pipe = intel_crtc->pipe;
> > >  
> > >  	/*
> > > @@ -3455,7 +3459,8 @@ static void
> > > skylake_disable_primary_plane(struct drm_plane *primary,
> > >  	 * plane's visiblity isn't actually changing neither is
> > > its watermarks.
> > >  	 */
> > >  	if (!crtc->primary->state->visible)
> > > -		skl_write_plane_wm(intel_crtc, &dev_priv-
> > > >wm.skl_results, 0);
> > > +		skl_write_plane_wm(intel_crtc, p_wm,
> > > +				   &dev_priv->wm.skl_results.ddb,
> > > 0);
> > >  
> > >  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
> > >  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
> > > @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct
> > > drm_crtc *crtc, u32 base,
> > >  	struct drm_device *dev = crtc->dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +	struct intel_crtc_state *cstate =
> > > to_intel_crtc_state(crtc->state);
> > >  	const struct skl_wm_values *wm = &dev_priv-
> > > >wm.skl_results;
> > > +	const struct skl_plane_wm *p_wm =
> > > +		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
> > >  	int pipe = intel_crtc->pipe;
> > >  	uint32_t cntl = 0;
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> > > drm_crtc_mask(crtc))
> > > -		skl_write_cursor_wm(intel_crtc, wm);
> > > +		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> > >  
> > >  	if (plane_state && plane_state->base.visible) {
> > >  		cntl = MCURSOR_GAMMA_ENABLE;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index d684f4f..958dc72 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct
> > > skl_ddb_allocation *old,
> > >  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> > >  				 struct intel_crtc *intel_crtc);
> > >  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > > -			 const struct skl_wm_values *wm);
> > > +			 const struct skl_plane_wm *wm,
> > > +			 const struct skl_ddb_allocation *ddb);
> > >  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> > > -			const struct skl_wm_values *wm,
> > > +			const struct skl_plane_wm *wm,
> > > +			const struct skl_ddb_allocation *ddb,
> > >  			int plane);
> > >  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> > > *pipe_config);
> > >  bool ilk_disable_lp_wm(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 5dbaf12..5cb537c 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct
> > > drm_atomic_state *state)
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_atomic_state *intel_state =
> > > to_intel_atomic_state(state);
> > >  	struct drm_crtc *crtc;
> > > +	struct intel_crtc_state *cstate;
> > > +	struct skl_plane_wm *wm;
> > >  	enum pipe pipe;
> > >  	int level, plane;
> > >  
> > > @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
> > > drm_atomic_state *state)
> > >  	/* Since we're now guaranteed to only have one active
> > > CRTC... */
> > >  	pipe = ffs(intel_state->active_crtcs) - 1;
> > >  	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > > +	cstate = intel_atomic_get_crtc_state(state,
> > > to_intel_crtc(crtc));
> > Wrong cstate here, should be crtc->state since it's committed and
> > crtc_state is the old state.
> 
> Ah... How many instances of the "you're looking at the wrong state
> struct" have we spotted/fixed ever since the introduction of the atomic
> code? Can't we try to do something more human-proof in order to avoid
> these problems from happening again and again?
> 
> Like for example copying the old states to a specific structure like
> drm_atomic_state->old_states_which_you_probably_shouldn_be_using, and
> then setting the CRTC/connector/plane states in the drm_atomic_state
> struct to NULL so bugs will be much easier to spot?

Maarten has patches somewhere to create explicit old/new state pointers in
drm_atomic_state. Not sure where exactly they're stuck.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1501689

FromMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date2016-10-17 10:10 +0200
Message-ID<stb9D-Tq-11@gated-at.bofh.it>
In reply to#1501623
Op 17-10-16 om 08:05 schreef Daniel Vetter:
> On Thu, Oct 13, 2016 at 05:04:03PM -0300, Paulo Zanoni wrote:
>> Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
>>> Op 08-10-16 om 02:11 schreef Lyude:
>>>> Now that we've make skl_wm_levels make a little more sense, we can
>>>> remove all of the redundant wm information. Up until now we'd been
>>>> storing two copies of all of the skl watermarks: one being the
>>>> skl_pipe_wm structs, the other being the global wm struct in
>>>> drm_i915_private containing the raw register values. This is
>>>> confusing
>>>> and problematic, since it means we're prone to accidentally letting
>>>> the
>>>> two copies go out of sync. So, get rid of all of the functions
>>>> responsible for computing the register values and just use a single
>>>> helper, skl_write_wm_level(), to convert and write the new
>>>> watermarks on
>>>> the fly.
>>>>
>>>> Changes since v1:
>>>> - Fixup skl_write_wm_level()
>>>> - Fixup skl_wm_level_from_reg_val()
>>>> - Don't forget to copy *active to intel_crtc->wm.active.skl
>>>>
>>>> Signed-off-by: Lyude <cpaul@redhat.com>
>>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_drv.h      |   2 -
>>>>  drivers/gpu/drm/i915/intel_display.c |  14 ++-
>>>>  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
>>>>  drivers/gpu/drm/i915/intel_pm.c      | 204 ++++++++++++-----------
>>>> ------------
>>>>  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
>>>>  5 files changed, 90 insertions(+), 144 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 0287c93..76583b2 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation {
>>>>  struct skl_wm_values {
>>>>  	unsigned dirty_pipes;
>>>>  	struct skl_ddb_allocation ddb;
>>>> -	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
>>>> -	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
>>>>  };
>>>>  
>>>>  struct skl_wm_level {
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>> index a71d05a..39400a0 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -3378,6 +3378,8 @@ static void
>>>> skylake_update_primary_plane(struct drm_plane *plane,
>>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
>>>>> base.crtc);
>>>>  	struct drm_framebuffer *fb = plane_state->base.fb;
>>>>  	const struct skl_wm_values *wm = &dev_priv-
>>>>> wm.skl_results;
>>>> +	const struct skl_plane_wm *p_wm =
>>>> +		&crtc_state->wm.skl.optimal.planes[0];
>>>>  	int pipe = intel_crtc->pipe;
>>>>  	u32 plane_ctl;
>>>>  	unsigned int rotation = plane_state->base.rotation;
>>>> @@ -3414,7 +3416,7 @@ static void
>>>> skylake_update_primary_plane(struct drm_plane *plane,
>>>>  	intel_crtc->adjusted_y = src_y;
>>>>  
>>>>  	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
>>>> -		skl_write_plane_wm(intel_crtc, wm, 0);
>>>> +		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
>>>>  
>>>>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>>>>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>>>> @@ -3448,6 +3450,8 @@ static void
>>>> skylake_disable_primary_plane(struct drm_plane *primary,
>>>>  	struct drm_device *dev = crtc->dev;
>>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>> +	struct intel_crtc_state *cstate =
>>>> to_intel_crtc_state(crtc->state);
>>>> +	const struct skl_plane_wm *p_wm = &cstate-
>>>>> wm.skl.optimal.planes[0];
>>>>  	int pipe = intel_crtc->pipe;
>>>>  
>>>>  	/*
>>>> @@ -3455,7 +3459,8 @@ static void
>>>> skylake_disable_primary_plane(struct drm_plane *primary,
>>>>  	 * plane's visiblity isn't actually changing neither is
>>>> its watermarks.
>>>>  	 */
>>>>  	if (!crtc->primary->state->visible)
>>>> -		skl_write_plane_wm(intel_crtc, &dev_priv-
>>>>> wm.skl_results, 0);
>>>> +		skl_write_plane_wm(intel_crtc, p_wm,
>>>> +				   &dev_priv->wm.skl_results.ddb,
>>>> 0);
>>>>  
>>>>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>>>>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
>>>> @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct
>>>> drm_crtc *crtc, u32 base,
>>>>  	struct drm_device *dev = crtc->dev;
>>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>> +	struct intel_crtc_state *cstate =
>>>> to_intel_crtc_state(crtc->state);
>>>>  	const struct skl_wm_values *wm = &dev_priv-
>>>>> wm.skl_results;
>>>> +	const struct skl_plane_wm *p_wm =
>>>> +		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>>>>  	int pipe = intel_crtc->pipe;
>>>>  	uint32_t cntl = 0;
>>>>  
>>>>  	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
>>>> drm_crtc_mask(crtc))
>>>> -		skl_write_cursor_wm(intel_crtc, wm);
>>>> +		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
>>>>  
>>>>  	if (plane_state && plane_state->base.visible) {
>>>>  		cntl = MCURSOR_GAMMA_ENABLE;
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index d684f4f..958dc72 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct
>>>> skl_ddb_allocation *old,
>>>>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>>>>  				 struct intel_crtc *intel_crtc);
>>>>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
>>>> -			 const struct skl_wm_values *wm);
>>>> +			 const struct skl_plane_wm *wm,
>>>> +			 const struct skl_ddb_allocation *ddb);
>>>>  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
>>>> -			const struct skl_wm_values *wm,
>>>> +			const struct skl_plane_wm *wm,
>>>> +			const struct skl_ddb_allocation *ddb,
>>>>  			int plane);
>>>>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
>>>> *pipe_config);
>>>>  bool ilk_disable_lp_wm(struct drm_device *dev);
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>>> b/drivers/gpu/drm/i915/intel_pm.c
>>>> index 5dbaf12..5cb537c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct
>>>> drm_atomic_state *state)
>>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>>  	struct intel_atomic_state *intel_state =
>>>> to_intel_atomic_state(state);
>>>>  	struct drm_crtc *crtc;
>>>> +	struct intel_crtc_state *cstate;
>>>> +	struct skl_plane_wm *wm;
>>>>  	enum pipe pipe;
>>>>  	int level, plane;
>>>>  
>>>> @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
>>>> drm_atomic_state *state)
>>>>  	/* Since we're now guaranteed to only have one active
>>>> CRTC... */
>>>>  	pipe = ffs(intel_state->active_crtcs) - 1;
>>>>  	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>>>> +	cstate = intel_atomic_get_crtc_state(state,
>>>> to_intel_crtc(crtc));
>>> Wrong cstate here, should be crtc->state since it's committed and
>>> crtc_state is the old state.
>> Ah... How many instances of the "you're looking at the wrong state
>> struct" have we spotted/fixed ever since the introduction of the atomic
>> code? Can't we try to do something more human-proof in order to avoid
>> these problems from happening again and again?
>>
>> Like for example copying the old states to a specific structure like
>> drm_atomic_state->old_states_which_you_probably_shouldn_be_using, and
>> then setting the CRTC/connector/plane states in the drm_atomic_state
>> struct to NULL so bugs will be much easier to spot?
> Maarten has patches somewhere to create explicit old/new state pointers in
> drm_atomic_state. Not sure where exactly they're stuck.
> -Daniel

Waiting for drm-misc merge window to reopen, but making things more explicit will help a lot.

~Maarten

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web