Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference
On Thu, Nov 12, 2015 at 07:50:09PM +0200, Imre Deak wrote: > On to, 2015-11-12 at 17:04 +, Chris Wilson wrote: > > As it stands since we don't actually cancel the hangcheck when we > drop > > the rpm wakelock in intel_mark_idle() it can very well come to pass > > that > > we execute this whilst the device is asleep. However, if the device > > is > > alseep, we now that we are no longer executing. > > But how could this run while asleep, since we flush the work in the > runtime suspend handler before turning off the HW? But even if it can't > run your idea would be clearer imo.. We shouldn't be flushing the hangcheck worker there. That's a leaky abstraction attempting to paper over something that shouldn't have been a problem in the first place. Note that there is a similar issue with the existing request waiters that currently may race against intel_mark_idle(). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR activation.
On Thu, 2015-11-12 at 13:50 +, R, Durgadoss wrote: > Hi Rodrigo, > > > -Original Message- > > From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On > > Behalf Of Rodrigo Vivi > > Sent: Thursday, November 12, 2015 1:07 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Vivi, Rodrigo > > Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR > > activation. > > > > When debuging the frozen screen caused by HW tracking with low > > power state I noticed that if we keep moving the mouse non stop > > you will miss the screen updates for a while. At least > > until we stop moving the mouse for a small time and move again. > > > > The actual enabling should happen immediately after > > Display Port enabling sequence finished with links trained and > > everything enabled. However we face many issues when enabling PSR > > right after a modeset. > > > > On VLV/CHV we face blank screens on this scenario and on HSW+ > > we face a recoverable frozen screen, at least until next > > exit-activate sequence. > > > > Another workaround for the same issue here would be to increase > > re-enable idle time from 100 to 500 as we did for VLV/CHV. > > However this patch workaround this issue in a better > > way since it doesn't reduce PSR residency and also > > allow us to reduce the delay time between re-enables at least > > on VLV/CHV. > > > > This is also important to make the sysfs toggle working properly. > > > > Signed-off-by: Rodrigo Vivi> > --- > > drivers/gpu/drm/i915/intel_psr.c | 18 -- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 213581c..6b24c24 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -427,6 +427,19 @@ void intel_psr_enable(struct intel_dp > > *intel_dp) > > vlv_psr_enable_source(intel_dp); > > } > > > > + /* > > +* FIXME: Activation should happen immediately since this > > function > > +* is just called after pipe is fully trained and enabled. > > +* However on every platform we face issues when first > > activation > > +* follows a modeset so quickly. > > +* - On VLV/CHV we get bank screen on first activation > > +* - On HSW/BDW we get a recoverable frozen screen > > until next > > +* exit-activate sequence. > > +*/ > > + if (INTEL_INFO(dev)->gen < 9) > > + schedule_delayed_work(_priv->psr.work, > > + msecs_to_jiffies(intel_dp > > ->panel_power_cycle_delay * 5)); > > + > > dev_priv->psr.enabled = intel_dp; > > unlock: > > mutex_unlock(_priv->psr.lock); > > @@ -735,8 +748,9 @@ void intel_psr_flush(struct drm_device *dev, > > } > > > > if (!dev_priv->psr.active && !dev_priv > > ->psr.busy_frontbuffer_bits) > > - schedule_delayed_work(_priv->psr.work, > > - msecs_to_jiffies(delay_ms)); > > + if (!work_busy(_priv->psr.work.work)) > > + schedule_delayed_work(_priv->psr.work, > > + > > msecs_to_jiffies(delay_ms)); > > Agree with the theory of the patch as such.. But, Is there any > specific reason for > the !work_busy() check here ? > > I believe when the later work runs, it will anyway bail out in > _activate > function, if it sees PSR_ENABLE bit set already. So, is this check > just to > prevent scheduling one more work item when there is one pending > already ? (or it helps in something else also ?) The !work_busy is to prevent that eventual _activate call reduce the first activation time. for instance: 0s - we enable and schedule first activation to 2.5s 1s - we got a page flip that flushed fb tracking and called psr_activation to 0.1s 1.1s - psr is activated while we want 0s - we enable and schedule first activation to 2.5s 1s - we got a page flip that flushed fb tracking and called psr_activation to 0.1s # just ignore and move ahead since we are going to activate it soon. 2.5s - psr is activated I'm open to hear ideas to make it better or more clear. > > Thanks, > Durga Thank you very much for all the reviews! > > > mutex_unlock(_priv->psr.lock); > > } > > > > -- > > 2.4.3 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 8/8] drm/i915: Give meaningful names to all the planes
From: Ville SyrjäläLet's name our planes in a way that makes sense wrt. the spec: - skl+ -> "plane 1A", "plane 2A", "plane 1C", "cursor A" etc. - g4x+ -> "primary A", "primary B", "sprite A", "cursor C" etc. - pre-g4x -> "plane A", "cursor B" etc. v2: Rebase on top of the fixed/cleaned error paths Use a local 'name' variable to make things easier Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 30 ++ drivers/gpu/drm/i915/intel_sprite.c | 14 ++ 2 files changed, 44 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bb49d7d..15b133e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13787,10 +13787,18 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc, */ void intel_plane_destroy(struct drm_plane *plane) { + char *name; + if (!plane) return; + /* +* drm_plane_cleanup() zeroes the structure, so +* need an extra dance to avoid leaking the name. +*/ + name = plane->name; drm_plane_cleanup(plane); + kfree(name); kfree(to_intel_plane(plane)); } @@ -13811,6 +13819,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, { struct intel_plane *primary = NULL; struct intel_plane_state *state = NULL; + char *name = NULL; const uint32_t *intel_primary_formats; unsigned int num_formats; int ret; @@ -13839,6 +13848,19 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) primary->plane = !pipe; + if (INTEL_INFO(dev)->gen >= 9) + name = kasprintf(GFP_KERNEL, "plane 1%c", +pipe_name(pipe)); + else if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) + name = kasprintf(GFP_KERNEL, "primary %c", +pipe_name(pipe)); + else + name = kasprintf(GFP_KERNEL, "plane %c", +plane_name(primary->plane)); + if (!name) + goto fail; + primary->base.name = name; + if (INTEL_INFO(dev)->gen >= 9) { intel_primary_formats = skl_primary_formats; num_formats = ARRAY_SIZE(skl_primary_formats); @@ -13865,6 +13887,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, return >base; fail: + kfree(name); kfree(state); kfree(primary); @@ -13975,6 +13998,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, { struct intel_plane *cursor = NULL; struct intel_plane_state *state = NULL; + char *name = NULL; int ret; cursor = kzalloc(sizeof(*cursor), GFP_KERNEL); @@ -13995,6 +14019,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, cursor->commit_plane = intel_commit_cursor_plane; cursor->disable_plane = intel_disable_cursor_plane; + name = kasprintf(GFP_KERNEL, "cursor %c", pipe_name(pipe)); + if (!name) + goto fail; + cursor->base.name = name; + ret = drm_universal_plane_init(dev, >base, 0, _plane_funcs, intel_cursor_formats, @@ -14023,6 +14052,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, return >base; fail: + kfree(name); kfree(state); kfree(cursor); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 44e1e39..50e53ee 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1040,6 +1040,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) { struct intel_plane *intel_plane = NULL; struct intel_plane_state *state = NULL; + char *name = NULL; unsigned long possible_crtcs; const uint32_t *plane_formats; int num_plane_formats; @@ -1123,6 +1124,18 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) intel_plane->check_plane = intel_check_sprite_plane; intel_plane->commit_plane = intel_commit_sprite_plane; + if (INTEL_INFO(dev)->gen >= 9) + name = kasprintf(GFP_KERNEL, "plane %d%c", +plane + 2, pipe_name(pipe)); + else + name = kasprintf(GFP_KERNEL, "sprite %c", +sprite_name(pipe, plane)); + if (!name) { + ret = -ENOMEM; + goto fail; + } + intel_plane->base.name = name; + possible_crtcs = (1 << pipe); ret =
[Intel-gfx] [PATCH v2 5/8] drm/i915: Set crtc->name to "pipe A", "pipe B", etc.
From: Ville Syrjäläv2: Fix intel_crtc leak on failure to allocate the name Use a local 'name' variable to make things easier Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 21 + 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b628dab..972e17f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10711,6 +10711,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_device *dev = crtc->dev; struct intel_unpin_work *work; + char *name; spin_lock_irq(>event_lock); work = intel_crtc->unpin_work; @@ -10722,8 +10723,13 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) kfree(work); } + /* +* drm_crtc_cleanup() zeroes the structure, so +* need an extra dance to avoid leaking the name. +*/ + name = crtc->name; drm_crtc_cleanup(crtc); - + kfree(name); kfree(intel_crtc); } @@ -14026,15 +14032,16 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr static void intel_crtc_init(struct drm_device *dev, int pipe) { struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *intel_crtc; + struct intel_crtc *intel_crtc = NULL; struct intel_crtc_state *crtc_state = NULL; struct drm_plane *primary = NULL; struct drm_plane *cursor = NULL; + char *name = NULL; int i, ret; intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL); - if (intel_crtc == NULL) - return; + if (!intel_crtc) + goto fail; crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL); if (!crtc_state) @@ -14043,6 +14050,11 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) intel_crtc->base.state = _state->base; crtc_state->base.crtc = _crtc->base; + name = kasprintf(GFP_KERNEL, "pipe %c", pipe_name(pipe)); + if (!name) + goto fail; + intel_crtc->base.name = name; + /* initialize shared scalers */ if (INTEL_INFO(dev)->gen >= 9) { if (pipe == PIPE_C) @@ -14105,6 +14117,7 @@ fail: drm_plane_cleanup(primary); if (cursor) drm_plane_cleanup(cursor); + kfree(name); kfree(crtc_state); kfree(intel_crtc); } -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 0/8] drm: Give crtcs and planes actual names (v2)
From: Ville SyrjäläOK, so another attempt. This time the error handling and cleanup should be more solid. Previus attempt was at http://lists.freedesktop.org/archives/dri-devel/2015-November/094331.html I pushed v2 to: git://github.com/vsyrjala/linux.git crtc_plane_name_2 Ville Syrjälä (8): drm: Add crtc->name and use it in debug messages drm: Add plane->name and use it in debug prints drm/i915: Use crtc->name in debug messages drm/i915: Use plane->name in debug prints drm/i915: Set crtc->name to "pipe A", "pipe B", etc. drm/i915: Fix plane init failure paths drm/i915: Don't leak primary/cursor planes on crtc init failure drm/i915: Give meaningful names to all the planes drivers/gpu/drm/drm_atomic.c | 53 - drivers/gpu/drm/drm_atomic_helper.c | 60 +- drivers/gpu/drm/drm_crtc.c | 11 +- drivers/gpu/drm/drm_crtc_helper.c| 24 ++-- drivers/gpu/drm/i915/intel_display.c | 218 +++ drivers/gpu/drm/i915/intel_fbdev.c | 5 +- drivers/gpu/drm/i915/intel_sprite.c | 48 +--- include/drm/drm_crtc.h | 4 + 8 files changed, 265 insertions(+), 158 deletions(-) -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/8] drm/i915: Use crtc->name in debug messages
From: Ville SyrjäläSigned-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 55 drivers/gpu/drm/i915/intel_fbdev.c | 5 ++-- 2 files changed, 33 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b5f7493..9845687 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4220,8 +4220,9 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc, i = (enum intel_dpll_id) crtc->pipe; pll = _priv->shared_dplls[i]; - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", - crtc->base.base.id, pll->name); + DRM_DEBUG_KMS("[CRTC:%d:%s] using pre-allocated %s\n", + crtc->base.base.id, crtc->base.name, + pll->name); WARN_ON(shared_dpll[i].crtc_mask); @@ -4241,8 +4242,9 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc, /* 1:1 mapping between ports and PLLs */ i = (enum intel_dpll_id)intel_dig_port->port; pll = _priv->shared_dplls[i]; - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", - crtc->base.base.id, pll->name); + DRM_DEBUG_KMS("[CRTC:%d:%s] using pre-allocated %s\n", + crtc->base.base.id, crtc->base.name, + pll->name); WARN_ON(shared_dpll[i].crtc_mask); goto found; @@ -4258,9 +4260,9 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc, if (memcmp(_state->dpll_hw_state, _dpll[i].hw_state, sizeof(crtc_state->dpll_hw_state)) == 0) { - DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n", - crtc->base.base.id, pll->name, - shared_dpll[i].crtc_mask, + DRM_DEBUG_KMS("[CRTC:%d:%s] sharing existing %s (crtc mask 0x%08x, ative %d)\n", + crtc->base.base.id, crtc->base.name, + pll->name, shared_dpll[i].crtc_mask, pll->active); goto found; } @@ -4270,8 +4272,9 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc, for (i = 0; i < dev_priv->num_shared_dpll; i++) { pll = _priv->shared_dplls[i]; if (shared_dpll[i].crtc_mask == 0) { - DRM_DEBUG_KMS("CRTC:%d allocated %s\n", - crtc->base.base.id, pll->name); + DRM_DEBUG_KMS("[CRTC:%d:%s] allocated %s\n", + crtc->base.base.id, crtc->base.name, + pll->name); goto found; } } @@ -4398,8 +4401,9 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state) struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc); const struct drm_display_mode *adjusted_mode = >base.adjusted_mode; - DRM_DEBUG_KMS("Updating scaler for [CRTC:%i] scaler_user index %u.%u\n", - intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX); + DRM_DEBUG_KMS("Updating scaler for [CRTC:%d:%s] scaler_user index %u.%u\n", + intel_crtc->base.base.id, intel_crtc->base.name, + intel_crtc->pipe, SKL_CRTC_INDEX); return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX, >scaler_state.scaler_id, DRM_ROTATE_0, @@ -11643,13 +11647,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, struct drm_i915_private *dev_priv = dev->dev_private; struct intel_plane_state *old_plane_state = to_intel_plane_state(plane->state); - int idx = intel_crtc->base.base.id, ret; int i = drm_plane_index(plane); bool mode_changed = needs_modeset(crtc_state); bool was_crtc_enabled = crtc->state->active; bool is_crtc_enabled = crtc_state->active; bool turn_off, turn_on, visible, was_visible; struct drm_framebuffer *fb = plane_state->fb; + int ret; if (crtc_state && INTEL_INFO(dev)->gen >= 9 && plane->type != DRM_PLANE_TYPE_CURSOR) { @@ -11675,7 +11679,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, turn_off = was_visible && (!visible || mode_changed); turn_on = visible && (!was_visible || mode_changed); - DRM_DEBUG_ATOMIC("[CRTC:%i] has [PLANE:%i] with fb %i\n", idx, +
Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference
On Thu, 2015-11-12 at 20:41 +, Chris Wilson wrote: > On Thu, Nov 12, 2015 at 07:50:09PM +0200, Imre Deak wrote: > > On to, 2015-11-12 at 17:04 +, Chris Wilson wrote: > > > As it stands since we don't actually cancel the hangcheck when we > > drop > > > the rpm wakelock in intel_mark_idle() it can very well come to pass > > > that > > > we execute this whilst the device is asleep. However, if the device > > > is > > > alseep, we now that we are no longer executing. > > > > But how could this run while asleep, since we flush the work in the > > runtime suspend handler before turning off the HW? But even if it can't > > run your idea would be clearer imo.. > > We shouldn't be flushing the hangcheck worker there. That's a leaky > abstraction attempting to paper over something that shouldn't have been > a problem in the first place. Note that there is a similar issue with > the existing request waiters that currently may race against > intel_mark_idle(). Ok. I think your idea is an improvement, but it will change functionality, so how about doing it as a follow-up? --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 7/8] drm/i915: Don't leak primary/cursor planes on crtc init failure
From: Ville SyrjäläCall intel_plane_destroy() instead of drm_plane_cleanup() so that we also free the plane struct itself when bailing out of the crtc init. And make intel_plane_destroy() NULL tolerant to avoid having to check for it in the caller. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 752283c..bb49d7d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13787,9 +13787,11 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc, */ void intel_plane_destroy(struct drm_plane *plane) { - struct intel_plane *intel_plane = to_intel_plane(plane); + if (!plane) + return; + drm_plane_cleanup(plane); - kfree(intel_plane); + kfree(to_intel_plane(plane)); } const struct drm_plane_funcs intel_plane_funcs = { @@ -14127,10 +14129,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) return; fail: - if (primary) - drm_plane_cleanup(primary); - if (cursor) - drm_plane_cleanup(cursor); + intel_plane_destroy(primary); + intel_plane_destroy(cursor); kfree(name); kfree(crtc_state); kfree(intel_crtc); -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/8] drm: Add crtc->name and use it in debug messages
From: Ville SyrjäläSigned-off-by: Ville Syrjälä --- drivers/gpu/drm/drm_atomic.c| 41 ++- drivers/gpu/drm/drm_atomic_helper.c | 56 +++-- drivers/gpu/drm/drm_crtc.c | 8 -- drivers/gpu/drm/drm_crtc_helper.c | 24 include/drm/drm_crtc.h | 2 ++ 5 files changed, 71 insertions(+), 60 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7bb3845..2944655 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -288,8 +288,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, state->crtcs[index] = crtc; crtc_state->state = state; - DRM_DEBUG_ATOMIC("Added [CRTC:%d] %p state to %p\n", -crtc->base.id, crtc_state, state); + DRM_DEBUG_ATOMIC("Added [CRTC:%d:%s] %p state to %p\n", +crtc->base.id, crtc->name, crtc_state, state); return crtc_state; } @@ -480,8 +480,8 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, */ if (state->active && !state->enable) { - DRM_DEBUG_ATOMIC("[CRTC:%d] active without enabled\n", -crtc->base.id); + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active without enabled\n", +crtc->base.id, crtc->name); return -EINVAL; } @@ -490,15 +490,15 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, * be able to trigger. */ if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) && WARN_ON(state->enable && !state->mode_blob)) { - DRM_DEBUG_ATOMIC("[CRTC:%d] enabled without mode blob\n", -crtc->base.id); + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled without mode blob\n", +crtc->base.id, crtc->name); return -EINVAL; } if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) && WARN_ON(!state->enable && state->mode_blob)) { - DRM_DEBUG_ATOMIC("[CRTC:%d] disabled with mode blob\n", -crtc->base.id); + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] disabled with mode blob\n", +crtc->base.id, crtc->name); return -EINVAL; } @@ -980,8 +980,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state, } if (crtc) - DRM_DEBUG_ATOMIC("Link plane state %p to [CRTC:%d]\n", -plane_state, crtc->base.id); + DRM_DEBUG_ATOMIC("Link plane state %p to [CRTC:%d:%s]\n", +plane_state, crtc->base.id, crtc->name); else DRM_DEBUG_ATOMIC("Link plane state %p to [NOCRTC]\n", plane_state); @@ -1048,8 +1048,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, conn_state->crtc = crtc; if (crtc) - DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d]\n", -conn_state, crtc->base.id); + DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n", +conn_state, crtc->base.id, crtc->name); else DRM_DEBUG_ATOMIC("Link connector state %p to [NOCRTC]\n", conn_state); @@ -1088,8 +1088,8 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, if (ret) return ret; - DRM_DEBUG_ATOMIC("Adding all current connectors for [CRTC:%d] to %p\n", -crtc->base.id, state); + DRM_DEBUG_ATOMIC("Adding all current connectors for [CRTC:%d:%s] to %p\n", +crtc->base.id, crtc->name, state); /* * Changed connectors are already in @state, so only need to look at the @@ -1169,8 +1169,9 @@ drm_atomic_connectors_for_crtc(struct drm_atomic_state *state, num_connected_connectors++; } - DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d]\n", -state, num_connected_connectors, crtc->base.id); + DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d:%s]\n", +state, num_connected_connectors, +crtc->base.id, crtc->name); return num_connected_connectors; } @@ -1237,8 +1238,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state) for_each_crtc_in_state(state, crtc, crtc_state, i) { ret = drm_atomic_crtc_check(crtc, crtc_state); if (ret) { - DRM_DEBUG_ATOMIC("[CRTC:%d] atomic core check failed\n", -crtc->base.id); +
[Intel-gfx] [PATCH 6/8] drm/i915: Fix plane init failure paths
From: Ville SyrjäläDeal with errors from drm_universal_plane_init() in primary and cursor plane init paths (sprites were already covered). Also make the code neater by using goto for error handling. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 64 ++-- drivers/gpu/drm/i915/intel_sprite.c | 34 +++ 2 files changed, 60 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 972e17f..752283c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13807,20 +13807,19 @@ const struct drm_plane_funcs intel_plane_funcs = { static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, int pipe) { - struct intel_plane *primary; - struct intel_plane_state *state; + struct intel_plane *primary = NULL; + struct intel_plane_state *state = NULL; const uint32_t *intel_primary_formats; unsigned int num_formats; + int ret; primary = kzalloc(sizeof(*primary), GFP_KERNEL); - if (primary == NULL) - return NULL; + if (!primary) + goto fail; state = intel_create_plane_state(>base); - if (!state) { - kfree(primary); - return NULL; - } + if (!state) + goto fail; primary->base.state = >base; primary->can_scale = false; @@ -13849,10 +13848,12 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, num_formats = ARRAY_SIZE(i8xx_primary_formats); } - drm_universal_plane_init(dev, >base, 0, -_plane_funcs, -intel_primary_formats, num_formats, -DRM_PLANE_TYPE_PRIMARY); + ret = drm_universal_plane_init(dev, >base, 0, + _plane_funcs, + intel_primary_formats, num_formats, + DRM_PLANE_TYPE_PRIMARY); + if (ret) + goto fail; if (INTEL_INFO(dev)->gen >= 4) intel_create_rotation_property(dev, primary); @@ -13860,6 +13861,12 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, drm_plane_helper_add(>base, _plane_helper_funcs); return >base; + +fail: + kfree(state); + kfree(primary); + + return NULL; } void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane) @@ -13964,18 +13971,17 @@ update: static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, int pipe) { - struct intel_plane *cursor; - struct intel_plane_state *state; + struct intel_plane *cursor = NULL; + struct intel_plane_state *state = NULL; + int ret; cursor = kzalloc(sizeof(*cursor), GFP_KERNEL); - if (cursor == NULL) - return NULL; + if (!cursor) + goto fail; state = intel_create_plane_state(>base); - if (!state) { - kfree(cursor); - return NULL; - } + if (!state) + goto fail; cursor->base.state = >base; cursor->can_scale = false; @@ -13987,11 +13993,13 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, cursor->commit_plane = intel_commit_cursor_plane; cursor->disable_plane = intel_disable_cursor_plane; - drm_universal_plane_init(dev, >base, 0, -_plane_funcs, -intel_cursor_formats, -ARRAY_SIZE(intel_cursor_formats), -DRM_PLANE_TYPE_CURSOR); + ret = drm_universal_plane_init(dev, >base, 0, + _plane_funcs, + intel_cursor_formats, + ARRAY_SIZE(intel_cursor_formats), + DRM_PLANE_TYPE_CURSOR); + if (ret) + goto fail; if (INTEL_INFO(dev)->gen >= 4) { if (!dev->mode_config.rotation_property) @@ -14011,6 +14019,12 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, drm_plane_helper_add(>base, _plane_helper_funcs); return >base; + +fail: + kfree(state); + kfree(cursor); + + return NULL; } static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc, diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index a2c15f8..44e1e39 100644 ---
[Intel-gfx] [PATCH 2/8] drm: Add plane->name and use it in debug prints
From: Ville SyrjäläSigned-off-by: Ville Syrjälä --- drivers/gpu/drm/drm_atomic.c| 12 ++-- drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- drivers/gpu/drm/drm_crtc.c | 3 +++ include/drm/drm_crtc.h | 2 ++ 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 2944655..df84060 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -543,8 +543,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, state->planes[index] = plane; plane_state->state = state; - DRM_DEBUG_ATOMIC("Added [PLANE:%d] %p state to %p\n", -plane->base.id, plane_state, state); + DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", +plane->base.id, plane->name, plane_state, state); if (plane_state->crtc) { struct drm_crtc_state *crtc_state; @@ -755,8 +755,8 @@ static int drm_atomic_plane_check(struct drm_plane *plane, } if (plane_switching_crtc(state->state, plane, state)) { - DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n", -plane->base.id); + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", +plane->base.id, plane->name); return -EINVAL; } @@ -1229,8 +1229,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state) for_each_plane_in_state(state, plane, plane_state, i) { ret = drm_atomic_plane_check(plane, plane_state); if (ret) { - DRM_DEBUG_ATOMIC("[PLANE:%d] atomic core check failed\n", -plane->base.id); + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n", +plane->base.id, plane->name); return ret; } } diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fc90af50..387d95c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -503,8 +503,8 @@ drm_atomic_helper_check_planes(struct drm_device *dev, ret = funcs->atomic_check(plane, plane_state); if (ret) { - DRM_DEBUG_ATOMIC("[PLANE:%d] atomic driver check failed\n", -plane->base.id); + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic driver check failed\n", +plane->base.id, plane->name); return ret; } } diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ea00a69..8dc4052 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1174,6 +1174,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, drm_modeset_lock_init(>mutex); + if (!plane->name) + plane->name = ""; + plane->base.properties = >properties; plane->dev = dev; plane->funcs = funcs; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index a8279b4..a08e256 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -848,6 +848,8 @@ struct drm_plane { struct drm_device *dev; struct list_head head; + char *name; + struct drm_modeset_lock mutex; struct drm_mode_object base; -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/8] drm/i915: Use plane->name in debug prints
From: Ville SyrjäläSigned-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 38 +--- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9845687..b628dab 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4433,9 +4433,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, bool force_detach = !fb || !plane_state->visible; - DRM_DEBUG_KMS("Updating scaler for [PLANE:%d] scaler_user index %u.%u\n", - intel_plane->base.base.id, intel_crtc->pipe, - drm_plane_index(_plane->base)); + DRM_DEBUG_KMS("Updating scaler for [PLANE:%d:%s] scaler_user index %u.%u\n", + intel_plane->base.base.id, intel_plane->base.name, + intel_crtc->pipe, drm_plane_index(_plane->base)); ret = skl_update_scaler(crtc_state, force_detach, drm_plane_index(_plane->base), @@ -4451,8 +4451,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, /* check colorkey */ if (plane_state->ckey.flags != I915_SET_COLORKEY_NONE) { - DRM_DEBUG_KMS("[PLANE:%d] scaling with color key not allowed", - intel_plane->base.base.id); + DRM_DEBUG_KMS("[PLANE:%d:%s] scaling with color key not allowed", + intel_plane->base.base.id, + intel_plane->base.name); return -EINVAL; } @@ -4471,8 +4472,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, case DRM_FORMAT_VYUY: break; default: - DRM_DEBUG_KMS("[PLANE:%d] FB:%d unsupported scaling format 0x%x\n", - intel_plane->base.base.id, fb->base.id, fb->pixel_format); + DRM_DEBUG_KMS("[PLANE:%d:%s] FB:%d unsupported scaling format 0x%x\n", + intel_plane->base.base.id, intel_plane->base.name, + fb->base.id, fb->pixel_format); return -EINVAL; } @@ -11679,13 +11681,15 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, turn_off = was_visible && (!visible || mode_changed); turn_on = visible && (!was_visible || mode_changed); - DRM_DEBUG_ATOMIC("[CRTC:%d:%s] has [PLANE:%i] with fb %i\n", + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] has [PLANE:%d:%s] with fb %i\n", intel_crtc->base.base.id, intel_crtc->base.name, -plane->base.id, fb ? fb->base.id : -1); +plane->base.id, plane->name, +fb ? fb->base.id : -1); - DRM_DEBUG_ATOMIC("[PLANE:%i] visible %i -> %i, off %i, on %i, ms %i\n", -plane->base.id, was_visible, visible, + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] visible %i -> %i, off %i, on %i, ms %i\n", +plane->base.id, plane->name, +was_visible, visible, turn_off, turn_on, mode_changed); if (turn_on) { @@ -12088,18 +12092,20 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, state = to_intel_plane_state(plane->state); fb = state->base.fb; if (!fb) { - DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d " - "disabled, scaler_id = %d\n", + DRM_DEBUG_KMS("%s [PLANE:%d:%s] plane: %u.%u idx: %d " + "disabled, scaler_id = %d\n", plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD", - plane->base.id, intel_plane->pipe, + plane->base.id, plane->name, + intel_plane->pipe, (crtc->base.primary == plane) ? 0 : intel_plane->plane + 1, drm_plane_index(plane), state->scaler_id); continue; } - DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled", + DRM_DEBUG_KMS("%s [PLANE:%d:%s] plane: %u.%u idx: %d enabled", plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD", - plane->base.id, intel_plane->pipe, + plane->base.id, plane->name, + intel_plane->pipe, crtc->base.primary == plane ? 0 : intel_plane->plane + 1, drm_plane_index(plane)); DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x", -- 2.4.10
Re: [Intel-gfx] [PATCH v6] drm/i915: respect the VBT minimum backlight brightness again
Hi, I have seen you were working on brightness. Right, but this is useful only if many worlwide users would be able to activate it. I have never been able to make it working on my intel systemes (core i5 - 4300u , core i3 ...) Very difficult to check where the problem is coming from, altough you have either an intel_backlight or acpi_backlight folder in the sys directory Regarding many many posts on the web, this sounds tricky and difficult to know why it has not worked, or simply the documentation is lacking Regards, S.Ancelot On 12/11/2015 06:43, Shih-Yuan Lee (FourDollars) wrote: There was a wonderful period after commit 6dda730e55f412a6dfb181cae6784822ba463847 Author: Jani Nikula Date: Tue Jun 24 18:27:40 2014 +0300 drm/i915: respect the VBT minimum backlight brightness The backlight class 0 brightness means the PWM min and it does not turn off the backlight. After kernel 3.18, the backlight class 0 brightness is used to turn off the backlight and the PWM min is missing. Because of commit e6755fb78e8f20ecadf2a4080084121336624ad9 Author: Jani Nikula Date: Tue Aug 12 17:11:42 2014 +0300 drm/i915: switch off backlight for backlight class 0 brightness Use "VBT backlight PWM modulation frequency 200 Hz, active high, min brightness 10, level 255" as an example. It means the VBT min is 10 out of [0..255] and the PWM max is 937 from other place so the corresponding PWM min should be 37 (10 * 937 / 255). When we set backlight class 0 brightness, the backlight is turned off. Althought the PWM value is 37 when the backlight is off but it is useless. When we set set backlight class 1 brightness, the backlight is on but the PWM value is 38 and it doesn't match the corresponding PWM min of the VBT minimum backlight. And it has another minor issue that there are some backlight class brightness values are mapped to the same PWM value because the backlight class brightness range is larger than the valid PWM brightness range. The backlight class brightness range [0..937] The valid PWM brightness range [37..937] This commit makes that backlight class 1 brightness means the PWM min and backlight class max_brightness means the PWM max, and the ranges become The backlight class brightness range [0..901] The valid PWM brightness range [36..937] That's no backlight class brightness value mapped to the same PWM value. Signed-off-by: Shih-Yuan Lee (FourDollars)--- drivers/gpu/drm/i915/intel_panel.c | 26 ++ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index b05c6d9..8efa199 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1194,10 +1194,9 @@ static int intel_backlight_device_register(struct intel_connector *connector) props.type = BACKLIGHT_RAW; /* -* Note: Everything should work even if the backlight device max -* presented to the userspace is arbitrarily chosen. +* Expose the whole valid PWM brightness range to the backlight class. */ - props.max_brightness = panel->backlight.max; + props.max_brightness = panel->backlight.max - panel->backlight.min; props.brightness = scale_hw_to_user(connector, panel->backlight.level, props.max_brightness); @@ -1400,7 +1399,8 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector) struct drm_device *dev = connector->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_panel *panel = >panel; - int min; + int vbt_min; + u32 pwm_min; WARN_ON(panel->backlight.max == 0); @@ -1411,14 +1411,24 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector) * against this by letting the minimum be at most (arbitrarily chosen) * 25% of the max. */ - min = clamp_t(int, dev_priv->vbt.backlight.min_brightness, 0, 64); - if (min != dev_priv->vbt.backlight.min_brightness) { + vbt_min = clamp_t(int, dev_priv->vbt.backlight.min_brightness, 0, 64); + if (vbt_min != dev_priv->vbt.backlight.min_brightness) { DRM_DEBUG_KMS("clamping VBT min backlight %d/255 to %d/255\n", - dev_priv->vbt.backlight.min_brightness, min); + dev_priv->vbt.backlight.min_brightness, vbt_min); } /* vbt value is a coefficient in range [0..255] */ - return scale(min, 0, 255, 0, panel->backlight.max); + pwm_min = scale(vbt_min, 0, 255, 0, panel->backlight.max); + + /* +* Because backlight class brightness 0 is used to turn off the backlight, we +* need to step down a little bit here to make backlight class brightness 1 +* match the real PWM min. +*/ + if (pwm_min > 0) +
Re: [Intel-gfx] [PATCH i-g-t] igt/pm_rps: current freq < user specified min is no longer a fail
On ke, 2015-11-11 at 13:37 -0800, Bob Paauwe wrote: > Since commit > > commit aed242ff7ebb697e4dff912bd4dc7ec7192f7581 > Author: Chris Wilson> Date: Wed Mar 18 09:48:21 2015 + > > drm/i915: Relax RPS contraints to allows setting minfreq on > idle > > it is now possible that the current frequency will drop below the > user > specified minimum frequency. Update the pm_rps tests to reflect that > this is no longer considered a failure. Yes, in case the GPU is idle the frequency will drop now to the idle fr equency regardless of the minimum limit set by the user. We should still check though if the frequency drops to the idle value not something higher. You can use the i915_frequency_info in debugfs for that. > > Signed-off-by: Bob Paauwe > --- > tests/pm_rps.c | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > index 74f08f4..e92ca3b 100644 > --- a/tests/pm_rps.c > +++ b/tests/pm_rps.c > @@ -146,7 +146,6 @@ static void checkit(const int *freqs) > { > igt_assert_lte(freqs[MIN], freqs[MAX]); > igt_assert_lte(freqs[CUR], freqs[MAX]); > - igt_assert_lte(freqs[MIN], freqs[CUR]); > igt_assert_lte(freqs[RPn], freqs[MIN]); > igt_assert_lte(freqs[MAX], freqs[RP0]); > igt_assert_lte(freqs[RP1], freqs[RP0]); > @@ -472,14 +471,14 @@ static void idle_check(void) > read_freqs(freqs); > dump(freqs); > checkit(freqs); > - if (freqs[CUR] == freqs[MIN]) > + if (freqs[CUR] <= freqs[MIN]) > break; > usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); > wait += IDLE_WAIT_TIMESTEP_MSEC; > } while (wait < IDLE_WAIT_TIMEOUT_MSEC); > > - igt_assert_eq(freqs[CUR], freqs[MIN]); > - igt_debug("Required %d msec to reach cur=min\n", wait); > + igt_assert_lte(freqs[CUR], freqs[MIN]); > + igt_debug("Required %d msec to reach cur<=min\n", wait); > } > > #define LOADED_WAIT_TIMESTEP_MSEC 100 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6] drm/i915: respect the VBT minimum backlight brightness again
On 12/11/2015 09:28, Shih-Yuan Lee (FourDollars) wrote: I think the first step is to identify which backlight interface is able to actually change the brightness. Usually there are two backlight interfaces under /sys/class/backlight/, i.e. /sys/class/backlight/acpi_video0 and /sys/class/backlight/intel_backlight. However you may see other different backlight interfaces under /sys/class/backlight/. It depends on what hardware you are using. Executing `cat max_brightness` under those folders can see the max brightness and then you can execute `echo $(($(cat max_brightness)/2)) | sudo tee brightness` to adjust it to about 50% brightness. only /sys/class/backlight/intel_backlight is present. max_brightness is 937 these commands have no effect on display brightness: echo 0 >brightness echo 468 >brightness echo 937 >brightness Regards, Steph On Thu, Nov 12, 2015 at 4:08 PM, Stéphane ANCELOT> wrote: Hi, I have seen you were working on brightness. Right, but this is useful only if many worlwide users would be able to activate it. I have never been able to make it working on my intel systemes (core i5 - 4300u , core i3 ...) Very difficult to check where the problem is coming from, altough you have either an intel_backlight or acpi_backlight folder in the sys directory Regarding many many posts on the web, this sounds tricky and difficult to know why it has not worked, or simply the documentation is lacking Regards, S.Ancelot On 12/11/2015 06:43, Shih-Yuan Lee (FourDollars) wrote: There was a wonderful period after commit 6dda730e55f412a6dfb181cae6784822ba463847 Author: Jani Nikula http://intel.com>> Date: Tue Jun 24 18:27:40 2014 +0300 drm/i915: respect the VBT minimum backlight brightness The backlight class 0 brightness means the PWM min and it does not turn off the backlight. After kernel 3.18, the backlight class 0 brightness is used to turn off the backlight and the PWM min is missing. Because of commit e6755fb78e8f20ecadf2a4080084121336624ad9 Author: Jani Nikula http://intel.com>> Date: Tue Aug 12 17:11:42 2014 +0300 drm/i915: switch off backlight for backlight class 0 brightness Use "VBT backlight PWM modulation frequency 200 Hz, active high, min brightness 10, level 255" as an example. It means the VBT min is 10 out of [0..255] and the PWM max is 937 from other place so the corresponding PWM min should be 37 (10 * 937 / 255). When we set backlight class 0 brightness, the backlight is turned off. Althought the PWM value is 37 when the backlight is off but it is useless. When we set set backlight class 1 brightness, the backlight is on but the PWM value is 38 and it doesn't match the corresponding PWM min of the VBT minimum backlight. And it has another minor issue that there are some backlight class brightness values are mapped to the same PWM value because the backlight class brightness range is larger than the valid PWM brightness range. The backlight class brightness range [0..937] The valid PWM brightness range [37..937] This commit makes that backlight class 1 brightness means the PWM min and backlight class max_brightness means the PWM max, and the ranges become The backlight class brightness range [0..901] The valid PWM brightness range [36..937] That's no backlight class brightness value mapped to the same PWM value. Signed-off-by: Shih-Yuan Lee (FourDollars) > --- drivers/gpu/drm/i915/intel_panel.c | 26 ++ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index b05c6d9..8efa199 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1194,10 +1194,9 @@ static int intel_backlight_device_register(struct intel_connector *connector) props.type = BACKLIGHT_RAW; /* -* Note: Everything should work even if the backlight device max -* presented to the userspace is arbitrarily chosen. +* Expose the whole valid PWM brightness range to the backlight class. */ - props.max_brightness = panel->backlight.max; + props.max_brightness = panel->backlight.max - panel->backlight.min; props.brightness = scale_hw_to_user(connector,
Re: [Intel-gfx] [PATCH] drm/i915/skl: Implement DP Aux Mutex framework
"aux mutex framework" in the title sounds a bit grandiose, to be honest. "add support for DP AUX hardware mutex" is what I would have used. Further comments inline. On Thu, 12 Nov 2015, Wayne Boyerwrote: > From: "Boyer, Wayne" > > Beginning with SKL the DP Aux channel communication can be protected > using a built in HW mutex. > > When PSR is enabled the HW takes control on AUX and uses it to > control panel exit/entry states. > > When validating PSR with automated tests, grabbing CRC from sink > revealed strange aux communication issues. Aux reads were returning > a message read size equal to 0 and 0 is a forbidden message. > > By using the HW mutex the HW is blocked from using aux when running > the automated PSR tests. > > This patch provides an initial implementation for using that mutex. > The use is currently limited to protecting the sink crc request based > on feedback from the H/W designers indicating that using the mutex > for all aux channel communication is not recommended. > > v2: Improved commit message to explain the case where the HW mutex is > helpful. Also added bug reference. > v3: Fix typos in commit message. > > Signed-off-by: Wayne Boyer > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91437 > Reviewed-by: Rodrigo Vivi > Tested-by: Rodrigo Vivi > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 5 > drivers/gpu/drm/i915/intel_dp.c | 52 > - > 3 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index d5cf30b..ac7ed0d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2585,6 +2585,7 @@ struct drm_i915_cmd_table { > > #define HAS_DDI(dev) (INTEL_INFO(dev)->has_ddi) > #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) > +#define HAS_AUX_MUTEX(dev) (INTEL_INFO(dev)->gen >= 9) > #define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \ >IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev) || \ >IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 8bd2699..f9ee874 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4288,6 +4288,11 @@ enum skl_disp_power_wells { > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5) > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1) > > +#define DP_AUX_MUTEX_A 0x6402C > +#define DP_AUX_MUTEX_B 0x6412C Please add all of these at once, using an underscore prefix, and add DP_AUX_MUTEX(port) to pick the right one. Plenty of examples in the file. And then Ville doesn't have to clean it up afterwards. > +#define DP_AUX_MUTEX_ENABLE(1 << 31) > +#define DP_AUX_MUTEX_STATUS(1 << 30) > + > /* > * Computing GMCH M and N values for the Display Port link > * > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index da02ed7..b3c7d82 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -781,6 +781,47 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp > *intel_dp, > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > } > > +static bool skl_aux_mutex(struct intel_dp *intel_dp, bool get) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + uint32_t aux_ch_mutex, status; > + int count = 0; > + > + if (!HAS_AUX_MUTEX(dev)) > + return false; > + > + /* > + * FIXME: determine actual aux channel > + * Hard coded to channel A for now to protect sink crc requests on eDP. > + */ If you want to leave it like this for now, the please add "|| !is_edp()" alongside !HAS_AUX_MUTEX() for an early return. > + aux_ch_mutex = DP_AUX_MUTEX_A; > + > + if (!get) { > + I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE | > DP_AUX_MUTEX_STATUS); > + return false; > + } > + > + /* > + * The Bspec specifies waiting 500us between attempts to acquire the > + * mutex. Ten retries should be adequate to balance successfully > + * acquirng the mutex and spending too much time trying. > + */ > + while (count++ < 10) { Please use a for loop whenever you can use a for loop. The reader has to pause for a second to ensure "while (count++ < 10)" does the right thing and count is initialized properly; "for (i = 0; i < 10; i++)" comes from the spine. > + I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE); > +
Re: [Intel-gfx] [PATCH 0/5] Sink CRC stabilization
On Wed, 11 Nov 2015, Rodrigo Viviwrote: > Let's start spliting that big series that enables PSR with this sink crc > stabilization. > > Also I'm adding Wayne's mutex that stabilizes sink CRC on Skylake. > > All patches already reviewed and ready to merge. Disagreed on the hw mutex patch. BR, Jani. > > Thank you very much Paulo for the review and Thank you Wayne for the > SKL aux mutex. > > Thanks, > Rodrigo. > > Boyer, Wayne (1): > drm/i915/skl: implement DP Aux Mutex framework > > Rodrigo Vivi (4): > drm/i915: Allow 1 vblank to let Sink CRC calculation to start or stop. > drm/i915: Make Sink crc calculation waiting for counter to reset. > drm/i915: Stop tracking last calculated Sink CRC. > drm/i915: Rely on TEST_SINK_START instead of tracking Sink CRC state > on dev_priv. > > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 5 ++ > drivers/gpu/drm/i915/intel_dp.c | 126 > +++ > drivers/gpu/drm/i915/intel_drv.h | 7 --- > 4 files changed, 93 insertions(+), 46 deletions(-) -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6] drm/i915: respect the VBT minimum backlight brightness again
You may need to put "drm.debug=0xe" into the kernel parameter and reboot the system to collect some system log. Did you check the brightness value after you change it? On Thu, Nov 12, 2015 at 4:40 PM, Stéphane ANCELOTwrote: > On 12/11/2015 09:28, Shih-Yuan Lee (FourDollars) wrote: > > I think the first step is to identify which backlight interface is able to > actually change the brightness. > Usually there are two backlight interfaces under /sys/class/backlight/, > i.e. /sys/class/backlight/acpi_video0 and > /sys/class/backlight/intel_backlight. > However you may see other different backlight interfaces under > /sys/class/backlight/. It depends on what hardware you are using. > Executing `cat max_brightness` under those folders can see the max > brightness and then you can execute `echo $(($(cat max_brightness)/2)) | > sudo tee brightness` to adjust it to about 50% brightness. > > only /sys/class/backlight/intel_backlight is present. > max_brightness is 937 > > these commands have no effect on display brightness: > echo 0 >brightness > echo 468 >brightness > echo 937 >brightness > > Regards, > Steph > > > > On Thu, Nov 12, 2015 at 4:08 PM, Stéphane ANCELOT > wrote: > >> Hi, >> I have seen you were working on brightness. Right, but this is useful >> only if many worlwide users would be able to activate it. >> I have never been able to make it working on my intel systemes (core i5 - >> 4300u , core i3 ...) >> Very difficult to check where the problem is coming from, altough you >> have either an intel_backlight or acpi_backlight folder in the sys directory >> Regarding many many posts on the web, this sounds tricky and difficult to >> know why it has not worked, or simply the documentation is lacking >> >> Regards, >> S.Ancelot >> >> >> On 12/11/2015 06:43, Shih-Yuan Lee (FourDollars) wrote: >> >>> There was a wonderful period after >>> >>> commit 6dda730e55f412a6dfb181cae6784822ba463847 >>> Author: Jani Nikula >>> Date: Tue Jun 24 18:27:40 2014 +0300 >>> >>> drm/i915: respect the VBT minimum backlight brightness >>> >>> The backlight class 0 brightness means the PWM min and it does not turn >>> off the backlight. After kernel 3.18, the backlight class 0 brightness >>> is used to turn off the backlight and the PWM min is missing. >>> >>> Because of >>> >>> commit e6755fb78e8f20ecadf2a4080084121336624ad9 >>> Author: Jani Nikula >>> Date: Tue Aug 12 17:11:42 2014 +0300 >>> >>> drm/i915: switch off backlight for backlight class 0 brightness >>> >>> Use "VBT backlight PWM modulation frequency 200 Hz, active high, min >>> brightness 10, level 255" as an example. It means the VBT min is 10 >>> out of [0..255] and the PWM max is 937 from other place so the >>> corresponding PWM min should be 37 (10 * 937 / 255). >>> >>> When we set backlight class 0 brightness, the backlight is turned off. >>> Althought the PWM value is 37 when the backlight is off but it is >>> useless. When we set set backlight class 1 brightness, the backlight is >>> on but the PWM value is 38 and it doesn't match the corresponding PWM >>> min of the VBT minimum backlight. >>> >>> And it has another minor issue that there are some backlight class >>> brightness values are mapped to the same PWM value because the backlight >>> class brightness range is larger than the valid PWM brightness range. >>> >>> The backlight class brightness range [0..937] >>> The valid PWM brightness range [37..937] >>> >>> This commit makes that backlight class 1 brightness means the PWM min >>> and backlight class max_brightness means the PWM max, and the ranges >>> become >>> >>> The backlight class brightness range [0..901] >>> The valid PWM brightness range [36..937] >>> >>> That's no backlight class brightness value mapped to the same PWM value. >>> >>> Signed-off-by: Shih-Yuan Lee (FourDollars) >>> --- >>> drivers/gpu/drm/i915/intel_panel.c | 26 ++ >>> 1 file changed, 18 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_panel.c >>> b/drivers/gpu/drm/i915/intel_panel.c >>> index b05c6d9..8efa199 100644 >>> --- a/drivers/gpu/drm/i915/intel_panel.c >>> +++ b/drivers/gpu/drm/i915/intel_panel.c >>> @@ -1194,10 +1194,9 @@ static int intel_backlight_device_register(struct >>> intel_connector *connector) >>> props.type = BACKLIGHT_RAW; >>> /* >>> -* Note: Everything should work even if the backlight device max >>> -* presented to the userspace is arbitrarily chosen. >>> +* Expose the whole valid PWM brightness range to the backlight >>> class. >>> */ >>> - props.max_brightness = panel->backlight.max; >>> + props.max_brightness = panel->backlight.max - >>> panel->backlight.min; >>> props.brightness = scale_hw_to_user(connector, >>> panel->backlight.level, >>>
Re: [Intel-gfx] [PATCH 03/12] drm/i915: Clean up AUX power domain handling
On Wed, Nov 11, 2015 at 08:37:36PM +0200, Ville Syrjälä wrote: > On Wed, Nov 11, 2015 at 08:22:03PM +0200, Imre Deak wrote: > > On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote: > > > From: Ville Syrjälä> > > > > > Introduce intel_display_port_aux_power_domain() which simply returns > > > the appropriate AUX power domain for a specific port, and then > > > replace > > > the intel_display_port_power_domain() with calls to the new function > > > in the DP code. As long as we're not actually enabling the port we > > > don't > > > need the lane power domains, and those are handled now purely from > > > modeset_update_crtc_power_domains(). > > > > > > My initial motivation for this was to see if I could keep the DPIO > > > power > > > wells powered down while doing AUX on CHV, but turns out I can't so > > > this > > > doesn't change anything for CHV at least. But I think it's still a > > > worthwile change. > > > > > > Signed-off-by: Ville Syrjälä > > > Reviewed-by: Patrik Jakobsson > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 40 > > > ++ > > > drivers/gpu/drm/i915/intel_dp.c | 48 +++--- > > > -- > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > 3 files changed, 56 insertions(+), 34 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index d0fec07..c2578d9 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -5143,6 +5143,23 @@ static enum intel_display_power_domain > > > port_to_power_domain(enum port port) > > > } > > > } > > > > > > +static enum intel_display_power_domain port_to_aux_power_domain(enum > > > port port) > > > +{ > > > + switch (port) { > > > + case PORT_A: > > > + return POWER_DOMAIN_AUX_A; > > > + case PORT_B: > > > + return POWER_DOMAIN_AUX_B; > > > + case PORT_C: > > > + return POWER_DOMAIN_AUX_C; > > > + case PORT_D: > > > + return POWER_DOMAIN_AUX_D; > > > + default: > > > + WARN_ON_ONCE(1); > > > + return POWER_DOMAIN_AUX_A; > > > + } > > > +} > > > > Looks like port E is missing. I chatted with Ville he has some idea to > > fix this. > > Yeah, so there's no dedicated AUX block for port E, and instead VBT > tells us which AUX block to use. The current code doin that is rather > messy, but I have a cleaned it up during my register type-safety > journey. I just reposted the remaining AUX related patches [1] from > that series. > > So I was thinking that we could include an 'enum port aux_port' inside > intel_dp, and use that to pick the right power domain. > > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079918.html Ok so one of Aux A-D is hardwired to port E and only VBT can tell us which? Do we need to change anything in this patch or can we add the aux_port enum later on? > > > > > > + > > > #define for_each_power_domain(domain, mask) > > > \ > > > for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) > > > \ > > > if ((1 << (domain)) & (mask)) > > > @@ -5174,6 +5191,29 @@ intel_display_port_power_domain(struct > > > intel_encoder *intel_encoder) > > > } > > > } > > > > > > +enum intel_display_power_domain > > > +intel_display_port_aux_power_domain(struct intel_encoder > > > *intel_encoder) > > > +{ > > > + struct drm_device *dev = intel_encoder->base.dev; > > > + struct intel_digital_port *intel_dig_port; > > > + > > > + switch (intel_encoder->type) { > > > + case INTEL_OUTPUT_UNKNOWN: > > > + /* Only DDI platforms should ever use this output > > > type */ > > > + WARN_ON_ONCE(!HAS_DDI(dev)); > > > + case INTEL_OUTPUT_DISPLAYPORT: > > > + case INTEL_OUTPUT_EDP: > > > + intel_dig_port = enc_to_dig_port(_encoder > > > ->base); > > > + return port_to_aux_power_domain(intel_dig_port > > > ->port); > > > + case INTEL_OUTPUT_DP_MST: > > > + intel_dig_port = enc_to_mst(_encoder->base) > > > ->primary; > > > + return port_to_aux_power_domain(intel_dig_port > > > ->port); > > > + default: > > > + WARN_ON_ONCE(1); > > > + return POWER_DOMAIN_AUX_A; > > > + } > > > +} > > > + > > > static unsigned long get_crtc_power_domains(struct drm_crtc *crtc) > > > { > > > struct drm_device *dev = crtc->dev; > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 4655af0..3978540 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -277,7 +277,7 @@ static void pps_lock(struct intel_dp *intel_dp) > > >* See vlv_power_sequencer_reset() why we need > > >* a power domain reference here. > > >*/ > > > - power_domain = intel_display_port_power_domain(encoder); > > > +
Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw
Hey, Gabriel Feceoru schreef op wo 11-11-2015 om 20:27 [+0200]: > > On 11.11.2015 16:21, Jani Nikula wrote: > > On Wed, 11 Nov 2015, Ander Conselvan De Oliveira> > wrote: > >> On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote: > >>> Ander, Maarten, where are we with this? Is it horribly wrong to merge > >>> the original patch in this ever-growing and diverging thread? > >> > >> I think the patch as is will cause problems with DP, since we might clear > >> the > >> pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix > >> disregarding the discussion in this thread is to drop another memset in > >> intel_crt_compute_config(). Like this > > > > > > Ander, please post this as a proper patch for review. > > > > BR, > > Jani. > > Hi, > I tested this patch on my system and I can confirm it fixes the original > issue. > However there are a few memset in *_ddi_pll_select functions which might > not be needed anymore. For instance I tried to remove the hsw one and > didn't see any regression. Could you test http://lists.freedesktop.org/archives/intel-gfx/2015-September/075964.html ? Should be a less duct-tape fix.. ~Maarten - Intel International B.V. Registered in The Netherlands under number 34098535 Statutory seat: Haarlemmermeer Registered address: Capronilaan 37, 1119NG Schiphol-Rijk This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6] drm/i915: respect the VBT minimum backlight brightness again
I think the first step is to identify which backlight interface is able to actually change the brightness. Usually there are two backlight interfaces under /sys/class/backlight/, i.e. /sys/class/backlight/acpi_video0 and /sys/class/backlight/intel_backlight. However you may see other different backlight interfaces under /sys/class/backlight/. It depends on what hardware you are using. Executing `cat max_brightness` under those folders can see the max brightness and then you can execute `echo $(($(cat max_brightness)/2)) | sudo tee brightness` to adjust it to about 50% brightness. On Thu, Nov 12, 2015 at 4:08 PM, Stéphane ANCELOTwrote: > Hi, > I have seen you were working on brightness. Right, but this is useful only > if many worlwide users would be able to activate it. > I have never been able to make it working on my intel systemes (core i5 - > 4300u , core i3 ...) > Very difficult to check where the problem is coming from, altough you have > either an intel_backlight or acpi_backlight folder in the sys directory > Regarding many many posts on the web, this sounds tricky and difficult to > know why it has not worked, or simply the documentation is lacking > > Regards, > S.Ancelot > > > On 12/11/2015 06:43, Shih-Yuan Lee (FourDollars) wrote: > >> There was a wonderful period after >> >> commit 6dda730e55f412a6dfb181cae6784822ba463847 >> Author: Jani Nikula >> Date: Tue Jun 24 18:27:40 2014 +0300 >> >> drm/i915: respect the VBT minimum backlight brightness >> >> The backlight class 0 brightness means the PWM min and it does not turn >> off the backlight. After kernel 3.18, the backlight class 0 brightness >> is used to turn off the backlight and the PWM min is missing. >> >> Because of >> >> commit e6755fb78e8f20ecadf2a4080084121336624ad9 >> Author: Jani Nikula >> Date: Tue Aug 12 17:11:42 2014 +0300 >> >> drm/i915: switch off backlight for backlight class 0 brightness >> >> Use "VBT backlight PWM modulation frequency 200 Hz, active high, min >> brightness 10, level 255" as an example. It means the VBT min is 10 >> out of [0..255] and the PWM max is 937 from other place so the >> corresponding PWM min should be 37 (10 * 937 / 255). >> >> When we set backlight class 0 brightness, the backlight is turned off. >> Althought the PWM value is 37 when the backlight is off but it is >> useless. When we set set backlight class 1 brightness, the backlight is >> on but the PWM value is 38 and it doesn't match the corresponding PWM >> min of the VBT minimum backlight. >> >> And it has another minor issue that there are some backlight class >> brightness values are mapped to the same PWM value because the backlight >> class brightness range is larger than the valid PWM brightness range. >> >> The backlight class brightness range [0..937] >> The valid PWM brightness range [37..937] >> >> This commit makes that backlight class 1 brightness means the PWM min >> and backlight class max_brightness means the PWM max, and the ranges >> become >> >> The backlight class brightness range [0..901] >> The valid PWM brightness range [36..937] >> >> That's no backlight class brightness value mapped to the same PWM value. >> >> Signed-off-by: Shih-Yuan Lee (FourDollars) >> --- >> drivers/gpu/drm/i915/intel_panel.c | 26 ++ >> 1 file changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c >> b/drivers/gpu/drm/i915/intel_panel.c >> index b05c6d9..8efa199 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -1194,10 +1194,9 @@ static int intel_backlight_device_register(struct >> intel_connector *connector) >> props.type = BACKLIGHT_RAW; >> /* >> -* Note: Everything should work even if the backlight device max >> -* presented to the userspace is arbitrarily chosen. >> +* Expose the whole valid PWM brightness range to the backlight >> class. >> */ >> - props.max_brightness = panel->backlight.max; >> + props.max_brightness = panel->backlight.max - >> panel->backlight.min; >> props.brightness = scale_hw_to_user(connector, >> panel->backlight.level, >> props.max_brightness); >> @@ -1400,7 +1399,8 @@ static u32 get_backlight_min_vbt(struct >> intel_connector *connector) >> struct drm_device *dev = connector->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_panel *panel = >panel; >> - int min; >> + int vbt_min; >> + u32 pwm_min; >> WARN_ON(panel->backlight.max == 0); >> @@ -1411,14 +1411,24 @@ static u32 get_backlight_min_vbt(struct >> intel_connector *connector) >> * against this by letting the minimum be at most (arbitrarily >> chosen) >> * 25% of the max. >> */ >> - min = clamp_t(int,
Re: [Intel-gfx] [PATCH 2/7] drm/i915: Replace the aux ddc name switch statement with kasprintf()
On Wed, 11 Nov 2015, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > Use kasprintf() to generate the "DPDDC-" name for the aux helper. > > To deal with errors properly make intel_dp_aux_init() return something, > and adjust the caller to match. It seems we were also missing a > intel_dp_mst_encoder_cleanup() call on edp (non-port A) init failures, > so add that too. > > The whole error/cleanup ordering doesn't feel entirely sane to me, but > I'll leave that part alone for now. > > v2: Use kasprintf() instead of a table, reorder patches (Chis) > > Cc: Chris Wilson > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_dp.c | 75 > + > 1 file changed, 46 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 891a7f8..df2a2d2 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1009,6 +1009,13 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct > drm_dp_aux_msg *msg) > } > > static void > +intel_dp_aux_fini(struct intel_dp *intel_dp) > +{ > + drm_dp_aux_unregister(_dp->aux); > + kfree(intel_dp->aux.name); > +} > + > +static int > intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector > *connector) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > @@ -1016,7 +1023,6 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct > intel_connector *connector) > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > enum port port = intel_dig_port->port; > struct ddi_vbt_port_info *info = _priv->vbt.ddi_port_info[port]; > - const char *name = NULL; > uint32_t porte_aux_ctl_reg = DPA_AUX_CH_CTL; > int ret; > > @@ -1043,23 +1049,18 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct > intel_connector *connector) > switch (port) { > case PORT_A: > intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL; > - name = "DPDDC-A"; > break; > case PORT_B: > intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL; > - name = "DPDDC-B"; > break; > case PORT_C: > intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL; > - name = "DPDDC-C"; > break; > case PORT_D: > intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL; > - name = "DPDDC-D"; > break; > case PORT_E: > intel_dp->aux_ch_ctl_reg = porte_aux_ctl_reg; > - name = "DPDDC-E"; > break; > default: > BUG(); > @@ -1077,27 +1078,36 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct > intel_connector *connector) > if (!IS_HASWELL(dev) && !IS_BROADWELL(dev) && port != PORT_E) > intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10; > > - intel_dp->aux.name = name; > + intel_dp->aux.name = kasprintf(GFP_KERNEL, "DPDDC-%c", port_name(port)); > + if (!intel_dp->aux.name) > + return -ENOMEM; > + > intel_dp->aux.dev = dev->dev; > intel_dp->aux.transfer = intel_dp_aux_transfer; > > - DRM_DEBUG_KMS("registering %s bus for %s\n", name, > + DRM_DEBUG_KMS("registering %s bus for %s\n", > + intel_dp->aux.name, > connector->base.kdev->kobj.name); > > ret = drm_dp_aux_register(_dp->aux); > if (ret < 0) { > DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n", > - name, ret); > - return; > + intel_dp->aux.name, ret); > + kfree(intel_dp->aux.name); > + return ret; > } > > ret = sysfs_create_link(>base.kdev->kobj, > _dp->aux.ddc.dev.kobj, > intel_dp->aux.ddc.dev.kobj.name); > if (ret < 0) { > - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, > ret); > - drm_dp_aux_unregister(_dp->aux); > + DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", > + intel_dp->aux.name, ret); > + intel_dp_aux_fini(intel_dp); > + return ret; > } > + > + return 0; > } > > static void > @@ -4771,7 +4781,7 @@ void intel_dp_encoder_destroy(struct drm_encoder > *encoder) > struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); > struct intel_dp *intel_dp = _dig_port->dp; > > - drm_dp_aux_unregister(_dp->aux); > + intel_dp_aux_fini(intel_dp); > intel_dp_mst_encoder_cleanup(intel_dig_port); > if (is_edp(intel_dp)) { > cancel_delayed_work_sync(_dp->panel_vdd_work); > @@ -5752,7 +5762,7 @@ intel_dp_init_connector(struct intel_digital_port > *intel_dig_port, > struct drm_device
Re: [Intel-gfx] [PATCH 0/7] drm/i915: Reordered AUX patches from type safety series
On Wed, 11 Nov 2015, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > Chris requested that I try to reorder the DP AUX patches in the last > register type safety series [1] to form a better story. Here is the > result. The final code is exactly the same as before (apart from the > kasprintf() changes), so I kept the previous r-b's in place, with > some added version annotations. I reviewed the one patch that was missing r-b, and eyeballed the ones with previous r-b's a bit, and it all looks good to go. BR, Jani. > > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079453.html > > Ville Syrjälä (7): > drm/i915: Replace aux_ch_ctl_reg check with port check > drm/i915: Replace the aux ddc name switch statement with kasprintf() > drm/i915: Parametrize AUX registers > drm/i915: Remove the magic AUX_CTL is at DP + foo tricks > drm/i915: Store aux data reg offsets in intel_dp->aux_ch_data_reg[] > drm/i915: Add dev_priv->psr_mmio_base > drm/i915: Model PSR AUX register selection more like the normal AUX > code > > drivers/gpu/drm/i915/i915_debugfs.c | 4 +- > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_reg.h | 115 --- > drivers/gpu/drm/i915/intel_dp.c | 282 > +--- > drivers/gpu/drm/i915/intel_drv.h| 1 + > drivers/gpu/drm/i915/intel_psr.c| 51 +-- > 6 files changed, 298 insertions(+), 157 deletions(-) -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 3/7] drm/i915: use assert_rpm_wakelock_held instead of opencoding it
On Thu, Nov 12, 2015 at 06:40:17PM +0200, Imre Deak wrote: > Signed-off-by: Imre Deak> Reviewed-by: Chris Wilson (v1) Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 5/7] drm/i915: sanitize the asserts in the RPM get/put helpers
On Thu, Nov 12, 2015 at 06:40:19PM +0200, Imre Deak wrote: > There is no point in checking the refcount just after increasing it so > remove the assert from the get helper. Otoh, we should check the > refcount before decreasing it, so add it to the put helper. > > Signed-off-by: Imre Deak> --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 64da5af..db63b8a 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -2132,7 +2132,6 @@ void intel_runtime_pm_get(struct drm_i915_private > *dev_priv) > pm_runtime_get_sync(device); > > atomic_inc(_priv->pm.wakelock_count); > - assert_rpm_wakelock_held(dev_priv); Otoh, assert_device_not_suspended() still makes sense here. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Dump in-flight plane state while dumping in-flight CRTC state
The intel_dump_pipe_config() always dumps the currently active plane state for each plane on a CRTC. If we're calling this function to dump CRTC state that is in-flight (not yet active), then this mismatch can be misleading and confusing. Let's pay attention to whether the state we're dumping is part of an in-flight transaction (because crtc_state->state is non-NULL); if it is, we'll dump the corresponding in-flight plane state instead of the active state. Signed-off-by: Matt Roper--- drivers/gpu/drm/i915/intel_display.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b5f7493..7bbcb98 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12074,11 +12074,23 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, DRM_DEBUG_KMS("planes on this crtc\n"); list_for_each_entry(plane, >mode_config.plane_list, head) { + struct drm_plane_state *ps = NULL; + intel_plane = to_intel_plane(plane); if (intel_plane->pipe != crtc->pipe) continue; - state = to_intel_plane_state(plane->state); + /* Get in-flight plane state, if any */ + if (pipe_config->base.state) + ps = drm_atomic_get_existing_plane_state(pipe_config->base.state, +plane); + + /* If no in-flight state, use active state instead */ + if (!ps) + ps = plane->state; + + state = to_intel_plane_state(ps); + fb = state->base.fb; if (!fb) { DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d " -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.
Commit (89251b17) intended to remove this line and let only one DP_PSR_EN_CFG set, but it was wrong and this call is now duplicated at the code. Also "& ~DP_PSR_MAIN_LINK_ACTIVE" doesn't do anything at all. It was like that since I introduced this call but probably the idea was to be informative and make clear statement that we were not using the link standby. So it is better to remove this one here and let the code a bit cleaner. v2: Improve commit message as requested by Paulo. Cc: Paulo ZanoniTested-by: Brian Norris Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/intel_psr.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index e5ae844..e5b3fce 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -172,9 +172,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp) aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0); - drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, - DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE); - /* Enable AUX frame sync at sink */ if (dev_priv->psr.aux_frame_sync) drm_dp_dpcd_writeb(_dp->aux, -- 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference
On Thu, Nov 12, 2015 at 10:49:29PM +0200, Imre Deak wrote: > On Thu, 2015-11-12 at 20:41 +, Chris Wilson wrote: > > On Thu, Nov 12, 2015 at 07:50:09PM +0200, Imre Deak wrote: > > > On to, 2015-11-12 at 17:04 +, Chris Wilson wrote: > > > > As it stands since we don't actually cancel the hangcheck when we > > > drop > > > > the rpm wakelock in intel_mark_idle() it can very well come to pass > > > > that > > > > we execute this whilst the device is asleep. However, if the device > > > > is > > > > alseep, we now that we are no longer executing. > > > > > > But how could this run while asleep, since we flush the work in the > > > runtime suspend handler before turning off the HW? But even if it can't > > > run your idea would be clearer imo.. > > > > We shouldn't be flushing the hangcheck worker there. That's a leaky > > abstraction attempting to paper over something that shouldn't have been > > a problem in the first place. Note that there is a similar issue with > > the existing request waiters that currently may race against > > intel_mark_idle(). > > Ok. I think your idea is an improvement, but it will change > functionality, so how about doing it as a follow-up? Yes, since intel_runtime_suspend() already has a mechanism that should avoid the assert, relaxing that assert and removing that bit of unnecessary work from the rpm suspend path can be done separately. (I hadn't realised that cancel_work had snuck in there.) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/7] drm/i915: add assert_rpm_wakelock_held helper
On Thu, Nov 12, 2015 at 06:40:16PM +0200, Imre Deak wrote: > As a preparation for follow-up patches add a new helper that checks > whether we hold an RPM reference, since this is what we want most of > the cases. Atm this helper will only check for the HW suspended state, a > follow-up patch will do the actual change to check the refcount instead. > One exception is the forcewake release timer function, where it's > guaranteed that the HW is on even though the RPM refcount drops to zero. > This guarantee is provided by flushing the timer in the runtime suspend > handler. So leave the assert_device_not_suspended check in place there. > > Also rename assert_device_suspended for consistency and export these > helpers as a preparation for the follow-up patches. > > No functional change. > > v3: > - change the assert warning message to be more meaningful (Chris) > > Signed-off-by: Imre DeakReviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference
On Thu, Nov 12, 2015 at 06:40:18PM +0200, Imre Deak wrote: > Atm, we assert that the device is not suspended until the point when the > HW is truly put to a suspended state. This is fine, but we can catch > more problems if we check the RPM refcount. After that one drops to zero > we shouldn't access the HW any more, although the actual suspend may be > delayed. Based on this change the assert_rpm_wakelock_held helper to > check the refcount instead of the device suspended state. > > After this change we need to annotate every place explicitly in the code > where we expect that the HW is in D0 state. Atm in the driver load > function the D0 state is implicit until we enable runtime PM, but for > these asserts to work we need to add explicit RPM get/put calls, so fix > this up. > > Another place where the D0 state is implicit even with a 0 RPM refcount > is the system and runtime sudpend/resume handlers and the hangcheck > work. In the former case the susend/resume handlers themselves determine > at which exact spot the HW is truly on/off and in the latter case the > work will be flushed in the suspend handler before turning off the HW. > We regard these cases special and disable the RPM asserts for their > duration. In the hangcheck work we can nevertheless check that the > device is not suspended. Fix up these things. > > These explicit annotations also have the positive side effect of > documenting our assumptions better. > > This caught additional WARNs from the atomic modeset path, those should > be fixed separately. > > v2: > - remove the redundant HAS_RUNTIME_PM check (moved to patch 1) (Ville) > v3: > - use a new dedicated RPM wakelock refcount to also catch cases where > our own RPM get/put functions were not called (Chris) > - assert also that the new RPM wakelock refcount is 0 in the RPM > suspend handler (Chris) > - change the assert error message to be more meaningful (Chris) > - prevent false assert errors and check that the RPM wakelock is 0 in > the RPM resume handler too > - prevent false assert errors in the hangcheck work too > - add a device not suspended assert check to the hangcheck work > > Signed-off-by: Imre DeakReviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: Clarify plane state during CRTC state dumps
During state dumping, list planes that have an FB but are invisible (e.g., because they're offscreen or clipped by other planes) as "not visible" rather than "enabled." While we're at it, throw the bpp value into the debugging output. Signed-off-by: Matt Roper--- drivers/gpu/drm/i915/intel_display.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7bbcb98..d994f52 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12102,13 +12102,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, continue; } - DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled", + DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d %s", plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD", plane->base.id, intel_plane->pipe, crtc->base.primary == plane ? 0 : intel_plane->plane + 1, - drm_plane_index(plane)); - DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x", - fb->base.id, fb->width, fb->height, fb->pixel_format); + drm_plane_index(plane), + state->visible ? "enabled" : "not visible"); + DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x bpp = %d", + fb->base.id, fb->width, fb->height, fb->pixel_format, + fb->bits_per_pixel); DRM_DEBUG_KMS("\tscaler:%d src (%u, %u) %ux%u dst (%u, %u) %ux%u\n", state->scaler_id, state->src.x1 >> 16, state->src.y1 >> 16, -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] drm/i915: Setup clipped src/dest coordinates during FB reconstruction
Plane state objects contain two copies of src/dest coordinates: the original (requested by userspace) coordinates in the base drm_plane_state object, and a second, clipped copy (i.e., what we actually want to program to the hardware) in intel_plane_state. We've only been setting up the former set of values during boot time FB reconstruction, but we should really be initializing both. Note that the code here probably still needs some more work. We seem to be making two questionable assumptions: * BIOS will program the primary plane to the entire display size (potentially false on gen9+ platforms where the primary plane can be windowed) * The BIOS fb size actually matches the plane/display size. It seems that there's nothing stopping the BIOS from overallocating the FB (potentially to do an extended mode FB that spans multiple displays), so setting our src/dest coordinates to the FB size here may not always be right. Signed-off-by: Matt Roper--- drivers/gpu/drm/i915/intel_display.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9fa041c..3145c9d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2601,6 +2601,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, struct drm_i915_gem_object *obj; struct drm_plane *primary = intel_crtc->base.primary; struct drm_plane_state *plane_state = primary->state; + struct intel_plane_state *intel_state = + to_intel_plane_state(plane_state); struct drm_framebuffer *fb; if (!plane_config->fb) @@ -2648,6 +2650,15 @@ valid_fb: plane_state->crtc_w = fb->width; plane_state->crtc_h = fb->height; + intel_state->src.x1 = plane_state->src_x; + intel_state->src.y1 = plane_state->src_y; + intel_state->src.x2 = plane_state->src_x + plane_state->src_w; + intel_state->src.y2 = plane_state->src_y + plane_state->src_h; + intel_state->dst.x1 = plane_state->crtc_x; + intel_state->dst.y1 = plane_state->crtc_y; + intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w; + intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h; + obj = intel_fb_obj(fb); if (obj->tiling_mode != I915_TILING_NONE) dev_priv->preserve_bios_swizzle = true; -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915: Dump pipe config after initial FB is reconstructed
We dump during HW state readout, but that's too early to reflect how the primary plane is actually configured. Signed-off-by: Matt Roper--- drivers/gpu/drm/i915/intel_display.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d994f52..9fa041c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15118,6 +15118,9 @@ void intel_modeset_init(struct drm_device *dev) * just get the first one. */ intel_find_initial_plane_obj(crtc, _config); + + intel_dump_pipe_config(crtc, crtc->config, + "[state after init fb reconstruction]"); } } -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/7] drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers
On Thu, Nov 12, 2015 at 06:40:15PM +0200, Imre Deak wrote: > We don't really need to check this flag in the get/put/assert helpers, > as on platforms without RPM support we won't ever enable RPM. That means > pm.suspend will be always false and the assert will be always true. > > Do this to simplify the code and to let us extend the RPM asserts to all > platforms for a better coverage. > > Motivated by Ville. > > Signed-off-by: Imre DeakReviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/31] drm/i915: Fix IPS disable sequence.
Hi, On 11 November 2015 at 23:31, Vivi, Rodrigowrote: > On Tue, 2015-11-10 at 16:34 +, Daniel Stone wrote: >> On 5 November 2015 at 18:49, Rodrigo Vivi >> wrote: >> > +void intel_ips_disable_if_alone(struct intel_crtc *crtc) >> > +{ >> > + struct drm_device *dev = crtc->base.dev; >> > + struct drm_i915_private *dev_priv = dev->dev_private; >> > + bool ips_enabled; >> > + struct intel_plane *intel_plane; >> > + >> > + mutex_lock(_priv->display_ips.lock); >> > + ips_enabled = dev_priv->display_ips.enabled; >> > + mutex_unlock(_priv->display_ips.lock); >> > + >> > + if (!ips_enabled) >> > + return; >> > + >> > + for_each_intel_plane_on_crtc(dev, crtc, intel_plane) { >> > + enum plane plane = intel_plane->plane; >> > + >> > + if (plane != PLANE_A && >> > + !!(I915_READ(DSPCNTR(plane)) & >> > DISPLAY_PLANE_ENABLE)) >> > + return; >> > + intel_ips_disable(crtc); >> > + } >> > +} >> >> Rather than reading the registers, this should just inspect >> plane_state->visible. Reading the registers introduces the same bug >> as >> I mentioned the last mail, but in a different way: >> - IPS is enabled >> - primary and overlay planes are both enabled >> - user commits an atomic state which disables both primary and >> overlay planes, so IPS must be disabled >> - disabling the primary plane calls this function, which sees that >> the overlay plane is still active, so IPS can remain enabled >> - the overlay plane gets disabled, with IPS still active >> - :( > > You are absolutely right on this case... :/ Thanks for spotting this > case. > > So I was considering your idea for the unified place but I ended up in > some concerns questions here. > > First is the disable must occur on pre-update and enable on post > -update, so I would prefer to still let them spread and reactive. Right, they have to be separate - which applies doubly for async modesets as well. > But now I believe that we need to detach the atomic > ->ips_{enable,disable} from primary and do for every plane on/off. So > if we are enabling any plane we just call ips_enable(). > And if plane is being disabled and there is no other plane->visible in > this crtc we call intel_disable(). > > But I wonder how to skip the plane itself on for_each_plane_in_state... > Or should I just counter the number of state->visible and disable if <= > 1 and let enable if we count more than 1 visible plane. Any better > idea? Yeah, exactly that, but even easier: bool ips_target_state = !!crtc_state->plane_mask; So my idea would be that, in prepare_commit, you calculate the target state based on the updated plane_mask. When actually committing the state, call intel_ips_disable() in intel_pre_plane_update (if !ips_target_state && intel_crtc->ips_enabled), and intel_ips_enable() after the commit has finished (if ips_target_state && !intel_crtc->ips_enabled). That split would be a really good fit for async/atomic, where we need to calculate the target state in advance, and only apply it some time later. Same goes for PSR. Basically, any work we need to do for modeset needs to be quite statically calculated in the prepare stage, so we can apply it from the commit stage, without needing to pull any further state pointers (we can't do this with async), and certainly without reference to the actual hardware configuration (e.g. inspecting registers). Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 10/12] drm/i915/gen9: Turn DC handling into a power well
On Wed, Nov 11, 2015 at 08:57:19PM +0200, Imre Deak wrote: > On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote: > > Handle DC off as a power well where enabling the power well will > > prevent > > the DMC to enter selected DC states (required around modesets and Aux > > A). Disabling the power well will allow DC states again. For now the > > highest DC state is DC6 for Skylake and DC5 for Broxton but will be > > configurable for Skylake in a later patch. > > > > v2: Check both DC5 and DC6 bits in power well enabled function > > (Ville) > > > > Signed-off-by: Patrik Jakobsson> > --- > > drivers/gpu/drm/i915/i915_drv.c | 6 -- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_display.c| 6 ++ > > drivers/gpu/drm/i915/intel_runtime_pm.c | 110 > > +++- > > 4 files changed, 88 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index 5a63f9a..0c7f435 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1072,9 +1072,6 @@ static int i915_pm_resume(struct device *dev) > > > > static int skl_suspend_complete(struct drm_i915_private *dev_priv) > > { > > - if (dev_priv->csr.dmc_payload) > > - skl_enable_dc6(dev_priv); > > - > > return 0; > > } > > > > @@ -1119,9 +1116,6 @@ static int bxt_resume_prepare(struct > > drm_i915_private *dev_priv) > > > > static int skl_resume_prepare(struct drm_i915_private *dev_priv) > > { > > - if (dev_priv->csr.dmc_payload) > > - skl_disable_dc6(dev_priv); > > - > > return 0; > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 31b3a84..df445ba 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -636,6 +636,7 @@ enum skl_disp_power_wells { > > > > /* Not actual bit groups. Used as IDs for > > lookup_power_well() */ > > SKL_DISP_PW_ALWAYS_ON, > > + SKL_DISP_PW_DC_OFF, > > Imo it would be less confusing to call it DC3 power well. Looking at th > e DC spec, DC4 is only a transitory state to DC5/6, so what we expect > when we disable DC6/5 is DC3 or shallower power states (DC2/1/0). I've been changing the name quite a few times but settled on "DC off" to keep it generic. The main mechanism for the power well is to prevent any DC states that can cause us problems during certain operations (i.e. modeset). The DC states we need to block could change or be different between platforms. For that reason I would prefer not to be as specific with the naming. > > > }; > > > > #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 649ac34..856d801 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13320,6 +13320,9 @@ static int intel_atomic_commit(struct > > drm_device *dev, > > to_intel_crtc_state(crtc->state) > > ->update_pipe; > > unsigned long put_domains = 0; > > > > + if (modeset) > > + intel_display_power_get(dev_priv, > > POWER_DOMAIN_MODESET); > > + > > if (modeset && crtc->state->active) { > > update_scanline_offset(to_intel_crtc(crtc)); > > dev_priv->display.crtc_enable(crtc); > > @@ -13343,6 +13346,9 @@ static int intel_atomic_commit(struct > > drm_device *dev, > > modeset_put_power_domains(dev_priv, > > put_domains); > > > > intel_post_plane_update(intel_crtc); > > + > > + if (modeset) > > + intel_display_power_put(dev_priv, > > POWER_DOMAIN_MODESET); > > } > > > > /* FIXME: add subpixel order */ > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index edf753e..95c3fcc 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -49,9 +49,6 @@ > > * present for a given platform. > > */ > > > > -#define GEN9_ENABLE_DC5(dev) 0 > > -#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev) > > - > > #define for_each_power_well(i, power_well, domain_mask, > > power_domains) \ > > for (i = 0; > > \ > > i < (power_domains)->power_well_count && > > \ > > @@ -309,9 +306,14 @@ static void hsw_set_power_well(struct > > drm_i915_private *dev_priv, > > #define SKL_DISPLAY_DDI_D_POWER_DOMAINS ( \ > > BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |\ > > BIT(POWER_DOMAIN_INIT)) > > +#define SKL_DISPLAY_DC_OFF_POWER_DOMAINS ( \ > > + BIT(POWER_DOMAIN_MODESET) | \ > > + BIT(POWER_DOMAIN_AUX_A) | \ > > +
Re: [Intel-gfx] [PATCH v2 10/12] drm/i915/gen9: Turn DC handling into a power well
On Wed, Nov 11, 2015 at 09:23:32PM +0200, Imre Deak wrote: > On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote: > > Handle DC off as a power well where enabling the power well will > > prevent > > the DMC to enter selected DC states (required around modesets and Aux > > A). Disabling the power well will allow DC states again. For now the > > highest DC state is DC6 for Skylake and DC5 for Broxton but will be > > configurable for Skylake in a later patch. > > > > v2: Check both DC5 and DC6 bits in power well enabled function > > (Ville) > > > > Signed-off-by: Patrik Jakobsson> > --- > > drivers/gpu/drm/i915/i915_drv.c | 6 -- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_display.c| 6 ++ > > drivers/gpu/drm/i915/intel_runtime_pm.c | 110 > > +++- > > 4 files changed, 88 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index 5a63f9a..0c7f435 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1072,9 +1072,6 @@ static int i915_pm_resume(struct device *dev) > > > > static int skl_suspend_complete(struct drm_i915_private *dev_priv) > > { > > - if (dev_priv->csr.dmc_payload) > > - skl_enable_dc6(dev_priv); > > - > > return 0; > > } > > > > @@ -1119,9 +1116,6 @@ static int bxt_resume_prepare(struct > > drm_i915_private *dev_priv) > > > > static int skl_resume_prepare(struct drm_i915_private *dev_priv) > > { > > - if (dev_priv->csr.dmc_payload) > > - skl_disable_dc6(dev_priv); > > - > > return 0; > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 31b3a84..df445ba 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -636,6 +636,7 @@ enum skl_disp_power_wells { > > > > /* Not actual bit groups. Used as IDs for > > lookup_power_well() */ > > SKL_DISP_PW_ALWAYS_ON, > > + SKL_DISP_PW_DC_OFF, > > }; > > > > #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 649ac34..856d801 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13320,6 +13320,9 @@ static int intel_atomic_commit(struct > > drm_device *dev, > > to_intel_crtc_state(crtc->state) > > ->update_pipe; > > unsigned long put_domains = 0; > > > > + if (modeset) > > + intel_display_power_get(dev_priv, > > POWER_DOMAIN_MODESET); > > + > > if (modeset && crtc->state->active) { > > update_scanline_offset(to_intel_crtc(crtc)); > > dev_priv->display.crtc_enable(crtc); > > @@ -13343,6 +13346,9 @@ static int intel_atomic_commit(struct > > drm_device *dev, > > modeset_put_power_domains(dev_priv, > > put_domains); > > > > intel_post_plane_update(intel_crtc); > > + > > + if (modeset) > > + intel_display_power_put(dev_priv, > > POWER_DOMAIN_MODESET); > > } > > > > /* FIXME: add subpixel order */ > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index edf753e..95c3fcc 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -49,9 +49,6 @@ > > * present for a given platform. > > */ > > > > -#define GEN9_ENABLE_DC5(dev) 0 > > -#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev) > > - > > #define for_each_power_well(i, power_well, domain_mask, > > power_domains) \ > > for (i = 0; > > \ > > i < (power_domains)->power_well_count && > > \ > > @@ -309,9 +306,14 @@ static void hsw_set_power_well(struct > > drm_i915_private *dev_priv, > > #define SKL_DISPLAY_DDI_D_POWER_DOMAINS ( \ > > BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |\ > > BIT(POWER_DOMAIN_INIT)) > > +#define SKL_DISPLAY_DC_OFF_POWER_DOMAINS ( \ > > + BIT(POWER_DOMAIN_MODESET) | \ > > + BIT(POWER_DOMAIN_AUX_A) | \ > > + BIT(POWER_DOMAIN_INIT)) > > #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS ( \ > > (POWER_DOMAIN_MASK & ~( \ > > - SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS)) | \ > > + SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \ > > + SKL_DISPLAY_DC_OFF_POWER_DOMAINS)) |\ > > BIT(POWER_DOMAIN_INIT)) > > > > #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (\ > > @@ -339,6 +341,10 @@ static void hsw_set_power_well(struct > > drm_i915_private *dev_priv, > > BIT(POWER_DOMAIN_AUX_A) | \ > >
Re: [Intel-gfx] [PATCH i-g-t 2/3 v3] Unify handling of slow/combinatorial tests
On Fri, Oct 30, 2015 at 01:52:48PM +, Chris Wilson wrote: > On Fri, Oct 30, 2015 at 03:18:30PM +0200, David Weinehall wrote: > > @@ -931,16 +930,20 @@ run_basic_modes(const struct access_mode *mode, > > struct buffers buffers; > > > > for (h = hangs; h->suffix; h++) { > > - if (!all && *h->suffix) > > - continue; > > + unsigned int subtest_flags; > > > > - for (p = all ? pipelines : pskip; p->prefix; p++) { > > + if (*h->suffix) > > + subtest_flags = SUBTEST_TYPE_SLOW; > > They aren't all slow though. The hang tests are (because it takes a long > time for a hang to occur and we need to race many times for reasonable > coverage). Many of the tests here were being skipped because QA couldn't > handle the full set. Of course. But unlike the creators of the tests in question I have very little knowledge about why the tests in question weren't included in the standard test set. Sorting tests into slow/cornercase/whatever is something that should be done by people who properly know what categories the bugs best belong to. Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Remove redundant check in i915_gem_obj_to_vma
From: Tvrtko UrsulinNo need to verify VMA belongs to GGTT since: 1. The function must return a normal VMA belonging to passed in VM. 2. There can only be one normal VMA for any VM. Signed-off-by: Tvrtko Ursulin Cc: Chris Wilson --- Maybe now even a candidate for making a static inline? drivers/gpu/drm/i915/i915_gem.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index d85c63dc36ac..a913387a89cb 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4485,10 +4485,8 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, { struct i915_vma *vma; list_for_each_entry(vma, >vma_list, vma_link) { - if (i915_is_ggtt(vma->vm) && - vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) - continue; - if (vma->vm == vm) + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL && + vma->vm == vm) return vma; } return NULL; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 03/13] drm/i915/gen9: move assert_csr_loaded into intel_rpm.c
On ke, 2015-10-28 at 23:58 +0200, Imre Deak wrote: > From: Daniel Vetter> > Avoids non-static functions since all the callers are in intel_rpm.c. > > Cc: Damien Lespiau > Cc: Imre Deak > Cc: Sunil Kamath > Signed-off-by: Daniel Vetter > Signed-off-by: Animesh Manna > [imre: removed note about reg definitions from commit message, since > it's not relevant any more] > Reviewed-by: Imre Deak > --- > drivers/gpu/drm/i915/intel_csr.c| 10 -- > drivers/gpu/drm/i915/intel_drv.h| 1 - > drivers/gpu/drm/i915/intel_runtime_pm.c | 8 > 3 files changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > b/drivers/gpu/drm/i915/intel_csr.c > index 2c9bf3f..cd6fb58d 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -485,13 +485,3 @@ void intel_csr_ucode_fini(struct drm_device > *dev) > intel_csr_load_status_set(dev_priv, FW_FAILED); > kfree(dev_priv->csr.dmc_payload); > } > - > -void assert_csr_loaded(struct drm_i915_private *dev_priv) > -{ > - WARN_ONCE(intel_csr_load_status_get(dev_priv) != FW_LOADED, > - "CSR is not loaded.\n"); > - WARN_ONCE(!I915_READ(CSR_PROGRAM(0)), > - "CSR program storage start is NULL\n"); > - WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not > fine\n"); > - WARN_ONCE(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n"); > -} > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 1a3bbdc..3d1c744 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1211,7 +1211,6 @@ void intel_csr_load_status_set(struct > drm_i915_private *dev_priv, > enum csr_state state); > void intel_csr_load_program(struct drm_device *dev); > void intel_csr_ucode_fini(struct drm_device *dev); > -void assert_csr_loaded(struct drm_i915_private *dev_priv); > > /* intel_dp.c */ > void intel_dp_init(struct drm_device *dev, int output_reg, enum port > port); > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index e50cc88..92746e1 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -458,6 +458,14 @@ static void > gen9_set_dc_state_debugmask_memory_up( > } > } > > +void assert_csr_loaded(struct drm_i915_private *dev_priv) I missed it during review, but this is static. With that added my R-b still holds. > +{ > + WARN_ONCE(!I915_READ(CSR_PROGRAM(0)), > + "CSR program storage start is NULL\n"); > + WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not > fine\n"); > + WARN_ONCE(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n"); > +} > + > static void assert_can_enable_dc5(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 11/12] drm/i915/gen9: Add boot parameter for disabling DC6
On Wed, Nov 11, 2015 at 09:04:09PM +0200, Imre Deak wrote: > On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote: > > v2: Use _unsafe (Jani) > > > > Signed-off-by: Patrik Jakobsson> > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_params.c | 6 ++ > > drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++-- > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index c0252ef..5628c5a 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2639,6 +2639,7 @@ struct i915_params { > > int panel_use_ssc; > > int vbt_sdvo_panel_type; > > int enable_rc6; > > + int enable_dc6; > > int enable_fbc; > > int enable_ppgtt; > > int enable_execlists; > > diff --git a/drivers/gpu/drm/i915/i915_params.c > > b/drivers/gpu/drm/i915/i915_params.c > > index 368df67..6457f3a 100644 > > --- a/drivers/gpu/drm/i915/i915_params.c > > +++ b/drivers/gpu/drm/i915/i915_params.c > > @@ -32,6 +32,7 @@ struct i915_params i915 __read_mostly = { > > .panel_use_ssc = -1, > > .vbt_sdvo_panel_type = -1, > > .enable_rc6 = -1, > > + .enable_dc6 = 1, > > .enable_fbc = -1, > > .enable_execlists = -1, > > .enable_hangcheck = true, > > @@ -79,6 +80,11 @@ MODULE_PARM_DESC(enable_rc6, > > "For example, 3 would enable rc6 and deep rc6, and 7 would > > enable everything. " > > "default: -1 (use per-chip default)"); > > > > +module_param_named_unsafe(enable_dc6, i915.enable_dc6, int, 0400); > > +MODULE_PARM_DESC(enable_dc6, > > + "Enable power-saving display C-state 6. " > > + "(0 = disable; 1 = enable [default])"); > > + > > It would be more generic to have something like enable_dc, -1=per-chip > default, 0=disable, 1=up to dc5, 2=up to dc6. I'm not sure if this parameter is going to stay for long but if it does I suppose we should have DC9 as well. But do we really need this level of control? My intention was to work around the DC6 corner case. Do you think we could make good use of a more generic interface? Perhaps useful for testing? If so, I definitely think we should go with your more generic solution. Otherwise I'd rather keep it simple. Feel free to override my decision here. Also, what would 0=disable be? Not starting the DMC at all or DC3/4? > > > module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600); > > MODULE_PARM_DESC(enable_fbc, > > "Enable frame buffer compression for power savings " > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index 95c3fcc..62c1273 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -708,7 +708,7 @@ static void gen9_dc_off_power_well_enable(struct > > drm_i915_private *dev_priv, > > static void gen9_dc_off_power_well_disable(struct drm_i915_private > > *dev_priv, > > struct i915_power_well > > *power_well) > > { > > - if (IS_SKYLAKE(dev_priv)) > > + if (IS_SKYLAKE(dev_priv) && i915.enable_dc6) > > skl_enable_dc6(dev_priv); > > else > > gen9_enable_dc5(dev_priv); > > @@ -720,7 +720,7 @@ static void gen9_dc_off_power_well_sync_hw(struct > > drm_i915_private *dev_priv, > > if (power_well->count > 0) { > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > } else { > > - if (IS_SKYLAKE(dev_priv)) > > + if (IS_SKYLAKE(dev_priv) && i915.enable_dc6) > > gen9_set_dc_state(dev_priv, > > DC_STATE_EN_UPTO_DC6); > > else > > gen9_set_dc_state(dev_priv, > > DC_STATE_EN_UPTO_DC5); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/12] drm/i915: Clean up AUX power domain handling
On Thu, Nov 12, 2015 at 10:02:36AM +0100, Patrik Jakobsson wrote: > On Wed, Nov 11, 2015 at 08:37:36PM +0200, Ville Syrjälä wrote: > > On Wed, Nov 11, 2015 at 08:22:03PM +0200, Imre Deak wrote: > > > On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote: > > > > From: Ville Syrjälä> > > > > > > > Introduce intel_display_port_aux_power_domain() which simply returns > > > > the appropriate AUX power domain for a specific port, and then > > > > replace > > > > the intel_display_port_power_domain() with calls to the new function > > > > in the DP code. As long as we're not actually enabling the port we > > > > don't > > > > need the lane power domains, and those are handled now purely from > > > > modeset_update_crtc_power_domains(). > > > > > > > > My initial motivation for this was to see if I could keep the DPIO > > > > power > > > > wells powered down while doing AUX on CHV, but turns out I can't so > > > > this > > > > doesn't change anything for CHV at least. But I think it's still a > > > > worthwile change. > > > > > > > > Signed-off-by: Ville Syrjälä > > > > Reviewed-by: Patrik Jakobsson > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 40 > > > > ++ > > > > drivers/gpu/drm/i915/intel_dp.c | 48 +++--- > > > > -- > > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > > 3 files changed, 56 insertions(+), 34 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index d0fec07..c2578d9 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -5143,6 +5143,23 @@ static enum intel_display_power_domain > > > > port_to_power_domain(enum port port) > > > > } > > > > } > > > > > > > > +static enum intel_display_power_domain port_to_aux_power_domain(enum > > > > port port) > > > > +{ > > > > + switch (port) { > > > > + case PORT_A: > > > > + return POWER_DOMAIN_AUX_A; > > > > + case PORT_B: > > > > + return POWER_DOMAIN_AUX_B; > > > > + case PORT_C: > > > > + return POWER_DOMAIN_AUX_C; > > > > + case PORT_D: > > > > + return POWER_DOMAIN_AUX_D; > > > > + default: > > > > + WARN_ON_ONCE(1); > > > > + return POWER_DOMAIN_AUX_A; > > > > + } > > > > +} > > > > > > Looks like port E is missing. I chatted with Ville he has some idea to > > > fix this. > > > > Yeah, so there's no dedicated AUX block for port E, and instead VBT > > tells us which AUX block to use. The current code doin that is rather > > messy, but I have a cleaned it up during my register type-safety > > journey. I just reposted the remaining AUX related patches [1] from > > that series. > > > > So I was thinking that we could include an 'enum port aux_port' inside > > intel_dp, and use that to pick the right power domain. > > > > [1] > > http://lists.freedesktop.org/archives/intel-gfx/2015-November/079918.html > > Ok so one of Aux A-D is hardwired to port E and only VBT can tell us which? Yep. > Do > we need to change anything in this patch or can we add the aux_port enum later > on? Hmm. Lemme think. Before, we too the port power domain, so DDI_E, and after the patch we would hit the default: case and get the WARN. So that would be a regression of sorts. I guess as a quick hack you could return eg. POWER_DOMAIN_AUX_D for port E, and add a FIXME that it needs to be fixed, if you don't want to tackle the aux_port idea (or something silimar right now). On a side note maybe we should add MISSING_CASE_ONCE() to get some better debug output from these kinds of cases... > > > > > > > > > > + > > > > #define for_each_power_domain(domain, mask) > > > > \ > > > > for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) > > > > \ > > > > if ((1 << (domain)) & (mask)) > > > > @@ -5174,6 +5191,29 @@ intel_display_port_power_domain(struct > > > > intel_encoder *intel_encoder) > > > > } > > > > } > > > > > > > > +enum intel_display_power_domain > > > > +intel_display_port_aux_power_domain(struct intel_encoder > > > > *intel_encoder) > > > > +{ > > > > + struct drm_device *dev = intel_encoder->base.dev; > > > > + struct intel_digital_port *intel_dig_port; > > > > + > > > > + switch (intel_encoder->type) { > > > > + case INTEL_OUTPUT_UNKNOWN: > > > > + /* Only DDI platforms should ever use this output > > > > type */ > > > > + WARN_ON_ONCE(!HAS_DDI(dev)); > > > > + case INTEL_OUTPUT_DISPLAYPORT: > > > > + case INTEL_OUTPUT_EDP: > > > > + intel_dig_port = enc_to_dig_port(_encoder > > > > ->base); > > > > + return
Re: [Intel-gfx] [PATCH] drm/i915: Remove redundant check in i915_gem_obj_to_vma
On Thu, Nov 12, 2015 at 11:59:55AM +, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin> > No need to verify VMA belongs to GGTT since: > > 1. The function must return a normal VMA belonging to passed in VM. > 2. There can only be one normal VMA for any VM. > > Signed-off-by: Tvrtko Ursulin > Cc: Chris Wilson Reviewed-by Chris Wilson > Maybe now even a candidate for making a static inline? No. After tracking vma, i915_gem_obj_to_vma() is moved off the hotpaths and mostly used for the first lookup by an execbuffer, context creation, modesetting and debug code. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 0/3] fbdev fixes (reviewed)
On Thu, 12 Nov 2015, Lukas Wunnerwrote: > I use msmtp instead of git send-email, which preserves the Date-header. Any particular reason not to use git send-email? You can configure it to use a sendmail-like program instead of a server, and actually that's the default. If you have msmtp installed as sendmail (for example msmtp-mta package does this on Debian) git send-email will use that out of the box. FWIW this is exactly what I do. > When I edit commits, git commit --amend updates the commit date but not > the author date. > > That's why you see these ancient timestamps. > > If you find that annoying I'll see to it that I modify the author date > manually before sending out a new series. (At least in commits of my own. > I try to avoid tampering with other people's commits.) It's just that my mail client uses the Date: header for sorting, and the old ones tend to get buried. It's disadvantageous to you to use old dates. ;) >> For future reference, please consider posting new versions of series as >> new threads. This one got pretty messy in the end, with so many >> different versions. > > Daniel asked me to submit a patch "in-reply the previous version" in > <20150922091757.GZ3383@phenom.ffwll.local> and I adhered to that request > also when sending a new version of an entire series. In that case I'll > *not* submit in-reply-to in the future, got that. You'll probably get a different reply from everyone you ask. ;) My rule of thumb is that if you update individual patches (whether in a series or not), send them in-reply to the previous version. Each patch in-reply to its preceding version in the series. If you update more than about half the patches in a series, it's perhaps less confusing to send a new series with no in-reply-to. (Oh, and this contradicts with the example in git send-email man page.) BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v7] drm/i915: A better backlight class brightness range.
Expose the whole valid PWM brightness range to the backlight class and respect the VBT minimum backlight brightness. Signed-off-by: Shih-Yuan Lee (FourDollars)--- drivers/gpu/drm/i915/intel_panel.c | 31 ++- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index b05c6d9..73936581 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1194,10 +1194,9 @@ static int intel_backlight_device_register(struct intel_connector *connector) props.type = BACKLIGHT_RAW; /* -* Note: Everything should work even if the backlight device max -* presented to the userspace is arbitrarily chosen. +* Expose the whole valid PWM brightness range to the backlight class. */ - props.max_brightness = panel->backlight.max; + props.max_brightness = panel->backlight.max - panel->backlight.min; props.brightness = scale_hw_to_user(connector, panel->backlight.level, props.max_brightness); @@ -1400,25 +1399,23 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector) struct drm_device *dev = connector->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_panel *panel = >panel; - int min; + u32 min; WARN_ON(panel->backlight.max == 0); + /* vbt value is a coefficient in range [0..255] */ + min = scale(dev_priv->vbt.backlight.min_brightness, 0, 255, + 0, panel->backlight.max); + /* -* XXX: If the vbt value is 255, it makes min equal to max, which leads -* to problems. There are such machines out there. Either our -* interpretation is wrong or the vbt has bogus data. Or both. Safeguard -* against this by letting the minimum be at most (arbitrarily chosen) -* 25% of the max. +* The backlight class brightness 0 is mapping to PWM 0 and it is used to +* turn off the backlight, so we need to step down a little bit here to make +* backlight class brightness 1 map to the real PWM min. */ - min = clamp_t(int, dev_priv->vbt.backlight.min_brightness, 0, 64); - if (min != dev_priv->vbt.backlight.min_brightness) { - DRM_DEBUG_KMS("clamping VBT min backlight %d/255 to %d/255\n", - dev_priv->vbt.backlight.min_brightness, min); - } - - /* vbt value is a coefficient in range [0..255] */ - return scale(min, 0, 255, 0, panel->backlight.max); + if (min > 0) + return min - 1; + else + return 0; } static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unused) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v7] drm/i915: A better backlight class brightness range.
Since v3.17-rc1, it contains commit 6dda730e55f412a6dfb181cae6784822ba463847 Author: Jani Nikula Date: Tue Jun 24 18:27:40 2014 +0300 drm/i915: respect the VBT minimum backlight brightness The backlight class 0 brightness means the PWM min and it does not turn off the backlight. However there is a little problem that some values of backlight class brightness are mapped to the same PWM value. Taking VBT min 10 and PWD max 937 as an example. The corresponding PWM min will be 37 because of round(10 * 937 / 255). But sysfs : PWM --- 0 -> 37 1 -> 38 ... 12 -> 49 13 -> 49 ... 924 -> 925 925 -> 925 You can see there are some duplicates due to the backlight class brighness range is larger than the valid PWM brightness range. I prepare a python script to calculate the complete list. https://gist.github.com/fourdollars/e99c4e03c7dab0110ae7 Since v3.18-rc1, it contains commit e6755fb78e8f20ecadf2a4080084121336624ad9 Author: Jani Nikula Date: Tue Aug 12 17:11:42 2014 +0300 drm/i915: switch off backlight for backlight class 0 brightness The status becomes worse because it uses the backlight class 0 brightness to turn off the backlight and the corresponding PWM min is gone. sysfs : PWM --- 0 -> 0 1 -> 38 ... 12 -> 49 13 -> 49 ... 924 -> 925 925 -> 925 Since v3.18-rc5, it contains commit e1c412e75754ab7b7002f3e18a2652d999c40d4b Author: Jani Nikula Date: Wed Nov 5 14:46:31 2014 +0200 drm/i915: safeguard against too high minimum brightness It is for the corner case https://bugzilla.kernel.org/show_bug.cgi?id=86551. But what if it is on purpose, maybe the hardware vendor just wants to on and off the backlight and there is no intermediate stage. Taking VBT min 255 and PWD max 937 as an example. Currenly 255 will be forcely changed to 64 so the corresponding PWM min will be 235 because of round(64 * 937 / 255), and it becomes sysfs : PWM --- 0 -> 0 (off) 1 -> 236 (probably off) ... 934 -> 935 (probably off) 935 -> 936 (probably off) 936 -> 936 (probably off) 937 -> 937 (on) But it doesn't make sense if 0~936 are all used to disable the backlight and only 937 is used to turn on the backlight. This patch can solve all problems above. Taking VBT min 10 and PWD max 937 as an example. The corresponding PWM min will be 37 because of round(10 * 937 / 255). sysfs : PWM --- 0 -> 0 (off) 1 -> 37 ... 900 -> 936 901 -> 937 There is no duplicate and 37 matched the corresponding PWM min by VBT min. Taking VBT min 255 and PWD max 937 as an example. The corresponding PWM min will be 937 because of round(255 * 937 / 255). sysfs : PWM --- 0 -> 0 (off) 1 -> 937 Even for the corner case, it still works well. Shih-Yuan Lee (FourDollars) (1): drm/i915: A better backlight class brightness range. drivers/gpu/drm/i915/intel_panel.c | 31 ++- 1 file changed, 14 insertions(+), 17 deletions(-) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH] drm/i915: Consider SPLL as another shared pll.
On Wed, 21 Oct 2015, Daniel Vetterwrote: > On Wed, Sep 16, 2015 at 09:23:59AM +0200, Maarten Lankhorst wrote: >> When diagnosing a unrelated bug for someone on irc, it would seem the >> hardware can >> be brought up by the BIOS with the embedded displayport using the SPLL for >> spread spectrum. >> >> Right now this is not handled well in i915, and it calculates the crtc needs >> to >> be reprogrammed on the first modeset without SSC, but the SPLL itself was >> kept >> active. Fix this by exposing SPLL as a shared pll that will not be returned >> by intel_get_shared_dpll; you have to know it exists to use it. ;-) >> >> Cc: Emil Renner Berthing >> Signed-off-by: Maarten Lankhorst >> --- >> RFC because I haven't tested it with VGA, but it seems to work according to >> fix >> the problem mentioned above. >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 81adf89b92f1..cacdac67d9ba 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -345,6 +345,8 @@ enum intel_dpll_id { >> /* hsw/bdw */ >> DPLL_ID_WRPLL1 = 0, >> DPLL_ID_WRPLL2 = 1, >> +DPLL_ID_SPLL = 2, >> + >> /* skl */ >> DPLL_ID_SKL_DPLL1 = 0, >> DPLL_ID_SKL_DPLL2 = 1, >> diff --git a/drivers/gpu/drm/i915/intel_crt.c >> b/drivers/gpu/drm/i915/intel_crt.c >> index af5e43bef4a4..592d8fe9f991 100644 >> --- a/drivers/gpu/drm/i915/intel_crt.c >> +++ b/drivers/gpu/drm/i915/intel_crt.c >> @@ -138,18 +138,6 @@ static void hsw_crt_get_config(struct intel_encoder >> *encoder, >> pipe_config->base.adjusted_mode.flags |= intel_crt_get_flags(encoder); >> } >> >> -static void hsw_crt_pre_enable(struct intel_encoder *encoder) >> -{ >> -struct drm_device *dev = encoder->base.dev; >> -struct drm_i915_private *dev_priv = dev->dev_private; >> - >> -WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL already enabled\n"); >> -I915_WRITE(SPLL_CTL, >> - SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC); >> -POSTING_READ(SPLL_CTL); >> -udelay(20); >> -} >> - >> /* Note: The caller is required to filter out dpms modes not supported by >> the >> * platform. */ >> static void intel_crt_set_dpms(struct intel_encoder *encoder, int mode) >> @@ -216,19 +204,6 @@ static void pch_post_disable_crt(struct intel_encoder >> *encoder) >> intel_disable_crt(encoder); >> } >> >> -static void hsw_crt_post_disable(struct intel_encoder *encoder) >> -{ >> -struct drm_device *dev = encoder->base.dev; >> -struct drm_i915_private *dev_priv = dev->dev_private; >> -uint32_t val; >> - >> -DRM_DEBUG_KMS("Disabling SPLL\n"); >> -val = I915_READ(SPLL_CTL); >> -WARN_ON(!(val & SPLL_PLL_ENABLE)); >> -I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE); >> -POSTING_READ(SPLL_CTL); >> -} >> - >> static void intel_enable_crt(struct intel_encoder *encoder) >> { >> struct intel_crt *crt = intel_encoder_to_crt(encoder); >> @@ -280,6 +255,8 @@ static bool intel_crt_compute_config(struct >> intel_encoder *encoder, >> if (HAS_DDI(dev)) { >> pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL; >> pipe_config->port_clock = 135000 * 2; >> +pipe_config->dpll_hw_state.wrpll = >> +SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; >> } >> >> return true; >> @@ -861,8 +838,6 @@ void intel_crt_init(struct drm_device *dev) >> if (HAS_DDI(dev)) { >> crt->base.get_config = hsw_crt_get_config; >> crt->base.get_hw_state = intel_ddi_get_hw_state; >> -crt->base.pre_enable = hsw_crt_pre_enable; >> -crt->base.post_disable = hsw_crt_post_disable; >> } else { >> crt->base.get_config = intel_crt_get_config; >> crt->base.get_hw_state = intel_crt_get_hw_state; >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index 61575f67a626..dabd903147fa 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1269,6 +1269,18 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc, >> } >> >> crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id); >> +} else if (crtc_state->ddi_pll_sel == PORT_CLK_SEL_SPLL) { >> +struct drm_atomic_state *state = crtc_state->base.state; >> +struct intel_shared_dpll_config *spll = >> + >> _atomic_get_shared_dpll_state(state)[DPLL_ID_SPLL]; >> + >> +if (spll->crtc_mask && >> +WARN_ON(spll->hw_state.wrpll != >> crtc_state->dpll_hw_state.wrpll)) >> +return false; >> + >> +crtc_state->shared_dpll = DPLL_ID_SPLL; >> +spll->hw_state.wrpll = crtc_state->dpll_hw_state.wrpll; >> +spll->crtc_mask |= 1 << intel_crtc->pipe; >> } >> >>
Re: [Intel-gfx] [PATCH 09/12] drm/i915: Explain usage of power well IDs vs bit groups
On Wed, Nov 11, 2015 at 09:13:27PM +0200, Imre Deak wrote: > On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote: > > Signed-off-by: Patrik Jakobsson> > --- > > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index e6d88f5..31b3a84 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -621,6 +621,7 @@ enum punit_power_well { > > PUNIT_POWER_WELL_DPIO_RX1 = 11, > > PUNIT_POWER_WELL_DPIO_CMN_D = 12, > > > > + /* Not actual bit groups. Used as IDs for > > lookup_power_well() */ > > PUNIT_POWER_WELL_ALWAYS_ON, > > }; > > > > @@ -633,6 +634,7 @@ enum skl_disp_power_wells { > > SKL_DISP_PW_1 = 14, > > SKL_DISP_PW_2, > > > > + /* Not actual bit groups. Used as IDs for > > lookup_power_well() */ > > It would be good to mention that these IDs are fixed since they are > also used to index HW flags. With that fixed: > Reviewed-by: Imre Deak Good point, will add that. > > > > > > SKL_DISP_PW_ALWAYS_ON, > > }; > > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 10/12] drm/i915/gen9: Turn DC handling into a power well
On to, 2015-11-12 at 13:24 +0100, Patrik Jakobsson wrote: > On Wed, Nov 11, 2015 at 08:57:19PM +0200, Imre Deak wrote: > > On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote: > > > Handle DC off as a power well where enabling the power well will > > > prevent > > > the DMC to enter selected DC states (required around modesets and > > > Aux > > > A). Disabling the power well will allow DC states again. For now > > > the > > > highest DC state is DC6 for Skylake and DC5 for Broxton but will > > > be > > > configurable for Skylake in a later patch. > > > > > > v2: Check both DC5 and DC6 bits in power well enabled function > > > (Ville) > > > > > > Signed-off-by: Patrik Jakobsson> > > > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 6 -- > > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > > drivers/gpu/drm/i915/intel_display.c| 6 ++ > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 110 > > > +++- > > > 4 files changed, 88 insertions(+), 35 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > b/drivers/gpu/drm/i915/i915_drv.c > > > index 5a63f9a..0c7f435 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -1072,9 +1072,6 @@ static int i915_pm_resume(struct device > > > *dev) > > > > > > static int skl_suspend_complete(struct drm_i915_private > > > *dev_priv) > > > { > > > - if (dev_priv->csr.dmc_payload) > > > - skl_enable_dc6(dev_priv); > > > - > > > return 0; > > > } > > > > > > @@ -1119,9 +1116,6 @@ static int bxt_resume_prepare(struct > > > drm_i915_private *dev_priv) > > > > > > static int skl_resume_prepare(struct drm_i915_private *dev_priv) > > > { > > > - if (dev_priv->csr.dmc_payload) > > > - skl_disable_dc6(dev_priv); > > > - > > > return 0; > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > b/drivers/gpu/drm/i915/i915_reg.h > > > index 31b3a84..df445ba 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -636,6 +636,7 @@ enum skl_disp_power_wells { > > > > > > /* Not actual bit groups. Used as IDs for > > > lookup_power_well() */ > > > SKL_DISP_PW_ALWAYS_ON, > > > + SKL_DISP_PW_DC_OFF, > > > > Imo it would be less confusing to call it DC3 power well. Looking > > at th > > e DC spec, DC4 is only a transitory state to DC5/6, so what we > > expect > > when we disable DC6/5 is DC3 or shallower power states (DC2/1/0). > > I've been changing the name quite a few times but settled on "DC off" > to keep it > generic. The main mechanism for the power well is to prevent any DC > states that > can cause us problems during certain operations (i.e. modeset). The > DC states we > need to block could change or be different between platforms. For > that reason I > would prefer not to be as specific with the naming. All the power well names are platform specific so it would be logical and more meaningful to give this one a platform specific name too. Also 'DC off' is kind of a double negation leading to debug messages like 'enable DC off' which isn't that nice. But now that I think of it my idea isn't that great either. 'enable DC3' is confusing too, one could think we enable some power saving state at that point, which is not the case. So let's keep things as-is for now at least: Reviewed-by: Imre Deak > > > > > > }; > > > > > > #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 649ac34..856d801 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -13320,6 +13320,9 @@ static int intel_atomic_commit(struct > > > drm_device *dev, > > > to_intel_crtc_state(crtc->state) > > > ->update_pipe; > > > unsigned long put_domains = 0; > > > > > > + if (modeset) > > > + intel_display_power_get(dev_priv, > > > POWER_DOMAIN_MODESET); > > > + > > > if (modeset && crtc->state->active) { > > > update_scanline_offset(to_intel_crtc(crt > > > c)); > > > dev_priv->display.crtc_enable(crtc); > > > @@ -13343,6 +13346,9 @@ static int intel_atomic_commit(struct > > > drm_device *dev, > > > modeset_put_power_domains(dev_priv, > > > put_domains); > > > > > > intel_post_plane_update(intel_crtc); > > > + > > > + if (modeset) > > > + intel_display_power_put(dev_priv, > > > POWER_DOMAIN_MODESET); > > > } > > > > > > /* FIXME: add subpixel order */ > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > index edf753e..95c3fcc 100644 > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > @@ -49,9
Re: [Intel-gfx] [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.
Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira: > On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote: >> When disable_noatomic is called plane_mask is not reliable yet, >> and plane_state->visible = true even after disabling the primary plane. > So the stale value of plane_state->visible causes a subsequent modeset to > enable > the primary again? Probably not because it would get recalculated in calc_changes, but it should really be set to false afterwards. in intel_plane_atomic_calc_changes: if (!was_crtc_enabled && WARN_ON(was_visible)) was_visible = false; and in intel_sanitize_crtc when intel_check_plane_mapping fails, it gets set to true to disable it. This is really a special case and it's for disable_noatomic only. As the name says it can't rely on the atomic state. >> Fix this by unsetting plane->visible if it was visible, and calling >> disable_planes with the primary plane as mask. >> >> The other planes are already disabled in intel_sanitize_crtc, so >> they don't have to be handled here. >> >> Cc: sta...@vger.kernel.org #v4.3, v4.2? >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655 >> Tested-by: Tomas Mezzadra>> Signed-off-by: Maarten Lankhorst >> --- >> drivers/gpu/drm/i915/intel_display.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index b5f7493213b7..bc3282ab5ed2 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct >> drm_crtc >> *crtc) >> WARN_ON(intel_crtc->unpin_work); >> >> intel_pre_disable_primary(crtc); >> + >> +intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc >> ->primary)); >> +to_intel_plane_state(crtc->primary->state)->visible = false; >> } >> >> -intel_crtc_disable_planes(crtc, crtc->state->plane_mask); > Can't we just make plane_mask reliable? We know whether the primary plane is > enabled and all others were disabled prior to that. Or did I miss something. > In the case of hardware readout you can't. The plane_mask gets set only if we can preserve the initial framebuffer else the primary plane gets disabled. ~Maarten ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Remove redundant check in i915_gem_obj_to_vma
On Thu, 12 Nov 2015, Chris Wilsonwrote: > On Thu, Nov 12, 2015 at 11:59:55AM +, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin >> >> No need to verify VMA belongs to GGTT since: >> >> 1. The function must return a normal VMA belonging to passed in VM. >> 2. There can only be one normal VMA for any VM. >> >> Signed-off-by: Tvrtko Ursulin >> Cc: Chris Wilson > Reviewed-by Chris Wilson Pushed to drm-intel-next-queued, thanks for the patch and review. BR, Jani. > >> Maybe now even a candidate for making a static inline? > > No. After tracking vma, i915_gem_obj_to_vma() is moved off the hotpaths > and mostly used for the first lookup by an execbuffer, context creation, > modesetting and debug code. > -Chris -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: refactor stepping info retrieval
On Wed, 21 Oct 2015, Jani Nikulawrote: > On Wed, 21 Oct 2015, Daniel Vetter wrote: >> On Tue, Oct 20, 2015 at 03:38:33PM +0300, Jani Nikula wrote: >>> Have only one if ladder for platforms and only one range check for >>> size. Makes it easier to handle new platforms. Remove the use of >>> negative return values in char, which might underflow to be positive for >>> some negative error codes. >>> >>> Signed-off-by: Jani Nikula >> >> On the series: Reviewed-by: Daniel Vetter > > Apparently this conflicts with some other firmware patches. I'll let > Imre decide if we should give them priority, or just merge these. Imre gave the green light, series pushed to drm-intel-next-queued, thanks for the review. BR, Jani. > > BR, > Jani. > > >> >>> --- >>> drivers/gpu/drm/i915/intel_csr.c | 46 >>> >>> 1 file changed, 23 insertions(+), 23 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_csr.c >>> b/drivers/gpu/drm/i915/intel_csr.c >>> index fe9a55cf8212..3e7953254803 100644 >>> --- a/drivers/gpu/drm/i915/intel_csr.c >>> +++ b/drivers/gpu/drm/i915/intel_csr.c >>> @@ -188,28 +188,25 @@ static const struct stepping_info bxt_stepping_info[] >>> = { >>> {'B', '0'}, {'B', '1'}, {'B', '2'} >>> }; >>> >>> -static char intel_get_stepping(struct drm_device *dev) >>> +static const struct stepping_info *intel_get_stepping_info(struct >>> drm_device *dev) >>> { >>> - if (IS_SKYLAKE(dev) && (dev->pdev->revision < >>> - ARRAY_SIZE(skl_stepping_info))) >>> - return skl_stepping_info[dev->pdev->revision].stepping; >>> - else if (IS_BROXTON(dev) && (dev->pdev->revision < >>> - ARRAY_SIZE(bxt_stepping_info))) >>> - return bxt_stepping_info[dev->pdev->revision].stepping; >>> - else >>> - return -ENODATA; >>> -} >>> + const struct stepping_info *si; >>> + unsigned int size; >>> + >>> + if (IS_SKYLAKE(dev)) { >>> + size = ARRAY_SIZE(skl_stepping_info); >>> + si = skl_stepping_info; >>> + } else if (IS_BROXTON(dev)) { >>> + size = ARRAY_SIZE(bxt_stepping_info); >>> + si = bxt_stepping_info; >>> + } else { >>> + return NULL; >>> + } >>> >>> -static char intel_get_substepping(struct drm_device *dev) >>> -{ >>> - if (IS_SKYLAKE(dev) && (dev->pdev->revision < >>> - ARRAY_SIZE(skl_stepping_info))) >>> - return skl_stepping_info[dev->pdev->revision].substepping; >>> - else if (IS_BROXTON(dev) && (dev->pdev->revision < >>> - ARRAY_SIZE(bxt_stepping_info))) >>> - return bxt_stepping_info[dev->pdev->revision].substepping; >>> - else >>> - return -ENODATA; >>> + if (INTEL_REVID(dev) < size) >>> + return si + INTEL_REVID(dev); >>> + >>> + return NULL; >>> } >>> >>> /** >>> @@ -296,8 +293,8 @@ static void finish_csr_load(const struct firmware *fw, >>> void *context) >>> struct intel_package_header *package_header; >>> struct intel_dmc_header *dmc_header; >>> struct intel_csr *csr = _priv->csr; >>> - char stepping = intel_get_stepping(dev); >>> - char substepping = intel_get_substepping(dev); >>> + const struct stepping_info *stepping_info = >>> intel_get_stepping_info(dev); >>> + char stepping, substepping; >>> uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes; >>> uint32_t i; >>> uint32_t *dmc_payload; >>> @@ -308,11 +305,14 @@ static void finish_csr_load(const struct firmware >>> *fw, void *context) >>> goto out; >>> } >>> >>> - if ((stepping == -ENODATA) || (substepping == -ENODATA)) { >>> + if (!stepping_info) { >>> DRM_ERROR("Unknown stepping info, firmware loading failed\n"); >>> goto out; >>> } >>> >>> + stepping = stepping_info->stepping; >>> + substepping = stepping_info->substepping; >>> + >>> /* Extract CSS Header information*/ >>> css_header = (struct intel_css_header *)fw->data; >>> if (sizeof(struct intel_css_header) != >>> -- >>> 2.1.4 >>> >>> ___ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/10] drm/i915: fix the power well ID for always on wells
On Wed, Nov 04, 2015 at 07:24:10PM +0200, Imre Deak wrote: > lookup_power_well() expects uniq power well IDs, but atm we have > uninitialized IDs which would clash with those power wells with a 0 > ID. This wasn't a problem so far since nothing looked up such a power > well, but an upcoming patch will (Misc IO for SKL), so fix this up on > platforms where this matters. I think the concept of using these bit positions as IDs for lookup_power_well() is a bit confusing. "Always on" and later on "DC off" are not bits in any register so IMO they don't belong in ->data. With that said, I don't think it's worth fixing right here and now and since we add some comments about this in later patches I'm ok with this. Reviewed-by: Patrik Jakobsson> > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_reg.h | 4 +++- > drivers/gpu/drm/i915/intel_runtime_pm.c | 2 ++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 72bbed2..c103f8d 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -621,7 +621,7 @@ enum punit_power_well { > PUNIT_POWER_WELL_DPIO_RX1 = 11, > PUNIT_POWER_WELL_DPIO_CMN_D = 12, > > - PUNIT_POWER_WELL_NUM, > + PUNIT_POWER_WELL_ALWAYS_ON, > }; > > enum skl_disp_power_wells { > @@ -632,6 +632,8 @@ enum skl_disp_power_wells { > SKL_DISP_PW_DDI_D, > SKL_DISP_PW_1 = 14, > SKL_DISP_PW_2, > + > + SKL_DISP_PW_ALWAYS_ON, > }; > > #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 3a989a7..fc5552c 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -1633,6 +1633,7 @@ static struct i915_power_well vlv_power_wells[] = { > .always_on = 1, > .domains = VLV_ALWAYS_ON_POWER_DOMAINS, > .ops = _always_on_power_well_ops, > + .data = PUNIT_POWER_WELL_ALWAYS_ON, > }, > { > .name = "display", > @@ -1734,6 +1735,7 @@ static struct i915_power_well skl_power_wells[] = { > .always_on = 1, > .domains = SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS, > .ops = _always_on_power_well_ops, > + .data = SKL_DISP_PW_ALWAYS_ON, > }, > { > .name = "power well 1", > -- > 2.1.4 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/10] drm/i915: fix the power well ID for always on wells
On Wed, Nov 04, 2015 at 07:24:10PM +0200, Imre Deak wrote: > lookup_power_well() expects uniq power well IDs, but atm we have > uninitialized IDs which would clash with those power wells with a 0 > ID. This wasn't a problem so far since nothing looked up such a power > well, but an upcoming patch will (Misc IO for SKL), so fix this up on > platforms where this matters. I thought we were moving the MISCIO and PW1 out from the power well framework? I think I'd rather do that than to fabricate stuff for .data. > > Signed-off-by: Imre Deak> --- > drivers/gpu/drm/i915/i915_reg.h | 4 +++- > drivers/gpu/drm/i915/intel_runtime_pm.c | 2 ++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 72bbed2..c103f8d 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -621,7 +621,7 @@ enum punit_power_well { > PUNIT_POWER_WELL_DPIO_RX1 = 11, > PUNIT_POWER_WELL_DPIO_CMN_D = 12, > > - PUNIT_POWER_WELL_NUM, > + PUNIT_POWER_WELL_ALWAYS_ON, > }; > > enum skl_disp_power_wells { > @@ -632,6 +632,8 @@ enum skl_disp_power_wells { > SKL_DISP_PW_DDI_D, > SKL_DISP_PW_1 = 14, > SKL_DISP_PW_2, > + > + SKL_DISP_PW_ALWAYS_ON, > }; > > #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 3a989a7..fc5552c 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -1633,6 +1633,7 @@ static struct i915_power_well vlv_power_wells[] = { > .always_on = 1, > .domains = VLV_ALWAYS_ON_POWER_DOMAINS, > .ops = _always_on_power_well_ops, > + .data = PUNIT_POWER_WELL_ALWAYS_ON, > }, > { > .name = "display", > @@ -1734,6 +1735,7 @@ static struct i915_power_well skl_power_wells[] = { > .always_on = 1, > .domains = SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS, > .ops = _always_on_power_well_ops, > + .data = SKL_DISP_PW_ALWAYS_ON, > }, > { > .name = "power well 1", > -- > 2.1.4 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] PSR Critical fixes
>-Original Message- >From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of >Rodrigo Vivi >Sent: Thursday, November 12, 2015 1:07 AM >To: intel-gfx@lists.freedesktop.org >Cc: Vivi, Rodrigo >Subject: [Intel-gfx] [PATCH 0/4] PSR Critical fixes > >Let's split critical PSR fixes from the series that contains other >reworks, stabilization and improvements. That really helped in review ;-) For patches 2,3,4 in this series: Reviewed-by: Durgadoss RThanks, Durga > >The second patch in this series isn't considered critical in terms >of functionality, but it depends on the first one and it can be consider >a fix for PSR residency on VLV/CHV. > >Thanks, >Rodrigo. > >Rodrigo Vivi (4): > drm/i915: Delay first PSR activation. > drm/i915: Reduce PSR re-activation time for VLV/CHV. > drm/i915: PSR: Don't Skip aux handshake on DP_PSR_NO_TRAIN_ON_EXIT. > drm/i915: Send TP1 TP2/3 even when panel claims no NO_TRAIN_ON_EXIT. > > drivers/gpu/drm/i915/intel_psr.c | 24 > 1 file changed, 16 insertions(+), 8 deletions(-) > >-- >2.4.3 > >___ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [V2 PATCH 1/2] dp/mst: add SDP stream support
Hi Jani, Do you have time to have a review? Thanks. Regards, Libin > -Original Message- > From: libin.y...@linux.intel.com [mailto:libin.y...@linux.intel.com] > Sent: Wednesday, November 11, 2015 1:33 PM > To: intel-gfx@lists.freedesktop.org; jani.nik...@linux.intel.com; Vetter, > Daniel; ville.syrj...@linux.intel.com > Cc: Yang, Libin; Libin Yang; Dave Airlie > Subject: [V2 PATCH 1/2] dp/mst: add SDP stream support > > From: Libin Yang> > This adds code to initialise the SDP streams > for a sink in the simplest ordering. > > I've no idea how you'd want to control the > ordering at this level, so don't bother > until someone comes up with a use case. > > Signed-off-by: Libin Yang > Signed-off-by: Dave Airlie > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 38 > --- > include/drm/drm_dp_mst_helper.h | 7 +-- > 2 files changed, 40 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 809959d..f50eb7b 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -666,7 +666,9 @@ static int build_enum_path_resources(struct > drm_dp_sideband_msg_tx *msg, int por > } > > static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, > int port_num, > - u8 vcpi, uint16_t pbn) > + u8 vcpi, uint16_t pbn, > + u8 number_sdp_streams, > + u8 *sdp_stream_sink) > { > struct drm_dp_sideband_msg_req_body req; > memset(, 0, sizeof(req)); > @@ -674,6 +676,9 @@ static int build_allocate_payload(struct > drm_dp_sideband_msg_tx *msg, int port_n > req.u.allocate_payload.port_number = port_num; > req.u.allocate_payload.vcpi = vcpi; > req.u.allocate_payload.pbn = pbn; > + req.u.allocate_payload.number_sdp_streams = > number_sdp_streams; > + memcpy(req.u.allocate_payload.sdp_stream_sink, > sdp_stream_sink, > +number_sdp_streams); > drm_dp_encode_sideband_req(, msg); > msg->path_msg = true; > return 0; > @@ -1562,6 +1567,8 @@ static int drm_dp_payload_send_msg(struct > drm_dp_mst_topology_mgr *mgr, > struct drm_dp_sideband_msg_tx *txmsg; > struct drm_dp_mst_branch *mstb; > int len, ret; > + u8 sinks[DRM_DP_MAX_SDP_STREAMS]; > + int i; > > mstb = drm_dp_get_validated_mstb_ref(mgr, port->parent); > if (!mstb) > @@ -1573,10 +1580,13 @@ static int > drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr, > goto fail_put; > } > > + for (i = 0; i < port->num_sdp_streams; i++) > + sinks[i] = i; > + > txmsg->dst = mstb; > len = build_allocate_payload(txmsg, port->port_num, >id, > - pbn); > + pbn, port->num_sdp_streams, sinks); > > drm_dp_queue_down_tx(mgr, txmsg); > > @@ -2259,6 +2269,27 @@ out: > EXPORT_SYMBOL(drm_dp_mst_detect_port); > > /** > + * drm_dp_mst_port_has_audio() - Check whether port has audio > capability or not > + * @mgr: manager for this port > + * @port: unverified pointer to a port. > + * > + * This returns whether the port supports audio or not. > + */ > +bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr > *mgr, > + struct drm_dp_mst_port *port) > +{ > + bool ret = false; > + > + port = drm_dp_get_validated_port_ref(mgr, port); > + if (!port) > + return ret; > + ret = port->has_audio; > + drm_dp_put_port(port); > + return ret; > +} > +EXPORT_SYMBOL(drm_dp_mst_port_has_audio); > + > +/** > * drm_dp_mst_get_edid() - get EDID for an MST port > * @connector: toplevel connector to get EDID for > * @mgr: manager for this port > @@ -2283,6 +2314,7 @@ struct edid *drm_dp_mst_get_edid(struct > drm_connector *connector, struct drm_dp_ > edid = drm_get_edid(connector, >aux.ddc); > drm_mode_connector_set_tile_property(connector); > } > + port->has_audio = drm_detect_monitor_audio(edid); > drm_dp_put_port(port); > return edid; > } > @@ -2566,7 +2598,7 @@ static void drm_dp_mst_dump_mstb(struct > seq_file *m, > > seq_printf(m, "%smst: %p, %d\n", prefix, mstb, mstb- > >num_ports); > list_for_each_entry(port, >ports, next) { > - seq_printf(m, "%sport: %d: ddps: %d ldps: %d, %p, > conn: %p\n", prefix, port->port_num, port->ddps, port->ldps, port, port- > >connector); > + seq_printf(m, "%sport: %d: ddps: %d ldps: %d, > sdp: %d/%d, %p, conn: %p\n", prefix, port->port_num, port->ddps, port- > >ldps, port->num_sdp_streams, port->num_sdp_stream_sinks, port, > port->connector); > if
Re: [Intel-gfx] [PATCH v3 03/13] drm/i915/gen9: move assert_csr_loaded into intel_rpm.c
On Thu, 12 Nov 2015, Imre Deakwrote: > On ke, 2015-10-28 at 23:58 +0200, Imre Deak wrote: >> From: Daniel Vetter >> >> Avoids non-static functions since all the callers are in intel_rpm.c. >> >> Cc: Damien Lespiau >> Cc: Imre Deak >> Cc: Sunil Kamath >> Signed-off-by: Daniel Vetter >> Signed-off-by: Animesh Manna >> [imre: removed note about reg definitions from commit message, since >> it's not relevant any more] >> Reviewed-by: Imre Deak >> --- >> drivers/gpu/drm/i915/intel_csr.c| 10 -- >> drivers/gpu/drm/i915/intel_drv.h| 1 - >> drivers/gpu/drm/i915/intel_runtime_pm.c | 8 >> 3 files changed, 8 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_csr.c >> b/drivers/gpu/drm/i915/intel_csr.c >> index 2c9bf3f..cd6fb58d 100644 >> --- a/drivers/gpu/drm/i915/intel_csr.c >> +++ b/drivers/gpu/drm/i915/intel_csr.c >> @@ -485,13 +485,3 @@ void intel_csr_ucode_fini(struct drm_device >> *dev) >> intel_csr_load_status_set(dev_priv, FW_FAILED); >> kfree(dev_priv->csr.dmc_payload); >> } >> - >> -void assert_csr_loaded(struct drm_i915_private *dev_priv) >> -{ >> -WARN_ONCE(intel_csr_load_status_get(dev_priv) != FW_LOADED, >> - "CSR is not loaded.\n"); >> -WARN_ONCE(!I915_READ(CSR_PROGRAM(0)), >> - "CSR program storage start is NULL\n"); >> -WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not >> fine\n"); >> -WARN_ONCE(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n"); >> -} >> diff --git a/drivers/gpu/drm/i915/intel_drv.h >> b/drivers/gpu/drm/i915/intel_drv.h >> index 1a3bbdc..3d1c744 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1211,7 +1211,6 @@ void intel_csr_load_status_set(struct >> drm_i915_private *dev_priv, >> enum csr_state state); >> void intel_csr_load_program(struct drm_device *dev); >> void intel_csr_ucode_fini(struct drm_device *dev); >> -void assert_csr_loaded(struct drm_i915_private *dev_priv); >> >> /* intel_dp.c */ >> void intel_dp_init(struct drm_device *dev, int output_reg, enum port >> port); >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c >> b/drivers/gpu/drm/i915/intel_runtime_pm.c >> index e50cc88..92746e1 100644 >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c >> @@ -458,6 +458,14 @@ static void >> gen9_set_dc_state_debugmask_memory_up( >> } >> } >> >> +void assert_csr_loaded(struct drm_i915_private *dev_priv) > > I missed it during review, but this is static. With that added my R-b > still holds. Pushed up to and including this patch, with the static added. The next one doesn't apply cleanly. BR, Jani. > >> +{ >> +WARN_ONCE(!I915_READ(CSR_PROGRAM(0)), >> + "CSR program storage start is NULL\n"); >> +WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not >> fine\n"); >> +WARN_ONCE(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n"); >> +} >> + >> static void assert_can_enable_dc5(struct drm_i915_private *dev_priv) >> { >> struct drm_device *dev = dev_priv->dev; -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.
On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote: > When disable_noatomic is called plane_mask is not reliable yet, > and plane_state->visible = true even after disabling the primary plane. So the stale value of plane_state->visible causes a subsequent modeset to enable the primary again? > Fix this by unsetting plane->visible if it was visible, and calling > disable_planes with the primary plane as mask. > > The other planes are already disabled in intel_sanitize_crtc, so > they don't have to be handled here. > > Cc: sta...@vger.kernel.org #v4.3, v4.2? > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655 > Tested-by: Tomas Mezzadra> Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/i915/intel_display.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index b5f7493213b7..bc3282ab5ed2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc > *crtc) > WARN_ON(intel_crtc->unpin_work); > > intel_pre_disable_primary(crtc); > + > + intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc > ->primary)); > + to_intel_plane_state(crtc->primary->state)->visible = false; > } > > - intel_crtc_disable_planes(crtc, crtc->state->plane_mask); Can't we just make plane_mask reliable? We know whether the primary plane is enabled and all others were disabled prior to that. Or did I miss something. Ander > dev_priv->display.crtc_disable(crtc); > intel_crtc->active = false; > intel_update_watermarks(crtc); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/10] drm/i915: fix lookup_power_well for power wells without any domain
On Wed, Nov 04, 2015 at 07:24:11PM +0200, Imre Deak wrote: > The current lookup code wouldn't find a power well if it's not in any > power domain. There wasn't any power wells before but an upcoming patch > will detach the power domains from power well#1 and the MISC IO power > wells, so fix things up accordingly. > Reviewed-by: Patrik Jakobsson> Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index fc5552c..868c0f2 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -962,10 +962,12 @@ static struct i915_power_well *lookup_power_well(struct > drm_i915_private *dev_pr >int power_well_id) > { > struct i915_power_domains *power_domains = _priv->power_domains; > - struct i915_power_well *power_well; > int i; > > - for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) { > + for (i = 0; i < power_domains->power_well_count; i++) { > + struct i915_power_well *power_well; > + > + power_well = _domains->power_wells[i]; > if (power_well->data == power_well_id) > return power_well; > } > -- > 2.1.4 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR activation.
Hi Rodrigo, >-Original Message- >From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of >Rodrigo Vivi >Sent: Thursday, November 12, 2015 1:07 AM >To: intel-gfx@lists.freedesktop.org >Cc: Vivi, Rodrigo >Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR activation. > >When debuging the frozen screen caused by HW tracking with low >power state I noticed that if we keep moving the mouse non stop >you will miss the screen updates for a while. At least >until we stop moving the mouse for a small time and move again. > >The actual enabling should happen immediately after >Display Port enabling sequence finished with links trained and >everything enabled. However we face many issues when enabling PSR >right after a modeset. > >On VLV/CHV we face blank screens on this scenario and on HSW+ >we face a recoverable frozen screen, at least until next >exit-activate sequence. > >Another workaround for the same issue here would be to increase >re-enable idle time from 100 to 500 as we did for VLV/CHV. >However this patch workaround this issue in a better >way since it doesn't reduce PSR residency and also >allow us to reduce the delay time between re-enables at least >on VLV/CHV. > >This is also important to make the sysfs toggle working properly. > >Signed-off-by: Rodrigo Vivi>--- > drivers/gpu/drm/i915/intel_psr.c | 18 -- > 1 file changed, 16 insertions(+), 2 deletions(-) > >diff --git a/drivers/gpu/drm/i915/intel_psr.c >b/drivers/gpu/drm/i915/intel_psr.c >index 213581c..6b24c24 100644 >--- a/drivers/gpu/drm/i915/intel_psr.c >+++ b/drivers/gpu/drm/i915/intel_psr.c >@@ -427,6 +427,19 @@ void intel_psr_enable(struct intel_dp *intel_dp) > vlv_psr_enable_source(intel_dp); > } > >+ /* >+ * FIXME: Activation should happen immediately since this function >+ * is just called after pipe is fully trained and enabled. >+ * However on every platform we face issues when first activation >+ * follows a modeset so quickly. >+ * - On VLV/CHV we get bank screen on first activation >+ * - On HSW/BDW we get a recoverable frozen screen until next >+ * exit-activate sequence. >+ */ >+ if (INTEL_INFO(dev)->gen < 9) >+ schedule_delayed_work(_priv->psr.work, >+ >msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5)); >+ > dev_priv->psr.enabled = intel_dp; > unlock: > mutex_unlock(_priv->psr.lock); >@@ -735,8 +748,9 @@ void intel_psr_flush(struct drm_device *dev, > } > > if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) >- schedule_delayed_work(_priv->psr.work, >-msecs_to_jiffies(delay_ms)); >+ if (!work_busy(_priv->psr.work.work)) >+ schedule_delayed_work(_priv->psr.work, >+msecs_to_jiffies(delay_ms)); Agree with the theory of the patch as such.. But, Is there any specific reason for the !work_busy() check here ? I believe when the later work runs, it will anyway bail out in _activate function, if it sees PSR_ENABLE bit set already. So, is this check just to prevent scheduling one more work item when there is one pending already ? (or it helps in something else also ?) Thanks, Durga > mutex_unlock(_priv->psr.lock); > } > >-- >2.4.3 > >___ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/10] drm/i915: fix the power well ID for always on wells
On to, 2015-11-12 at 15:39 +0200, Ville Syrjälä wrote: > On Wed, Nov 04, 2015 at 07:24:10PM +0200, Imre Deak wrote: > > lookup_power_well() expects uniq power well IDs, but atm we have > > uninitialized IDs which would clash with those power wells with a 0 > > ID. This wasn't a problem so far since nothing looked up such a > > power > > well, but an upcoming patch will (Misc IO for SKL), so fix this up > > on > > platforms where this matters. > > I thought we were moving the MISCIO and PW1 out from the power well > framework? I think I'd rather do that than to fabricate stuff for > .data. That's a cleaner way yes, but imo we could do that refactoring as a follow-up, thinking about the other users of lookup_power_well() at the same time. This change makes sense in any case, since whenever you use the lookup_power_well function you need to have unique IDs in place. > > Signed-off-by: Imre Deak> > --- > > drivers/gpu/drm/i915/i915_reg.h | 4 +++- > > drivers/gpu/drm/i915/intel_runtime_pm.c | 2 ++ > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 72bbed2..c103f8d 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -621,7 +621,7 @@ enum punit_power_well { > > PUNIT_POWER_WELL_DPIO_RX1 = 11, > > PUNIT_POWER_WELL_DPIO_CMN_D = 12, > > > > - PUNIT_POWER_WELL_NUM, > > + PUNIT_POWER_WELL_ALWAYS_ON, > > }; > > > > enum skl_disp_power_wells { > > @@ -632,6 +632,8 @@ enum skl_disp_power_wells { > > SKL_DISP_PW_DDI_D, > > SKL_DISP_PW_1 = 14, > > SKL_DISP_PW_2, > > + > > + SKL_DISP_PW_ALWAYS_ON, > > }; > > > > #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index 3a989a7..fc5552c 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -1633,6 +1633,7 @@ static struct i915_power_well > > vlv_power_wells[] = { > > .always_on = 1, > > .domains = VLV_ALWAYS_ON_POWER_DOMAINS, > > .ops = _always_on_power_well_ops, > > + .data = PUNIT_POWER_WELL_ALWAYS_ON, > > }, > > { > > .name = "display", > > @@ -1734,6 +1735,7 @@ static struct i915_power_well > > skl_power_wells[] = { > > .always_on = 1, > > .domains = SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS, > > .ops = _always_on_power_well_ops, > > + .data = SKL_DISP_PW_ALWAYS_ON, > > }, > > { > > .name = "power well 1", > > -- > > 2.1.4 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6] drm/i915: respect the VBT minimum backlight brightness again
On Thu, 12 Nov 2015, Stéphane ANCELOTwrote: > Hi, > I have seen you were working on brightness. Right, but this is useful > only if many worlwide users would be able to activate it. > I have never been able to make it working on my intel systemes (core i5 > - 4300u , core i3 ...) Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI=DRM/Intel BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/10] drm/i915: Make turning on/off PW1 and Misc I/O part of the init/fini sequences
On Wed, Nov 04, 2015 at 07:24:12PM +0200, Imre Deak wrote: > From: Damien Lespiau> > Before this patch, we used the intel_display_power_{get,put} functions > to make sure the PW1 and Misc I/O power wells were enabled all the > time while LCPLL was enabled. We called a get() at > intel_ddi_pll_init() when we discovered that LCPLL was enabled, then > we would call put/get at skl_{un,}init_cdclk(). > > The problem is that skl_uninit_cdclk() is indirectly called by > intel_runtime_suspend(). So it will only release its power well > _after_ we already decided to runtime suspend. But since we only > decide to runtime suspend after all power wells and refcounts are > released, that basically means we will never decide to runtime > suspend. > > So what this patch does to fix that problem is move the PW1 + Misc I/O > power well handling out of the runtime PM mechanism: instead of > calling intel_display_power_{get_put} - functions that touch the > refcount -, we'll call the low level intel_power_well_{en,dis}able, > which don't change the refcount. This way, it is now possible for the > refcount to actually reach zero, and we'll now start runtime > suspending/resuming. > > v2 (from Paulo): > - Write a commit message since the original patch left it empty. > - Rebase after the intel_power_well_{en,dis}able rename. > - Use lookup_power_well() instead of hardcoded indexes. > > Testcase: igt/pm_rpm/rte (and every other rpm test) > Signed-off-by: Damien Lespiau > Reviewed-by: Paulo Zanoni > Signed-off-by: Paulo Zanoni Reviewed-by: Patrik Jakobsson > --- > drivers/gpu/drm/i915/intel_ddi.c| 4 ++-- > drivers/gpu/drm/i915/intel_display.c| 5 +++-- > drivers/gpu/drm/i915/intel_drv.h| 2 ++ > drivers/gpu/drm/i915/intel_runtime_pm.c | 29 + > 4 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index b164122..cacc406 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2951,8 +2951,8 @@ void intel_ddi_pll_init(struct drm_device *dev) > dev_priv->skl_boot_cdclk = cdclk_freq; > if (skl_sanitize_cdclk(dev_priv)) > DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); > - else > - intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS); > + if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) > + DRM_ERROR("LCPLL1 is disabled\n"); > } else if (IS_BROXTON(dev)) { > broxton_init_cdclk(dev); > broxton_ddi_phy_init(dev); > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 7b3cfb6..ef2ebcd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5702,7 +5702,8 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv) > DRM_ERROR("Couldn't disable DPLL0\n"); > } > > - intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS); > + /* disable PG1 and Misc I/O */ > + skl_pw1_misc_io_fini(dev_priv); > } > > void skl_init_cdclk(struct drm_i915_private *dev_priv) > @@ -5715,7 +5716,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) > I915_WRITE(HSW_NDE_RSTWRN_OPT, val | RESET_PCH_HANDSHAKE_ENABLE); > > /* enable PG1 and Misc I/O */ > - intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS); > + skl_pw1_misc_io_init(dev_priv); > > /* DPLL0 not enabled (happens on early BIOS versions) */ > if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) { > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 5bc744a..3d9d1d3 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1381,6 +1381,8 @@ void intel_psr_single_frame_update(struct drm_device > *dev, > int intel_power_domains_init(struct drm_i915_private *); > void intel_power_domains_fini(struct drm_i915_private *); > void intel_power_domains_init_hw(struct drm_i915_private *dev_priv); > +void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv); > +void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv); > void intel_runtime_pm_enable(struct drm_i915_private *dev_priv); > > bool intel_display_power_is_enabled(struct drm_i915_private *dev_priv, > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 868c0f2..16691a3 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -1783,6 +1783,35 @@ static struct i915_power_well skl_power_wells[] = { > }, > }; > > +void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv) > +{ > + struct
Re: [Intel-gfx] [PATCH v7 0/3] fbdev fixes (reviewed)
Hi Jani, On Mon, Nov 09, 2015 at 04:23:34PM +0200, Jani Nikula wrote: > On Sat, 07 Nov 2015, Lukas Wunnerwrote: > > three patches with fbdev deadlock & failure path fixes, > > each with Reviewed-by tag by Ville or Daniel, the third one > > with amended commit message as requested by Daniel in > > <20151030182818.GR16848@phenom.ffwll.local>. > Pushed all three to drm-intel-next-queued. Thanks for the patches and > review. Thank you! > Also, I don't know how you generate and send your patches, but even the > updated patches had dates of the original or very old versions of > them. Like [1] is June 30 although you sent it just a couple of days > ago. Please look into that. git format-patch tries to tunnel the author date through RFC 5322 by setting the Date-header to it. git send-email, which most people use, strips that and replaces it with the date of its invocation. I use msmtp instead of git send-email, which preserves the Date-header. When I edit commits, git commit --amend updates the commit date but not the author date. That's why you see these ancient timestamps. If you find that annoying I'll see to it that I modify the author date manually before sending out a new series. (At least in commits of my own. I try to avoid tampering with other people's commits.) > For future reference, please consider posting new versions of series as > new threads. This one got pretty messy in the end, with so many > different versions. Daniel asked me to submit a patch "in-reply the previous version" in <20150922091757.GZ3383@phenom.ffwll.local> and I adhered to that request also when sending a new version of an entire series. In that case I'll *not* submit in-reply-to in the future, got that. Best regards, Lukas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 03/13] drm/i915/gen9: move assert_csr_loaded into intel_rpm.c
On to, 2015-11-12 at 16:48 +0200, Jani Nikula wrote: > On Thu, 12 Nov 2015, Imre Deakwrote: > > On ke, 2015-10-28 at 23:58 +0200, Imre Deak wrote: > > > From: Daniel Vetter > > > > > > Avoids non-static functions since all the callers are in > > > intel_rpm.c. > > > > > > Cc: Damien Lespiau > > > Cc: Imre Deak > > > Cc: Sunil Kamath > > > Signed-off-by: Daniel Vetter > > > Signed-off-by: Animesh Manna > > > [imre: removed note about reg definitions from commit message, > > > since > > > it's not relevant any more] > > > Reviewed-by: Imre Deak > > > --- > > > drivers/gpu/drm/i915/intel_csr.c| 10 -- > > > drivers/gpu/drm/i915/intel_drv.h| 1 - > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 8 > > > 3 files changed, 8 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > > > b/drivers/gpu/drm/i915/intel_csr.c > > > index 2c9bf3f..cd6fb58d 100644 > > > --- a/drivers/gpu/drm/i915/intel_csr.c > > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > > @@ -485,13 +485,3 @@ void intel_csr_ucode_fini(struct drm_device > > > *dev) > > > intel_csr_load_status_set(dev_priv, FW_FAILED); > > > kfree(dev_priv->csr.dmc_payload); > > > } > > > - > > > -void assert_csr_loaded(struct drm_i915_private *dev_priv) > > > -{ > > > - WARN_ONCE(intel_csr_load_status_get(dev_priv) != > > > FW_LOADED, > > > - "CSR is not loaded.\n"); > > > - WARN_ONCE(!I915_READ(CSR_PROGRAM(0)), > > > - "CSR program storage start is NULL\n"); > > > - WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not > > > fine\n"); > > > - WARN_ONCE(!I915_READ(CSR_HTP_SKL), "CSR HTP Not > > > fine\n"); > > > -} > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index 1a3bbdc..3d1c744 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1211,7 +1211,6 @@ void intel_csr_load_status_set(struct > > > drm_i915_private *dev_priv, > > > enum csr_state state); > > > void intel_csr_load_program(struct drm_device *dev); > > > void intel_csr_ucode_fini(struct drm_device *dev); > > > -void assert_csr_loaded(struct drm_i915_private *dev_priv); > > > > > > /* intel_dp.c */ > > > void intel_dp_init(struct drm_device *dev, int output_reg, enum > > > port > > > port); > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > index e50cc88..92746e1 100644 > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > @@ -458,6 +458,14 @@ static void > > > gen9_set_dc_state_debugmask_memory_up( > > > } > > > } > > > > > > +void assert_csr_loaded(struct drm_i915_private *dev_priv) > > > > I missed it during review, but this is static. With that added my R > > -b > > still holds. > > Pushed up to and including this patch, with the static added. The > next > one doesn't apply cleanly. Ok, there were two minor conflicts with Mika's firmware version patchse t that got merged meanwhile. I resend those two patches as v4. --Imre > > BR, > Jani. > > > > > > > +{ > > > + WARN_ONCE(!I915_READ(CSR_PROGRAM(0)), > > > + "CSR program storage start is NULL\n"); > > > + WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not > > > fine\n"); > > > + WARN_ONCE(!I915_READ(CSR_HTP_SKL), "CSR HTP Not > > > fine\n"); > > > +} > > > + > > > static void assert_can_enable_dc5(struct drm_i915_private > > > *dev_priv) > > > { > > > struct drm_device *dev = dev_priv->dev; > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v4 04/13] drm/i915/gen9: Remove csr.state, csr_lock and related code.
From: Daniel VetterThis removes two anti-patterns: - Locking shouldn't be used to synchronize with async work (of any form, whether callbacks, workers or other threads). This is what the mutex_lock/unlock seems to have been for in intel_csr_load_program. Instead ordering should be ensured with the generic wait_for_completion()/complete(). Or more specific functions provided by the core kernel like e.g. flush_work()/cancel_work_sync() in the case of synchronizing with a work item. - Don't invent own completion like the following code did with the (already removed) wait_for(csr_load_status_get()) pattern - it's really hard to get these right when you want them to be _really_ correct (and be fast) in all cases. Furthermore it's easier to read code using the well-known primitives than new ones using non-standard names. Before enabling/disabling DC6 check if the firmware is loaded successfully. This is guaranteed during runtime s/r, since otherwise we don't enable RPM, but not during system s/r. Note that it's still unclear whether we need to enable/disable DC6 during system s/r, until that's clarified, keep the current behavior and enable/disable DC6. Also after this patch there is a race during system s/r where the firmware may not be loaded yet, that's addressed in an upcoming patch. v2-v3: - unchanged v4: - rebased on latest drm-intel-nightly Cc: Damien Lespiau Cc: Imre Deak Cc: Sunil Kamath Signed-off-by: Daniel Vetter Signed-off-by: Animesh Manna [imre: added code and note about checking if the firmware loaded ok, before enabling/disabling it] Reviewed-by: Animesh Manna Signed-off-by: Imre Deak --- drivers/gpu/drm/i915/i915_dma.c | 1 - drivers/gpu/drm/i915/i915_drv.c | 11 ++-- drivers/gpu/drm/i915/i915_drv.h | 10 --- drivers/gpu/drm/i915/intel_csr.c| 46 + drivers/gpu/drm/i915/intel_drv.h| 3 --- drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ++-- 6 files changed, 5 insertions(+), 83 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index cbed87a..2326189 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -892,7 +892,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) spin_lock_init(_priv->mmio_flip_lock); mutex_init(_priv->sb_lock); mutex_init(_priv->modeset_restore_lock); - mutex_init(_priv->csr_lock); mutex_init(_priv->av_mutex); intel_pm_setup(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f55209..aa34fcb 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1087,18 +1087,11 @@ static int i915_pm_resume(struct device *dev) static int skl_suspend_complete(struct drm_i915_private *dev_priv) { - enum csr_state state; /* Enabling DC6 is not a hard requirement to enter runtime D3 */ skl_uninit_cdclk(dev_priv); - /* TODO: wait for a completion event or -* similar here instead of busy -* waiting using wait_for function. -*/ - wait_for((state = intel_csr_load_status_get(dev_priv)) != - FW_UNINITIALIZED, 1000); - if (state == FW_LOADED) + if (dev_priv->csr.dmc_payload) skl_enable_dc6(dev_priv); return 0; @@ -1147,7 +1140,7 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv) { struct drm_device *dev = dev_priv->dev; - if (intel_csr_load_status_get(dev_priv) == FW_LOADED) + if (dev_priv->csr.dmc_payload) skl_disable_dc6(dev_priv); skl_init_cdclk(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d5cf30b..389b8dd 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -738,12 +738,6 @@ struct intel_uncore { #define CSR_VERSION_MAJOR(version) ((version) >> 16) #define CSR_VERSION_MINOR(version) ((version) & 0x) -enum csr_state { - FW_UNINITIALIZED = 0, - FW_LOADED, - FW_FAILED -}; - struct intel_csr { const char *fw_path; uint32_t *dmc_payload; @@ -752,7 +746,6 @@ struct intel_csr { uint32_t mmio_count; uint32_t mmioaddr[8]; uint32_t mmiodata[8]; - enum csr_state state; }; #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ @@ -1709,9 +1702,6 @@ struct drm_i915_private { struct intel_csr csr; - /* Display CSR-related protection */ - struct mutex csr_lock; - struct intel_gmbus gmbus[GMBUS_NUM_PINS]; /** gmbus_mutex protects against concurrent usage of the single hw gmbus diff --git
Re: [Intel-gfx] [PATCH v3 00/13] drm/i915: Redesign dmc firmware loading.
On Wed, 28 Oct 2015, Imre Deakwrote: > This is a rebased version of [1], addressing the review comments. It > depends on Mika's FW version blacklisting/capture patchset [2]. Entire series pushed to drm-intel-next-queued, thanks for everyone involved. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v4 09/13] drm/i915/gen9: extract parse_csr_fw
From: Daniel VetterThe loader function will get a bit more complicated soon, extract the parsing code to make the control flow clearer. While doing that just use dev_priv->csr.dmc_payload as the indicator for whether it all suceeded or not. v2-v3: - unchanged v4: - rebased on top of latest drm-intel-nightly Cc: Damien Lespiau Cc: Imre Deak Cc: Sunil Kamath Signed-off-by: Daniel Vetter Signed-off-by: Animesh Manna [imre: remove note about BE cast from commit message, it's not relevant any more] Reviewed-by: Imre Deak Signed-off-by: Imre Deak --- drivers/gpu/drm/i915/intel_csr.c | 51 +--- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index d14d11c..496f54b 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -235,9 +235,9 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) } } -static void finish_csr_load(const struct firmware *fw, void *context) +static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, + const struct firmware *fw) { - struct drm_i915_private *dev_priv = context; struct drm_device *dev = dev_priv->dev; struct intel_css_header *css_header; struct intel_package_header *package_header; @@ -248,14 +248,13 @@ static void finish_csr_load(const struct firmware *fw, void *context) uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes; uint32_t i; uint32_t *dmc_payload; - bool fw_loaded = false; if (!fw) - goto out; + return NULL; if (!stepping_info) { DRM_ERROR("Unknown stepping info, firmware loading failed\n"); - goto out; + return NULL; } stepping = stepping_info->stepping; @@ -267,7 +266,7 @@ static void finish_csr_load(const struct firmware *fw, void *context) (css_header->header_len * 4)) { DRM_ERROR("Firmware has wrong CSS header length %u bytes\n", (css_header->header_len * 4)); - goto out; + return NULL; } csr->version = css_header->version; @@ -280,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context) CSR_VERSION_MINOR(csr->version), CSR_VERSION_MAJOR(SKL_CSR_VERSION_REQUIRED), CSR_VERSION_MINOR(SKL_CSR_VERSION_REQUIRED)); - goto out; + return NULL; } readcount += sizeof(struct intel_css_header); @@ -292,7 +291,7 @@ static void finish_csr_load(const struct firmware *fw, void *context) (package_header->header_len * 4)) { DRM_ERROR("Firmware has wrong package header length %u bytes\n", (package_header->header_len * 4)); - goto out; + return NULL; } readcount += sizeof(struct intel_package_header); @@ -312,7 +311,7 @@ static void finish_csr_load(const struct firmware *fw, void *context) } if (dmc_offset == CSR_DEFAULT_FW_OFFSET) { DRM_ERROR("Firmware not supported for %c stepping\n", stepping); - goto out; + return NULL; } readcount += dmc_offset; @@ -321,7 +320,7 @@ static void finish_csr_load(const struct firmware *fw, void *context) if (sizeof(struct intel_dmc_header) != (dmc_header->header_len)) { DRM_ERROR("Firmware has wrong dmc header length %u bytes\n", (dmc_header->header_len)); - goto out; + return NULL; } readcount += sizeof(struct intel_dmc_header); @@ -329,7 +328,7 @@ static void finish_csr_load(const struct firmware *fw, void *context) if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) { DRM_ERROR("Firmware has wrong mmio count %u\n", dmc_header->mmio_count); - goto out; + return NULL; } csr->mmio_count = dmc_header->mmio_count; for (i = 0; i < dmc_header->mmio_count; i++) { @@ -337,7 +336,7 @@ static void finish_csr_load(const struct firmware *fw, void *context) dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) { DRM_ERROR(" Firmware has wrong mmio address 0x%x\n", dmc_header->mmioaddr[i]); - goto out; + return NULL; } csr->mmioaddr[i] = dmc_header->mmioaddr[i]; csr->mmiodata[i] =
Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference
On to, 2015-11-12 at 17:04 +, Chris Wilson wrote: > On Thu, Nov 12, 2015 at 06:40:18PM +0200, Imre Deak wrote: > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index 825114a..ee3ef69 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2962,6 +2962,9 @@ static void i915_hangcheck_elapsed(struct > > work_struct *work) > > if (!i915.enable_hangcheck) > > return; > > > > + assert_rpm_device_not_suspended(dev_priv); > > + disable_rpm_asserts(dev_priv); > > + > > for_each_ring(ring, dev_priv, i) { > > u64 acthd; > > u32 seqno; > > @@ -3053,13 +3056,18 @@ static void i915_hangcheck_elapsed(struct > > work_struct *work) > > } > > } > > > > - if (rings_hung) > > - return i915_handle_error(dev, true, "Ring hung"); > > + if (rings_hung) { > > + i915_handle_error(dev, true, "Ring hung"); > > + goto out; > > + } > > > > if (busy_count) > > /* Reset timer case chip hangs without another > > request > > * being added */ > > i915_queue_hangcheck(dev); > > + > > +out: > > + enable_rpm_asserts(dev_priv); > > Nice catch! It triggered the new assert easily just before going to runtime suspend.. > Since the rpm wakelock here is covered by > intel_mark_busy/intel_mark_idle(), we should be able to do something > like: > > if (!intel_runtime_pm_tryget() > return; > > where intel_runtime_pm_tryget does something like > atomic_inc_unless_zero(). > > Is something like that possible? Yea, I could add this, but I'd like to better understand the need, see below. > As it stands since we don't actually cancel the hangcheck when we drop > the rpm wakelock in intel_mark_idle() it can very well come to pass > that > we execute this whilst the device is asleep. However, if the device > is > alseep, we now that we are no longer executing. But how could this run while asleep, since we flush the work in the runtime suspend handler before turning off the HW? But even if it can't run your idea would be clearer imo.. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Give meaningful names to all the planes
On Thu, Nov 12, 2015 at 05:38:48PM +, Emil Velikov wrote: > Hi Ville, > > On 12 November 2015 at 16:52,wrote: > > From: Ville Syrjälä > > > > Let's name our planes in a way that makes sense wrt. the spec: > > - skl+ -> "plane 1A", "plane 2A", "plane 1C", "cursor A" etc. > > - g4x+ -> "primary A", "primary B", "sprite A", "cursor C" etc. > > - pre-g4x -> "plane A", "cursor B" etc. > > > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/intel_display.c | 35 > > +-- > > drivers/gpu/drm/i915/intel_sprite.c | 14 ++ > > 2 files changed, 47 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 2b5e81a..82b2f58 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13788,7 +13788,15 @@ static void intel_finish_crtc_commit(struct > > drm_crtc *crtc, > > void intel_plane_destroy(struct drm_plane *plane) > > { > > struct intel_plane *intel_plane = to_intel_plane(plane); > > + char *name; > > + > > + /* > > +* drm_plane_cleanup() zeroes the structure, so > > +* need an extra dance to avoid leaking the name. > > +*/ > > + name = plane->name; > > drm_plane_cleanup(plane); > > + kfree(name); > > kfree(intel_plane); > > } > > > > @@ -13838,6 +13846,21 @@ static struct drm_plane > > *intel_primary_plane_create(struct drm_device *dev, > > if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > primary->plane = !pipe; > > > > + if (INTEL_INFO(dev)->gen >= 9) > > + primary->base.name = kasprintf(GFP_KERNEL, "plane 1%c", > > + pipe_name(pipe)); > > + else if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) > > + primary->base.name = kasprintf(GFP_KERNEL, "primary %c", > > + pipe_name(pipe)); > > + else > > + primary->base.name = kasprintf(GFP_KERNEL, "plane %c", > > + plane_name(primary->plane)); > > + if (!primary->base.name) { > > + kfree(state); > > + kfree(primary); > > + return NULL; > Worth adding a label and doing all the teardown there ? (same goes for > the rest of the patch) Dunno. Was feeling lazy, and so didn't go the extra mile. > > > + } > > + > > if (INTEL_INFO(dev)->gen >= 9) { > > intel_primary_formats = skl_primary_formats; > > num_formats = ARRAY_SIZE(skl_primary_formats); > > @@ -13987,6 +14010,14 @@ static struct drm_plane > > *intel_cursor_plane_create(struct drm_device *dev, > > cursor->commit_plane = intel_commit_cursor_plane; > > cursor->disable_plane = intel_disable_cursor_plane; > > > > + cursor->base.name = kasprintf(GFP_KERNEL, "cursor %c", > > + pipe_name(pipe)); > > + if (!cursor->base.name) { > > + kfree(state); > > + kfree(cursor); > > + return NULL; > > + } > > + > > drm_universal_plane_init(dev, >base, 0, > > _plane_funcs, > > intel_cursor_formats, > > @@ -14113,9 +14144,9 @@ static void intel_crtc_init(struct drm_device *dev, > > int pipe) > > > > fail: > > if (primary) > > - drm_plane_cleanup(primary); > > + intel_plane_destroy(primary); > > if (cursor) > > - drm_plane_cleanup(cursor); > > + intel_plane_destroy(cursor); > Something feels strange here. We are either leaking memory before or > we'll end up with double free after your patch. Worth > checking/mentioning in the commit message ? Yeah, I think we were leaking here. Forgot to add a note. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bxt: Broxton doesn't use gen9 scaling for rps frequencies.
On Thu, 12 Nov 2015 10:35:00 +0200 Imre Deakwrote: > On Wed, 2015-11-11 at 13:36 -0800, Bob Paauwe wrote: > > On Tue, 10 Nov 2015 11:04:22 +0200 > > Mika Kuoppala wrote: > > > > > Bob Paauwe writes: > > > > > > > Signed-off-by: Bob Paauwe > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92768 > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > > b/drivers/gpu/drm/i915/intel_pm.c > > > > index 647c0ff..fc5097f 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -7138,7 +7138,7 @@ static int chv_freq_opcode(struct > > > > drm_i915_private *dev_priv, int val) > > > > > > > > int intel_gpu_freq(struct drm_i915_private *dev_priv, int val) > > > > { > > > > - if (IS_GEN9(dev_priv->dev)) > > > > + if (IS_GEN9(dev_priv->dev) && !IS_BROXTON(dev_priv->dev)) > > > > return (val * GT_FREQUENCY_MULTIPLIER) / > > > > GEN9_FREQ_SCALER; > > > > > > Documentation disagrees with this patch. The units are 16.67Mhz > > > thus we should use 50/3. > > > > > > -Mika > > > > I'm not sure I trust the documentation in this case. Elsewhere, in > > gen6_init_rps_frequencies() we use GEN9_FREQ_SCALER for SKL only, not for > > BXT. > > On SKL the frequency _capability_ register uses 50MHz units, but the > frequency _request_ register uses 16.67MHz units. We store the frequency > limits in 16.67MHz units in rps.rp*_freq, hence the use of > GEN9_FREQ_SCALER in gen6_init_rps_frequencies(). When requesting a > frequency we only use GEN9_FREQ_SCALER to get the 16.67MHz constant > value. > > On BXT both the capability and request registers use 16.67MHz units, so > we don't need to convert in gen6_init_rps_frequencies(), but we still > use GEN9_FREQ_SCALER when requesting the frequency to get the 16.67MHz > constant value. > > Confusing like hell, but it works. That makes it a bit less confusing, thanks! Is this all documented somewhere? I'd like to read up on it so if you have any pointers please send me an email. > > > I tried changing gen6_init_rps_frequencies() to use it for both SKL and > > BXT but that didn't resolve the problem when running the ps_rps igt > > test. So if we really should be using the 50/3 scale for BXT, there's > > another bug somewhere. > > The problem is that atm we can't set the min/max frequencies via sysfs > to the same value that we read out from these same files, due to the > rounding down we do in the driver. Using round-to-closest is one way to > fix it I posted a patch to do this to the bug report. I saw your patch there, I'll give that a try. > > > > > else if (IS_CHERRYVIEW(dev_priv->dev)) > > > > return chv_gpu_freq(dev_priv, val); > > > > @@ -7150,7 +7150,7 @@ int intel_gpu_freq(struct drm_i915_private > > > > *dev_priv, int val) > > > > > > > > int intel_freq_opcode(struct drm_i915_private *dev_priv, int val) > > > > { > > > > - if (IS_GEN9(dev_priv->dev)) > > > > + if (IS_GEN9(dev_priv->dev) && !IS_BROXTON(dev_priv->dev)) > > > > return (val * GEN9_FREQ_SCALER) / > > > > GT_FREQUENCY_MULTIPLIER; > > > > else if (IS_CHERRYVIEW(dev_priv->dev)) > > > > return chv_freq_opcode(dev_priv, val); > > > > -- > > > > 2.4.3 > > > > > > > > ___ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- -- Bob Paauwe bob.j.paa...@intel.com IOTG / PED Software Organization Intel Corp. Folsom, CA (916) 356-6193 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] igt/pm_rps: current freq < user specified min is no longer a fail
On Thu, 12 Nov 2015 11:18:11 +0200 Imre Deakwrote: > On ke, 2015-11-11 at 13:37 -0800, Bob Paauwe wrote: > > Since commit > > > > commit aed242ff7ebb697e4dff912bd4dc7ec7192f7581 > > Author: Chris Wilson > > Date: Wed Mar 18 09:48:21 2015 + > > > > drm/i915: Relax RPS contraints to allows setting minfreq on > > idle > > > > it is now possible that the current frequency will drop below the > > user > > specified minimum frequency. Update the pm_rps tests to reflect that > > this is no longer considered a failure. > > Yes, in case the GPU is idle the frequency will drop now to the idle fr > equency regardless of the minimum limit set by the user. We should > still check though if the frequency drops to the idle value not > something higher. You can use the i915_frequency_info in debugfs for > that. > Yeah, good idea. Right now, the idle freq is the same as the hardware minimum. So I could have it check against the hardware min. Adding support to read the info out i915_frequency_info would be a larger change to the test. But that might be a better long term solution. > > > > Signed-off-by: Bob Paauwe > > --- > > tests/pm_rps.c | 7 +++ > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > > index 74f08f4..e92ca3b 100644 > > --- a/tests/pm_rps.c > > +++ b/tests/pm_rps.c > > @@ -146,7 +146,6 @@ static void checkit(const int *freqs) > > { > > igt_assert_lte(freqs[MIN], freqs[MAX]); > > igt_assert_lte(freqs[CUR], freqs[MAX]); > > - igt_assert_lte(freqs[MIN], freqs[CUR]); > > igt_assert_lte(freqs[RPn], freqs[MIN]); > > igt_assert_lte(freqs[MAX], freqs[RP0]); > > igt_assert_lte(freqs[RP1], freqs[RP0]); > > @@ -472,14 +471,14 @@ static void idle_check(void) > > read_freqs(freqs); > > dump(freqs); > > checkit(freqs); > > - if (freqs[CUR] == freqs[MIN]) > > + if (freqs[CUR] <= freqs[MIN]) > > break; > > usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); > > wait += IDLE_WAIT_TIMESTEP_MSEC; > > } while (wait < IDLE_WAIT_TIMEOUT_MSEC); > > > > - igt_assert_eq(freqs[CUR], freqs[MIN]); > > - igt_debug("Required %d msec to reach cur=min\n", wait); > > + igt_assert_lte(freqs[CUR], freqs[MIN]); > > + igt_debug("Required %d msec to reach cur<=min\n", wait); > > } > > > > #define LOADED_WAIT_TIMESTEP_MSEC 100 -- -- Bob Paauwe bob.j.paa...@intel.com IOTG / PED Software Organization Intel Corp. Folsom, CA (916) 356-6193 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference
On Thu, Nov 12, 2015 at 06:40:18PM +0200, Imre Deak wrote: > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 825114a..ee3ef69 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2962,6 +2962,9 @@ static void i915_hangcheck_elapsed(struct work_struct > *work) > if (!i915.enable_hangcheck) > return; > > + assert_rpm_device_not_suspended(dev_priv); > + disable_rpm_asserts(dev_priv); > + > for_each_ring(ring, dev_priv, i) { > u64 acthd; > u32 seqno; > @@ -3053,13 +3056,18 @@ static void i915_hangcheck_elapsed(struct work_struct > *work) > } > } > > - if (rings_hung) > - return i915_handle_error(dev, true, "Ring hung"); > + if (rings_hung) { > + i915_handle_error(dev, true, "Ring hung"); > + goto out; > + } > > if (busy_count) > /* Reset timer case chip hangs without another request >* being added */ > i915_queue_hangcheck(dev); > + > +out: > + enable_rpm_asserts(dev_priv); Nice catch! Since the rpm wakelock here is covered by intel_mark_busy/intel_mark_idle(), we should be able to do something like: if (!intel_runtime_pm_tryget() return; where intel_runtime_pm_tryget does something like atomic_inc_unless_zero(). Is something like that possible? As it stands since we don't actually cancel the hangcheck when we drop the rpm wakelock in intel_mark_idle() it can very well come to pass that we execute this whilst the device is asleep. However, if the device is alseep, we now that we are no longer executing. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix SKL i_boost level
On Wed, Nov 11, 2015 at 03:15:54PM +0200, Ander Conselvan de Oliveira wrote: > The i_boost level in the DDI translation tables are stored per level. > However, skl_ddi_set_iboos() would choose an entry of that table based > on the port argument. > > Cc: Jim Bride> Signed-off-by: Ander Conselvan de Oliveira > > --- Reviewed-by: Jim Bride > I noticed this while reviewing Jim's patch that updates Skylake's DDI > translation tables. Only compile-tested. > > Ander > > drivers/gpu/drm/i915/intel_ddi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index da46edd..8cfdad2 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2085,21 +2085,21 @@ static void skl_ddi_set_iboost(struct drm_device > *dev, u32 level, > iboost = dp_iboost; > } else { > ddi_translations = skl_get_buf_trans_dp(dev, > _entries); > - iboost = ddi_translations[port].i_boost; > + iboost = ddi_translations[level].i_boost; > } > } else if (type == INTEL_OUTPUT_EDP) { > if (dp_iboost) { > iboost = dp_iboost; > } else { > ddi_translations = skl_get_buf_trans_edp(dev, > _entries); > - iboost = ddi_translations[port].i_boost; > + iboost = ddi_translations[level].i_boost; > } > } else if (type == INTEL_OUTPUT_HDMI) { > if (hdmi_iboost) { > iboost = hdmi_iboost; > } else { > ddi_translations = skl_get_buf_trans_hdmi(dev, > _entries); > - iboost = ddi_translations[port].i_boost; > + iboost = ddi_translations[level].i_boost; > } > } else { > return; > -- > 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/6] drm/i915: Use plane->name in debug prints
From: Ville SyrjäläSigned-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 38 +--- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9845687..b628dab 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4433,9 +4433,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, bool force_detach = !fb || !plane_state->visible; - DRM_DEBUG_KMS("Updating scaler for [PLANE:%d] scaler_user index %u.%u\n", - intel_plane->base.base.id, intel_crtc->pipe, - drm_plane_index(_plane->base)); + DRM_DEBUG_KMS("Updating scaler for [PLANE:%d:%s] scaler_user index %u.%u\n", + intel_plane->base.base.id, intel_plane->base.name, + intel_crtc->pipe, drm_plane_index(_plane->base)); ret = skl_update_scaler(crtc_state, force_detach, drm_plane_index(_plane->base), @@ -4451,8 +4451,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, /* check colorkey */ if (plane_state->ckey.flags != I915_SET_COLORKEY_NONE) { - DRM_DEBUG_KMS("[PLANE:%d] scaling with color key not allowed", - intel_plane->base.base.id); + DRM_DEBUG_KMS("[PLANE:%d:%s] scaling with color key not allowed", + intel_plane->base.base.id, + intel_plane->base.name); return -EINVAL; } @@ -4471,8 +4472,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, case DRM_FORMAT_VYUY: break; default: - DRM_DEBUG_KMS("[PLANE:%d] FB:%d unsupported scaling format 0x%x\n", - intel_plane->base.base.id, fb->base.id, fb->pixel_format); + DRM_DEBUG_KMS("[PLANE:%d:%s] FB:%d unsupported scaling format 0x%x\n", + intel_plane->base.base.id, intel_plane->base.name, + fb->base.id, fb->pixel_format); return -EINVAL; } @@ -11679,13 +11681,15 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, turn_off = was_visible && (!visible || mode_changed); turn_on = visible && (!was_visible || mode_changed); - DRM_DEBUG_ATOMIC("[CRTC:%d:%s] has [PLANE:%i] with fb %i\n", + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] has [PLANE:%d:%s] with fb %i\n", intel_crtc->base.base.id, intel_crtc->base.name, -plane->base.id, fb ? fb->base.id : -1); +plane->base.id, plane->name, +fb ? fb->base.id : -1); - DRM_DEBUG_ATOMIC("[PLANE:%i] visible %i -> %i, off %i, on %i, ms %i\n", -plane->base.id, was_visible, visible, + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] visible %i -> %i, off %i, on %i, ms %i\n", +plane->base.id, plane->name, +was_visible, visible, turn_off, turn_on, mode_changed); if (turn_on) { @@ -12088,18 +12092,20 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, state = to_intel_plane_state(plane->state); fb = state->base.fb; if (!fb) { - DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d " - "disabled, scaler_id = %d\n", + DRM_DEBUG_KMS("%s [PLANE:%d:%s] plane: %u.%u idx: %d " + "disabled, scaler_id = %d\n", plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD", - plane->base.id, intel_plane->pipe, + plane->base.id, plane->name, + intel_plane->pipe, (crtc->base.primary == plane) ? 0 : intel_plane->plane + 1, drm_plane_index(plane), state->scaler_id); continue; } - DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled", + DRM_DEBUG_KMS("%s [PLANE:%d:%s] plane: %u.%u idx: %d enabled", plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD", - plane->base.id, intel_plane->pipe, + plane->base.id, plane->name, + intel_plane->pipe, crtc->base.primary == plane ? 0 : intel_plane->plane + 1, drm_plane_index(plane)); DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x", -- 2.4.10
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Give meaningful names to all the planes
Hi Ville, On 12 November 2015 at 16:52,wrote: > From: Ville Syrjälä > > Let's name our planes in a way that makes sense wrt. the spec: > - skl+ -> "plane 1A", "plane 2A", "plane 1C", "cursor A" etc. > - g4x+ -> "primary A", "primary B", "sprite A", "cursor C" etc. > - pre-g4x -> "plane A", "cursor B" etc. > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_display.c | 35 +-- > drivers/gpu/drm/i915/intel_sprite.c | 14 ++ > 2 files changed, 47 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 2b5e81a..82b2f58 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13788,7 +13788,15 @@ static void intel_finish_crtc_commit(struct drm_crtc > *crtc, > void intel_plane_destroy(struct drm_plane *plane) > { > struct intel_plane *intel_plane = to_intel_plane(plane); > + char *name; > + > + /* > +* drm_plane_cleanup() zeroes the structure, so > +* need an extra dance to avoid leaking the name. > +*/ > + name = plane->name; > drm_plane_cleanup(plane); > + kfree(name); > kfree(intel_plane); > } > > @@ -13838,6 +13846,21 @@ static struct drm_plane > *intel_primary_plane_create(struct drm_device *dev, > if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > primary->plane = !pipe; > > + if (INTEL_INFO(dev)->gen >= 9) > + primary->base.name = kasprintf(GFP_KERNEL, "plane 1%c", > + pipe_name(pipe)); > + else if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) > + primary->base.name = kasprintf(GFP_KERNEL, "primary %c", > + pipe_name(pipe)); > + else > + primary->base.name = kasprintf(GFP_KERNEL, "plane %c", > + plane_name(primary->plane)); > + if (!primary->base.name) { > + kfree(state); > + kfree(primary); > + return NULL; Worth adding a label and doing all the teardown there ? (same goes for the rest of the patch) > + } > + > if (INTEL_INFO(dev)->gen >= 9) { > intel_primary_formats = skl_primary_formats; > num_formats = ARRAY_SIZE(skl_primary_formats); > @@ -13987,6 +14010,14 @@ static struct drm_plane > *intel_cursor_plane_create(struct drm_device *dev, > cursor->commit_plane = intel_commit_cursor_plane; > cursor->disable_plane = intel_disable_cursor_plane; > > + cursor->base.name = kasprintf(GFP_KERNEL, "cursor %c", > + pipe_name(pipe)); > + if (!cursor->base.name) { > + kfree(state); > + kfree(cursor); > + return NULL; > + } > + > drm_universal_plane_init(dev, >base, 0, > _plane_funcs, > intel_cursor_formats, > @@ -14113,9 +14144,9 @@ static void intel_crtc_init(struct drm_device *dev, > int pipe) > > fail: > if (primary) > - drm_plane_cleanup(primary); > + intel_plane_destroy(primary); > if (cursor) > - drm_plane_cleanup(cursor); > + intel_plane_destroy(cursor); Something feels strange here. We are either leaking memory before or we'll end up with double free after your patch. Worth checking/mentioning in the commit message ? Regards, Emil ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference
Atm, we assert that the device is not suspended until the point when the HW is truly put to a suspended state. This is fine, but we can catch more problems if we check the RPM refcount. After that one drops to zero we shouldn't access the HW any more, although the actual suspend may be delayed. Based on this change the assert_rpm_wakelock_held helper to check the refcount instead of the device suspended state. After this change we need to annotate every place explicitly in the code where we expect that the HW is in D0 state. Atm in the driver load function the D0 state is implicit until we enable runtime PM, but for these asserts to work we need to add explicit RPM get/put calls, so fix this up. Another place where the D0 state is implicit even with a 0 RPM refcount is the system and runtime sudpend/resume handlers and the hangcheck work. In the former case the susend/resume handlers themselves determine at which exact spot the HW is truly on/off and in the latter case the work will be flushed in the suspend handler before turning off the HW. We regard these cases special and disable the RPM asserts for their duration. In the hangcheck work we can nevertheless check that the device is not suspended. Fix up these things. These explicit annotations also have the positive side effect of documenting our assumptions better. This caught additional WARNs from the atomic modeset path, those should be fixed separately. v2: - remove the redundant HAS_RUNTIME_PM check (moved to patch 1) (Ville) v3: - use a new dedicated RPM wakelock refcount to also catch cases where our own RPM get/put functions were not called (Chris) - assert also that the new RPM wakelock refcount is 0 in the RPM suspend handler (Chris) - change the assert error message to be more meaningful (Chris) - prevent false assert errors and check that the RPM wakelock is 0 in the RPM resume handler too - prevent false assert errors in the hangcheck work too - add a device not suspended assert check to the hangcheck work Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/i915_dma.c | 7 ++ drivers/gpu/drm/i915/i915_drv.c | 39 + drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 12 -- drivers/gpu/drm/i915/intel_drv.h| 39 + drivers/gpu/drm/i915/intel_pm.c | 1 + drivers/gpu/drm/i915/intel_runtime_pm.c | 6 + 7 files changed, 99 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index a7289be..780cbcd 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -896,6 +896,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) intel_pm_setup(dev); + intel_runtime_pm_get(dev_priv); + intel_display_crc_init(dev); i915_dump_device_info(dev_priv); @@ -1085,6 +1087,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) i915_audio_component_init(dev_priv); + intel_runtime_pm_put(dev_priv); + return 0; out_power_well: @@ -1120,6 +1124,9 @@ free_priv: kmem_cache_destroy(dev_priv->requests); kmem_cache_destroy(dev_priv->vmas); kmem_cache_destroy(dev_priv->objects); + + intel_runtime_pm_put(dev_priv); + kfree(dev_priv); return ret; } diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 77d183d..24d21bf 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -636,6 +636,8 @@ static int i915_drm_suspend(struct drm_device *dev) dev_priv->modeset_restore = MODESET_SUSPENDED; mutex_unlock(_priv->modeset_restore_lock); + disable_rpm_asserts(dev_priv); + /* We do a lot of poking in a lot of registers, make sure they work * properly. */ intel_display_set_init_power(dev_priv, true); @@ -648,7 +650,7 @@ static int i915_drm_suspend(struct drm_device *dev) if (error) { dev_err(>pdev->dev, "GEM idle failed, resume might fail\n"); - return error; + goto out; } intel_guc_suspend(dev); @@ -695,7 +697,10 @@ static int i915_drm_suspend(struct drm_device *dev) if (HAS_CSR(dev_priv)) flush_work(_priv->csr.work); - return 0; +out: + enable_rpm_asserts(dev_priv); + + return error; } static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) @@ -703,6 +708,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) struct drm_i915_private *dev_priv = drm_dev->dev_private; int ret; + disable_rpm_asserts(dev_priv); + intel_power_domains_suspend(dev_priv); ret = intel_suspend_complete(dev_priv); @@ -710,7 +717,7 @@ static int
[Intel-gfx] [PATCH v3 5/7] drm/i915: sanitize the asserts in the RPM get/put helpers
There is no point in checking the refcount just after increasing it so remove the assert from the get helper. Otoh, we should check the refcount before decreasing it, so add it to the put helper. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 64da5af..db63b8a 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -2132,7 +2132,6 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv) pm_runtime_get_sync(device); atomic_inc(_priv->pm.wakelock_count); - assert_rpm_wakelock_held(dev_priv); } /** @@ -2176,6 +2175,7 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; struct device *device = >pdev->dev; + assert_rpm_wakelock_held(dev_priv); atomic_dec(_priv->pm.wakelock_count); pm_runtime_mark_last_busy(device); -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 2/7] drm/i915: add assert_rpm_wakelock_held helper
As a preparation for follow-up patches add a new helper that checks whether we hold an RPM reference, since this is what we want most of the cases. Atm this helper will only check for the HW suspended state, a follow-up patch will do the actual change to check the refcount instead. One exception is the forcewake release timer function, where it's guaranteed that the HW is on even though the RPM refcount drops to zero. This guarantee is provided by flushing the timer in the runtime suspend handler. So leave the assert_device_not_suspended check in place there. Also rename assert_device_suspended for consistency and export these helpers as a preparation for the follow-up patches. No functional change. v3: - change the assert warning message to be more meaningful (Chris) Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_drv.h| 14 ++ drivers/gpu/drm/i915/intel_uncore.c | 16 +--- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0b86674..fabf639 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1427,6 +1427,20 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); void intel_display_power_put(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); + +static inline void +assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv) +{ + WARN_ONCE(dev_priv->pm.suspended, + "Device suspended during HW access\n"); +} + +static inline void +assert_rpm_wakelock_held(struct drm_i915_private *dev_priv) +{ + assert_rpm_device_not_suspended(dev_priv); +} + void intel_runtime_pm_get(struct drm_i915_private *dev_priv); void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv); void intel_runtime_pm_put(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 6560272..01923e0 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -50,12 +50,6 @@ intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id) return "unknown"; } -static void -assert_device_not_suspended(struct drm_i915_private *dev_priv) -{ - WARN_ONCE(dev_priv->pm.suspended, "Device suspended\n"); -} - static inline void fw_domain_reset(const struct intel_uncore_forcewake_domain *d) { @@ -235,7 +229,7 @@ static void intel_uncore_fw_release_timer(unsigned long arg) struct intel_uncore_forcewake_domain *domain = (void *)arg; unsigned long irqflags; - assert_device_not_suspended(domain->i915); + assert_rpm_device_not_suspended(domain->i915); spin_lock_irqsave(>i915->uncore.lock, irqflags); if (WARN_ON(domain->wake_count == 0)) @@ -627,7 +621,7 @@ hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv) #define GEN2_READ_HEADER(x) \ u##x val = 0; \ - assert_device_not_suspended(dev_priv); + assert_rpm_wakelock_held(dev_priv); #define GEN2_READ_FOOTER \ trace_i915_reg_rw(false, reg, val, sizeof(val), trace); \ @@ -668,7 +662,7 @@ __gen2_read(64) #define GEN6_READ_HEADER(x) \ unsigned long irqflags; \ u##x val = 0; \ - assert_device_not_suspended(dev_priv); \ + assert_rpm_wakelock_held(dev_priv); \ spin_lock_irqsave(_priv->uncore.lock, irqflags) #define GEN6_READ_FOOTER \ @@ -813,7 +807,7 @@ __gen6_read(64) #define GEN2_WRITE_HEADER \ trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \ - assert_device_not_suspended(dev_priv); \ + assert_rpm_wakelock_held(dev_priv); \ #define GEN2_WRITE_FOOTER @@ -852,7 +846,7 @@ __gen2_write(64) #define GEN6_WRITE_HEADER \ unsigned long irqflags; \ trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \ - assert_device_not_suspended(dev_priv); \ + assert_rpm_wakelock_held(dev_priv); \ spin_lock_irqsave(_priv->uncore.lock, irqflags) #define GEN6_WRITE_FOOTER \ -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 7/7] drm/i915: check that we are in an RPM atomic section in GGTT PTE updaters
The device should be on for the whole duration of the update, so check for this. v2: - use the existing dev_priv directly everywhere (Ville) v3: - check also that we are in an RPM atomic section (Chris) - add the assert to i915_ggtt_insert_entries/clear_range too (Chris) Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/i915_gem_gtt.c | 33 + 1 file changed, 33 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 016739e..b9cd332 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2357,6 +2357,9 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm, int i = 0; struct sg_page_iter sg_iter; dma_addr_t addr = 0; /* shut up gcc */ + int rpm_atomic_seq; + + rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv); for_each_sg_page(st->sgl, _iter, st->nents, 0) { addr = sg_dma_address(sg_iter.sg) + @@ -2383,6 +2386,8 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm, */ I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN); POSTING_READ(GFX_FLSH_CNTL_GEN6); + + assert_rpm_atomic_end(dev_priv, rpm_atomic_seq); } /* @@ -2403,6 +2408,9 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm, int i = 0; struct sg_page_iter sg_iter; dma_addr_t addr = 0; + int rpm_atomic_seq; + + rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv); for_each_sg_page(st->sgl, _iter, st->nents, 0) { addr = sg_page_iter_dma_address(_iter); @@ -2427,6 +2435,8 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm, */ I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN); POSTING_READ(GFX_FLSH_CNTL_GEN6); + + assert_rpm_atomic_end(dev_priv, rpm_atomic_seq); } static void gen8_ggtt_clear_range(struct i915_address_space *vm, @@ -2441,6 +2451,9 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm, (gen8_pte_t __iomem *) dev_priv->gtt.gsm + first_entry; const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry; int i; + int rpm_atomic_seq; + + rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv); if (WARN(num_entries > max_entries, "First entry = %d; Num entries = %d (max=%d)\n", @@ -2453,6 +2466,8 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm, for (i = 0; i < num_entries; i++) gen8_set_pte(_base[i], scratch_pte); readl(gtt_base); + + assert_rpm_atomic_end(dev_priv, rpm_atomic_seq); } static void gen6_ggtt_clear_range(struct i915_address_space *vm, @@ -2467,6 +2482,9 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm, (gen6_pte_t __iomem *) dev_priv->gtt.gsm + first_entry; const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry; int i; + int rpm_atomic_seq; + + rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv); if (WARN(num_entries > max_entries, "First entry = %d; Num entries = %d (max=%d)\n", @@ -2479,6 +2497,8 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm, for (i = 0; i < num_entries; i++) iowrite32(scratch_pte, _base[i]); readl(gtt_base); + + assert_rpm_atomic_end(dev_priv, rpm_atomic_seq); } static void i915_ggtt_insert_entries(struct i915_address_space *vm, @@ -2486,11 +2506,17 @@ static void i915_ggtt_insert_entries(struct i915_address_space *vm, uint64_t start, enum i915_cache_level cache_level, u32 unused) { + struct drm_i915_private *dev_priv = vm->dev->dev_private; unsigned int flags = (cache_level == I915_CACHE_NONE) ? AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY; + int rpm_atomic_seq; + + rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv); intel_gtt_insert_sg_entries(pages, start >> PAGE_SHIFT, flags); + assert_rpm_atomic_end(dev_priv, rpm_atomic_seq); + } static void i915_ggtt_clear_range(struct i915_address_space *vm, @@ -2498,9 +2524,16 @@ static void i915_ggtt_clear_range(struct i915_address_space *vm, uint64_t length, bool unused) { + struct drm_i915_private *dev_priv = vm->dev->dev_private; unsigned first_entry = start >> PAGE_SHIFT; unsigned num_entries = length >> PAGE_SHIFT; + int rpm_atomic_seq; + + rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv); + intel_gtt_clear_range(first_entry, num_entries); + + assert_rpm_atomic_end(dev_priv, rpm_atomic_seq); } static int ggtt_bind_vma(struct i915_vma *vma, -- 2.5.0
[Intel-gfx] [PATCH v3 0/7] drm/i915: improve the RPM device suspended assert
This is v3 of [1]. I addressed the review comments from Ville and Chris and added an RPM atomic section check as well requested by Chris. I was also considering using lockdep for more coverage, but decided to leave that out for now, we can also add that later if needed. The patchset depends on Patrik's DC rework patchset [2]. [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079777.html [2] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079758.html Imre Deak (7): drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers drm/i915: add assert_rpm_wakelock_held helper drm/i915: use assert_rpm_wakelock_held instead of opencoding it drm/i915: add support for checking if we hold an RPM reference drm/i915: sanitize the asserts in the RPM get/put helpers drm/i915: add support for checking RPM atomic sections drm/i915: check that we are in an RPM atomic section in GGTT PTE updaters drivers/gpu/drm/i915/i915_dma.c | 7 drivers/gpu/drm/i915/i915_drv.c | 39 +-- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_gem_gtt.c | 33 drivers/gpu/drm/i915/i915_irq.c | 12 +- drivers/gpu/drm/i915/intel_drv.h| 67 + drivers/gpu/drm/i915/intel_pm.c | 2 + drivers/gpu/drm/i915/intel_runtime_pm.c | 24 +--- drivers/gpu/drm/i915/intel_uncore.c | 19 +++--- 9 files changed, 172 insertions(+), 33 deletions(-) -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 3/7] drm/i915: use assert_rpm_wakelock_held instead of opencoding it
Signed-off-by: Imre DeakReviewed-by: Chris Wilson (v1) --- drivers/gpu/drm/i915/intel_runtime_pm.c | 10 -- drivers/gpu/drm/i915/intel_uncore.c | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 83ab03a..3d7ddc3 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -464,8 +464,7 @@ static void assert_can_enable_dc5(struct drm_i915_private *dev_priv) WARN_ONCE((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5), "DC5 already programmed to be enabled.\n"); - WARN_ONCE(dev_priv->pm.suspended, - "DC5 cannot be enabled, if platform is runtime-suspended.\n"); + assert_rpm_wakelock_held(dev_priv); assert_csr_loaded(dev_priv); } @@ -479,8 +478,7 @@ static void assert_can_disable_dc5(struct drm_i915_private *dev_priv) if (dev_priv->power_domains.initializing) return; - WARN_ONCE(dev_priv->pm.suspended, - "Disabling of DC5 while platform is runtime-suspended should never happen.\n"); + assert_rpm_wakelock_held(dev_priv); } static void gen9_enable_dc5(struct drm_i915_private *dev_priv) @@ -2132,7 +2130,7 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv) struct device *device = >pdev->dev; pm_runtime_get_sync(device); - WARN(dev_priv->pm.suspended, "Device still suspended.\n"); + assert_rpm_wakelock_held(dev_priv); } /** @@ -2157,7 +2155,7 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; struct device *device = >pdev->dev; - WARN(dev_priv->pm.suspended, "Getting nosync-ref while suspended.\n"); + assert_rpm_wakelock_held(dev_priv); pm_runtime_get_noresume(device); } diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 01923e0..bd3d101 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -404,7 +404,7 @@ void intel_uncore_forcewake_get(struct drm_i915_private *dev_priv, if (!dev_priv->uncore.funcs.force_wake_get) return; - WARN_ON(dev_priv->pm.suspended); + assert_rpm_wakelock_held(dev_priv); spin_lock_irqsave(_priv->uncore.lock, irqflags); __intel_uncore_forcewake_get(dev_priv, fw_domains); -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 1/7] drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers
We don't really need to check this flag in the get/put/assert helpers, as on platforms without RPM support we won't ever enable RPM. That means pm.suspend will be always false and the assert will be always true. Do this to simplify the code and to let us extend the RPM asserts to all platforms for a better coverage. Motivated by Ville. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_runtime_pm.c | 9 - drivers/gpu/drm/i915/intel_uncore.c | 3 +-- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 9c4d17d..83ab03a 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -2131,9 +2131,6 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; struct device *device = >pdev->dev; - if (!HAS_RUNTIME_PM(dev)) - return; - pm_runtime_get_sync(device); WARN(dev_priv->pm.suspended, "Device still suspended.\n"); } @@ -2160,9 +2157,6 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; struct device *device = >pdev->dev; - if (!HAS_RUNTIME_PM(dev)) - return; - WARN(dev_priv->pm.suspended, "Getting nosync-ref while suspended.\n"); pm_runtime_get_noresume(device); } @@ -2180,9 +2174,6 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; struct device *device = >pdev->dev; - if (!HAS_RUNTIME_PM(dev)) - return; - pm_runtime_mark_last_busy(device); pm_runtime_put_autosuspend(device); } diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 5bb269c..6560272 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -53,8 +53,7 @@ intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id) static void assert_device_not_suspended(struct drm_i915_private *dev_priv) { - WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended, - "Device suspended\n"); + WARN_ONCE(dev_priv->pm.suspended, "Device suspended\n"); } static inline void -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/6] drm: Give crtcs and planes actual names
From: Ville SyrjäläI got sick and tired of staring at the line noise produced by drm.debug=0x1e, so I decided to give the crtcs and planes human readable names. Each driver can give whatever names it wants, and for i915 I gave something that makes some sense w.r.t. to the spec. The only magic gotcha here is that if the driver dynamically allocates the name, it must be careful around drm_{crtc,plane}_cleanup() cause those guys just memset the entire structure to 0. I didn't want to put the kfree() into the cleanup functions to avoid having to kstrdup("") in the fallback case or forcing drivers to always use a dynamic allocation. Ville Syrjälä (6): drm: Add crtc->name and use it in debug messages drm: Add plane->name and use it in debug prints drm/i915: Use crtc->name in debug messages drm/i915: Use plane->name in debug prints drm/i915: Set crtc->name to "pipe A", "pipe B", etc. drm/i915: Give meaningful names to all the planes drivers/gpu/drm/drm_atomic.c | 53 --- drivers/gpu/drm/drm_atomic_helper.c | 60 + drivers/gpu/drm/drm_crtc.c | 11 ++- drivers/gpu/drm/drm_crtc_helper.c| 24 --- drivers/gpu/drm/i915/intel_display.c | 127 +++ drivers/gpu/drm/i915/intel_fbdev.c | 5 +- drivers/gpu/drm/i915/intel_sprite.c | 14 include/drm/drm_crtc.h | 4 ++ 8 files changed, 185 insertions(+), 113 deletions(-) -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/6] drm: Add crtc->name and use it in debug messages
From: Ville SyrjäläSigned-off-by: Ville Syrjälä --- drivers/gpu/drm/drm_atomic.c| 41 ++- drivers/gpu/drm/drm_atomic_helper.c | 56 +++-- drivers/gpu/drm/drm_crtc.c | 8 -- drivers/gpu/drm/drm_crtc_helper.c | 24 include/drm/drm_crtc.h | 2 ++ 5 files changed, 71 insertions(+), 60 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7bb3845..2944655 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -288,8 +288,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, state->crtcs[index] = crtc; crtc_state->state = state; - DRM_DEBUG_ATOMIC("Added [CRTC:%d] %p state to %p\n", -crtc->base.id, crtc_state, state); + DRM_DEBUG_ATOMIC("Added [CRTC:%d:%s] %p state to %p\n", +crtc->base.id, crtc->name, crtc_state, state); return crtc_state; } @@ -480,8 +480,8 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, */ if (state->active && !state->enable) { - DRM_DEBUG_ATOMIC("[CRTC:%d] active without enabled\n", -crtc->base.id); + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active without enabled\n", +crtc->base.id, crtc->name); return -EINVAL; } @@ -490,15 +490,15 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, * be able to trigger. */ if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) && WARN_ON(state->enable && !state->mode_blob)) { - DRM_DEBUG_ATOMIC("[CRTC:%d] enabled without mode blob\n", -crtc->base.id); + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled without mode blob\n", +crtc->base.id, crtc->name); return -EINVAL; } if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) && WARN_ON(!state->enable && state->mode_blob)) { - DRM_DEBUG_ATOMIC("[CRTC:%d] disabled with mode blob\n", -crtc->base.id); + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] disabled with mode blob\n", +crtc->base.id, crtc->name); return -EINVAL; } @@ -980,8 +980,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state, } if (crtc) - DRM_DEBUG_ATOMIC("Link plane state %p to [CRTC:%d]\n", -plane_state, crtc->base.id); + DRM_DEBUG_ATOMIC("Link plane state %p to [CRTC:%d:%s]\n", +plane_state, crtc->base.id, crtc->name); else DRM_DEBUG_ATOMIC("Link plane state %p to [NOCRTC]\n", plane_state); @@ -1048,8 +1048,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, conn_state->crtc = crtc; if (crtc) - DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d]\n", -conn_state, crtc->base.id); + DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n", +conn_state, crtc->base.id, crtc->name); else DRM_DEBUG_ATOMIC("Link connector state %p to [NOCRTC]\n", conn_state); @@ -1088,8 +1088,8 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, if (ret) return ret; - DRM_DEBUG_ATOMIC("Adding all current connectors for [CRTC:%d] to %p\n", -crtc->base.id, state); + DRM_DEBUG_ATOMIC("Adding all current connectors for [CRTC:%d:%s] to %p\n", +crtc->base.id, crtc->name, state); /* * Changed connectors are already in @state, so only need to look at the @@ -1169,8 +1169,9 @@ drm_atomic_connectors_for_crtc(struct drm_atomic_state *state, num_connected_connectors++; } - DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d]\n", -state, num_connected_connectors, crtc->base.id); + DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d:%s]\n", +state, num_connected_connectors, +crtc->base.id, crtc->name); return num_connected_connectors; } @@ -1237,8 +1238,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state) for_each_crtc_in_state(state, crtc, crtc_state, i) { ret = drm_atomic_crtc_check(crtc, crtc_state); if (ret) { - DRM_DEBUG_ATOMIC("[CRTC:%d] atomic core check failed\n", -crtc->base.id); +
[Intel-gfx] [PATCH 5/6] drm/i915: Set crtc->name to "pipe A", "pipe B", etc.
From: Ville SyrjäläSigned-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b628dab..2b5e81a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10711,6 +10711,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_device *dev = crtc->dev; struct intel_unpin_work *work; + char *name; spin_lock_irq(>event_lock); work = intel_crtc->unpin_work; @@ -10722,8 +10723,13 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) kfree(work); } + /* +* drm_crtc_cleanup() zeroes the structure, so +* need an extra dance to avoid leaking the name. +*/ + name = crtc->name; drm_crtc_cleanup(crtc); - + kfree(name); kfree(intel_crtc); } @@ -14036,6 +14042,11 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) if (intel_crtc == NULL) return; + intel_crtc->base.name = kasprintf(GFP_KERNEL, "pipe %c", + pipe_name(pipe)); + if (!intel_crtc->base.name) + return; + crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL); if (!crtc_state) goto fail; @@ -14106,6 +14117,7 @@ fail: if (cursor) drm_plane_cleanup(cursor); kfree(crtc_state); + kfree(intel_crtc->base.name); kfree(intel_crtc); } -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/6] drm: Add plane->name and use it in debug prints
From: Ville SyrjäläSigned-off-by: Ville Syrjälä --- drivers/gpu/drm/drm_atomic.c| 12 ++-- drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- drivers/gpu/drm/drm_crtc.c | 3 +++ include/drm/drm_crtc.h | 2 ++ 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 2944655..df84060 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -543,8 +543,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, state->planes[index] = plane; plane_state->state = state; - DRM_DEBUG_ATOMIC("Added [PLANE:%d] %p state to %p\n", -plane->base.id, plane_state, state); + DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", +plane->base.id, plane->name, plane_state, state); if (plane_state->crtc) { struct drm_crtc_state *crtc_state; @@ -755,8 +755,8 @@ static int drm_atomic_plane_check(struct drm_plane *plane, } if (plane_switching_crtc(state->state, plane, state)) { - DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n", -plane->base.id); + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", +plane->base.id, plane->name); return -EINVAL; } @@ -1229,8 +1229,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state) for_each_plane_in_state(state, plane, plane_state, i) { ret = drm_atomic_plane_check(plane, plane_state); if (ret) { - DRM_DEBUG_ATOMIC("[PLANE:%d] atomic core check failed\n", -plane->base.id); + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n", +plane->base.id, plane->name); return ret; } } diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fc90af50..387d95c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -503,8 +503,8 @@ drm_atomic_helper_check_planes(struct drm_device *dev, ret = funcs->atomic_check(plane, plane_state); if (ret) { - DRM_DEBUG_ATOMIC("[PLANE:%d] atomic driver check failed\n", -plane->base.id); + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic driver check failed\n", +plane->base.id, plane->name); return ret; } } diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ea00a69..8dc4052 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1174,6 +1174,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, drm_modeset_lock_init(>mutex); + if (!plane->name) + plane->name = ""; + plane->base.properties = >properties; plane->dev = dev; plane->funcs = funcs; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index a8279b4..a08e256 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -848,6 +848,8 @@ struct drm_plane { struct drm_device *dev; struct list_head head; + char *name; + struct drm_modeset_lock mutex; struct drm_mode_object base; -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/6] drm/i915: Give meaningful names to all the planes
From: Ville SyrjäläLet's name our planes in a way that makes sense wrt. the spec: - skl+ -> "plane 1A", "plane 2A", "plane 1C", "cursor A" etc. - g4x+ -> "primary A", "primary B", "sprite A", "cursor C" etc. - pre-g4x -> "plane A", "cursor B" etc. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 35 +-- drivers/gpu/drm/i915/intel_sprite.c | 14 ++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2b5e81a..82b2f58 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13788,7 +13788,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc, void intel_plane_destroy(struct drm_plane *plane) { struct intel_plane *intel_plane = to_intel_plane(plane); + char *name; + + /* +* drm_plane_cleanup() zeroes the structure, so +* need an extra dance to avoid leaking the name. +*/ + name = plane->name; drm_plane_cleanup(plane); + kfree(name); kfree(intel_plane); } @@ -13838,6 +13846,21 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) primary->plane = !pipe; + if (INTEL_INFO(dev)->gen >= 9) + primary->base.name = kasprintf(GFP_KERNEL, "plane 1%c", + pipe_name(pipe)); + else if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) + primary->base.name = kasprintf(GFP_KERNEL, "primary %c", + pipe_name(pipe)); + else + primary->base.name = kasprintf(GFP_KERNEL, "plane %c", + plane_name(primary->plane)); + if (!primary->base.name) { + kfree(state); + kfree(primary); + return NULL; + } + if (INTEL_INFO(dev)->gen >= 9) { intel_primary_formats = skl_primary_formats; num_formats = ARRAY_SIZE(skl_primary_formats); @@ -13987,6 +14010,14 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, cursor->commit_plane = intel_commit_cursor_plane; cursor->disable_plane = intel_disable_cursor_plane; + cursor->base.name = kasprintf(GFP_KERNEL, "cursor %c", + pipe_name(pipe)); + if (!cursor->base.name) { + kfree(state); + kfree(cursor); + return NULL; + } + drm_universal_plane_init(dev, >base, 0, _plane_funcs, intel_cursor_formats, @@ -14113,9 +14144,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) fail: if (primary) - drm_plane_cleanup(primary); + intel_plane_destroy(primary); if (cursor) - drm_plane_cleanup(cursor); + intel_plane_destroy(cursor); kfree(crtc_state); kfree(intel_crtc->base.name); kfree(intel_crtc); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index a2c15f8..b1520f1 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1119,7 +1119,21 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane); intel_plane->check_plane = intel_check_sprite_plane; intel_plane->commit_plane = intel_commit_sprite_plane; + + if (INTEL_INFO(dev)->gen >= 9) + intel_plane->base.name = kasprintf(GFP_KERNEL, "plane %d%c", + plane + 2, pipe_name(pipe)); + else + intel_plane->base.name = kasprintf(GFP_KERNEL, "sprite %c", + sprite_name(pipe, plane)); + if (!intel_plane->base.name) { + kfree(state); + kfree(intel_plane); + return -ENOMEM; + } + possible_crtcs = (1 << pipe); + ret = drm_universal_plane_init(dev, _plane->base, possible_crtcs, _plane_funcs, plane_formats, num_plane_formats, -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 6/7] drm/i915: add support for checking RPM atomic sections
In some cases we want to check whether we hold an RPM wakelock reference for the whole duration of a sequence. To achieve this add a new RPM atomic sequence counter that we increment any time the wakelock refcount drops to zero. Check whether the sequence number stays the same during the atomic section and that we hold the wakelock at the beginning of the section. Motivated by Chris. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_drv.h| 14 ++ drivers/gpu/drm/i915/intel_pm.c | 1 + drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ++- 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 658cb9b..c265d47 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1600,6 +1600,7 @@ struct skl_wm_level { */ struct i915_runtime_pm { atomic_t wakelock_count; + atomic_t atomic_seq; bool suspended; bool irqs_enabled; }; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index ee403d7..a884dc7 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1443,6 +1443,20 @@ assert_rpm_wakelock_held(struct drm_i915_private *dev_priv) "RPM wakelock not held during HW access"); } +static inline int +assert_rpm_atomic_begin(struct drm_i915_private *dev_priv) +{ + assert_rpm_wakelock_held(dev_priv); + return atomic_read(_priv->pm.atomic_seq); +} + +static inline void +assert_rpm_atomic_end(struct drm_i915_private *dev_priv, int begin_seq) +{ + WARN_ONCE(atomic_read(_priv->pm.atomic_seq) != begin_seq, +"HW access outside of RPM atomic section\n"); +} + /** * disable_rpm_asserts - disable the RPM assert checks * @dev_priv: i915 device instance diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 6d74d32..2390237 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -7215,4 +7215,5 @@ void intel_pm_setup(struct drm_device *dev) dev_priv->pm.suspended = false; atomic_set(_priv->pm.wakelock_count, 0); + atomic_set(_priv->pm.atomic_seq, 0); } diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index db63b8a..69375a9 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -2176,7 +2176,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv) struct device *device = >pdev->dev; assert_rpm_wakelock_held(dev_priv); - atomic_dec(_priv->pm.wakelock_count); + if (atomic_dec_and_test(_priv->pm.wakelock_count)) + atomic_inc(_priv->pm.atomic_seq); pm_runtime_mark_last_busy(device); pm_runtime_put_autosuspend(device); -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] Sink CRC stabilization
On Thu, 2015-11-12 at 12:17 +0200, Jani Nikula wrote: > On Wed, 11 Nov 2015, Rodrigo Viviwrote: > > Let's start spliting that big series that enables PSR with this > > sink crc > > stabilization. > > > > Also I'm adding Wayne's mutex that stabilizes sink CRC on Skylake. > > > > All patches already reviewed and ready to merge. > > Disagreed on the hw mutex patch. I wasn't considered it as nacked. But it seems that we have more to discuss around it. Although I don't see a reason of blocking a patch that follow spec, it is reviewed, and it is used to fix a bug, unblock validation and only used for the particular case that is just used on automated validation and not broadly to all aux communications. > > BR, > Jani. > > > > > Thank you very much Paulo for the review and Thank you Wayne for > > the > > SKL aux mutex. > > > > Thanks, > > Rodrigo. > > > > Boyer, Wayne (1): > > drm/i915/skl: implement DP Aux Mutex framework > > > > Rodrigo Vivi (4): > > drm/i915: Allow 1 vblank to let Sink CRC calculation to start or > > stop. > > drm/i915: Make Sink crc calculation waiting for counter to reset. > > drm/i915: Stop tracking last calculated Sink CRC. > > drm/i915: Rely on TEST_SINK_START instead of tracking Sink CRC > > state > > on dev_priv. > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_reg.h | 5 ++ > > drivers/gpu/drm/i915/intel_dp.c | 126 +++ > > > > drivers/gpu/drm/i915/intel_drv.h | 7 --- > > 4 files changed, 93 insertions(+), 46 deletions(-) > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw
On 12.11.2015 10:28, Lankhorst, Maarten wrote: Hey, Gabriel Feceoru schreef op wo 11-11-2015 om 20:27 [+0200]: On 11.11.2015 16:21, Jani Nikula wrote: On Wed, 11 Nov 2015, Ander Conselvan De Oliveirawrote: On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote: Ander, Maarten, where are we with this? Is it horribly wrong to merge the original patch in this ever-growing and diverging thread? I think the patch as is will cause problems with DP, since we might clear the pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix disregarding the discussion in this thread is to drop another memset in intel_crt_compute_config(). Like this Ander, please post this as a proper patch for review. BR, Jani. Hi, I tested this patch on my system and I can confirm it fixes the original issue. However there are a few memset in *_ddi_pll_select functions which might not be needed anymore. For instance I tried to remove the hsw one and didn't see any regression. Could you test http://lists.freedesktop.org/archives/intel-gfx/2015-September/075964.html ? Should be a less duct-tape fix.. Hi Marteen, I tested this (hsw only) and this is a good fix, too. I get similar results with Ander's patch. Gabriel. ~Maarten ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Give meaningful names to all the planes
On Thu, Nov 12, 2015 at 07:49:19PM +0200, Ville Syrjälä wrote: > On Thu, Nov 12, 2015 at 05:38:48PM +, Emil Velikov wrote: > > Hi Ville, > > > > On 12 November 2015 at 16:52,wrote: > > > From: Ville Syrjälä > > > > > > Let's name our planes in a way that makes sense wrt. the spec: > > > - skl+ -> "plane 1A", "plane 2A", "plane 1C", "cursor A" etc. > > > - g4x+ -> "primary A", "primary B", "sprite A", "cursor C" etc. > > > - pre-g4x -> "plane A", "cursor B" etc. > > > > > > Signed-off-by: Ville Syrjälä > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 35 > > > +-- > > > drivers/gpu/drm/i915/intel_sprite.c | 14 ++ > > > 2 files changed, 47 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 2b5e81a..82b2f58 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -13788,7 +13788,15 @@ static void intel_finish_crtc_commit(struct > > > drm_crtc *crtc, > > > void intel_plane_destroy(struct drm_plane *plane) > > > { > > > struct intel_plane *intel_plane = to_intel_plane(plane); > > > + char *name; > > > + > > > + /* > > > +* drm_plane_cleanup() zeroes the structure, so > > > +* need an extra dance to avoid leaking the name. > > > +*/ > > > + name = plane->name; > > > drm_plane_cleanup(plane); > > > + kfree(name); > > > kfree(intel_plane); > > > } > > > > > > @@ -13838,6 +13846,21 @@ static struct drm_plane > > > *intel_primary_plane_create(struct drm_device *dev, > > > if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > > primary->plane = !pipe; > > > > > > + if (INTEL_INFO(dev)->gen >= 9) > > > + primary->base.name = kasprintf(GFP_KERNEL, "plane 1%c", > > > + pipe_name(pipe)); > > > + else if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) > > > + primary->base.name = kasprintf(GFP_KERNEL, "primary %c", > > > + pipe_name(pipe)); > > > + else > > > + primary->base.name = kasprintf(GFP_KERNEL, "plane %c", > > > + > > > plane_name(primary->plane)); > > > + if (!primary->base.name) { > > > + kfree(state); > > > + kfree(primary); > > > + return NULL; > > Worth adding a label and doing all the teardown there ? (same goes for > > the rest of the patch) > > Dunno. Was feeling lazy, and so didn't go the extra mile. After a better look I saw that I fumbled the error paths in the crtc name patch too. So I went on to clean things up a bit. I think I'll repost the lot since there are now more patches. > > > > > > + } > > > + > > > if (INTEL_INFO(dev)->gen >= 9) { > > > intel_primary_formats = skl_primary_formats; > > > num_formats = ARRAY_SIZE(skl_primary_formats); > > > @@ -13987,6 +14010,14 @@ static struct drm_plane > > > *intel_cursor_plane_create(struct drm_device *dev, > > > cursor->commit_plane = intel_commit_cursor_plane; > > > cursor->disable_plane = intel_disable_cursor_plane; > > > > > > + cursor->base.name = kasprintf(GFP_KERNEL, "cursor %c", > > > + pipe_name(pipe)); > > > + if (!cursor->base.name) { > > > + kfree(state); > > > + kfree(cursor); > > > + return NULL; > > > + } > > > + > > > drm_universal_plane_init(dev, >base, 0, > > > _plane_funcs, > > > intel_cursor_formats, > > > @@ -14113,9 +14144,9 @@ static void intel_crtc_init(struct drm_device > > > *dev, int pipe) > > > > > > fail: > > > if (primary) > > > - drm_plane_cleanup(primary); > > > + intel_plane_destroy(primary); > > > if (cursor) > > > - drm_plane_cleanup(cursor); > > > + intel_plane_destroy(cursor); > > Something feels strange here. We are either leaking memory before or > > we'll end up with double free after your patch. Worth > > checking/mentioning in the commit message ? > > Yeah, I think we were leaking here. Forgot to add a note. > > -- > Ville Syrjälä > Intel OTC > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] Sink CRC stabilization
On Thu, Nov 12, 2015 at 10:56 AM Vivi, Rodrigowrote: > On Thu, 2015-11-12 at 12:17 +0200, Jani Nikula wrote: > > On Wed, 11 Nov 2015, Rodrigo Vivi wrote: > > > Let's start spliting that big series that enables PSR with this > > > sink crc > > > stabilization. > > > > > > Also I'm adding Wayne's mutex that stabilizes sink CRC on Skylake. > > > > > > All patches already reviewed and ready to merge. > > > > Disagreed on the hw mutex patch. > > I wasn't considered it as nacked. But it seems that we have more to > discuss around it. > > Although I don't see a reason of blocking a patch that follow spec, it > is reviewed, and it is used to fix a bug, unblock validation and only > used for the particular case that is just used on automated validation > and not broadly to all aux communications. > Oh, now I saw you had comments there on the patch. So please ignore this complain and accept my apologies. > > > > > BR, > > Jani. > > > > > > > > Thank you very much Paulo for the review and Thank you Wayne for > > > the > > > SKL aux mutex. > > > > > > Thanks, > > > Rodrigo. > > > > > > Boyer, Wayne (1): > > > drm/i915/skl: implement DP Aux Mutex framework > > > > > > Rodrigo Vivi (4): > > > drm/i915: Allow 1 vblank to let Sink CRC calculation to start or > > > stop. > > > drm/i915: Make Sink crc calculation waiting for counter to reset. > > > drm/i915: Stop tracking last calculated Sink CRC. > > > drm/i915: Rely on TEST_SINK_START instead of tracking Sink CRC > > > state > > > on dev_priv. > > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > drivers/gpu/drm/i915/i915_reg.h | 5 ++ > > > drivers/gpu/drm/i915/intel_dp.c | 126 +++ > > > > > > drivers/gpu/drm/i915/intel_drv.h | 7 --- > > > 4 files changed, 93 insertions(+), 46 deletions(-) > > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 7/7] drm/i915: check that we are in an RPM atomic section in GGTT PTE updaters
On Thu, Nov 12, 2015 at 06:40:21PM +0200, Imre Deak wrote: > The device should be on for the whole duration of the update, so check > for this. > > v2: > - use the existing dev_priv directly everywhere (Ville) > v3: > - check also that we are in an RPM atomic section (Chris) > - add the assert to i915_ggtt_insert_entries/clear_range too (Chris) > > Signed-off-by: Imre DeakFor this and its companion patch 6/7, Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)
On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote: > On Thu, 05 Nov 2015, Matt Roperwrote: > > On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote: > >> On Tue, 03 Nov 2015, Matt Roper wrote: > >> > Although we can do a good job of reading out hardware state, the > >> > graphics firmware may have programmed the watermarks in a creative way > >> > that doesn't match how i915 would have chosen to program them. We > >> > shouldn't trust the firmware's watermark programming, but should rather > >> > re-calculate how we think WM's should be programmed and then shove those > >> > values into the hardware. > >> > > >> > We can do this pretty easily by creating a dummy top-level state, > >> > running it through the check process to calculate all the values, and > >> > then just programming the watermarks for each CRTC. > >> > > >> > v2: Move watermark sanitization after our BIOS fb reconstruction; the > >> > watermark calculations that we do here need to look at pstate->fb, > >> > which isn't setup yet in intel_modeset_setup_hw_state(), even > >> > though we have an enabled & visible plane. > >> > > >> > Cc: Jani Nikula > >> > Cc: Maarten Lankhorst > >> > Signed-off-by: Matt Roper > >> > --- > >> > Jani, based on your logs it looks like the culprit is that we're > >> > calculating > >> > watermarks at startup time after we've read out preliminary hardware > >> > state (so > >> > we know the primary plane is enabled & visible), but before we > >> > reconstruct the > >> > BIOS fb (so pstate->fb is NULL). I think moving the watermark > >> > sanitization > >> > later in the process so that we'll have a proper fb setup should > >> > hopefully > >> > solve the issue. Can you test this version when you get a chance? I'll > >> > also > >> > send a rebased patch #4 since the code movement here means that the > >> > previous > >> > version won't apply cleanly. > >> > >> Sorry Matt, black screen with new versions of patches 3-4. Dmesg at > >> http://pastebin.com/3LXZwvWu > > > > Hmm, okay, looks like we're getting closer (successfully avoided the > > divide by zero problem), but I neglected to grab the necessary locks so > > the sanitization doesn't actually happen. Does applying > > http://paste.debian.net/322932 on top of the series work any better for > > you? I haven't had time to pull back out an ILK-style system, so that's > > only compile-tested at the moment. > > Still warns http://pastebin.com/yGtde5X2 > > BR, > Jani. Sorry this is taking so long to zero in on. When you have some free time would you mind running tag 'forjani/watermark_debug-v1' from https://github.com/mattrope/kernel.git with debug messages turned on? I was a bit confused by your earlier logs because apparently some of our startup-time state dumping was giving misleading/inaccurate information; hopefully with the changes here, I'll get more useful debug information and better understand what's leading to the failures on your system. Thanks again for your help. Matt > > > > > > > > Matt > > > >> > >> BR, > >> Jani. > >> > >> > >> > >> > > >> > drivers/gpu/drm/i915/i915_drv.h | 1 + > >> > drivers/gpu/drm/i915/intel_display.c | 55 > >> > > >> > drivers/gpu/drm/i915/intel_pm.c | 14 + > >> > 3 files changed, 64 insertions(+), 6 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> > b/drivers/gpu/drm/i915/i915_drv.h > >> > index 20cd6d8..09807c8 100644 > >> > --- a/drivers/gpu/drm/i915/i915_drv.h > >> > +++ b/drivers/gpu/drm/i915/i915_drv.h > >> > @@ -629,6 +629,7 @@ struct drm_i915_display_funcs { > >> >struct dpll *best_clock); > >> > int (*compute_pipe_wm)(struct intel_crtc *crtc, > >> > struct drm_atomic_state *state); > >> > +void (*program_watermarks)(struct intel_crtc_state *cstate); > >> > void (*update_wm)(struct drm_crtc *crtc); > >> > int (*modeset_calc_cdclk)(struct drm_atomic_state *state); > >> > void (*modeset_commit_cdclk)(struct drm_atomic_state *state); > >> > diff --git a/drivers/gpu/drm/i915/intel_display.c > >> > b/drivers/gpu/drm/i915/intel_display.c > >> > index 7b3cfb6..e289311 100644 > >> > --- a/drivers/gpu/drm/i915/intel_display.c > >> > +++ b/drivers/gpu/drm/i915/intel_display.c > >> > @@ -14936,6 +14936,54 @@ void intel_modeset_init_hw(struct drm_device > >> > *dev) > >> > intel_enable_gt_powersave(dev); > >> > } > >> > > >> > +/* > >> > + * Calculate what we think the watermarks should be for the state we've > >> > read > >> > + * out of the hardware and then immediately program those watermarks so > >> > that > >> > + * we ensure the hardware settings match our internal state. > >> > + */ > >> > +static void