| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 17 Oct 2016 10:07:45 +0200
From: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
To: Paulo Zanoni <paulo.r.zanoni@...el.com>, Lyude <cpaul@...hat.com>,
intel-gfx@...ts.freedesktop.org, David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel.vetter@...el.com>,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant
watermark values
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@...hat.com>
>>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@...ux.intel.com>
>>>> Cc: Paulo Zanoni <paulo.r.zanoni@...el.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
Powered by blists - more mailing lists