Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933170AbcJQIIs (ORCPT ); Mon, 17 Oct 2016 04:08:48 -0400 Received: from mga01.intel.com ([192.55.52.88]:44177 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932265AbcJQIHu (ORCPT ); Mon, 17 Oct 2016 04:07:50 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,356,1473145200"; d="scan'208";a="20295282" Subject: Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values To: Paulo Zanoni , 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> <20161017060500.GO20761@phenom.ffwll.local> From: Maarten Lankhorst Message-ID: Date: Mon, 17 Oct 2016 10:07:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20161017060500.GO20761@phenom.ffwll.local> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8026 Lines: 187 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 >>>> 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 Waiting for drm-misc merge window to reopen, but making things more explicit will help a lot. ~Maarten