Re: [Intel-gfx] [drm/i915/3.17] panic in i915_digport_work_func
> drm/i915: add DP 1.2 MST support (v0.7) > > This adds DP 1.2 MST support on Haswell systems. > I've attached a patch that might fix this, please test and let me know. Dave. From d407c946fbf5c48f30160591f5bd71fbe158aeb4 Mon Sep 17 00:00:00 2001 From: Dave Airlie Date: Mon, 1 Sep 2014 16:58:12 +1000 Subject: [PATCH] drm/i915: handle G45/GM45 pulse detection connected state. In the HPD pulse handler we check for long pulses if the port is actually connected, however we do that for IBX, but we use the pulse handling code on GM45 systems as well, so we need to use a diffent check. This patch refactors the digital port connected check out of the g4x detection path and reuses it in the hpd pulse path. Should fix: Message-ID: <1409382202.5141.36.ca...@marge.simpson.net> Reported-by: Mike Galbraith Signed-off-by: Dave Airlie --- drivers/gpu/drm/i915/intel_dp.c | 55 +++-- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 67cfed6..81d7681 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3661,24 +3661,12 @@ ironlake_dp_detect(struct intel_dp *intel_dp) return intel_dp_detect_dpcd(intel_dp); } -static enum drm_connector_status -g4x_dp_detect(struct intel_dp *intel_dp) +static int g4x_digital_port_connected(struct drm_device *dev, + struct intel_digital_port *intel_dig_port) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); uint32_t bit; - /* Can't disconnect eDP, but you can close the lid... */ - if (is_edp(intel_dp)) { - enum drm_connector_status status; - - status = intel_panel_detect(dev); - if (status == connector_status_unknown) - status = connector_status_connected; - return status; - } - if (IS_VALLEYVIEW(dev)) { switch (intel_dig_port->port) { case PORT_B: @@ -3691,7 +3679,7 @@ g4x_dp_detect(struct intel_dp *intel_dp) bit = PORTD_HOTPLUG_LIVE_STATUS_VLV; break; default: - return connector_status_unknown; + return -EINVAL; } } else { switch (intel_dig_port->port) { @@ -3705,11 +3693,36 @@ g4x_dp_detect(struct intel_dp *intel_dp) bit = PORTD_HOTPLUG_LIVE_STATUS_G4X; break; default: - return connector_status_unknown; + return -EINVAL; } } if ((I915_READ(PORT_HOTPLUG_STAT) & bit) == 0) + return 0; + return 1; +} + +static enum drm_connector_status +g4x_dp_detect(struct intel_dp *intel_dp) +{ + struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + int ret; + + /* Can't disconnect eDP, but you can close the lid... */ + if (is_edp(intel_dp)) { + enum drm_connector_status status; + + status = intel_panel_detect(dev); + if (status == connector_status_unknown) + status = connector_status_connected; + return status; + } + + ret = g4x_digital_port_connected(dev, intel_dig_port); + if (ret == -EINVAL) + return connector_status_unknown; + else if (ret == 0) return connector_status_disconnected; return intel_dp_detect_dpcd(intel_dp); @@ -4066,8 +4079,14 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) intel_display_power_get(dev_priv, power_domain); if (long_hpd) { - if (!ibx_digital_port_connected(dev_priv, intel_dig_port)) - goto mst_fail; + + if (HAS_PCH_SPLIT(dev)) { + if (!ibx_digital_port_connected(dev_priv, intel_dig_port)) +goto mst_fail; + } else { + if (g4x_digital_port_connected(dev, intel_dig_port) != 1) +goto mst_fail; + } if (!intel_dp_get_dpcd(intel_dp)) { goto mst_fail; -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [drm/i915/3.17] panic in i915_digport_work_func
On Mon, 2014-09-01 at 17:02 +1000, Dave Airlie wrote: > > drm/i915: add DP 1.2 MST support (v0.7) > > > > This adds DP 1.2 MST support on Haswell systems. > > > I've attached a patch that might fix this, please test and let me know. Lappy hasn't exploded in 20 boot cycles, which judging by previous behavior should mean all is well (knocks anti-deterministic wood). Thanks. -Mike ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Fix lock dropping in intel_tv_detect()
From: Ville Syrjälä When intel_tv_detect() fails to do load detection it would forget to drop the locks and clean up the acquire context. Fix it up. This is a regression from: commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44 Author: Ville Syrjälä Date: Mon Aug 11 13:15:35 2014 +0300 drm/i915: Fix locking for intel_enable_pipe_a() Cc: Tibor Billes Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_tv.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 32186a6..abbf0ea 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1311,7 +1311,8 @@ intel_tv_detect(struct drm_connector *connector, bool force) { struct drm_display_mode mode; struct intel_tv *intel_tv = intel_attached_tv(connector); - int type; + enum drm_connector_status status = connector->status; + int type = -1; DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n", connector->base.id, connector->name, @@ -1328,21 +1329,23 @@ intel_tv_detect(struct drm_connector *connector, bool force) if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) { type = intel_tv_detect_type(intel_tv, connector); intel_release_load_detect_pipe(connector, &tmp); + status = type < 0 ? + connector_status_disconnected : + connector_status_connected; } else - return connector_status_unknown; + status = connector_status_unknown; drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); - } else - return connector->status; + } if (type < 0) - return connector_status_disconnected; + return status; intel_tv->type = type; intel_tv_find_better_format(connector); - return connector_status_connected; + return status; } static const struct input_res { -- 1.8.5.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/7] drm/i915: Improved w/a for rps on Baytrail
On Thu, Jul 10, 2014 at 08:31:21PM +0100, Chris Wilson wrote: > Rewrite commit 31685c258e0b0ad6aa486c5ec001382cf8a64212 > Author: Deepak S > Date: Thu Jul 3 17:33:01 2014 -0400 > > drm/i915/vlv: WA for Turbo and RC6 to work together. > > Other than code clarity, the major improvement is to disable the extra > interrupts generated when idle. However, the reclocking remains rather > slow under the new manual regime, in particular it fails to downclock as > quickly as desired. > > Signed-off-by: Chris Wilson > Cc: Deepak S > Cc: Ville Syrjälä > Cc: Rodrigo Vivi > Cc: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_irq.c | 166 > --- > drivers/gpu/drm/i915/i915_reg.h | 4 +- > drivers/gpu/drm/i915/intel_display.c | 2 + > drivers/gpu/drm/i915/intel_drv.h | 2 + > drivers/gpu/drm/i915/intel_pm.c | 13 +++ > 5 files changed, 73 insertions(+), 114 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 8e19d031c05d..2db5dbb87ced 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1433,14 +1376,14 @@ static void gen6_pm_rps_work(struct work_struct *work) > > mutex_lock(&dev_priv->rps.hw_lock); > > + pm_iir |= vlv_wa_c0_ei(dev_priv, pm_iir); > + > adj = dev_priv->rps.last_adj; > if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) { > if (adj > 0) > adj *= 2; > - else { > - /* CHV needs even encode values */ > - adj = IS_CHERRYVIEW(dev_priv) ? 2 : 1; > - } > + else > + adj = 1; > new_delay = dev_priv->rps.cur_freq + adj; > > /* > @@ -1455,15 +1398,11 @@ static void gen6_pm_rps_work(struct work_struct *work) > else > new_delay = dev_priv->rps.min_freq_softlimit; > adj = 0; > - } else if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) { > - new_delay = vlv_calc_delay_from_C0_counters(dev_priv); > } else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) { > if (adj < 0) > adj *= 2; > - else { > - /* CHV needs even encode values */ > - adj = IS_CHERRYVIEW(dev_priv) ? -2 : -1; > - } > + else > + adj = -1; > new_delay = dev_priv->rps.cur_freq + adj; > } else { /* unknown event */ > new_delay = dev_priv->rps.cur_freq; > @@ -1475,6 +1414,9 @@ static void gen6_pm_rps_work(struct work_struct *work) > new_delay = clamp_t(int, new_delay, > dev_priv->rps.min_freq_softlimit, > dev_priv->rps.max_freq_softlimit); > + /* CHV needs even encode values */ > + if (IS_CHERRYVIEW(dev_priv)) > + new_delay = new_delay & ~1; This will effectively make the first up interrupt a nop. The current code is the way it is precisely to avoid that. I guess it's not a huge problem but still seems silly to not satisfy the GPU when it wants moar speed. -- 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 v2] drm/i915: Fix to Enable GT/PM Interrupts for cherryview.
On Thursday 28 August 2014 11:00 AM, Daniel Vetter wrote: On Fri, Aug 29, 2014 at 08:45:21AM +0530, Deepak S wrote: On Tuesday 26 August 2014 07:24 PM, Daniel Vetter wrote: On Fri, Aug 22, 2014 at 08:32:40AM +0530, deepa...@linux.intel.com wrote: From: Deepak S Programing GT IER interrupts was fumbled while enabling Interrupts for gen8 This is a regression from commit abd58f0175915bed644aa67c8f69dc571b8280e0 Author: Ben Widawsky Date: Sat Nov 2 21:07:09 2013 -0700 drm/i915/bdw: Implement interrupt changes _Really_ unlikely that this is a regression from this commit, since that introduced all the gen8 interrupt handling in the first place. I think this is because of the reworked interrupt handling over gpu resets, where we want to keep rps interrupts enabled. But I'm not terribly sure. I think a more convincing story is that this is an oversight from commit a6706b45a57a23a613b34793e1414991b60a09c1 Author: Deepak S Date: Sat Mar 15 20:23:22 2014 +0530 drm/i915: Track the enabled PM interrupts in dev_priv or more precisely (since chv wasn't public back then iirc) we forgot to add the corresponding patch to -internal. In any case please clarify the commit message and please also add a few words about why exactly we need this - I had to dig through git history to figure this all out. I've applied the patch already, so you can just reply with the revised commit message that I should put in. Thanks, Daniel Daniel, This patch is not just for chv right. it affects the gen8(BDW). Your right we might have missed in (drm/i915: Track the enabled PM interrupts in dev_priv). In -internal, for chv, We used to program the IER in enable_interrupts routing so it was already taken care. After the interrupt re-work, we are supposed to add IER in post_irq handler which we missed it. New commit msg: Is this fine? Do i need to resubmit the patch? Yeah fine and thanks for confirming. I've adjusted the commit message. -Daniel Thanks Daniel. -- Programing GT IER interrupts was fumbled while enabling Interrupts for gen8 We forgot to program PM IER interrupt in gen8_gt_irq_postinstall based on the new re-worked interrupt routines. v2: Kill the loop and init GT interrupts (Ville) Signed-off-by: Deepak S --- drivers/gpu/drm/i915/i915_irq.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index d5445e7..c33cf89 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -3799,8 +3799,6 @@ static int valleyview_irq_postinstall(struct drm_device *dev) static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv) { - int i; - /* These are interrupts we'll toggle with the ring mask register */ uint32_t gt_interrupts[] = { GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT | @@ -3817,10 +3815,11 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv) GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT }; - for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++) - GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]); - dev_priv->pm_irq_mask = 0x; + GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]); + GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]); + GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_irq_mask, dev_priv->pm_rps_events); + GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]); } static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) -- 1.9.1 ___ 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 4/7] drm/i915: Improved w/a for rps on Baytrail
On Mon, Sep 01, 2014 at 11:23:20AM +0300, Ville Syrjälä wrote: > On Thu, Jul 10, 2014 at 08:31:21PM +0100, Chris Wilson wrote: > > Rewrite commit 31685c258e0b0ad6aa486c5ec001382cf8a64212 > > Author: Deepak S > > Date: Thu Jul 3 17:33:01 2014 -0400 > > > > drm/i915/vlv: WA for Turbo and RC6 to work together. > > > > Other than code clarity, the major improvement is to disable the extra > > interrupts generated when idle. However, the reclocking remains rather > > slow under the new manual regime, in particular it fails to downclock as > > quickly as desired. > > > > Signed-off-by: Chris Wilson > > Cc: Deepak S > > Cc: Ville Syrjälä > > Cc: Rodrigo Vivi > > Cc: Daniel Vetter > > --- > > drivers/gpu/drm/i915/i915_irq.c | 166 > > --- > > drivers/gpu/drm/i915/i915_reg.h | 4 +- > > drivers/gpu/drm/i915/intel_display.c | 2 + > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > drivers/gpu/drm/i915/intel_pm.c | 13 +++ > > 5 files changed, 73 insertions(+), 114 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index 8e19d031c05d..2db5dbb87ced 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -1433,14 +1376,14 @@ static void gen6_pm_rps_work(struct work_struct > > *work) > > > > mutex_lock(&dev_priv->rps.hw_lock); > > > > + pm_iir |= vlv_wa_c0_ei(dev_priv, pm_iir); > > + > > adj = dev_priv->rps.last_adj; > > if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) { > > if (adj > 0) > > adj *= 2; > > - else { > > - /* CHV needs even encode values */ > > - adj = IS_CHERRYVIEW(dev_priv) ? 2 : 1; > > - } > > + else > > + adj = 1; > > new_delay = dev_priv->rps.cur_freq + adj; > > > > /* > > @@ -1455,15 +1398,11 @@ static void gen6_pm_rps_work(struct work_struct > > *work) > > else > > new_delay = dev_priv->rps.min_freq_softlimit; > > adj = 0; > > - } else if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) { > > - new_delay = vlv_calc_delay_from_C0_counters(dev_priv); > > } else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) { > > if (adj < 0) > > adj *= 2; > > - else { > > - /* CHV needs even encode values */ > > - adj = IS_CHERRYVIEW(dev_priv) ? -2 : -1; > > - } > > + else > > + adj = -1; > > new_delay = dev_priv->rps.cur_freq + adj; > > } else { /* unknown event */ > > new_delay = dev_priv->rps.cur_freq; > > @@ -1475,6 +1414,9 @@ static void gen6_pm_rps_work(struct work_struct *work) > > new_delay = clamp_t(int, new_delay, > > dev_priv->rps.min_freq_softlimit, > > dev_priv->rps.max_freq_softlimit); > > + /* CHV needs even encode values */ > > + if (IS_CHERRYVIEW(dev_priv)) > > + new_delay = new_delay & ~1; > > This will effectively make the first up interrupt a nop. The current > code is the way it is precisely to avoid that. I guess it's not a huge > problem but still seems silly to not satisfy the GPU when it wants moar > speed. Hmm, it's actually worse than that (see the last_adj). Ok, I should split this out and tidy it up more carefully. -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 11/16] drm/i915: Init important ns2501 registers
On Fri, Aug 15, 2014 at 01:22:03AM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > In my earlier rewrite I missed a few important registers. Thomas Richter > noticed that they're needed to make his machine resume correctly. > > Looks like IEGD does a one time init of these three registers. We don't > have a good one time init place in the ns2501 driver, so let's just > stick them into the .mode_set() hook and see if that helps things along. We have the encoder->reset hooks which are commonly used for such stuff. Not worth to wire that up for dvo, just an fyi really. -Daniel > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/dvo_ns2501.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/dvo_ns2501.c > b/drivers/gpu/drm/i915/dvo_ns2501.c > index b278571..345235b 100644 > --- a/drivers/gpu/drm/i915/dvo_ns2501.c > +++ b/drivers/gpu/drm/i915/dvo_ns2501.c > @@ -342,6 +342,12 @@ static const struct ns2501_reg regs_1024x768[][86] = { > }, > }; > > +static const struct ns2501_reg regs_init[] = { > + [0] = { .offset = 0x35, .value = 0xff, }, > + [1] = { .offset = 0x34, .value = 0x00, }, > + [2] = { .offset = 0x08, .value = 0x30, }, > +}; > + > struct ns2501_priv { > bool quiet; > const struct ns2501_reg *regs; > @@ -544,6 +550,10 @@ static void ns2501_mode_set(struct intel_dvo_device *dvo, > else > return; > > + /* Hopefully doing it every time won't hurt... */ > + for (i = 0; i < ARRAY_SIZE(regs_init); i++) > + ns2501_writeb(dvo, regs_init[i].offset, regs_init[i].value); > + > ns->regs = regs_1024x768[mode_idx]; > > for (i = 0; i < 84; i++) > -- > 1.8.5.5 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/16] drm/i915: Fix DVO 2x clock enable on 830M
On Fri, Aug 15, 2014 at 01:22:05AM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > The spec says: > "For the correct operation of the muxed DVO pins (GDEVSELB/ I2Cdata, > GIRDBY/I2CClk) and (GFRAMEB/DVI_Data, GTRDYB/DVI_Clk): Bit 31 > (DPLL VCO Enable) and Bit 30 (2X Clock Enable) must be set to “1” in > both the DPLL A Control Register (06014h-06017h) and DPLL B Control > Register (06018h-0601Bh)." > > The pipe A and B force quirks take care of DPLL_VCO_ENABLE, so we > just need a bit of special care to handle DPLL_DVO_2X_MODE. > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/i915_drv.h | 3 +++ > drivers/gpu/drm/i915/intel_display.c | 47 > +--- > 2 files changed, 46 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 541fb6f..54895a6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1561,6 +1561,9 @@ struct drm_i915_private { > > u16 orig_clock; > > + /* used to control DVO 2x clock enable on 830M */ > + uint8_t dvo_pipes; > + > bool mchbar_need_disable; > > struct intel_l3_parity l3_parity; > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 3eeb5ce..6462bcf 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1548,6 +1548,7 @@ static void i9xx_enable_pll(struct intel_crtc *crtc) > { > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + enum pipe pipe = crtc->pipe; > int reg = DPLL(crtc->pipe); > u32 dpll = crtc->config.dpll_hw_state.dpll; > > @@ -1560,7 +1561,16 @@ static void i9xx_enable_pll(struct intel_crtc *crtc) > if (IS_MOBILE(dev) && !IS_I830(dev)) > assert_panel_unlocked(dev_priv, crtc->pipe); > > - I915_WRITE(reg, dpll); > + /* enable DVO 2x clock on both PLLs if necessary */ > + if (IS_I830(dev)) { > + if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO)) > + dev_priv->dvo_pipes |= 1 << pipe; > + > + if (dev_priv->dvo_pipes) { > + dpll |= DPLL_DVO_2X_MODE; > + I915_WRITE(DPLL(!pipe), I915_READ(DPLL(!pipe)) | > DPLL_DVO_2X_MODE); > + } > + } > > /* Wait for the clocks to stabilize. */ > POSTING_READ(reg); > @@ -1599,8 +1609,23 @@ static void i9xx_enable_pll(struct intel_crtc *crtc) > * > * Note! This is for pre-ILK only. > */ > -static void i9xx_disable_pll(struct drm_i915_private *dev_priv, enum pipe > pipe) > +static void i9xx_disable_pll(struct intel_crtc *crtc) > { > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + enum pipe pipe = crtc->pipe; > + > + /* disable DVO 2x clock on both PLLs if necessary */ > + if (IS_I830(dev)) { > + if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO)) > + dev_priv->dvo_pipes &= ~(1 << pipe); > + > + if (!dev_priv->dvo_pipes) { > + I915_WRITE(DPLL(pipe), I915_READ(DPLL(pipe)) & > ~DPLL_DVO_2X_MODE); > + I915_WRITE(DPLL(!pipe), I915_READ(DPLL(!pipe)) & > ~DPLL_DVO_2X_MODE); Bikeshed: I'd just use explicit PIPE_A and PIPE_B here, except when we really have to turn of our pipe first before the other. > + } > + } > + > /* Don't disable pipe A or pipe A PLLs if needed */ > if (pipe == PIPE_A && (dev_priv->quirks & QUIRK_PIPEA_FORCE)) > return; > @@ -4788,7 +4813,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > else if (IS_VALLEYVIEW(dev)) > vlv_disable_pll(dev_priv, pipe); > else > - i9xx_disable_pll(dev_priv, pipe); > + i9xx_disable_pll(intel_crtc); > } > > if (!IS_GEN2(dev)) > @@ -5792,7 +5817,7 @@ static void i8xx_update_pll(struct intel_crtc *crtc, > dpll |= PLL_P2_DIVIDE_BY_4; > } > > - if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO)) > + if (!IS_I830(dev) && intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO)) > dpll |= DPLL_DVO_2X_MODE; > > if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) && > @@ -6298,6 +6323,9 @@ static bool i9xx_get_pipe_config(struct intel_crtc > *crtc, > } > pipe_config->dpll_hw_state.dpll = I915_READ(DPLL(crtc->pipe)); > if (!IS_VALLEYVIEW(dev)) { > + if (IS_I830(dev)) > + pipe_config->dpll_hw_state.dpll &= ~DPLL_DVO_2X_MODE; > + > pipe_config->dpll_hw_state.fp0 = I915_READ(FP0(crtc->pipe)); > pipe_config->dpll_hw_state.fp1 = I915_READ(FP1(crtc->pipe)); > } else { > @@ -13021,6 +13049,17 @@ void intel_modes
[Intel-gfx] [PULL] drm-intel-next
Hi Dave, drm-intel-next-2014-08-22: - basic code for execlist, which is the fancy new cmd submission on gen8. Still disabled by default (Ben, Oscar Mateo, Thomas Daniel et al) - remove the useless usage of console_lock for I915_FBDEV=n (Chris) - clean up relations between ctx and ppgtt - clean up ppgtt lifetime handling (Michel Thierry) - various cursor code improvements from Ville - execbuffer code cleanups and secure batch fixes (Chris) - prep work for dev -> dev_priv transition (Chris) - some of the prep patches for the seqno -> request object transition (Chris) - various small improvements all over Plus a fix from Imre to make sure this pull doesn't break suspend/resume badly on a bunch of machines on top. Cheers, Daniel The following changes since commit 2c0827cffca8ac0c654b888c58a1989a5172f007: drm/i915: Update DRIVER_DATE to 20140808 (2014-08-08 20:44:59 +0200) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/drm-intel-next-2014-09-01 for you to fetch changes up to 604effb782a8a4d9a20c8af16bcbf86d742db119: drm/i915: fix suspend/resume for GENs w/o runtime PM support (2014-08-26 13:13:03 +0200) Ben Widawsky (2): drm/i915/bdw: Implement context switching (somewhat) drm/i915/bdw: Print context state in debugfs Chris Wilson (12): drm/i915: Only perform set-to-gtt domain for objects bound to the global gtt drm/i915: Force CPU relocations if not GTT mapped drm/i915: Remove fenced_gpu_access and pending_fenced_gpu_access drm/i915: Copy PCI device id into the device info block drm/i915: Double check ring is idle before declaring the GPU wedged drm/i915: Agnostic INTEL_INFO drm/i915: Pre-validate the NEED_GTTS flag for execbuffer drm/i915: Remove redundant list_empty(eb->vmas) tests in execbuffer drm/i915: Simplify relocate_entry_gtt() and make 64-bit safe drm/i915: Replace __I915__ with typesafe variant drm/i915: Localise the fbdev console lock frobbing drm/i915: Print captured bo for all VM in error state Damien Lespiau (5): drm/i915: Fix erroneous conversion to u8 drm/i915: Fix wrong number of HDMI translation entries drm/i915: Make intel_disable_shared_dpll() static drm/i915: Remove set but unused 'gt_perf_status' drm/i915/bdw: Disable execlists by default Daniel Vetter (18): drm/i915: Fix secure dispatch with full ppgtt drm/i915: WARN if module opt sanitization goes out of order drm/i915/bdw: Add a context and an engine pointers to the ringbuffer drm/i915: Some cleanups for the ppgtt lifetime handling drm/i915: Track file_priv, not ctx in the ppgtt structure drm/i915: Only refcount ppgtt if it actually is one drm/i915: Add proper prefix to obj_to_ggtt drm/i915: Allow i915_gem_setup_global_gtt to fail drm/i915: Fix up checks for aliasing ppgtt drm/i915: Rework ppgtt init to no require an aliasing ppgtt drm/i915: Initialize the aliasing ppgtt as part of global gtt drm/i915: Only track real ppgtt for a context drm/i915: Drop create_vm argument to i915_gem_create_context drm/i915: Extract common cleanup into i915_ppgtt_release drm/i915: Extract commmon global gtt cleanup code drm/i915: Cleanup aliasging ppgtt alongside the global gtt drm/i915: Track cursor changes as frontbuffer tracking flushes drm/i915: Update DRIVER_DATE to 20140822 Imre Deak (1): drm/i915: fix suspend/resume for GENs w/o runtime PM support Michel Thierry (2): drm/i915: vma/ppgtt lifetime rules drm/i915/bdw: Two-stage execlist submit process Oscar Mateo (33): drm/i915/bdw: New source and header file for LRs, LRCs and Execlists drm/i915/bdw: Macro for LRCs and module option for Execlists drm/i915/bdw: Initialization for Logical Ring Contexts drm/i915/bdw: Introduce one context backing object per engine drm/i915/bdw: A bit more advanced LR context alloc/free drm/i915/bdw: Allocate ringbuffers for Logical Ring Contexts drm/i915/bdw: Populate LR contexts (somewhat) drm/i915/bdw: Deferred creation of user-created LRCs drm/i915: Abstract the legacy workload submission mechanism away drm/i915/bdw: Skeleton for the new logical rings submission path drm/i915/bdw: Generic logical ring init and cleanup drm/i915/bdw: GEN-specific logical ring init drm/i915/bdw: GEN-specific logical ring set/get seqno drm/i915/bdw: New logical ring submission mechanism drm/i915/bdw: GEN-specific logical ring emit request drm/i915/bdw: GEN-specific logical ring emit flush drm/i915/bdw: Ring idle and stop with logical rings drm/i915/bdw: Interrupts with logical rings drm/i915/bdw: GEN-specific logical ring emit batchbuffer start drm/i915/bdw: Workload submission mechanism for Execlists
Re: [Intel-gfx] [PATCH] drm/i915: Fix lock dropping in intel_tv_detect()
On Mon, Sep 01, 2014 at 11:09:46AM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > When intel_tv_detect() fails to do load detection it would forget to > drop the locks and clean up the acquire context. Fix it up. > > This is a regression from: > commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44 > Author: Ville Syrjälä > Date: Mon Aug 11 13:15:35 2014 +0300 > > drm/i915: Fix locking for intel_enable_pipe_a() > > Cc: Tibor Billes > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_tv.c | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > index 32186a6..abbf0ea 100644 > --- a/drivers/gpu/drm/i915/intel_tv.c > +++ b/drivers/gpu/drm/i915/intel_tv.c > @@ -1311,7 +1311,8 @@ intel_tv_detect(struct drm_connector *connector, bool > force) > { > struct drm_display_mode mode; > struct intel_tv *intel_tv = intel_attached_tv(connector); > - int type; > + enum drm_connector_status status = connector->status; > + int type = -1; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n", > connector->base.id, connector->name, > @@ -1328,21 +1329,23 @@ intel_tv_detect(struct drm_connector *connector, bool > force) > if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) { > type = intel_tv_detect_type(intel_tv, connector); > intel_release_load_detect_pipe(connector, &tmp); > + status = type < 0 ? > + connector_status_disconnected : > + connector_status_connected; > } else > - return connector_status_unknown; > + status = connector_status_unknown; > > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > - } else > - return connector->status; > + } > > if (type < 0) > - return connector_status_disconnected; > + return status; > > intel_tv->type = type; > intel_tv_find_better_format(connector); > > - return connector_status_connected; > + return status; Hmm, this makes the code unclear. if type != -1, status should always be connected. I think: if (status != connector_status_connected) return status; if (WARN_ON(type < 0)) return connector_status_disconnected; intel_tv->type = type; find_better_format(); return connector_status_connected; And leave type unset to encourage gcc. diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 32186a6..a109b7b 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool force) { struct drm_display_mode mode; struct intel_tv *intel_tv = intel_attached_tv(connector); + enum drm_connector_status status = connector->status; int type; DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n", @@ -1327,17 +1328,20 @@ intel_tv_detect(struct drm_connector *connector, bool force) if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) { type = intel_tv_detect_type(intel_tv, connector); + status = type < 0 ? + connector_status_disconnected : + connector_status_connected; + intel_release_load_detect_pipe(connector, &tmp); } else - return connector_status_unknown; + status = connector_status_unknown; drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); - } else - return connector->status; + } - if (type < 0) - return connector_status_disconnected; + if (status != connector_status_connected) + return status; intel_tv->type = type; intel_tv_find_better_format(connector); -- 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 00/16] drm/i915: 830M/ns201 fixes again
On Fri, Aug 15, 2014 at 10:57:21AM +0300, Ville Syrjälä wrote: > On Fri, Aug 15, 2014 at 01:21:52AM +0300, ville.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä > > > > Thomas asked me to repost my 830/ns2501 patches. So here they are. I added > > a few more patches (trickle feed and unused ring init) to fix some > > post-resume > > issues. The primary plane rmw elimination patches and some locking/load > > detect > > fixes already got merged. > > > > Apart from these we still lack the minimum watermark check. I guess > > we could just take the patch Thomas posted [1]. Doesn't look like a > > more advanced solution is coming any time soon. Though the commit > > message of that patch needs work and it lacks a s-o-b. > > > > The VGACNTR patch might not be necessary any longer since Daniel's > > vga/dummycon stuff. I don't recall if I tested without it, but my > > gut feeling is that it's no longer needed. But I included the patch > > here anyway. > > > > [1] http://patchwork.freedesktop.org/patch/27318/ > > Here's the branch (includes my earlier watermark hack): > > git://gitorious.org/vsyrjala/linux.git alm_fixes11 Ok, I've merged all the patches here except the dvo 2x mode thing. And imo we can also pull in the burst size hack since imo that's what we want - afaik the hw has some bits to select the burst size between 4, 8, ... entries, and atm we always set it to 8. And Bspec explicitly says that setting the fifo to something smaller than burst size will lead to tears. So the patch with a pimped commit message + bit a better comment is good to go. Originally I wanted to implement "proper" burst size selection, but that ran afoul of the general gen2/3 wm mess. Aside: We might want to give intel_calculate_wm some i9xx prefix, since it's very much not generic. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] A few fixes on top of the wa_regs patches
On Sun, Aug 31, 2014 at 08:32:55PM +0100, Siluvery, Arun wrote: > On 30/08/2014 16:50, Damien Lespiau wrote: > >Hi Arun, > > > >I've compiled a few patches that I think solve some small-ish issues around > >your wa_regs series. Could you please have a look at them and comment/give > >your > >r-b tag if you judge appropriate? > > > >On top of those patches, I'd love some comments on the issues I raised in the > >other mail and possible follow up patches to address them. > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-August/051514.html > > Hi Damien, > > I really appreciate you taking time to not just give review comments but > also sending patches to fix those issues. > > Chris suggested a way of emitting all LRIs using a simple function and I > really wanted to rework everything based on that suggestion. > > The LRIs are now organized in an array as opposed to sending them > individually also debugfs patch can make use of it. I have removed the > temporary array included in driver private structure. > I think now it looks clean and we can easily add new w/a with minimal > changes. > > Since all of the patches are modified I think it is better to squash them > with the merged ones rather than updating them with new patches so I have > folded your patches during rework and will send them after testing, please > review them and give your comments. Please don't squash fixup patches when I've merged your patch already - usually I only drop patches when they're terminally broken, so if you send me a new version I have to fiddle things to make it all apply. But squashing in a fixup patch is simpler. And imo also easier to review. And once we deal in fixup patches it's ok to have a bunch of them imo, too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [drm/i915/3.17] panic in i915_digport_work_func
On Mon, Sep 01, 2014 at 05:02:51PM +1000, Dave Airlie wrote: > > drm/i915: add DP 1.2 MST support (v0.7) > > > > This adds DP 1.2 MST support on Haswell systems. > > > I've attached a patch that might fix this, please test and let me know. > > Dave. > From d407c946fbf5c48f30160591f5bd71fbe158aeb4 Mon Sep 17 00:00:00 2001 > From: Dave Airlie > Date: Mon, 1 Sep 2014 16:58:12 +1000 > Subject: [PATCH] drm/i915: handle G45/GM45 pulse detection connected state. > > In the HPD pulse handler we check for long pulses if the port is actually > connected, however we do that for IBX, but we use the pulse handling code on > GM45 systems as well, so we need to use a diffent check. > > This patch refactors the digital port connected check out of the g4x detection > path and reuses it in the hpd pulse path. > > Should fix: > Message-ID: <1409382202.5141.36.ca...@marge.simpson.net> > > Reported-by: Mike Galbraith > Signed-off-by: Dave Airlie Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_dp.c | 55 > +++-- > 1 file changed, 37 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 67cfed6..81d7681 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3661,24 +3661,12 @@ ironlake_dp_detect(struct intel_dp *intel_dp) > return intel_dp_detect_dpcd(intel_dp); > } > > -static enum drm_connector_status > -g4x_dp_detect(struct intel_dp *intel_dp) > +static int g4x_digital_port_connected(struct drm_device *dev, > +struct intel_digital_port > *intel_dig_port) > { > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > uint32_t bit; > > - /* Can't disconnect eDP, but you can close the lid... */ > - if (is_edp(intel_dp)) { > - enum drm_connector_status status; > - > - status = intel_panel_detect(dev); > - if (status == connector_status_unknown) > - status = connector_status_connected; > - return status; > - } > - > if (IS_VALLEYVIEW(dev)) { > switch (intel_dig_port->port) { > case PORT_B: > @@ -3691,7 +3679,7 @@ g4x_dp_detect(struct intel_dp *intel_dp) > bit = PORTD_HOTPLUG_LIVE_STATUS_VLV; > break; > default: > - return connector_status_unknown; > + return -EINVAL; > } > } else { > switch (intel_dig_port->port) { > @@ -3705,11 +3693,36 @@ g4x_dp_detect(struct intel_dp *intel_dp) > bit = PORTD_HOTPLUG_LIVE_STATUS_G4X; > break; > default: > - return connector_status_unknown; > + return -EINVAL; > } > } > > if ((I915_READ(PORT_HOTPLUG_STAT) & bit) == 0) > + return 0; > + return 1; > +} > + > +static enum drm_connector_status > +g4x_dp_detect(struct intel_dp *intel_dp) > +{ > + struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + int ret; > + > + /* Can't disconnect eDP, but you can close the lid... */ > + if (is_edp(intel_dp)) { > + enum drm_connector_status status; > + > + status = intel_panel_detect(dev); > + if (status == connector_status_unknown) > + status = connector_status_connected; > + return status; > + } > + > + ret = g4x_digital_port_connected(dev, intel_dig_port); > + if (ret == -EINVAL) > + return connector_status_unknown; > + else if (ret == 0) > return connector_status_disconnected; > > return intel_dp_detect_dpcd(intel_dp); > @@ -4066,8 +4079,14 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > intel_display_power_get(dev_priv, power_domain); > > if (long_hpd) { > - if (!ibx_digital_port_connected(dev_priv, intel_dig_port)) > - goto mst_fail; > + > + if (HAS_PCH_SPLIT(dev)) { > + if (!ibx_digital_port_connected(dev_priv, > intel_dig_port)) > + goto mst_fail; > + } else { > + if (g4x_digital_port_connected(dev, intel_dig_port) != > 1) > + goto mst_fail; > + } > > if (!intel_dp_get_dpcd(intel_dp)) { > goto mst_fail; > -- > 1.9.3 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-g
Re: [Intel-gfx] [PATCH 0/5] A few fixes on top of the wa_regs patches
On 01/09/2014 10:08, Daniel Vetter wrote: On Sun, Aug 31, 2014 at 08:32:55PM +0100, Siluvery, Arun wrote: On 30/08/2014 16:50, Damien Lespiau wrote: Hi Arun, I've compiled a few patches that I think solve some small-ish issues around your wa_regs series. Could you please have a look at them and comment/give your r-b tag if you judge appropriate? On top of those patches, I'd love some comments on the issues I raised in the other mail and possible follow up patches to address them. http://lists.freedesktop.org/archives/intel-gfx/2014-August/051514.html Hi Damien, I really appreciate you taking time to not just give review comments but also sending patches to fix those issues. Chris suggested a way of emitting all LRIs using a simple function and I really wanted to rework everything based on that suggestion. The LRIs are now organized in an array as opposed to sending them individually also debugfs patch can make use of it. I have removed the temporary array included in driver private structure. I think now it looks clean and we can easily add new w/a with minimal changes. Since all of the patches are modified I think it is better to squash them with the merged ones rather than updating them with new patches so I have folded your patches during rework and will send them after testing, please review them and give your comments. Please don't squash fixup patches when I've merged your patch already - usually I only drop patches when they're terminally broken, so if you send me a new version I have to fiddle things to make it all apply. But squashing in a fixup patch is simpler. And imo also easier to review. In this case all of the code is newly added so most of it should apply cleanly but if the preference is to not squash them I can send fix-up patches accordingly. regards Arun And once we deal in fixup patches it's ok to have a bunch of them imo, too. -Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix lock dropping in intel_tv_detect()
On Mon, Sep 01, 2014 at 09:50:22AM +0100, Chris Wilson wrote: > On Mon, Sep 01, 2014 at 11:09:46AM +0300, ville.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä > > > > When intel_tv_detect() fails to do load detection it would forget to > > drop the locks and clean up the acquire context. Fix it up. > > > > This is a regression from: > > commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44 > > Author: Ville Syrjälä > > Date: Mon Aug 11 13:15:35 2014 +0300 > > > > drm/i915: Fix locking for intel_enable_pipe_a() > > > > Cc: Tibor Billes > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/intel_tv.c | 15 +-- > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_tv.c > > b/drivers/gpu/drm/i915/intel_tv.c > > index 32186a6..abbf0ea 100644 > > --- a/drivers/gpu/drm/i915/intel_tv.c > > +++ b/drivers/gpu/drm/i915/intel_tv.c > > @@ -1311,7 +1311,8 @@ intel_tv_detect(struct drm_connector *connector, bool > > force) > > { > > struct drm_display_mode mode; > > struct intel_tv *intel_tv = intel_attached_tv(connector); > > - int type; > > + enum drm_connector_status status = connector->status; > > + int type = -1; > > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n", > > connector->base.id, connector->name, > > @@ -1328,21 +1329,23 @@ intel_tv_detect(struct drm_connector *connector, > > bool force) > > if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) { > > type = intel_tv_detect_type(intel_tv, connector); > > intel_release_load_detect_pipe(connector, &tmp); > > + status = type < 0 ? > > + connector_status_disconnected : > > + connector_status_connected; > > } else > > - return connector_status_unknown; > > + status = connector_status_unknown; > > > > drm_modeset_drop_locks(&ctx); > > drm_modeset_acquire_fini(&ctx); > > - } else > > - return connector->status; > > + } > > > > if (type < 0) > > - return connector_status_disconnected; > > + return status; > > > > intel_tv->type = type; > > intel_tv_find_better_format(connector); > > > > - return connector_status_connected; > > + return status; > > Hmm, this makes the code unclear. if type != -1, status should always be > connected. > > I think: > > if (status != connector_status_connected) > return status; > > if (WARN_ON(type < 0)) >return connector_status_disconnected; > > intel_tv->type = type; > find_better_format(); > return connector_status_connected; > > And leave type unset to encourage gcc. > > > > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > index 32186a6..a109b7b 100644 > --- a/drivers/gpu/drm/i915/intel_tv.c > +++ b/drivers/gpu/drm/i915/intel_tv.c > @@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool > force) > { > struct drm_display_mode mode; > struct intel_tv *intel_tv = intel_attached_tv(connector); > + enum drm_connector_status status = connector->status; > int type; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n", > @@ -1327,17 +1328,20 @@ intel_tv_detect(struct drm_connector *connector, bool > force) > > if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) > { > type = intel_tv_detect_type(intel_tv, connector); > + status = type < 0 ? > + connector_status_disconnected : > + connector_status_connected; > + > intel_release_load_detect_pipe(connector, &tmp); > } else > - return connector_status_unknown; > + status = connector_status_unknown; > > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > - } else > - return connector->status; > + } > > - if (type < 0) > - return connector_status_disconnected; > + if (status != connector_status_connected) > + return status; > > intel_tv->type = type; > intel_tv_find_better_format(connector); With this approach we're going to have to keep the !force else branch to avoid populating intel_tv->type with stack garbage. But I suppose the resulting code might still be a bit clearer. -- 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: Fix lock dropping in intel_tv_detect()
On Mon, Sep 01, 2014 at 12:45:56PM +0300, Ville Syrjälä wrote: > On Mon, Sep 01, 2014 at 09:50:22AM +0100, Chris Wilson wrote: > > diff --git a/drivers/gpu/drm/i915/intel_tv.c > > b/drivers/gpu/drm/i915/intel_tv.c > > index 32186a6..a109b7b 100644 > > --- a/drivers/gpu/drm/i915/intel_tv.c > > +++ b/drivers/gpu/drm/i915/intel_tv.c > > @@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool > > force) > > { > > struct drm_display_mode mode; > > struct intel_tv *intel_tv = intel_attached_tv(connector); > > + enum drm_connector_status status = connector->status; > > int type; > > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n", > > @@ -1327,17 +1328,20 @@ intel_tv_detect(struct drm_connector *connector, > > bool force) > > > > if (intel_get_load_detect_pipe(connector, &mode, &tmp, > > &ctx)) { > > type = intel_tv_detect_type(intel_tv, connector); > > + status = type < 0 ? > > + connector_status_disconnected : > > + connector_status_connected; > > + > > intel_release_load_detect_pipe(connector, &tmp); > > } else > > - return connector_status_unknown; > > + status = connector_status_unknown; > > > > drm_modeset_drop_locks(&ctx); > > drm_modeset_acquire_fini(&ctx); > > - } else > > - return connector->status; > > + } > > > > - if (type < 0) > > - return connector_status_disconnected; > > + if (status != connector_status_connected) > > + return status; > > > > intel_tv->type = type; > > intel_tv_find_better_format(connector); > > With this approach we're going to have to keep the !force else branch > to avoid populating intel_tv->type with stack garbage. But I suppose > the resulting code might still be a bit clearer. Or just set intel_tv->type directly inside status = intel_tv_detect_type() ? Hmm. Indeed, we should not be touching them at all for !forced, as type is only set for forced, and so we can similarly ignore hpd of better formats. diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 32186a656816..e80eac5e5239 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1169,7 +1169,7 @@ static const struct drm_display_mode reported_modes[] = { * \return true if TV is connected. * \return false if TV is disconnected. */ -static int +static enum drm_connector_status intel_tv_detect_type(struct intel_tv *intel_tv, struct drm_connector *connector) { @@ -1178,10 +1178,10 @@ intel_tv_detect_type(struct intel_tv *intel_tv, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; + enum drm_connector_status status; unsigned long irqflags; u32 tv_ctl, save_tv_ctl; u32 tv_dac, save_tv_dac; - int type; /* Disable TV interrupts around load detect or we'll recurse */ if (connector->polled & DRM_CONNECTOR_POLL_HPD) { @@ -1229,7 +1229,6 @@ intel_tv_detect_type(struct intel_tv *intel_tv, intel_wait_for_vblank(intel_tv->base.base.dev, to_intel_crtc(intel_tv->base.base.crtc)->pipe); - type = -1; tv_dac = I915_READ(TV_DAC); DRM_DEBUG_KMS("TV detected: %x, %x\n", tv_ctl, tv_dac); /* @@ -1238,18 +1237,19 @@ intel_tv_detect_type(struct intel_tv *intel_tv, * 1 0 X svideo * 0 0 0 Component */ + status = connector_status_connected; if ((tv_dac & TVDAC_SENSE_MASK) == (TVDAC_B_SENSE | TVDAC_C_SENSE)) { DRM_DEBUG_KMS("Detected Composite TV connection\n"); - type = DRM_MODE_CONNECTOR_Composite; + intel_tv->type = DRM_MODE_CONNECTOR_Composite; } else if ((tv_dac & (TVDAC_A_SENSE|TVDAC_B_SENSE)) == TVDAC_A_SENSE) { DRM_DEBUG_KMS("Detected S-Video TV connection\n"); - type = DRM_MODE_CONNECTOR_SVIDEO; + intel_tv->type = DRM_MODE_CONNECTOR_SVIDEO; } else if ((tv_dac & TVDAC_SENSE_MASK) == 0) { DRM_DEBUG_KMS("Detected Component TV connection\n"); - type = DRM_MODE_CONNECTOR_Component; + intel_tv->type = DRM_MODE_CONNECTOR_Component; } else { DRM_DEBUG_KMS("Unrecognised TV connection\n"); - type = -1; + status = connector_status_disconnected; } I915_WRITE(TV_DAC, save_tv_dac & ~TVDAC_STATE_CHG_EN); @@ -1269,7 +1269,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv, spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); } - return type; +
[Intel-gfx] [PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()
From: Ville Syrjälä When intel_tv_detect() fails to do load detection it would forget to drop the locks and clean up the acquire context. Fix it up. This is a regression from: commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44 Author: Ville Syrjälä Date: Mon Aug 11 13:15:35 2014 +0300 drm/i915: Fix locking for intel_enable_pipe_a() v2: Make the code more readable (Chris) Cc: Tibor Billes Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_tv.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 32186a6..c6b00f20 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool force) { struct drm_display_mode mode; struct intel_tv *intel_tv = intel_attached_tv(connector); + enum drm_connector_status status; int type; DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n", @@ -1328,15 +1329,21 @@ intel_tv_detect(struct drm_connector *connector, bool force) if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) { type = intel_tv_detect_type(intel_tv, connector); intel_release_load_detect_pipe(connector, &tmp); + status = type < 0 ? + connector_status_disconnected : + connector_status_connected; } else - return connector_status_unknown; + status = connector_status_unknown; drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); } else return connector->status; - if (type < 0) + if (status != connector_status_connected) + return status; + + if (WARN_ON(type < 0)) return connector_status_disconnected; intel_tv->type = type; -- 1.8.5.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()
On Mon, Sep 01, 2014 at 01:07:40PM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > When intel_tv_detect() fails to do load detection it would forget to > drop the locks and clean up the acquire context. Fix it up. > > This is a regression from: > commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44 > Author: Ville Syrjälä > Date: Mon Aug 11 13:15:35 2014 +0300 > > drm/i915: Fix locking for intel_enable_pipe_a() > > v2: Make the code more readable (Chris) > > Cc: Tibor Billes > Signed-off-by: Ville Syrjälä Hmm, if we use WARN_ON() you should init type. Otherwise, Reviewed-by: Chris Wilson -Chrsi -- 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 v2] drm/i915: Fix lock dropping in intel_tv_detect()
On Mon, Sep 01, 2014 at 11:20:09AM +0100, Chris Wilson wrote: > On Mon, Sep 01, 2014 at 01:07:40PM +0300, ville.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä > > > > When intel_tv_detect() fails to do load detection it would forget to > > drop the locks and clean up the acquire context. Fix it up. > > > > This is a regression from: > > commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44 > > Author: Ville Syrjälä > > Date: Mon Aug 11 13:15:35 2014 +0300 > > > > drm/i915: Fix locking for intel_enable_pipe_a() > > > > v2: Make the code more readable (Chris) > > > > Cc: Tibor Billes > > Signed-off-by: Ville Syrjälä > > Hmm, if we use WARN_ON() you should init type. type is always set in the branch that sets status=connected. > > Otherwise, > Reviewed-by: Chris Wilson > -Chrsi > > -- > Chris Wilson, Intel Open Source Technology Centre -- 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 v2] drm/i915: Fix lock dropping in intel_tv_detect()
On Mon, Sep 01, 2014 at 01:36:37PM +0300, Ville Syrjälä wrote: > On Mon, Sep 01, 2014 at 11:20:09AM +0100, Chris Wilson wrote: > > On Mon, Sep 01, 2014 at 01:07:40PM +0300, ville.syrj...@linux.intel.com > > wrote: > > > From: Ville Syrjälä > > > > > > When intel_tv_detect() fails to do load detection it would forget to > > > drop the locks and clean up the acquire context. Fix it up. > > > > > > This is a regression from: > > > commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44 > > > Author: Ville Syrjälä > > > Date: Mon Aug 11 13:15:35 2014 +0300 > > > > > > drm/i915: Fix locking for intel_enable_pipe_a() > > > > > > v2: Make the code more readable (Chris) > > > > > > Cc: Tibor Billes > > > Signed-off-by: Ville Syrjälä > > > > Hmm, if we use WARN_ON() you should init type. > > type is always set in the branch that sets status=connected. Back to thinking about readability and making sure that the WARN_ON never happens with just a glance. Otherwise, the WARN_ON would be better as WARN_ON(unsigned)type >= last_tv_type); Or something. Anway, take your pick and slap my r-b on it. :) -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/5] drm/i915: Rename intel_wa_registers with a i915_ prefix
On 30/08/2014 16:50, Damien Lespiau wrote: Those debugfs files are prefixed by i915, the name of the kernel module, presumably to make the difference with files exposed by core DRM. Also, add a ',' at the end of the last entry. This is to ease the conflict resolution when rebasing internal patches that add a member at the end of the array. Without it, wiggle can't do its job as we need to modify an existing line (appending the ','). Signed-off-by: Damien Lespiau --- drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 1467cc1..fc3d582a 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2628,7 +2628,7 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused) return 0; } -static int intel_wa_registers(struct seq_file *m, void *unused) +static int i915_wa_registers(struct seq_file *m, void *unused) { int i; int ret; @@ -4198,7 +4198,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_semaphore_status", i915_semaphore_status, 0}, {"i915_shared_dplls_info", i915_shared_dplls_info, 0}, {"i915_dp_mst_info", i915_dp_mst_info, 0}, - {"intel_wa_registers", intel_wa_registers, 0} + {"i915_wa_registers", i915_wa_registers, 0}, }; #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list) Reviewed-by: Arun Siluvery This is only for this patch, remaining patches are not required in the rework. regards Arun ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm: i915: reduce memory footprint when debugging
There is no need to use hex_dump_to_buffer() since we have a kernel helper to dump up to 64 bytes just via printk(). In our case the actual size is 15 bytes. Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/i915/intel_dp.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 6db84bf..00b79f5 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3318,15 +3318,11 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) struct drm_device *dev = dig_port->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3]; - if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd, sizeof(intel_dp->dpcd)) < 0) return false; /* aux transfer failed */ - hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd), - 32, 1, dpcd_hex_dump, sizeof(dpcd_hex_dump), false); - DRM_DEBUG_KMS("DPCD: %s\n", dpcd_hex_dump); + DRM_DEBUG_KMS("DPCD: %*ph\n", sizeof(intel_dp->dpcd), intel_dp->dpcd); if (intel_dp->dpcd[DP_DPCD_REV] == 0) return false; /* DPCD not present */ -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/14] drm/i915: Track which port is using which pipe's power sequencer
Reviewed-by: Antti Koskipaa -- - Antti On 08/18/2014 10:16 PM, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > VLV/CHV have a per-pipe panel power sequencer which locks onto the > port once used. We need to keep track wich power sequencers are > locked to which ports. > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_dp.c | 185 > ++- > drivers/gpu/drm/i915/intel_drv.h | 6 ++ > 2 files changed, 168 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 556e4de..4614e6e 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -294,28 +294,99 @@ static enum pipe > vlv_power_sequencer_pipe(struct intel_dp *intel_dp) > { > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > - struct drm_crtc *crtc = intel_dig_port->base.base.crtc; > struct drm_device *dev = intel_dig_port->base.base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - enum port port = intel_dig_port->port; > - enum pipe pipe; > + struct intel_encoder *encoder; > + unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B); > + struct edp_power_seq power_seq; > > lockdep_assert_held(&dev_priv->pps_mutex); > > - /* modeset should have pipe */ > - if (crtc) > - return to_intel_crtc(crtc)->pipe; > + if (intel_dp->pps_pipe != INVALID_PIPE) > + return intel_dp->pps_pipe; > + > + /* > + * We don't have power sequencer currently. > + * Pick one that's not used by other ports. > + */ > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) > { > + struct intel_dp *tmp; > + > + if (encoder->type != INTEL_OUTPUT_EDP) > + continue; > + > + tmp = enc_to_intel_dp(&encoder->base); > + > + if (tmp->pps_pipe != INVALID_PIPE) > + pipes &= ~(1 << tmp->pps_pipe); > + } > + > + /* > + * Didn't find one. This should not happen since there > + * are two power sequencers and up to two eDP ports. > + */ > + if (WARN_ON(pipes == 0)) > + return PIPE_A; > + > + intel_dp->pps_pipe = ffs(pipes) - 1; > + > + DRM_DEBUG_KMS("picked pipe %c power sequencer for port %c\n", > + pipe_name(intel_dp->pps_pipe), > port_name(intel_dig_port->port)); > + > + /* init power sequencer on this pipe and port */ > + intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, > + &power_seq); > + > + return intel_dp->pps_pipe; > +} > + > +static enum pipe > +vlv_initial_power_sequencer_pipe(struct drm_i915_private *dev_priv, > + enum port port) > +{ > + enum pipe pipe; > > - /* init time, try to find a pipe with this port selected */ > for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) { > u32 port_sel = I915_READ(VLV_PIPE_PP_ON_DELAYS(pipe)) & > PANEL_PORT_SELECT_MASK; > - if (port_sel == PANEL_PORT_SELECT_VLV(port)) > - return pipe; > + > + if (port_sel != PANEL_PORT_SELECT_VLV(port)) > + continue; > + > + return pipe; > + } > + > + return INVALID_PIPE; > +} > + > +static void > +vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp) > +{ > + 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; > + struct edp_power_seq power_seq; > + enum port port = intel_dig_port->port; > + > + lockdep_assert_held(&dev_priv->pps_mutex); > + > + /* try to find a pipe with this port selected */ > + intel_dp->pps_pipe = vlv_initial_power_sequencer_pipe(dev_priv, port); > + > + /* didn't find one? just let vlv_power_sequencer_pipe() pick one when > needed */ > + if (intel_dp->pps_pipe == INVALID_PIPE) { > + DRM_DEBUG_KMS("no initial power sequencer for port %c\n", > + port_name(port)); > + return; > } > > - /* shrug */ > - return PIPE_A; > + DRM_DEBUG_KMS("initial power sequencer for port %c: pipe %c\n", > + port_name(port), pipe_name(intel_dp->pps_pipe)); > + > + intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, > + &power_seq); > } > > static u32 _pp_ctrl_reg(struct intel_dp *intel_dp) > @@ -2209,6 +2280,76 @@ static void g4x_pre_enable_dp(struct intel_encoder > *encoder) > } > } > > +static void vlv_steal_power_sequenc
[Intel-gfx] DRI3 only DDX driver
I tried building the xorg intel ddx driver with only DRI3 support, with DRI1 and DRI2 disabled. glxinfo says direct rendering is enabled, but gives no core contexts*. gnome-shell appears to be using software fallback, generally there seems to be no hardware acceleration. I'm wondering if DRI3 isn't ever initialising successfully, and now there's no DRI2 to fall back to. What is the minimal kernel version for DRI3? I'm currently stuck on the 3.14 stable series due to an unresolved kernel bug (which I need to bisect...), is it too old? I remember before the intel (SNA) support landed LIBGL_DEBUG=verbose used to mention DRI3 failing, I see nothing mentioning DRI3 at all now. Is there some way of determining which DRI extension is in use? DRI3 is properly listed in xdpyinfo. * I also get no core contexts also with xwayland/glx, related? signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915: Don't restrict i915_wa_registers to BDW
On 30/08/2014 16:51, Damien Lespiau wrote: We have CHV code that already makes the test obsolete. Besides, when num_wa_regs is 0 (platforms not gathering that W/A data), we expose something sensible already. Signed-off-by: Damien Lespiau --- drivers/gpu/drm/i915/i915_debugfs.c | 5 - 1 file changed, 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index fc3d582a..cd4f045 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2636,11 +2636,6 @@ static int i915_wa_registers(struct seq_file *m, void *unused) struct drm_device *dev = node->minor->dev; struct drm_i915_private *dev_priv = dev->dev_private; - if (!IS_BROADWELL(dev)) { - DRM_DEBUG_DRIVER("Workaround table not available !!\n"); - return -EINVAL; - } - ret = mutex_lock_interruptible(&dev->struct_mutex); if (ret) return ret; This can also be taken, so patches 1 and 5 in this series. Reviewed-by: Arun Siluvery regards Arun ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Cache EDID for a detection cycle
On Tue, Aug 12, 2014 at 09:36:03AM +0100, Chris Wilson wrote: > As we may query the edid multiple times following a detect, record the > EDID found during output discovery and reuse it. This is a separate > issue from caching the output EDID across detection cycles. My only real concern here is what happens when someone forces the connector to connected. Given you only populate detect_edid in ->detect() we wouldn't be able to get the modes from the EDID in that case. I guess one solution would be to implement the ->force() hook and attempt the edid read there. Looking at the code we should always do a ->detect() or ->force() before ->get_modes(). > > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/intel_dp.c | 95 > > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 29 insertions(+), 67 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index ed37407..1eef55c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3764,27 +3764,10 @@ intel_dp_get_edid(struct drm_connector *connector, > struct i2c_adapter *adapter) > return drm_get_edid(connector, adapter); > } > > -static int > -intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter > *adapter) > -{ > - struct intel_connector *intel_connector = to_intel_connector(connector); > - > - /* use cached edid if we have one */ > - if (intel_connector->edid) { > - /* invalid edid */ > - if (IS_ERR(intel_connector->edid)) > - return 0; > - > - return intel_connector_update_modes(connector, > - intel_connector->edid); > - } > - > - return intel_ddc_get_modes(connector, adapter); > -} > - > static enum drm_connector_status > intel_dp_detect(struct drm_connector *connector, bool force) > { > + struct intel_connector *intel_connector = to_intel_connector(connector); > struct intel_dp *intel_dp = intel_attached_dp(connector); > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > struct intel_encoder *intel_encoder = &intel_dig_port->base; > @@ -3795,21 +3778,20 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > struct edid *edid = NULL; > bool ret; > > - power_domain = intel_display_port_power_domain(intel_encoder); > - intel_display_power_get(dev_priv, power_domain); > - > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, connector->name); > + kfree(intel_connector->detect_edid); > + intel_connector->detect_edid = NULL; > > if (intel_dp->is_mst) { > /* MST devices are disconnected from a monitor POV */ > if (intel_encoder->type != INTEL_OUTPUT_EDP) > intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > - status = connector_status_disconnected; > - goto out; > + return connector_status_disconnected; > } > > - intel_dp->has_audio = false; > + power_domain = intel_display_port_power_domain(intel_encoder); > + intel_display_power_get(dev_priv, power_domain); > > if (HAS_PCH_SPLIT(dev)) > status = ironlake_dp_detect(intel_dp); > @@ -3831,15 +3813,13 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > goto out; > } > > - if (intel_dp->force_audio != HDMI_AUDIO_AUTO) { > + edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc); > + intel_connector->detect_edid = edid; > + > + if (intel_dp->force_audio != HDMI_AUDIO_AUTO) > intel_dp->has_audio = (intel_dp->force_audio == HDMI_AUDIO_ON); > - } else { > - edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc); > - if (edid) { > - intel_dp->has_audio = drm_detect_monitor_audio(edid); > - kfree(edid); > - } > - } > + else > + intel_dp->has_audio = drm_detect_monitor_audio(edid); > > if (intel_encoder->type != INTEL_OUTPUT_EDP) > intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > @@ -3852,61 +3832,41 @@ out: > > static int intel_dp_get_modes(struct drm_connector *connector) > { > - struct intel_dp *intel_dp = intel_attached_dp(connector); > - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > - struct intel_encoder *intel_encoder = &intel_dig_port->base; > struct intel_connector *intel_connector = to_intel_connector(connector); > - struct drm_device *dev = connector->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - enum intel_display_power_domain power_domain; > - int ret; > - > - /* We should parse the EDID data and find out if it has an audio sink > - */ > - > - power_domain = intel_display_por
Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Cache EDID for a detection cycle
On Mon, Sep 01, 2014 at 03:05:26PM +0300, Ville Syrjälä wrote: > On Tue, Aug 12, 2014 at 09:36:03AM +0100, Chris Wilson wrote: > > As we may query the edid multiple times following a detect, record the > > EDID found during output discovery and reuse it. This is a separate > > issue from caching the output EDID across detection cycles. > > My only real concern here is what happens when someone forces the > connector to connected. Given you only populate detect_edid in > ->detect() we wouldn't be able to get the modes from the EDID in > that case. > > I guess one solution would be to implement the ->force() hook and > attempt the edid read there. Looking at the code we should always do > a ->detect() or ->force() before ->get_modes(). That should fix our audio detection in those cases as well. Sounds good. -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: reduce memory footprint when debugging
On Mon, 01 Sep 2014, Andy Shevchenko wrote: > There is no need to use hex_dump_to_buffer() since we have a kernel helper to > dump up to 64 bytes just via printk(). In our case the actual size is 15 > bytes. Reviewed-by: Jani Nikula > Signed-off-by: Andy Shevchenko > --- > drivers/gpu/drm/i915/intel_dp.c | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 6db84bf..00b79f5 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3318,15 +3318,11 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > struct drm_device *dev = dig_port->base.base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > > - char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3]; > - > if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd, > sizeof(intel_dp->dpcd)) < 0) > return false; /* aux transfer failed */ > > - hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd), > -32, 1, dpcd_hex_dump, sizeof(dpcd_hex_dump), false); > - DRM_DEBUG_KMS("DPCD: %s\n", dpcd_hex_dump); > + DRM_DEBUG_KMS("DPCD: %*ph\n", sizeof(intel_dp->dpcd), intel_dp->dpcd); > > if (intel_dp->dpcd[DP_DPCD_REV] == 0) > return false; /* DPCD not present */ > -- > 2.1.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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/edid: Reduce horizontal timings for pixel replicated modes
On Tue, Aug 26, 2014 at 10:11:07AM +0200, Daniel Vetter wrote: > On Tue, Aug 26, 2014 at 10:10:09AM +0200, Daniel Vetter wrote: > > On Tue, Aug 19, 2014 at 02:21:21PM -0700, clinton.a.tay...@intel.com wrote: > > > From: Clint Taylor > > > > > > Pixel replicated modes should be 720 horizontal pixel and pixel > > > replicated by the HW across the HDMI cable at 2X pixel clock. Current > > > horizontal resolution of 1440 does not allow pixel duplication to > > > occur and scaling artifacts occur on the TV. HDMI certification > > > 7-26 currently fails for all pixel replicated modes. This change fizes > > > the HDMI certification issues with 480i/576i. > > > > > > V2: Removed interlace flag from VICs 44 and 45. Will be submitted in > > > another patch. Various other formatting fixes. > > > > > > V3: 576i@200 htotal fixed. Check min and max pixel clocks. > > > > > > Signed-off-by: Clint Taylor > > > --- > > > drivers/gpu/drm/drm_edid.c| 96 > > > ++--- > > > drivers/gpu/drm/i915/intel_hdmi.c | 15 -- > > > > Still plea to split out the i915 change. > > Also it's good practice to add all the people who commented on earlier > versions of this patch to the in-patch Cc: list. So Ville should be here > for this one. Also I've totally lost track of these now. Please use --in-reply-to when sending revised patches. -- 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/edid: Reduce horizontal timings for pixel replicated modes
On Mon, Sep 01, 2014 at 04:12:27PM +0300, Ville Syrjälä wrote: > On Tue, Aug 26, 2014 at 10:11:07AM +0200, Daniel Vetter wrote: > > On Tue, Aug 26, 2014 at 10:10:09AM +0200, Daniel Vetter wrote: > > > On Tue, Aug 19, 2014 at 02:21:21PM -0700, clinton.a.tay...@intel.com > > > wrote: > > > > From: Clint Taylor > > > > > > > > Pixel replicated modes should be 720 horizontal pixel and pixel > > > > replicated by the HW across the HDMI cable at 2X pixel clock. Current > > > > horizontal resolution of 1440 does not allow pixel duplication to > > > > occur and scaling artifacts occur on the TV. HDMI certification > > > > 7-26 currently fails for all pixel replicated modes. This change fizes > > > > the HDMI certification issues with 480i/576i. > > > > > > > > V2: Removed interlace flag from VICs 44 and 45. Will be submitted in > > > > another patch. Various other formatting fixes. > > > > > > > > V3: 576i@200 htotal fixed. Check min and max pixel clocks. > > > > > > > > Signed-off-by: Clint Taylor > > > > --- > > > > drivers/gpu/drm/drm_edid.c| 96 > > > > ++--- > > > > drivers/gpu/drm/i915/intel_hdmi.c | 15 -- > > > > > > Still plea to split out the i915 change. > > > > Also it's good practice to add all the people who commented on earlier > > versions of this patch to the in-patch Cc: list. So Ville should be here > > for this one. > > Also I've totally lost track of these now. Please use --in-reply-to > when sending revised patches. Oh and please use --to --cc etc. also instead of sending the same patch separately to multiple lists. -- 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/edid: Reduce horizontal timings for pixel replicated modes
On Tue, Aug 19, 2014 at 02:21:21PM -0700, clinton.a.tay...@intel.com wrote: > From: Clint Taylor > > Pixel replicated modes should be 720 horizontal pixel and pixel > replicated by the HW across the HDMI cable at 2X pixel clock. Current > horizontal resolution of 1440 does not allow pixel duplication to > occur and scaling artifacts occur on the TV. HDMI certification > 7-26 currently fails for all pixel replicated modes. This change fizes > the HDMI certification issues with 480i/576i. > > V2: Removed interlace flag from VICs 44 and 45. Will be submitted in > another patch. Various other formatting fixes. > > V3: 576i@200 htotal fixed. Check min and max pixel clocks. > > Signed-off-by: Clint Taylor Assuming you split it up like Daniel wanted you can slap Reviewed-by: Ville Syrjälä onto both parts. > --- > drivers/gpu/drm/drm_edid.c| 96 > ++--- > drivers/gpu/drm/i915/intel_hdmi.c | 15 -- > 2 files changed, 60 insertions(+), 51 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index f905c63..dc25999 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -632,27 +632,27 @@ static const struct drm_display_mode edid_cea_modes[] = > { > DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC | > DRM_MODE_FLAG_INTERLACE), > .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, }, > - /* 6 - 1440x480i@60Hz */ > - { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478, > -1602, 1716, 0, 480, 488, 494, 525, 0, > + /* 6 - 720(1440)x480i@60Hz */ > + { DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 13500, 720, 739, > +801, 858, 0, 480, 488, 494, 525, 0, > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | > DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), > .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, }, > - /* 7 - 1440x480i@60Hz */ > - { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478, > -1602, 1716, 0, 480, 488, 494, 525, 0, > + /* 7 - 720(1440)x480i@60Hz */ > + { DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 13500, 720, 739, > +801, 858, 0, 480, 488, 494, 525, 0, > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | > DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), > .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, }, > - /* 8 - 1440x240@60Hz */ > - { DRM_MODE("1440x240", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478, > -1602, 1716, 0, 240, 244, 247, 262, 0, > + /* 8 - 720(1440)x240@60Hz */ > + { DRM_MODE("720x240", DRM_MODE_TYPE_DRIVER, 13500, 720, 739, > +801, 858, 0, 240, 244, 247, 262, 0, > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | > DRM_MODE_FLAG_DBLCLK), > .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, }, > - /* 9 - 1440x240@60Hz */ > - { DRM_MODE("1440x240", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478, > -1602, 1716, 0, 240, 244, 247, 262, 0, > + /* 9 - 720(1440)x240@60Hz */ > + { DRM_MODE("720x240", DRM_MODE_TYPE_DRIVER, 13500, 720, 739, > +801, 858, 0, 240, 244, 247, 262, 0, > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | > DRM_MODE_FLAG_DBLCLK), > .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, }, > @@ -714,27 +714,27 @@ static const struct drm_display_mode edid_cea_modes[] = > { > DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC | > DRM_MODE_FLAG_INTERLACE), > .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, }, > - /* 21 - 1440x576i@50Hz */ > - { DRM_MODE("1440x576i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1464, > -1590, 1728, 0, 576, 580, 586, 625, 0, > + /* 21 - 720(1440)x576i@50Hz */ > + { DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 13500, 720, 732, > +795, 864, 0, 576, 580, 586, 625, 0, > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | > DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), > .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, }, > - /* 22 - 1440x576i@50Hz */ > - { DRM_MODE("1440x576i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1464, > -1590, 1728, 0, 576, 580, 586, 625, 0, > + /* 22 - 720(1440)x576i@50Hz */ > + { DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 13500, 720, 732, > +795, 864, 0, 576, 580, 586, 625, 0, > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | > DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), > .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, }, > - /* 23 - 1440x288@50Hz */ > - { DRM_MODE
Re: [Intel-gfx] [PATCH] drm/i915: Prevent recursive deadlock on releasing a busy userptr
On 08/07/2014 02:20 PM, Chris Wilson wrote: During release of the GEM object we hold the struct_mutex. As the object may be holding onto the last reference for the task->mm, calling mmput() may trigger exit_mmap() which close the vma which will call drm_gem_vm_close() and attempt to reacquire the struct_mutex. In order to avoid that recursion, we have to defer the mmput() until after we drop the struct_mutex, i.e. we need to schedule a worker to do the clean up. A further issue spotted by Tvrtko was caused when we took a GTT mmapping of a userptr buffer object. In that case, we would never call mmput as the object would be cyclically referenced by the GTT mmapping and not freed upon process exit - keeping the entire process mm alive after the process task was reaped. The fix employed is to replace the mm_users/mmput() reference handling to mm_count/mmdrop() for the shared i915_mm_struct. INFO: task test_surfaces:1632 blocked for more than 120 seconds. Tainted: GF O 3.14.5+ #1 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. test_surfaces D 0 1632 1590 0x0082 88014914baa8 0046 88014914a010 00012c40 00012c40 8800a0058210 88014784b010 88014914a010 880037b1c820 8800a0058210 880037b1c824 Call Trace: [] schedule+0x29/0x70 [] schedule_preempt_disabled+0xe/0x10 [] __mutex_lock_slowpath+0x183/0x220 [] mutex_lock+0x23/0x40 [] drm_gem_vm_close+0x33/0x70 [drm] [] remove_vma+0x33/0x70 [] exit_mmap+0x11c/0x170 [] mmput+0x6b/0x100 [] i915_gem_userptr_release+0x89/0xc0 [i915] [] i915_gem_free_object+0x126/0x250 [i915] [] drm_gem_object_free+0x2a/0x40 [drm] [] drm_gem_object_handle_unreference_unlocked+0xe2/0x120 [drm] [] drm_gem_object_release_handle+0x64/0x90 [drm] [] idr_for_each+0xab/0x100 [] ? drm_gem_object_handle_unreference_unlocked+0x120/0x120 [drm] [] ? mutex_lock+0x16/0x40 [] drm_gem_release+0x24/0x40 [drm] [] drm_release+0x3fb/0x480 [drm] [] __fput+0xb2/0x260 [] fput+0xe/0x10 [] task_work_run+0x8f/0xf0 [] do_exit+0x1a8/0x480 [] do_group_exit+0x51/0xc0 [] SyS_exit_group+0x17/0x20 [] system_call_fastpath+0x16/0x1b v2: Incorporate feedback from Tvrtko and remove the unnessary mm referencing when creating the i915_mm_struct and improve some of the function names and comments. [snip] I reviewed this some weeks back and did not spot any issues. Reviewed-by: Tvrtko Ursulin Thanks, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [drm/i915/3.17] panic in i915_digport_work_func
On Mon, 2014-09-01 at 17:02 +1000, Dave Airlie wrote: > From: Dave Airlie > Date: Mon, 1 Sep 2014 16:58:12 +1000 > Subject: [PATCH] drm/i915: handle G45/GM45 pulse detection connected > state. > > In the HPD pulse handler we check for long pulses if the port is > actually > connected, however we do that for IBX, but we use the pulse handling > code on > GM45 systems as well, so we need to use a diffent check. > > This patch refactors the digital port connected check out of the g4x > detection > path and reuses it in the hpd pulse path. > > Should fix: > Message-ID: <1409382202.5141.36.ca...@marge.simpson.net> > > Reported-by: Mike Galbraith > Signed-off-by: Dave Airlie Daniel reviewed this already, but Jani asked me to take a look, so: Acked-by: Imre Deak One thing for the future is to move ibx_digital_port_connected() to intel_dp.c too and make its return value match that of g4x_digital_port_connected(). --Imre > --- > drivers/gpu/drm/i915/intel_dp.c | 55 > +++-- > 1 file changed, 37 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index 67cfed6..81d7681 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3661,24 +3661,12 @@ ironlake_dp_detect(struct intel_dp *intel_dp) > return intel_dp_detect_dpcd(intel_dp); > } > > -static enum drm_connector_status > -g4x_dp_detect(struct intel_dp *intel_dp) > +static int g4x_digital_port_connected(struct drm_device *dev, > + struct intel_digital_port > *intel_dig_port) > { > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_digital_port *intel_dig_port = > dp_to_dig_port(intel_dp); > uint32_t bit; > > - /* Can't disconnect eDP, but you can close the lid... */ > - if (is_edp(intel_dp)) { > - enum drm_connector_status status; > - > - status = intel_panel_detect(dev); > - if (status == connector_status_unknown) > - status = connector_status_connected; > - return status; > - } > - > if (IS_VALLEYVIEW(dev)) { > switch (intel_dig_port->port) { > case PORT_B: > @@ -3691,7 +3679,7 @@ g4x_dp_detect(struct intel_dp *intel_dp) > bit = PORTD_HOTPLUG_LIVE_STATUS_VLV; > break; > default: > - return connector_status_unknown; > + return -EINVAL; > } > } else { > switch (intel_dig_port->port) { > @@ -3705,11 +3693,36 @@ g4x_dp_detect(struct intel_dp *intel_dp) > bit = PORTD_HOTPLUG_LIVE_STATUS_G4X; > break; > default: > - return connector_status_unknown; > + return -EINVAL; > } > } > > if ((I915_READ(PORT_HOTPLUG_STAT) & bit) == 0) > + return 0; > + return 1; > +} > + > +static enum drm_connector_status > +g4x_dp_detect(struct intel_dp *intel_dp) > +{ > + struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct intel_digital_port *intel_dig_port = > dp_to_dig_port(intel_dp); > + int ret; > + > + /* Can't disconnect eDP, but you can close the lid... */ > + if (is_edp(intel_dp)) { > + enum drm_connector_status status; > + > + status = intel_panel_detect(dev); > + if (status == connector_status_unknown) > + status = connector_status_connected; > + return status; > + } > + > + ret = g4x_digital_port_connected(dev, intel_dig_port); > + if (ret == -EINVAL) > + return connector_status_unknown; > + else if (ret == 0) > return connector_status_disconnected; > > return intel_dp_detect_dpcd(intel_dp); > @@ -4066,8 +4079,14 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > intel_display_power_get(dev_priv, power_domain); > > if (long_hpd) { > - if (!ibx_digital_port_connected(dev_priv, > intel_dig_port)) > - goto mst_fail; > + > + if (HAS_PCH_SPLIT(dev)) { > + if (!ibx_digital_port_connected(dev_priv, > intel_dig_port)) > + goto mst_fail; > + } else { > + if (g4x_digital_port_connected(dev, > intel_dig_port) != 1) > + goto mst_fail; > + } > > if (!intel_dp_get_dpcd(intel_dp)) { > goto mst_fail; signature.asc Description: This is a digitally signed message part
[Intel-gfx] [PATCH 0/4] Rework workaround init fn for BDW and CHV
The patches that initialize workarounds for BDW and CHV using LRIs are already merged in the tree but I received few more comments from Chris and Damien. I have reworked these patches as per their comments; it fixes some issues and the code now looks clean. we can easily add more workarounds with minimal changes. I have tried to split BDW and CHV changes as separate patches but it was getting ugly hence combined them also since it is a fixup patch it should be ok. In the previous implementation we were emitting each LRI individually so the total number was not clear during init so a temporary array was used to save w/a data (this is exported to debugfs) but if not initialized we could potentially overrun array and also the condition was not correct. In this rework, w/a are organized as an array so we know the exact count from the beginning and temporary array is no longer required. The corresponding i-g-t is gem_workarounds.c and it is also updated. Arun Siluvery (2): drm/i915: Rework workaround init functions for BDW and CHV drm/i915: Rework workaround data exporting to debugfs Damien Lespiau (2): drm/i915: Rename intel_wa_registers with a i915_ prefix drm/i915: Don't restrict i915_wa_registers to BDW drivers/gpu/drm/i915/i915_debugfs.c | 42 ++--- drivers/gpu/drm/i915/i915_drv.h | 14 --- drivers/gpu/drm/i915/intel_ringbuffer.c | 162 ++-- drivers/gpu/drm/i915/intel_ringbuffer.h | 8 ++ 4 files changed, 107 insertions(+), 119 deletions(-) -- 2.0.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Rework workaround init functions for BDW and CHV
This rework is based on suggestion from Chris. Now w/a are organized in an array and all of them are emitted in single fn instead of sending them individually. This approach is very clean and new w/a can be added with minimal changes. The same array can be used when exporting them to debugfs and the temporary array in the current implementation is not required. Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/intel_ringbuffer.c | 149 +--- 1 file changed, 58 insertions(+), 91 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index c5e4dc7..bae1527 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -650,87 +650,71 @@ err: return ret; } -static inline void intel_ring_emit_wa(struct intel_engine_cs *ring, - u32 addr, u32 value) +static int i915_request_emit_lri(struct intel_engine_cs *ring, +int num_registers, const u32 *lri_list) { - struct drm_device *dev = ring->dev; - struct drm_i915_private *dev_priv = dev->dev_private; + int i; + int ret; - if (dev_priv->num_wa_regs > I915_MAX_WA_REGS) - return; + ret = intel_ring_begin(ring, (2 * num_registers + 1)); + if (ret) + return ret; - intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1)); - intel_ring_emit(ring, addr); - intel_ring_emit(ring, value); + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_registers)); + for (i = 0; i < 2*num_registers; i += 2) { + intel_ring_emit(ring, *(lri_list + i)); + intel_ring_emit(ring, *(lri_list + i + 1)); + } - dev_priv->intel_wa_regs[dev_priv->num_wa_regs].addr = addr; - dev_priv->intel_wa_regs[dev_priv->num_wa_regs].mask = (value) & 0x; - /* value is updated with the status of remaining bits of this -* register when it is read from debugfs file -*/ - dev_priv->intel_wa_regs[dev_priv->num_wa_regs].value = value; - dev_priv->num_wa_regs++; + intel_ring_emit(ring, MI_NOOP); + intel_ring_advance(ring); - return; + return 0; } -static int bdw8_init_workarounds(struct intel_engine_cs *ring) -{ - int ret; - struct drm_device *dev = ring->dev; - struct drm_i915_private *dev_priv = dev->dev_private; +static const u32 bdw_ring_init_context[] = { /* * workarounds applied in this fn are part of register state context, * they need to be re-initialized followed by gpu reset, suspend/resume, * module reload. */ - dev_priv->num_wa_regs = 0; - memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs)); - - /* -* update the number of dwords required based on the -* actual number of workarounds applied -*/ - ret = intel_ring_begin(ring, 24); - if (ret) - return ret; /* WaDisablePartialInstShootdown:bdw */ /* WaDisableThreadStallDopClockGating:bdw */ /* FIXME: Unclear whether we really need this on production bdw. */ - intel_ring_emit_wa(ring, GEN8_ROW_CHICKEN, - _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE -| STALL_DOP_GATING_DISABLE)); + GEN8_ROW_CHICKEN, + _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE) | + _MASKED_BIT_ENABLE(STALL_DOP_GATING_DISABLE), /* WaDisableDopClockGating:bdw May not be needed for production */ - intel_ring_emit_wa(ring, GEN7_ROW_CHICKEN2, - _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE)); + GEN7_ROW_CHICKEN2, + _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE), /* * This GEN8_CENTROID_PIXEL_OPT_DIS W/A is only needed for * pre-production hardware */ - intel_ring_emit_wa(ring, HALF_SLICE_CHICKEN3, - _MASKED_BIT_ENABLE(GEN8_CENTROID_PIXEL_OPT_DIS - | GEN8_SAMPLER_POWER_BYPASS_DIS)); + HALF_SLICE_CHICKEN3, + _MASKED_BIT_ENABLE(GEN8_CENTROID_PIXEL_OPT_DIS + | GEN8_SAMPLER_POWER_BYPASS_DIS), - intel_ring_emit_wa(ring, GEN7_HALF_SLICE_CHICKEN1, - _MASKED_BIT_ENABLE(GEN7_SINGLE_SUBSCAN_DISPATCH_ENABLE)); + GEN7_HALF_SLICE_CHICKEN1, + _MASKED_BIT_ENABLE(GEN7_SINGLE_SUBSCAN_DISPATCH_ENABLE), - intel_ring_emit_wa(ring, COMMON_SLICE_CHICKEN2, - _MASKED_BIT_ENABLE(GEN8_CSC2_SBE_VUE_CACHE_CONSERVATIVE)); + COMMON_SLICE_CHICKEN2, + _MASKED_BIT_ENABLE(GEN8_CSC2_SBE_VUE_CACHE_CONSERVATIVE), /* Use Force Non-Coherent whenever executing a 3D context. This is a * workaround for for a possible hang in the unlikely event a TL
[Intel-gfx] [PATCH 4/4] drm/i915: Rework workaround data exporting to debugfs
Now w/a are organized in an array so we know exactly how many of them are applied; use the same array while exporting data to debugfs and remove the temporary array we currently have in driver priv structure. Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_debugfs.c | 41 +++-- drivers/gpu/drm/i915/i915_drv.h | 14 --- drivers/gpu/drm/i915/intel_ringbuffer.c | 15 drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++ 4 files changed, 52 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 2727bda..bab0408 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2465,6 +2465,14 @@ static int i915_wa_registers(struct seq_file *m, void *unused) struct drm_info_node *node = (struct drm_info_node *) m->private; struct drm_device *dev = node->minor->dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_ring_context_rodata ro_data; + + ret = ring_context_rodata(dev, &ro_data); + if (ret) { + seq_printf(m, "Workarounds applied: 0\n"); + DRM_DEBUG_DRIVER("Workaround table not available !!\n"); + return -EINVAL; + } ret = mutex_lock_interruptible(&dev->struct_mutex); if (ret) @@ -2472,18 +2480,27 @@ static int i915_wa_registers(struct seq_file *m, void *unused) intel_runtime_pm_get(dev_priv); - seq_printf(m, "Workarounds applied: %d\n", dev_priv->num_wa_regs); - for (i = 0; i < dev_priv->num_wa_regs; ++i) { - u32 addr, mask; - - addr = dev_priv->intel_wa_regs[i].addr; - mask = dev_priv->intel_wa_regs[i].mask; - dev_priv->intel_wa_regs[i].value = I915_READ(addr) | mask; - if (dev_priv->intel_wa_regs[i].addr) - seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n", - dev_priv->intel_wa_regs[i].addr, - dev_priv->intel_wa_regs[i].value, - dev_priv->intel_wa_regs[i].mask); + seq_printf(m, "Workarounds applied: %d\n", ro_data.num_items/2); + for (i = 0; i < ro_data.num_items; i += 2) { + u32 addr, mask, value; + + addr = ro_data.init_context[i]; + /* +* Most of workarounds are masked registers; +* to set a bit in lower 16-bits we set a mask bit in +* upper 16-bits so we can take either of them as mask but +* it doesn't work if the w/a is about clearing a bit so +* use upper 16-bits to cover both cases. +*/ + mask = ro_data.init_context[i+1] >> 16; + + /* +* value represents the status of other bits in the +* register besides w/a bits +*/ + value = I915_READ(addr) | mask; + seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n", + addr, value, mask); } intel_runtime_pm_put(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 49b7be7..bcf79f0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1553,20 +1553,6 @@ struct drm_i915_private { struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; int dpio_phy_iosf_port[I915_NUM_PHYS_VLV]; - /* -* workarounds are currently applied at different places and -* changes are being done to consolidate them so exact count is -* not clear at this point, use a max value for now. -*/ -#define I915_MAX_WA_REGS 16 - struct { - u32 addr; - u32 value; - /* bitmask representing WA bits */ - u32 mask; - } intel_wa_regs[I915_MAX_WA_REGS]; - u32 num_wa_regs; - /* Reclocking support */ bool render_reclock_avail; bool lvds_downclock_avail; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index bae1527..6c07e69 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -760,6 +760,21 @@ static int ring_init_context(struct intel_engine_cs *ring) return ret; } +int ring_context_rodata(struct drm_device *dev, + struct intel_ring_context_rodata *ro_data) +{ + if (IS_CHERRYVIEW(dev)) { + ro_data->init_context = chv_ring_init_context; + ro_data->num_items = ARRAY_SIZE(chv_ring_init_context); + } else if (IS_BROADWELL(dev)) { + ro_data->init_context = bdw_ring_init_context; + ro_data->num_items = ARRAY_SIZE(bdw_ring_init_context); + } else + return -EINVAL; + + return 0; +
[Intel-gfx] [PATCH 3/4] drm/i915: Don't restrict i915_wa_registers to BDW
From: Damien Lespiau We have CHV code that already makes the test obsolete. Besides, when num_wa_regs is 0 (platforms not gathering that W/A data), we expose something sensible already. Signed-off-by: Damien Lespiau Reviewed-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_debugfs.c | 5 - 1 file changed, 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index f09ba8c..2727bda 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2466,11 +2466,6 @@ static int i915_wa_registers(struct seq_file *m, void *unused) struct drm_device *dev = node->minor->dev; struct drm_i915_private *dev_priv = dev->dev_private; - if (!IS_BROADWELL(dev)) { - DRM_DEBUG_DRIVER("Workaround table not available !!\n"); - return -EINVAL; - } - ret = mutex_lock_interruptible(&dev->struct_mutex); if (ret) return ret; -- 2.0.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: Rename intel_wa_registers with a i915_ prefix
From: Damien Lespiau Those debugfs files are prefixed by i915, the name of the kernel module, presumably to make the difference with files exposed by core DRM. Also, add a ',' at the end of the last entry. This is to ease the conflict resolution when rebasing internal patches that add a member at the end of the array. Without it, wiggle can't do its job as we need to modify an existing line (appending the ','). Signed-off-by: Damien Lespiau --- drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index f0d63f6..f09ba8c 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2458,7 +2458,7 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused) return 0; } -static int intel_wa_registers(struct seq_file *m, void *unused) +static int i915_wa_registers(struct seq_file *m, void *unused) { int i; int ret; @@ -4026,7 +4026,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_semaphore_status", i915_semaphore_status, 0}, {"i915_shared_dplls_info", i915_shared_dplls_info, 0}, {"i915_dp_mst_info", i915_dp_mst_info, 0}, - {"intel_wa_registers", intel_wa_registers, 0} + {"i915_wa_registers", i915_wa_registers, 0}, }; #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list) -- 2.0.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/2] Rework workaround igt
Rework of the test as kernel patch is modified. Arun Siluvery (1): igt/gem_workarounds: rework igt to test workaround registers Damien Lespiau (1): gem_workarounds: intel_wa_registers is now prefixed with i915 tests/gem_workarounds.c | 46 -- 1 file changed, 24 insertions(+), 22 deletions(-) -- 2.0.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] igt/gem_workarounds: rework igt to test workaround registers
kernel patch that exports w/a data to debugfs is reworked so update igt accordingly. Address review comments from Damien. - if kernel is not exposing w/a data instead of failing just skip instead. - if the platform is not exposing w/a table then no of workarounds applied are 0; we can use this data to skip platform checks. Signed-off-by: Arun Siluvery --- tests/gem_workarounds.c | 44 +++- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/tests/gem_workarounds.c b/tests/gem_workarounds.c index 32156d2..fae4382 100644 --- a/tests/gem_workarounds.c +++ b/tests/gem_workarounds.c @@ -62,7 +62,7 @@ int drm_fd; uint32_t devid; static drm_intel_bufmgr *bufmgr; struct intel_batchbuffer *batch; -int num_wa_regs; +int num_wa_regs = 0; struct intel_wa_reg *wa_regs; @@ -153,7 +153,7 @@ static void check_workarounds(enum operation op, int num) igt_info("Address\tbefore\t\tafter\t\tw/a mask\tresult\n"); for (i = 0; i < num; ++i) { status = (current_wa[i].value & current_wa[i].mask) != - (wa_regs[i].value & wa_regs[i].mask); + wa_regs[i].mask; if (status) ++fail_count; @@ -171,21 +171,15 @@ out: igt_main { igt_fixture { - int i; int fd; int ret; FILE *file; char *line = NULL; size_t line_size; - drm_fd = drm_open_any(); - - bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096); - devid = intel_get_drm_devid(drm_fd); - batch = intel_batchbuffer_alloc(bufmgr, devid); - fd = igt_debugfs_open("i915_wa_registers", O_RDONLY); - igt_assert(fd >= 0); + if (fd < 0) + igt_skip_on("No Workaround table available !!\n"); file = fdopen(fd, "r"); igt_assert(file > 0); @@ -193,32 +187,40 @@ igt_main ret = getline(&line, &line_size, file); igt_assert(ret > 0); sscanf(line, "Workarounds applied: %d", &num_wa_regs); - igt_assert(num_wa_regs > 0); - wa_regs = malloc(num_wa_regs * sizeof(*wa_regs)); + if (num_wa_regs) { + int i = 0; - i = 0; - while(getline(&line, &line_size, file) > 0) { - sscanf(line, "0x%X: 0x%08X, mask: 0x%08X", - &wa_regs[i].addr, &wa_regs[i].value, - &wa_regs[i].mask); - ++i; - } + wa_regs = malloc(num_wa_regs * sizeof(*wa_regs)); + while (getline(&line, &line_size, file) > 0) { + sscanf(line, "0x%X: 0x%08X, mask: 0x%08X", + &wa_regs[i].addr, &wa_regs[i].value, + &wa_regs[i].mask); + ++i; + } + } else + igt_info("No workarounds exported\n"); free(line); fclose(file); close(fd); + + drm_fd = drm_open_any(); + + bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096); + devid = intel_get_drm_devid(drm_fd); + batch = intel_batchbuffer_alloc(bufmgr, devid); } igt_subtest("check-workaround-data-after-reset") { - if (IS_BROADWELL(devid)) + if (num_wa_regs) check_workarounds(GPU_RESET, num_wa_regs); else igt_skip_on("No Workaround table available!!\n"); } igt_subtest("check-workaround-data-after-suspend-resume") { - if (IS_BROADWELL(devid)) + if (num_wa_regs) check_workarounds(SUSPEND_RESUME, num_wa_regs); else igt_skip_on("No Workaround table available!!\n"); -- 2.0.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] gem_workarounds: intel_wa_registers is now prefixed with i915
From: Damien Lespiau Signed-off-by: Damien Lespiau --- tests/gem_workarounds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/gem_workarounds.c b/tests/gem_workarounds.c index 6826562..32156d2 100644 --- a/tests/gem_workarounds.c +++ b/tests/gem_workarounds.c @@ -184,7 +184,7 @@ igt_main devid = intel_get_drm_devid(drm_fd); batch = intel_batchbuffer_alloc(bufmgr, devid); - fd = igt_debugfs_open("intel_wa_registers", O_RDONLY); + fd = igt_debugfs_open("i915_wa_registers", O_RDONLY); igt_assert(fd >= 0); file = fdopen(fd, "r"); -- 2.0.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Rework workaround data exporting to debugfs
On Mon, Sep 01, 2014 at 02:28:53PM +0100, Arun Siluvery wrote: > Now w/a are organized in an array so we know exactly how many of them > are applied; use the same array while exporting data to debugfs and > remove the temporary array we currently have in driver priv structure. > > Signed-off-by: Arun Siluvery > --- > drivers/gpu/drm/i915/i915_debugfs.c | 41 > +++-- > drivers/gpu/drm/i915/i915_drv.h | 14 --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 15 > drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++ > 4 files changed, 52 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 2727bda..bab0408 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2465,6 +2465,14 @@ static int i915_wa_registers(struct seq_file *m, void > *unused) > struct drm_info_node *node = (struct drm_info_node *) m->private; > struct drm_device *dev = node->minor->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_ring_context_rodata ro_data; > + > + ret = ring_context_rodata(dev, &ro_data); > + if (ret) { > + seq_printf(m, "Workarounds applied: 0\n"); seq_puts() > + DRM_DEBUG_DRIVER("Workaround table not available !!\n"); > + return -EINVAL; > + } > > ret = mutex_lock_interruptible(&dev->struct_mutex); > if (ret) > @@ -2472,18 +2480,27 @@ static int i915_wa_registers(struct seq_file *m, void > *unused) > > intel_runtime_pm_get(dev_priv); > > - seq_printf(m, "Workarounds applied: %d\n", dev_priv->num_wa_regs); > - for (i = 0; i < dev_priv->num_wa_regs; ++i) { > - u32 addr, mask; > - > - addr = dev_priv->intel_wa_regs[i].addr; > - mask = dev_priv->intel_wa_regs[i].mask; > - dev_priv->intel_wa_regs[i].value = I915_READ(addr) | mask; > - if (dev_priv->intel_wa_regs[i].addr) > - seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n", > -dev_priv->intel_wa_regs[i].addr, > -dev_priv->intel_wa_regs[i].value, > -dev_priv->intel_wa_regs[i].mask); > + seq_printf(m, "Workarounds applied: %d\n", ro_data.num_items/2); > + for (i = 0; i < ro_data.num_items; i += 2) { > + u32 addr, mask, value; > + > + addr = ro_data.init_context[i]; > + /* > + * Most of workarounds are masked registers; > + * to set a bit in lower 16-bits we set a mask bit in > + * upper 16-bits so we can take either of them as mask but > + * it doesn't work if the w/a is about clearing a bit so > + * use upper 16-bits to cover both cases. > + */ > + mask = ro_data.init_context[i+1] >> 16; "Most" doesn't seem good here. Either it's "all" and we're happy, or we need a generic way to describe the W/A (masked register or not). value + mask is generic enough to code for both cases. > + > + /* > + * value represents the status of other bits in the > + * register besides w/a bits > + */ > + value = I915_READ(addr) | mask; > + seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n", > +addr, value, mask); > } I still don't get it. 'value' is supposed to be the reference value for the W/A, but you're or'ing the mask here, so you treat the mask as if it were the reference value. This won't work if the W/A is about setting multi-bits fields or about clearing a bit. The comment is still not clear enough. You're saying "other bits besides the w/a bits", but or'ing the mask doesn't do that. Why do we care about the "other bits" in the reference value? they don't matter. Why use something else than (ro_data.init_context[i+1] & 0x) for the value here (as long we're talking about masked registers)? -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Include task->name and master status in debugfs clients info
Hi On Sat, Aug 9, 2014 at 8:22 AM, Chris Wilson wrote: > Showing who is the current master is useful for trying to decypher > errors when trying to acquire master (e.g. a race with X taking over > from plymouth). By including the process name as well as the pid > simplifies the task of grabbing enough information remotely at the point > of error. > > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/drm_info.c | 17 - > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c > index 15ec9f4..d813430 100644 > --- a/drivers/gpu/drm/drm_info.c > +++ b/drivers/gpu/drm/drm_info.c > @@ -184,14 +184,21 @@ int drm_clients_info(struct seq_file *m, void *data) > struct drm_file *priv; > > mutex_lock(&dev->struct_mutex); > - seq_printf(m, "a devpiduid magic\n\n"); > - list_for_each_entry(priv, &dev->filelist, lhead) { > - seq_printf(m, "%c %3d %5d %5d %10u\n", > - priv->authenticated ? 'y' : 'n', > - priv->minor->index, > + seq_printf(m, " pid dev master a uid > magic\n"); Maybe mention "task" here? Or is this supposed to be a logical part of "pid"? > + list_for_each_entry_reverse(priv, &dev->filelist, lhead) { No idea why you do backwards traversal, but ok.. > + struct task_struct *task; > + > + rcu_read_lock(); What's that rcu-lock for? task->comm is pre-allocated, priv->pid is static, kuid... no idea? Anyway, patch looks good otherwise. Especially task->comm sounds really handy in that list. Thanks David > + task = pid_task(priv->pid, PIDTYPE_PID); > + seq_printf(m, "%20s %5d %3d %c%c %5d %10u\n", > + task ? task->comm : "", >pid_vnr(priv->pid), > + priv->minor->index, > + priv->is_master ? 'y' : 'n', > + priv->authenticated ? 'y' : 'n', >from_kuid_munged(seq_user_ns(m), priv->uid), >priv->magic); > + rcu_read_unlock(); > } > mutex_unlock(&dev->struct_mutex); > return 0; > -- > 1.9.1 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] igt/gem_workarounds: rework igt to test workaround registers
On Mon, Sep 01, 2014 at 02:29:47PM +0100, Arun Siluvery wrote: > kernel patch that exports w/a data to debugfs is reworked so > update igt accordingly. > > Address review comments from Damien. > - if kernel is not exposing w/a data instead of failing just skip instead. > - if the platform is not exposing w/a table then no of workarounds > applied are 0; we can use this data to skip platform checks. > > Signed-off-by: Arun Siluvery > --- > tests/gem_workarounds.c | 44 +++- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/tests/gem_workarounds.c b/tests/gem_workarounds.c > index 32156d2..fae4382 100644 > --- a/tests/gem_workarounds.c > +++ b/tests/gem_workarounds.c > @@ -62,7 +62,7 @@ int drm_fd; > uint32_t devid; > static drm_intel_bufmgr *bufmgr; > struct intel_batchbuffer *batch; > -int num_wa_regs; > +int num_wa_regs = 0; > struct intel_wa_reg *wa_regs; > > > @@ -153,7 +153,7 @@ static void check_workarounds(enum operation op, int num) > igt_info("Address\tbefore\t\tafter\t\tw/a mask\tresult\n"); > for (i = 0; i < num; ++i) { > status = (current_wa[i].value & current_wa[i].mask) != > - (wa_regs[i].value & wa_regs[i].mask); > + wa_regs[i].mask; Hum. This looks so fishy it can't be right. The heart of the problem is that you're not clear what a mask or value should be. To me: - A mask selects bits - value is the reference W/A value (containing only the correct bits for a single W/A, some of those bits can be 0, mask is telling us which bits we're interested about. - read(reg) & mask is the W/A value read back from the register -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [drm/i915/3.17] panic in i915_digport_work_func
On Mon, 01 Sep 2014, Imre Deak wrote: > On Mon, 2014-09-01 at 17:02 +1000, Dave Airlie wrote: >> From: Dave Airlie >> Date: Mon, 1 Sep 2014 16:58:12 +1000 >> Subject: [PATCH] drm/i915: handle G45/GM45 pulse detection connected >> state. >> >> In the HPD pulse handler we check for long pulses if the port is >> actually >> connected, however we do that for IBX, but we use the pulse handling >> code on >> GM45 systems as well, so we need to use a diffent check. >> >> This patch refactors the digital port connected check out of the g4x >> detection >> path and reuses it in the hpd pulse path. >> >> Should fix: >> Message-ID: <1409382202.5141.36.ca...@marge.simpson.net> >> >> Reported-by: Mike Galbraith >> Signed-off-by: Dave Airlie > > Daniel reviewed this already, but Jani asked me to take a look, so: > Acked-by: Imre Deak Pushed to drm-intel-fixes, thanks for the patch, review, and testing. BR, Jani. > > One thing for the future is to move ibx_digital_port_connected() to > intel_dp.c too and make its return value match that of > g4x_digital_port_connected(). > > --Imre > >> --- >> drivers/gpu/drm/i915/intel_dp.c | 55 >> +++-- >> 1 file changed, 37 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c >> index 67cfed6..81d7681 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3661,24 +3661,12 @@ ironlake_dp_detect(struct intel_dp *intel_dp) >> return intel_dp_detect_dpcd(intel_dp); >> } >> >> -static enum drm_connector_status >> -g4x_dp_detect(struct intel_dp *intel_dp) >> +static int g4x_digital_port_connected(struct drm_device *dev, >> + struct intel_digital_port >> *intel_dig_port) >> { >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); >> struct drm_i915_private *dev_priv = dev->dev_private; >> - struct intel_digital_port *intel_dig_port = >> dp_to_dig_port(intel_dp); >> uint32_t bit; >> >> - /* Can't disconnect eDP, but you can close the lid... */ >> - if (is_edp(intel_dp)) { >> - enum drm_connector_status status; >> - >> - status = intel_panel_detect(dev); >> - if (status == connector_status_unknown) >> - status = connector_status_connected; >> - return status; >> - } >> - >> if (IS_VALLEYVIEW(dev)) { >> switch (intel_dig_port->port) { >> case PORT_B: >> @@ -3691,7 +3679,7 @@ g4x_dp_detect(struct intel_dp *intel_dp) >> bit = PORTD_HOTPLUG_LIVE_STATUS_VLV; >> break; >> default: >> - return connector_status_unknown; >> + return -EINVAL; >> } >> } else { >> switch (intel_dig_port->port) { >> @@ -3705,11 +3693,36 @@ g4x_dp_detect(struct intel_dp *intel_dp) >> bit = PORTD_HOTPLUG_LIVE_STATUS_G4X; >> break; >> default: >> - return connector_status_unknown; >> + return -EINVAL; >> } >> } >> >> if ((I915_READ(PORT_HOTPLUG_STAT) & bit) == 0) >> + return 0; >> + return 1; >> +} >> + >> +static enum drm_connector_status >> +g4x_dp_detect(struct intel_dp *intel_dp) >> +{ >> + struct drm_device *dev = intel_dp_to_dev(intel_dp); >> + struct intel_digital_port *intel_dig_port = >> dp_to_dig_port(intel_dp); >> + int ret; >> + >> + /* Can't disconnect eDP, but you can close the lid... */ >> + if (is_edp(intel_dp)) { >> + enum drm_connector_status status; >> + >> + status = intel_panel_detect(dev); >> + if (status == connector_status_unknown) >> + status = connector_status_connected; >> + return status; >> + } >> + >> + ret = g4x_digital_port_connected(dev, intel_dig_port); >> + if (ret == -EINVAL) >> + return connector_status_unknown; >> + else if (ret == 0) >> return connector_status_disconnected; >> >> return intel_dp_detect_dpcd(intel_dp); >> @@ -4066,8 +4079,14 @@ intel_dp_hpd_pulse(struct intel_digital_port >> *intel_dig_port, bool long_hpd) >> intel_display_power_get(dev_priv, power_domain); >> >> if (long_hpd) { >> - if (!ibx_digital_port_connected(dev_priv, >> intel_dig_port)) >> - goto mst_fail; >> + >> + if (HAS_PCH_SPLIT(dev)) { >> + if (!ibx_digital_port_connected(dev_priv, >> intel_dig_port)) >> + goto mst_fail; >> + } else { >> + if (g4x_digital_port_connected(dev, >> intel_dig_port) != 1) >> +
Re: [Intel-gfx] [PATCH] drm: Include task->name and master status in debugfs clients info
On Mon, Sep 01, 2014 at 04:11:43PM +0200, David Herrmann wrote: > Hi > > On Sat, Aug 9, 2014 at 8:22 AM, Chris Wilson wrote: > > Showing who is the current master is useful for trying to decypher > > errors when trying to acquire master (e.g. a race with X taking over > > from plymouth). By including the process name as well as the pid > > simplifies the task of grabbing enough information remotely at the point > > of error. > > > > Signed-off-by: Chris Wilson > > --- > > drivers/gpu/drm/drm_info.c | 17 - > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c > > index 15ec9f4..d813430 100644 > > --- a/drivers/gpu/drm/drm_info.c > > +++ b/drivers/gpu/drm/drm_info.c > > @@ -184,14 +184,21 @@ int drm_clients_info(struct seq_file *m, void *data) > > struct drm_file *priv; > > > > mutex_lock(&dev->struct_mutex); > > - seq_printf(m, "a devpiduid magic\n\n"); > > - list_for_each_entry(priv, &dev->filelist, lhead) { > > - seq_printf(m, "%c %3d %5d %5d %10u\n", > > - priv->authenticated ? 'y' : 'n', > > - priv->minor->index, > > + seq_printf(m, " pid dev master a uid > > magic\n"); > > Maybe mention "task" here? Or is this supposed to be a logical part of "pid"? Yeah, I was thinking that comm/pid were essentially the same column. Top uses command which would be reasonable to reuse. > > + list_for_each_entry_reverse(priv, &dev->filelist, lhead) { > > No idea why you do backwards traversal, but ok.. Clients are added youngest-first. So traversing backwards gives kernel X/display manager clients > > + struct task_struct *task; > > + > > + rcu_read_lock(); > > What's that rcu-lock for? task->comm is pre-allocated, priv->pid is > static, kuid... no idea? Cargo-culting the locking from the discussion with Tetsuo: commit 3ec2f427e6f82b9b8f9b18dd2c758b864385df39 Author: Tetsuo Handa Date: Fri Jan 3 20:42:18 2014 +0900 drm/i915: Fix refcount leak and possible NULL pointerdereference. Since get_pid_task() grabs a reference on the task_struct, we have to drop the refcount after reading that task's comm name. Use pid_task() with RCU instead. Also, avoid directly reading like pid_task()->comm because pid_task() will return NULL if the task have already exit()ed. This patch fixes both problems. > Anyway, patch looks good otherwise. Especially task->comm sounds > really handy in that list. It was. Thanks for the feedback, will add the extra header and comment. -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: Include task->name and master status in debugfs clients info
Hi On Mon, Sep 1, 2014 at 4:19 PM, Chris Wilson wrote: > On Mon, Sep 01, 2014 at 04:11:43PM +0200, David Herrmann wrote: >> Hi >> >> On Sat, Aug 9, 2014 at 8:22 AM, Chris Wilson >> wrote: >> > Showing who is the current master is useful for trying to decypher >> > errors when trying to acquire master (e.g. a race with X taking over >> > from plymouth). By including the process name as well as the pid >> > simplifies the task of grabbing enough information remotely at the point >> > of error. >> > >> > Signed-off-by: Chris Wilson >> > --- >> > drivers/gpu/drm/drm_info.c | 17 - >> > 1 file changed, 12 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c >> > index 15ec9f4..d813430 100644 >> > --- a/drivers/gpu/drm/drm_info.c >> > +++ b/drivers/gpu/drm/drm_info.c >> > @@ -184,14 +184,21 @@ int drm_clients_info(struct seq_file *m, void *data) >> > struct drm_file *priv; >> > >> > mutex_lock(&dev->struct_mutex); >> > - seq_printf(m, "a devpiduid magic\n\n"); >> > - list_for_each_entry(priv, &dev->filelist, lhead) { >> > - seq_printf(m, "%c %3d %5d %5d %10u\n", >> > - priv->authenticated ? 'y' : 'n', >> > - priv->minor->index, >> > + seq_printf(m, " pid dev master a uid >> > magic\n"); >> >> Maybe mention "task" here? Or is this supposed to be a logical part of "pid"? > > Yeah, I was thinking that comm/pid were essentially the same column. Top > uses command which would be reasonable to reuse. Sounds all fine. Just wanted to go sure it's not a typo. >> > + list_for_each_entry_reverse(priv, &dev->filelist, lhead) { >> >> No idea why you do backwards traversal, but ok.. > > Clients are added youngest-first. So traversing backwards gives > kernel > X/display manager > clients > >> > + struct task_struct *task; >> > + >> > + rcu_read_lock(); >> >> What's that rcu-lock for? task->comm is pre-allocated, priv->pid is >> static, kuid... no idea? > > Cargo-culting the locking from the discussion with Tetsuo: > > commit 3ec2f427e6f82b9b8f9b18dd2c758b864385df39 > Author: Tetsuo Handa > Date: Fri Jan 3 20:42:18 2014 +0900 > > drm/i915: Fix refcount leak and possible NULL pointerdereference. > > Since get_pid_task() grabs a reference on the task_struct, we have to > drop the > refcount after reading that task's comm name. Use pid_task() with RCU > instead. > > Also, avoid directly reading like pid_task()->comm because > pid_task() will return NULL if the task have already exit()ed. > > This patch fixes both problems. Ah, right, this is for pid lookup not task->comm. Should have seen that.. Feel free to add: Reviewed-by: David Herrmann Thanks David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Rework workaround data exporting to debugfs
On 01/09/2014 15:06, Damien Lespiau wrote: On Mon, Sep 01, 2014 at 02:28:53PM +0100, Arun Siluvery wrote: Now w/a are organized in an array so we know exactly how many of them are applied; use the same array while exporting data to debugfs and remove the temporary array we currently have in driver priv structure. Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_debugfs.c | 41 +++-- drivers/gpu/drm/i915/i915_drv.h | 14 --- drivers/gpu/drm/i915/intel_ringbuffer.c | 15 drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++ 4 files changed, 52 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 2727bda..bab0408 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2465,6 +2465,14 @@ static int i915_wa_registers(struct seq_file *m, void *unused) struct drm_info_node *node = (struct drm_info_node *) m->private; struct drm_device *dev = node->minor->dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_ring_context_rodata ro_data; + + ret = ring_context_rodata(dev, &ro_data); + if (ret) { + seq_printf(m, "Workarounds applied: 0\n"); seq_puts() + DRM_DEBUG_DRIVER("Workaround table not available !!\n"); + return -EINVAL; + } ret = mutex_lock_interruptible(&dev->struct_mutex); if (ret) @@ -2472,18 +2480,27 @@ static int i915_wa_registers(struct seq_file *m, void *unused) intel_runtime_pm_get(dev_priv); - seq_printf(m, "Workarounds applied: %d\n", dev_priv->num_wa_regs); - for (i = 0; i < dev_priv->num_wa_regs; ++i) { - u32 addr, mask; - - addr = dev_priv->intel_wa_regs[i].addr; - mask = dev_priv->intel_wa_regs[i].mask; - dev_priv->intel_wa_regs[i].value = I915_READ(addr) | mask; - if (dev_priv->intel_wa_regs[i].addr) - seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n", - dev_priv->intel_wa_regs[i].addr, - dev_priv->intel_wa_regs[i].value, - dev_priv->intel_wa_regs[i].mask); + seq_printf(m, "Workarounds applied: %d\n", ro_data.num_items/2); + for (i = 0; i < ro_data.num_items; i += 2) { + u32 addr, mask, value; + + addr = ro_data.init_context[i]; + /* +* Most of workarounds are masked registers; +* to set a bit in lower 16-bits we set a mask bit in +* upper 16-bits so we can take either of them as mask but +* it doesn't work if the w/a is about clearing a bit so +* use upper 16-bits to cover both cases. +*/ + mask = ro_data.init_context[i+1] >> 16; "Most" doesn't seem good here. Either it's "all" and we're happy, or we need a generic way to describe the W/A (masked register or not). value + mask is generic enough to code for both cases. It seems some of them could be unmasked registers. We can use 'mask' itself to determine whether it is a masked/unmasked register. mask == 0 if it is an unmasked register. + + /* +* value represents the status of other bits in the +* register besides w/a bits +*/ + value = I915_READ(addr) | mask; + seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n", + addr, value, mask); } I still don't get it. 'value' is supposed to be the reference value for the W/A, but you're or'ing the mask here, so you treat the mask as if it were the reference value. This won't work if the W/A is about setting multi-bits fields or about clearing a bit. The comment is still not clear enough. You're saying "other bits besides the w/a bits", but or'ing the mask doesn't do that. Why do we care about the "other bits" in the reference value? they don't matter. Why use something else than (ro_data.init_context[i+1] & 0x) for the value here (as long we're talking about masked registers)? I have always considered value as the register value (remaining bits of the register and w/a bits) and now I see your point. Yes lower 16-bits can be used as reference value, depending on whether it is a masked/unmasked we can use/not use the mask in conjunction with value in the test. regards Arun ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Rework workaround data exporting to debugfs
On Mon, Sep 01, 2014 at 03:45:29PM +0100, Siluvery, Arun wrote: > >>+ /* > >>+* Most of workarounds are masked registers; > >>+* to set a bit in lower 16-bits we set a mask bit in > >>+* upper 16-bits so we can take either of them as mask but > >>+* it doesn't work if the w/a is about clearing a bit so > >>+* use upper 16-bits to cover both cases. > >>+*/ > >>+ mask = ro_data.init_context[i+1] >> 16; > > > >"Most" doesn't seem good here. Either it's "all" and we're happy, or we > >need a generic way to describe the W/A (masked register or not). value + > >mask is generic enough to code for both cases. > > > It seems some of them could be unmasked registers. > We can use 'mask' itself to determine whether it is a > masked/unmasked register. mask == 0 if it is an unmasked register. I don't think we can use mask == 0 when it's an unmasked register. In this case, we still need to be able to have a mask that tells us which are the interesting bits in the value. If we want something generic that can be applied to masked and unmasked register, we'll something like: struct reg_wa { unsigned int masked_register : 1; u32 addr; u32 mask; u32 value; }; So: 1. masked register case: - masked_register = 1 - addr - mask (selects the lower 16 bits that are coding for the W/A value) - value (it only makes sense to some of the lower 16 bits set here) 2. 'normal' register case - masked_register = 0 - addr - mask - value >From that generic description you can cover all cases, eg. 1. the write for masked registers becomes: mask << 16 | value 2. the write for normal registers becomes: (read(addr) & ~mask) | value 2. check a W/A has been applied (both normal and masked register): read(addr) & mask == value We seem to have only masked registers atm, so it's fine to handle just that case, but if you envision that we'll need more than masked registers, we need a better W/A definition. -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Don't call intel_plane_restore() when the prop value didn't change
From: Ville Syrjälä No point in calling intel_plane_restore() in .set_property() if the value didn't change. More importantly this papers over a bug where the current primary plane code forgets to update the user coordinates we store under intel_plane unless the primary plane .update_plane() hook is actually called. This means we have 0 in the coordinates straight after boot and any call to intel_restore_plane() (such as from restore_fbdev_mode()) will actually turn off the primary plane. This mess needs to be fixed properly but that's a bigger task and the first step there is killing off intel_pipe_set_base() and just calling the primary plane .update_plane() hook. For the immediate problem of black screen after boot this small patch is enough to hide it. The problem originates from these two commits: commit 3a5f87c286515c54ff5c52c3e64d0c522b7570c0 Author: Thomas Wood Date: Wed Aug 20 14:45:00 2014 +0100 drm: fix plane rotation when restoring fbdev configuration commit d91a2cb8e5104233c02bbde539bd4ee455ec12ac Author: Sonika Jindal Date: Fri Aug 22 14:06:04 2014 +0530 drm/i915: Add 180 degree primary plane rotation support Cc: Thomas Wood Cc: Sonika Jindal Tested-by: Mika Kuoppala Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_sprite.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index fd5f271..cf596cd 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1232,6 +1232,9 @@ int intel_plane_set_property(struct drm_plane *plane, if (hweight32(val & 0xf) != 1) return -EINVAL; + if (intel_plane->rotation == val) + return 0; + old_val = intel_plane->rotation; intel_plane->rotation = val; ret = intel_plane_restore(plane); -- 1.8.5.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't call intel_plane_restore() when the prop value didn't change
On Mon, Sep 01, 2014 at 06:08:25PM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > No point in calling intel_plane_restore() in .set_property() if the > value didn't change. > > More importantly this papers over a bug where the current primary plane > code forgets to update the user coordinates we store under intel_plane > unless the primary plane .update_plane() hook is actually called. This > means we have 0 in the coordinates straight after boot and any call > to intel_restore_plane() (such as from restore_fbdev_mode()) will > actually turn off the primary plane. This mess needs to be fixed properly > but that's a bigger task and the first step there is killing off > intel_pipe_set_base() and just calling the primary plane > .update_plane() hook. For the immediate problem of black screen after > boot this small patch is enough to hide it. > > The problem originates from these two commits: > commit 3a5f87c286515c54ff5c52c3e64d0c522b7570c0 > Author: Thomas Wood > Date: Wed Aug 20 14:45:00 2014 +0100 > > drm: fix plane rotation when restoring fbdev configuration > > commit d91a2cb8e5104233c02bbde539bd4ee455ec12ac > Author: Sonika Jindal > Date: Fri Aug 22 14:06:04 2014 +0530 > > drm/i915: Add 180 degree primary plane rotation support > > Cc: Thomas Wood > Cc: Sonika Jindal > Tested-by: Mika Kuoppala > Signed-off-by: Ville Syrjälä Reviewed-by: Damien Lespiau -- Damien > --- > drivers/gpu/drm/i915/intel_sprite.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index fd5f271..cf596cd 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1232,6 +1232,9 @@ int intel_plane_set_property(struct drm_plane *plane, > if (hweight32(val & 0xf) != 1) > return -EINVAL; > > + if (intel_plane->rotation == val) > + return 0; > + > old_val = intel_plane->rotation; > intel_plane->rotation = val; > ret = intel_plane_restore(plane); > -- > 1.8.5.5 > > ___ > 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] Regression in drm-intel caused by d91a2cb8e510
Daniel: I'm encountering a problem with the drm-intel-nightly branch in your drm-intel repository. The symptom is that when I boot my laptop, at some point during the start-up procedure the screen goes totally blank, as though the backlight were turned off, and it remains that way. I can't tell if the backlight really is off -- it may be that there's just nothing being displayed -- and playing with the screen-brightness function keys doesn't help. Git bisect located the source of the problem as commit d91a2cb8e510 (drm/i915: Add 180 degree primary plane rotation support). The problem occurs when I boot a kernel built from that commit, and it doesn't occur when I boot a kernel built from the preceding commit (19ac737dc3c3). If necessary, I can get debugging info by logging in over a network connection. Tell me what you need. Alan Stern ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Regression in drm-intel caused by d91a2cb8e510
On Mon, Sep 01, 2014 at 04:19:23PM -0400, Alan Stern wrote: > Daniel: > > I'm encountering a problem with the drm-intel-nightly branch in your > drm-intel repository. The symptom is that when I boot my laptop, at > some point during the start-up procedure the screen goes totally blank, > as though the backlight were turned off, and it remains that way. I > can't tell if the backlight really is off -- it may be that there's > just nothing being displayed -- and playing with the screen-brightness > function keys doesn't help. > > Git bisect located the source of the problem as commit d91a2cb8e510 > (drm/i915: Add 180 degree primary plane rotation support). The problem > occurs when I boot a kernel built from that commit, and it doesn't > occur when I boot a kernel built from the preceding commit > (19ac737dc3c3). > > If necessary, I can get debugging info by logging in over a network > connection. Tell me what you need. It seems like Ville and Mika already caught a similar bug today, could you give Ville's patch a go (available on the intel-gfx): drm/i915: Don't call intel_plane_restore() when the prop value didn't change Thanks, -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Regression in drm-intel caused by d91a2cb8e510
On Mon, 1 Sep 2014, Damien Lespiau wrote: > On Mon, Sep 01, 2014 at 04:19:23PM -0400, Alan Stern wrote: > > Daniel: > > > > I'm encountering a problem with the drm-intel-nightly branch in your > > drm-intel repository. The symptom is that when I boot my laptop, at > > some point during the start-up procedure the screen goes totally blank, > > as though the backlight were turned off, and it remains that way. I > > can't tell if the backlight really is off -- it may be that there's > > just nothing being displayed -- and playing with the screen-brightness > > function keys doesn't help. > > > > Git bisect located the source of the problem as commit d91a2cb8e510 > > (drm/i915: Add 180 degree primary plane rotation support). The problem > > occurs when I boot a kernel built from that commit, and it doesn't > > occur when I boot a kernel built from the preceding commit > > (19ac737dc3c3). > > > > If necessary, I can get debugging info by logging in over a network > > connection. Tell me what you need. > > It seems like Ville and Mika already caught a similar bug today, could > you give Ville's patch a go (available on the intel-gfx): > > drm/i915: Don't call intel_plane_restore() when the prop value didn't change Yes, that patch fixed the problem. Thank you. Tested-by: Alan Stern Alan Stern ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx