Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755386AbcJQGXO (ORCPT ); Mon, 17 Oct 2016 02:23:14 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:33082 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750752AbcJQGXM (ORCPT ); Mon, 17 Oct 2016 02:23:12 -0400 Date: Mon, 17 Oct 2016 08:05:00 +0200 From: Daniel Vetter To: Paulo Zanoni Cc: Maarten Lankhorst , Lyude , intel-gfx@lists.freedesktop.org, David Airlie , Daniel Vetter , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values Message-ID: <20161017060500.GO20761@phenom.ffwll.local> Mail-Followup-To: Paulo Zanoni , Maarten Lankhorst , Lyude , intel-gfx@lists.freedesktop.org, David Airlie , Daniel Vetter , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org References: <1475885497-6094-1-git-send-email-cpaul@redhat.com> <1475885497-6094-6-git-send-email-cpaul@redhat.com> <1476389043.2478.30.camel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1476389043.2478.30.camel@intel.com> X-Operating-System: Linux phenom 4.6.0-1-amd64 User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8115 Lines: 189 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 > > > Reviewed-by: Maarten Lankhorst > > > Cc: Ville Syrj?l? > > > Cc: Paulo Zanoni > > > --- > > > ?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