Re: [Intel-gfx] [PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed
Reviewed-by: Rodrigo Vivi On Tue, Aug 06, 2013 at 06:57:16PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > If the error interrupts are already disabled, don't disable and > reenable them. This is going to be needed when we're in PC8+, where > all the interrupts are disabled so we won't risk re-enabling > DE_ERR_INT_IVB. > > v2: Use dev_priv->irq_mask (Chris) > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_irq.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index d96bd1b..5e7e6f6 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1373,6 +1373,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void > *arg) > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > u32 de_iir, gt_iir, de_ier, sde_ier = 0; > irqreturn_t ret = IRQ_NONE; > + bool err_int_reenable = false; > > atomic_inc(&dev_priv->irq_received); > > @@ -1401,7 +1402,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void > *arg) >* handler. */ > if (IS_HASWELL(dev)) { > spin_lock(&dev_priv->irq_lock); > - ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB); > + err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB; > + if (err_int_reenable) > + ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB); > spin_unlock(&dev_priv->irq_lock); > } > > @@ -1437,7 +1440,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void > *arg) > } > } > > - if (IS_HASWELL(dev)) { > + if (err_int_reenable) { > spin_lock(&dev_priv->irq_lock); > if (ivb_can_enable_err_int(dev)) > ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB); > -- > 1.8.1.2 > > ___ > 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 5/9] drm/i915: add dev_priv->pm_irq_mask
On Tue, Aug 06, 2013 at 06:57:15PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > Just like irq_mask and gt_irq_mask, use it to track the status of > GEN6_PMIMR so we don't need to read it again every time we call > snb_update_pm_irq. > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_irq.c | 12 +++- > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 9ff09a2..b621f5e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1097,6 +1097,7 @@ typedef struct drm_i915_private { > /** Cached value of IMR to avoid reads in updating the bitfield */ > u32 irq_mask; > u32 gt_irq_mask; > + u32 pm_irq_mask; > > struct work_struct hotplug_work; > bool enable_hotplug_processing; > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index a1255da..d96bd1b 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -142,16 +142,17 @@ static void snb_update_pm_irq(struct drm_i915_private > *dev_priv, > uint32_t interrupt_mask, > uint32_t enabled_irq_mask) > { > - uint32_t pmimr, new_val; > + uint32_t new_val; > > assert_spin_locked(&dev_priv->irq_lock); > > - pmimr = new_val = I915_READ(GEN6_PMIMR); > + new_val = dev_priv->pm_irq_mask; > new_val &= ~interrupt_mask; > new_val |= (~enabled_irq_mask & interrupt_mask); > > - if (new_val != pmimr) { > - I915_WRITE(GEN6_PMIMR, new_val); > + if (new_val != dev_priv->pm_irq_mask) { > + dev_priv->pm_irq_mask = new_val; > + I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask); > POSTING_READ(GEN6_PMIMR); > } > } > @@ -2221,8 +,9 @@ static void gen5_gt_irq_postinstall(struct drm_device > *dev) > if (HAS_VEBOX(dev)) > pm_irqs |= PM_VEBOX_USER_INTERRUPT; > > + dev_priv->pm_irq_mask = 0x; > I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > - I915_WRITE(GEN6_PMIMR, 0x); Same write happening at gen5_gt_irq_preinstall... it is already strange a gen5_ func using a GEN6 reg, but maybe we have to use this same pm_irq_mask there also... > + I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask); > I915_WRITE(GEN6_PMIER, pm_irqs); > POSTING_READ(GEN6_PMIER); > } > -- > 1.8.1.2 Anyways, feel free to use: Reviewed-by: Rodrigo Vivi > > ___ > 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/9] drm/i915: don't update GEN6_PMIMR when it's not needed
Reviewed-by: Rodrigo Vivi I liked this very much... we should do this kind of check in more places... On Tue, Aug 06, 2013 at 06:57:14PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > I did some brief tests and the "new_val = pmimr" condition usually > happens a few times after exiting games. > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_irq.c | 14 +- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index a00fe05..a1255da 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -142,14 +142,18 @@ static void snb_update_pm_irq(struct drm_i915_private > *dev_priv, > uint32_t interrupt_mask, > uint32_t enabled_irq_mask) > { > - uint32_t pmimr = I915_READ(GEN6_PMIMR); > - pmimr &= ~interrupt_mask; > - pmimr |= (~enabled_irq_mask & interrupt_mask); > + uint32_t pmimr, new_val; > > assert_spin_locked(&dev_priv->irq_lock); > > - I915_WRITE(GEN6_PMIMR, pmimr); > - POSTING_READ(GEN6_PMIMR); > + pmimr = new_val = I915_READ(GEN6_PMIMR); > + new_val &= ~interrupt_mask; > + new_val |= (~enabled_irq_mask & interrupt_mask); > + > + if (new_val != pmimr) { > + I915_WRITE(GEN6_PMIMR, new_val); > + POSTING_READ(GEN6_PMIMR); > + } > } > > void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask) > -- > 1.8.1.2 > > ___ > 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 3/9] drm/i915: wrap GEN6_PMIMR changes
Reviewed-by: Rodrigo Vivi On Tue, Aug 06, 2013 at 06:57:13PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > Just like we're doing with the other IMR changes. > > One of the functional changes is that not every caller was doing the > POSTING_READ. > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_irq.c | 47 > - > drivers/gpu/drm/i915/intel_drv.h| 3 +++ > drivers/gpu/drm/i915/intel_pm.c | 2 +- > drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++ > 4 files changed, 46 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index a6e98ea..a00fe05 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -132,6 +132,41 @@ void ilk_disable_gt_irq(struct drm_i915_private > *dev_priv, uint32_t mask) > ilk_update_gt_irq(dev_priv, mask, 0); > } > > +/** > + * snb_update_pm_irq - update GEN6_PMIMR > + * @dev_priv: driver private > + * @interrupt_mask: mask of interrupt bits to update > + * @enabled_irq_mask: mask of interrupt bits to enable > + */ > +static void snb_update_pm_irq(struct drm_i915_private *dev_priv, > + uint32_t interrupt_mask, > + uint32_t enabled_irq_mask) > +{ > + uint32_t pmimr = I915_READ(GEN6_PMIMR); > + pmimr &= ~interrupt_mask; > + pmimr |= (~enabled_irq_mask & interrupt_mask); > + > + assert_spin_locked(&dev_priv->irq_lock); > + > + I915_WRITE(GEN6_PMIMR, pmimr); > + POSTING_READ(GEN6_PMIMR); > +} > + > +void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask) > +{ > + snb_update_pm_irq(dev_priv, mask, mask); > +} > + > +void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask) > +{ > + snb_update_pm_irq(dev_priv, mask, 0); > +} > + > +static void snb_set_pm_irq(struct drm_i915_private *dev_priv, uint32_t val) > +{ > + snb_update_pm_irq(dev_priv, 0x, ~val); this confused more than the first one, but it works! > +} > + > static bool ivb_can_enable_err_int(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -739,15 +774,14 @@ static void gen6_pm_rps_work(struct work_struct *work) > { > drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, > rps.work); > - u32 pm_iir, pm_imr; > + u32 pm_iir; > u8 new_delay; > > spin_lock_irq(&dev_priv->irq_lock); > pm_iir = dev_priv->rps.pm_iir; > dev_priv->rps.pm_iir = 0; > - pm_imr = I915_READ(GEN6_PMIMR); > /* Make sure not to corrupt PMIMR state used by ringbuffer code */ > - I915_WRITE(GEN6_PMIMR, pm_imr & ~GEN6_PM_RPS_EVENTS); > + snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS); > spin_unlock_irq(&dev_priv->irq_lock); > > if ((pm_iir & GEN6_PM_RPS_EVENTS) == 0) > @@ -921,8 +955,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private > *dev_priv, > > spin_lock(&dev_priv->irq_lock); > dev_priv->rps.pm_iir |= pm_iir; > - I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir); > - POSTING_READ(GEN6_PMIMR); > + snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir); > spin_unlock(&dev_priv->irq_lock); > > queue_work(dev_priv->wq, &dev_priv->rps.work); > @@ -1005,8 +1038,8 @@ static void hsw_pm_irq_handler(struct drm_i915_private > *dev_priv, > if (pm_iir & GEN6_PM_RPS_EVENTS) { > spin_lock(&dev_priv->irq_lock); > dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS; > - I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir); > - /* never want to mask useful interrupts. (also posting read) */ > + snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir); > + /* never want to mask useful interrupts. */ > WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS); > spin_unlock(&dev_priv->irq_lock); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 82bc78e..db7cbd5 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -841,5 +841,8 @@ extern void hsw_restore_lcpll(struct drm_i915_private > *dev_priv); > extern void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t > mask); > extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, > uint32_t mask); > +extern void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t > mask); > +extern void snb_disable_pm_irq(struct drm_i915_private *dev_priv, > +uint32_t mask); > > #endif /* __INTEL_DRV_H__ */ > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 9b8c90ea..984250d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3324,7 +3324,7 @@ stat
Re: [Intel-gfx] [PATCH 2/9] drm/i915: wrap GTIMR changes
Reviewed-by: Rodrigo Vivi .On Tue, Aug 06, 2013 at 06:57:12PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > Just like the functions that touch DEIMR and SDEIMR, but for GTIMR. > The new functions contain a POSTING_READ(GTIMR) which was not present > at the 2 callers inside i915_irq.c. > > The implementation is based on ibx_display_interrupt_update. > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_irq.c | 34 > + > drivers/gpu/drm/i915/intel_drv.h| 3 +++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 22 ++--- > 3 files changed, 39 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 6a1c207..a6e98ea 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -104,6 +104,34 @@ ironlake_disable_display_irq(drm_i915_private_t > *dev_priv, u32 mask) > } > } > > +/** > + * ilk_update_gt_irq - update GTIMR > + * @dev_priv: driver private > + * @interrupt_mask: mask of interrupt bits to update > + * @enabled_irq_mask: mask of interrupt bits to enable > + */ > +static void ilk_update_gt_irq(struct drm_i915_private *dev_priv, > + uint32_t interrupt_mask, > + uint32_t enabled_irq_mask) > +{ > + assert_spin_locked(&dev_priv->irq_lock); > + > + dev_priv->gt_irq_mask &= ~interrupt_mask; > + dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask); my little mind got confused with logic above, but after some minutes I convinced myself this works ;) > + I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > + POSTING_READ(GTIMR); > +} > + > +void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask) > +{ > + ilk_update_gt_irq(dev_priv, mask, mask); > +} > + > +void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask) > +{ > + ilk_update_gt_irq(dev_priv, mask, 0); > +} > + > static bool ivb_can_enable_err_int(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -806,8 +834,7 @@ static void ivybridge_parity_work(struct work_struct > *work) > I915_WRITE(GEN7_MISCCPCTL, misccpctl); > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > - dev_priv->gt_irq_mask &= ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > + ilk_enable_gt_irq(dev_priv, GT_RENDER_L3_PARITY_ERROR_INTERRUPT); > spin_unlock_irqrestore(&dev_priv->irq_lock, flags); > > mutex_unlock(&dev_priv->dev->struct_mutex); > @@ -837,8 +864,7 @@ static void ivybridge_parity_error_irq_handler(struct > drm_device *dev) > return; > > spin_lock(&dev_priv->irq_lock); > - dev_priv->gt_irq_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > + ilk_disable_gt_irq(dev_priv, GT_RENDER_L3_PARITY_ERROR_INTERRUPT); > spin_unlock(&dev_priv->irq_lock); > > queue_work(dev_priv->wq, &dev_priv->l3_parity.error_work); > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 54e389d..82bc78e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -838,5 +838,8 @@ extern void intel_edp_psr_update(struct drm_device *dev); > extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv, > bool switch_to_fclk, bool allow_power_down); > extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv); > +extern void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t > mask); > +extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, > +uint32_t mask); > > #endif /* __INTEL_DRV_H__ */ > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 74d02a7..6eeca1e 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -836,11 +836,8 @@ gen5_ring_get_irq(struct intel_ring_buffer *ring) > return false; > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > - if (ring->irq_refcount++ == 0) { > - dev_priv->gt_irq_mask &= ~ring->irq_enable_mask; > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > - POSTING_READ(GTIMR); > - } > + if (ring->irq_refcount++ == 0) > + ilk_enable_gt_irq(dev_priv, ring->irq_enable_mask); > spin_unlock_irqrestore(&dev_priv->irq_lock, flags); > > return true; > @@ -854,11 +851,8 @@ gen5_ring_put_irq(struct intel_ring_buffer *ring) > unsigned long flags; > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > - if (--ring->irq_refcount == 0) { > - dev_priv->gt_irq_mask |= ring->irq_enable_mask; > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > - POSTING_READ(GTIMR); > - } >
Re: [Intel-gfx] Improved IGT support for text fixtures
Awesome! Daniel, I have checked the latest igt with piglit, it have 303 tests(including subtests). Best Regards~~ Open Source Technology Center (OTC) Terence Yang(杨光) Tel: 86-021-61167360 iNet: 8821-7360 > -Original Message- > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] > Sent: Thursday, August 15, 2013 12:33 AM > To: Jin, Gordon; Sun, Yi; Yang, Guang A > Cc: intel-gfx > Subject: Improved IGT support for text fixtures > > Hi all, > > So while wreaking havoc with igt tests and adding some nice infrastructure to > handle subtest status reporting a bit better I've also added support to > annotate > the common test fixture code. And rolled it out over all tests. > > I plan to do a decent write up on all the new infrastructure and how to use it > when writing testcases, but the immediate benefit is that enumerating > testcases > now doesn't need to be done on a system with an intel gpu any more. > Furthermore (and that's the reason I've done this) we can also finally > enumerate > the prime_nv subtests without the need for an nvidia gpu. > > Yang guang: Can you please check that everything works correctly on the QA > setup? Right now we should have 303 igt tests when enumerating them with > piglit (i.e. subtests included). > > Cheers, 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 4/4] [v4] drm/i915: Convert execbuf code to use vmas
On Wed, Aug 14, 2013 at 11:43:58PM +0100, Chris Wilson wrote: > These are my numbers for a beefy haswell box (note the really > interesting numbers will be on Baytrail): > > unpatched: > > relocation: buffers= 1: old= 21945 + 34.4*reloc, lut= 21814 + 34.0*reloc > (ns) > relocation: buffers= 2: old= 15947 + 36.4*reloc, lut= 16169 + 35.4*reloc > (ns) > relocation: buffers= 4: old= 12711 + 37.6*reloc, lut= 13039 + 36.7*reloc > (ns) > relocation: buffers= 8: old= 6154 + 40.9*reloc, lut= 7201 + 38.9*reloc > (ns) > relocation: buffers= 16: old= 4846 + 41.6*reloc, lut= 5337 + 40.6*reloc > (ns) > relocation: buffers= 32: old= 7097 + 41.9*reloc, lut= 6943 + 41.0*reloc > (ns) > relocation: buffers= 64: old= 13318 + 41.9*reloc, lut= 12748 + 41.2*reloc > (ns) > relocation: buffers= 128: old= 27282 + 43.0*reloc, lut= 25778 + 41.7*reloc > (ns) > relocation: buffers= 256: old= 54535 + 45.2*reloc, lut= 51912 + 43.7*reloc > (ns) > relocation: buffers= 512: old= 137447 + 53.2*reloc, lut= 129333 + 45.5*reloc > (ns) > relocation: buffers=1024: old= 307347 + 66.5*reloc, lut= 291487 + 48.1*reloc > (ns) > relocation: buffers=2048: old= 606300 + 92.1*reloc, lut= 574774 + 51.6*reloc > (ns) > skip-relocs: buffers= 1: old= 1583 + 15.6*reloc, lut= 1516 + 14.5*reloc > (ns) > skip-relocs: buffers= 2: old= 1621 + 15.6*reloc, lut= 1603 + 14.5*reloc > (ns) > skip-relocs: buffers= 4: old= 1791 + 15.6*reloc, lut= 1777 + 14.5*reloc > (ns) > skip-relocs: buffers= 8: old= 2009 + 15.6*reloc, lut= 2024 + 14.6*reloc > (ns) > skip-relocs: buffers= 16: old= 2637 + 15.7*reloc, lut= 2564 + 14.6*reloc > (ns) > skip-relocs: buffers= 32: old= 3835 + 15.8*reloc, lut= 3785 + 14.7*reloc > (ns) > skip-relocs: buffers= 64: old= 6996 + 15.8*reloc, lut= 6681 + 14.7*reloc > (ns) > skip-relocs: buffers= 128: old= 14333 + 16.4*reloc, lut= 13560 + 15.2*reloc > (ns) > skip-relocs: buffers= 256: old= 28092 + 17.7*reloc, lut= 26759 + 16.2*reloc > (ns) > skip-relocs: buffers= 512: old= 70885 + 25.2*reloc, lut= 66713 + 17.9*reloc > (ns) > skip-relocs: buffers=1024: old= 158520 + 35.2*reloc, lut= 150828 + 20.1*reloc > (ns) > skip-relocs: buffers=2048: old= 314208 + 54.3*reloc, lut= 298343 + 22.1*reloc > (ns) > no-relocs: buffers= 1: old= 1533 + 5.2*reloc, lut= 1498 + 4.9*reloc (ns) > no-relocs: buffers= 2: old= 1518 + 5.2*reloc, lut= 1505 + 4.9*reloc (ns) > no-relocs: buffers= 4: old= 1647 + 5.2*reloc, lut= 1593 + 4.9*reloc (ns) > no-relocs: buffers= 8: old= 1882 + 5.3*reloc, lut= 1874 + 5.0*reloc (ns) > no-relocs: buffers= 16: old= 2399 + 5.3*reloc, lut= 2341 + 5.0*reloc (ns) > no-relocs: buffers= 32: old= 3638 + 5.3*reloc, lut= 3554 + 5.0*reloc (ns) > no-relocs: buffers= 64: old= 6622 + 5.3*reloc, lut= 6308 + 5.1*reloc (ns) > no-relocs: buffers= 128: old= 13584 + 5.3*reloc, lut= 12872 + 5.1*reloc (ns) > no-relocs: buffers= 256: old= 26519 + 5.8*reloc, lut= 25234 + 5.5*reloc (ns) > no-relocs: buffers= 512: old= 67128 + 5.4*reloc, lut= 63054 + 5.2*reloc (ns) > no-relocs: buffers=1024: old= 146705 + 5.2*reloc, lut= 139020 + 5.1*reloc (ns) > no-relocs: buffers=2048: old= 290319 + 5.4*reloc, lut= 274705 + 5.4*reloc (ns) > > vma(execbuffer): > > relocation: buffers= 1: old= 21922 + 34.6*reloc, lut= 21510 + 34.0*reloc > (ns) > relocation: buffers= 2: old= 16851 + 37.4*reloc, lut= 17123 + 35.4*reloc > (ns) > relocation: buffers= 4: old= 13234 + 37.8*reloc, lut= 13436 + 36.9*reloc > (ns) > relocation: buffers= 8: old= 6549 + 40.8*reloc, lut= 6512 + 39.8*reloc > (ns) > relocation: buffers= 16: old= 5012 + 41.8*reloc, lut= 4883 + 41.0*reloc > (ns) > relocation: buffers= 32: old= 8591 + 42.2*reloc, lut= 8377 + 41.1*reloc > (ns) > relocation: buffers= 64: old= 16051 + 42.8*reloc, lut= 15658 + 41.7*reloc > (ns) > relocation: buffers= 128: old= 33397 + 44.5*reloc, lut= 32705 + 43.3*reloc > (ns) > relocation: buffers= 256: old= 68012 + 46.8*reloc, lut= 66904 + 45.5*reloc > (ns) > relocation: buffers= 512: old= 160162 + 56.4*reloc, lut= 155586 + 49.1*reloc > (ns) > relocation: buffers=1024: old= 348728 + 71.8*reloc, lut= 338113 + 55.1*reloc > (ns) > relocation: buffers=2048: old= 699331 + 98.7*reloc, lut= 675969 + 62.2*reloc > (ns) > skip-relocs: buffers= 1: old= 1642 + 16.5*reloc, lut= 1588 + 15.6*reloc > (ns) > skip-relocs: buffers= 2: old= 1676 + 16.4*reloc, lut= 1663 + 15.6*reloc > (ns) > skip-relocs: buffers= 4: old= 1926 + 16.4*reloc, lut= 1891 + 15.6*reloc > (ns) > skip-relocs: buffers= 8: old= 2218 + 16.6*reloc, lut= 2212 + 15.7*reloc > (ns) > skip-relocs: buffers= 16: old= 2933 + 16.6*reloc, lut= 2880 + 15.7*reloc > (ns) > skip-relocs: buffers= 32: old= 4594 + 16.6*reloc, lut= 4523 + 15.8*reloc > (ns) > skip-relocs: buffers= 64: old= 8414 + 16.8*reloc, lut= 8210 + 15.9*reloc > (ns) > skip-relocs: buffers= 128: old= 17429 + 17.9*reloc
[Intel-gfx] [PATCH 19/20] drm/prime: make drm_prime_lookup_buf_handle static
... and move it to the top of the function to avoid a forward declaration. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_prime.c | 29 +++-- include/drm/drmP.h | 1 - 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 5e543e9..ed1ea5c 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -83,6 +83,21 @@ static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, return 0; } +static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv, + struct dma_buf *dma_buf, + uint32_t *handle) +{ + struct drm_prime_member *member; + + list_for_each_entry(member, &prime_fpriv->head, entry) { + if (member->dma_buf == dma_buf) { + *handle = member->handle; + return 0; + } + } + return -ENOENT; +} + static int drm_gem_map_attach(struct dma_buf *dma_buf, struct device *target_dev, struct dma_buf_attachment *attach) @@ -655,20 +670,6 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv) } EXPORT_SYMBOL(drm_prime_destroy_file_private); -int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle) -{ - struct drm_prime_member *member; - - list_for_each_entry(member, &prime_fpriv->head, entry) { - if (member->dma_buf == dma_buf) { - *handle = member->handle; - return 0; - } - } - return -ENOENT; -} -EXPORT_SYMBOL(drm_prime_lookup_buf_handle); - void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf) { mutex_lock(&prime_fpriv->lock); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0c8a627..8047f76 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1543,7 +1543,6 @@ int drm_gem_dumb_destroy(struct drm_file *file, void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv); void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv); -int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle); void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf); int drm_prime_add_dma_buf(struct drm_device *dev, struct drm_gem_object *obj); -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 20/20] drm/prime: Always add exported buffers to the handle cache
... not only when the dma-buf is freshly created. In contrived examples someone else could have exported/imported the dma-buf already and handed us the gem object with a flink name. If such on object gets reexported as a dma_buf we won't have it in the handle cache already, which breaks the guarantee that for dma-buf imports we always hand back an existing handle if there is one. This is exercised by igt/prime_self_import/with_one_bo_two_files Now if we extend the locked sections just a notch more we can also plug th racy buf/handle cache setup in handle_to_fd: If evil userspace races a concurrent gem close against a prime export operation we can end up tearing down the gem handle before the dma buf handle cache is set up. When handle_to_fd gets around to adding the handle to the cache there will be no one left to clean it up, effectily leaking the bo (and the dma-buf, since the handle cache holds a ref on the dma-buf): Thread AThread B handle_to_fd: lookup gem object from handle creates new dma_buf gem_close on the same handle obj->dma_buf is set, but file priv buf handle cache has no entry obj->handle_count drops to 0 drm_prime_add_buf_handle sets up the handle cache -> We have a dma-buf reference in the handle cache, but since the handle_count of the gem object already dropped to 0 no on will clean it up. When closing the drm device fd we'll hit the WARN_ON in drm_prime_destroy_file_private. The important change is to extend the critical section of the filp->prime.lock to cover the gem handle lookup. This serializes with a concurrent gem handle close. This leak is exercised by igt/prime_self_import/export-vs-gem_close-race Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 6 ++-- drivers/gpu/drm/drm_prime.c | 81 +++-- include/drm/drmP.h | 2 +- 3 files changed, 53 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 9d72028..a3654fe 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -195,10 +195,12 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) * Note: obj->dma_buf can't disappear as long as we still hold a * handle reference in obj->handle_count. */ + mutex_lock(&filp->prime.lock); if (obj->dma_buf) { - drm_prime_remove_buf_handle(&filp->prime, - obj->dma_buf); + drm_prime_remove_buf_handle_locked(&filp->prime, + obj->dma_buf); } + mutex_unlock(&filp->prime.lock); } static void drm_gem_object_ref_bug(struct kref *list_kref) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index ed1ea5c..7ae2bfc 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -83,6 +83,19 @@ static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, return 0; } +static struct dma_buf *drm_prime_lookup_buf_by_handle(struct drm_prime_file_private *prime_fpriv, + uint32_t handle) +{ + struct drm_prime_member *member; + + list_for_each_entry(member, &prime_fpriv->head, entry) { + if (member->handle == handle) + return member->dma_buf; + } + + return NULL; +} + static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle) @@ -146,9 +159,8 @@ static void drm_gem_map_detach(struct dma_buf *dma_buf, attach->priv = NULL; } -static void drm_prime_remove_buf_handle_locked( - struct drm_prime_file_private *prime_fpriv, - struct dma_buf *dma_buf) +void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv, + struct dma_buf *dma_buf) { struct drm_prime_member *member, *safe; @@ -337,6 +349,8 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, */ obj->dma_buf = dmabuf; get_dma_buf(obj->dma_buf); + /* Grab a new ref since the callers is now used by the dma-buf */ + drm_gem_object_reference(obj); return dmabuf; } @@ -349,10 +363,20 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, int ret = 0; struct dma_buf *dmabuf; + mutex_lock(&file_priv->prime.lock); obj = drm_gem_object_lookup(dev, file_priv, handle); - if (!obj) - return -ENOENT; + if (!obj) { + ret = -ENOENT; + goto out_unlock; + } + + dmabuf = drm_prime_lookup_buf_by_hand
[Intel-gfx] [PATCH 18/20] drm/prime: Simplify drm_gem_remove_prime_handles
with the reworking semantics and locking of the obj->dma_buf pointer this pointer is always set as long as there's still a gem handle around and a dma_buf associated with this gem object. Also, the per file-priv lookup-cache for dma-buf importing is also unified between foreign and native objects. Hence we don't need to special case the clean any more and can simply drop the clause which only runs for foreing objects, i.e. with obj->import_attach set. Note that with this change (actually with the previous one to always set up obj->dma_buf even for foreign objects) it is no longer required to set obj->import_attach when importing a foreing object. So update comments accordingly, too. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 5 - include/drm/drmP.h| 5 + 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 6b2b54e..9d72028 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -191,11 +191,6 @@ EXPORT_SYMBOL(drm_gem_object_alloc); static void drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) { - if (obj->import_attach) { - drm_prime_remove_buf_handle(&filp->prime, - obj->import_attach->dmabuf); - } - /* * Note: obj->dma_buf can't disappear as long as we still hold a * handle reference in obj->handle_count. diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 1db42c0..0c8a627 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -703,6 +703,11 @@ struct drm_gem_object { * * The driver's ->gem_free_object callback is responsible for cleaning * up the dma_buf attachment and references acquired at import time. +* +* Note that the drm gem/prime core does not depend upon drivers setting +* this field any more. So for drivers where this doesn't make sense +* (e.g. virtual devices or a displaylink behind an usb bus) they can +* simply leave it as NULL. */ struct dma_buf_attachment *import_attach; }; -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 17/20] drm/prime: proper locking+refcounting for obj->dma_buf link
The export dma-buf cache is semantically similar to an flink name. So semantically it makes sense to treat it the same and remove the name (i.e. the dma_buf pointer) and its references when the last gem handle disappears. Again we need to be careful, but double so: Not just could someone race and export with a gem close ioctl (so we need to recheck obj->handle_count again when assigning the new name), but multiple exports can also race against each another. This is prevented by holding the dev->object_name_lock across the entire section which touches obj->dma_buf. With the new scheme we also need to reinstate the obj->dma_buf link at import time (in case the only reference userspace has held in-between was through the dma-buf fd and not through any native gem handle). For simplicity we don't check whether it's a native object but unconditionally set up that link - with the new scheme of removing the obj->dma_buf reference when the last handle disappears we can do that. To make it clear that this is not just for exported buffers anymore als rename it from export_dma_buf to dma_buf. To make sure that now one can race a fd_to_handle or handle_to_fd with gem_close we use the same tricks as in flink of extending the dev->object_name_locking critical section. With this change we finally have a guaranteed 1:1 relationship (at least for native objects) between gem objects and dma-bufs, even accounting for races (which can happen since the dma-buf itself holds a reference while in-flight). This prevent igt/prime_self_import/export-vs-gem_close-race from Oopsing the kernel. There is still a leak though since the per-file priv dma-buf/handle cache handling is racy. That will be fixed in a later patch. v2: Remove the bogus dma_buf_put from the export_and_register_object failure path if we've raced with the handle count dropping to 0. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_fops.c | 1 + drivers/gpu/drm/drm_gem.c | 24 ++-- drivers/gpu/drm/drm_prime.c | 70 +++-- include/drm/drmP.h | 12 ++-- 4 files changed, 87 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 72acae9..faf93ff3 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -545,6 +545,7 @@ int drm_release(struct inode *inode, struct file *filp) if (dev->driver->postclose) dev->driver->postclose(dev, file_priv); + if (drm_core_check_feature(dev, DRIVER_PRIME)) drm_prime_destroy_file_private(&file_priv->prime); diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 0e9a9c7..6b2b54e 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -195,9 +195,14 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) drm_prime_remove_buf_handle(&filp->prime, obj->import_attach->dmabuf); } - if (obj->export_dma_buf) { + + /* +* Note: obj->dma_buf can't disappear as long as we still hold a +* handle reference in obj->handle_count. +*/ + if (obj->dma_buf) { drm_prime_remove_buf_handle(&filp->prime, - obj->export_dma_buf); + obj->dma_buf); } } @@ -231,6 +236,15 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) } } +static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj) +{ + /* Unbreak the reference cycle if we have an exported dma_buf. */ + if (obj->dma_buf) { + dma_buf_put(obj->dma_buf); + obj->dma_buf = NULL; + } +} + static void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { @@ -244,8 +258,10 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) */ mutex_lock(&obj->dev->object_name_lock); - if (--obj->handle_count == 0) + if (--obj->handle_count == 0) { drm_gem_object_handle_free(obj); + drm_gem_object_exported_dma_buf_free(obj); + } mutex_unlock(&obj->dev->object_name_lock); drm_gem_object_unreference_unlocked(obj); @@ -589,6 +605,8 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private) void drm_gem_object_release(struct drm_gem_object *obj) { + WARN_ON(obj->dma_buf); + if (obj->filp) fput(obj->filp); } diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 3d57601..5e543e9 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -193,11 +193,8 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf) { struct drm_gem_object *obj = dma_buf->priv; - if (obj->export_dma_buf == dma_buf) { - /* drop the reference on the export fd holds */ - obj->export
[Intel-gfx] [PATCH 16/20] drm/gem: completely close gem_open vs. gem_close races
The gem flink name holds a reference onto the object itself, and this self-reference would prevent an flink'ed object from every being freed. To break that loop we remove the flink name when the last userspace handle disappears, i.e. when obj->handle_count reaches 0. Now in gem_open we drop the dev->object_name_lock between the flink name lookup and actually adding the handle. This means a concurrent gem_close of the last handle could result in the flink name getting reaped right inbetween, i.e. Thread 1Thread 2 gem_opengem_close flink -> obj lookup handle_count drops to 0 remove flink name create_handle handle_count++ If someone now flinks this object again, we'll get a new flink name. We can close this race by removing the lock dropping and making the entire lookup+handle_create sequence atomic. Unfortunately to still be able to share the handle_create logic this requires a handle_create_tail function which drops the lock - we can't hold the object_name_lock while calling into a driver's ->gem_open callback. Note that for flink fixing this race isn't really important, since racing gem_open against gem_close is clearly a userspace bug. And no matter how the race ends, we won't leak any references. But with dma-buf where the userspace dma-buf fd itself is refcounted this is a valid sequence and hence we should fix it. Therefore this patch here is just a warm-up exercise (and for consistency between flink buffer sharing and dma-buf buffer sharing with self-imports). Also note that this extension of the critical section in gem_open protected by dev->object_name_lock only works because it's now a mutex: A spinlock would conflict with the potential memory allocation in idr_preload(). This is exercises by igt/gem_flink_race/flink_name. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 42 +++--- include/drm/drmP.h| 3 +++ 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index c00e273..0e9a9c7 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -308,23 +308,26 @@ int drm_gem_dumb_destroy(struct drm_file *file, EXPORT_SYMBOL(drm_gem_dumb_destroy); /** - * Create a handle for this object. This adds a handle reference - * to the object, which includes a regular reference count. Callers - * will likely want to dereference the object afterwards. + * drm_gem_handle_create_tail - internal functions to create a handle + * + * This expects the dev->object_name_lock to be held already and will drop it + * before returning. Used to avoid races in establishing new handles when + * importing an object from either an flink name or a dma-buf. */ int -drm_gem_handle_create(struct drm_file *file_priv, - struct drm_gem_object *obj, - u32 *handlep) +drm_gem_handle_create_tail(struct drm_file *file_priv, + struct drm_gem_object *obj, + u32 *handlep) { struct drm_device *dev = obj->dev; int ret; + WARN_ON(!mutex_is_locked(&dev->object_name_lock)); + /* * Get the user-visible handle using idr. Preload and perform * allocation under our spinlock. */ - mutex_lock(&dev->object_name_lock); idr_preload(GFP_KERNEL); spin_lock(&file_priv->table_lock); @@ -351,6 +354,21 @@ drm_gem_handle_create(struct drm_file *file_priv, return 0; } + +/** + * Create a handle for this object. This adds a handle reference + * to the object, which includes a regular reference count. Callers + * will likely want to dereference the object afterwards. + */ +int +drm_gem_handle_create(struct drm_file *file_priv, + struct drm_gem_object *obj, + u32 *handlep) +{ + mutex_lock(&obj->dev->object_name_lock); + + return drm_gem_handle_create_tail(file_priv, obj, handlep); +} EXPORT_SYMBOL(drm_gem_handle_create); @@ -504,13 +522,15 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data, mutex_lock(&dev->object_name_lock); obj = idr_find(&dev->object_name_idr, (int) args->name); - if (obj) + if (obj) { drm_gem_object_reference(obj); - mutex_unlock(&dev->object_name_lock); - if (!obj) + } else { + mutex_unlock(&dev->object_name_lock); return -ENOENT; + } - ret = drm_gem_handle_create(file_priv, obj, &handle); + /* drm_gem_handle_create_tail unlocks dev->object_name_lock. */ + ret = drm_gem_handle_create_tail(file_priv, obj, &handle); drm_gem_object_unreference_unlocked(obj); if (ret) return ret; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 49e514b..3b346bc 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1614,6
[Intel-gfx] [PATCH 15/20] drm/gem: switch dev->object_name_lock to a mutex
I want to wrap the creation of a dma-buf from a gem object in it, so that the obj->export_dma_buf cache can be atomically filled in. Instead of creating a new mutex just for that variable I've figured I can reuse the existing dev->object_name_lock, especially since the new semantics will exactly mirror the flink obj->name already protected by that lock. v2: idr_preload/idr_preload_end is now an atomic section, so need to move the mutex locking outside. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 18 +- include/drm/drmP.h| 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index b554154..c00e273 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -93,7 +93,7 @@ drm_gem_init(struct drm_device *dev) { struct drm_gem_mm *mm; - spin_lock_init(&dev->object_name_lock); + mutex_init(&dev->object_name_lock); idr_init(&dev->object_name_idr); mm = kzalloc(sizeof(struct drm_gem_mm), GFP_KERNEL); @@ -243,10 +243,10 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) * checked for a name */ - spin_lock(&obj->dev->object_name_lock); + mutex_lock(&obj->dev->object_name_lock); if (--obj->handle_count == 0) drm_gem_object_handle_free(obj); - spin_unlock(&obj->dev->object_name_lock); + mutex_unlock(&obj->dev->object_name_lock); drm_gem_object_unreference_unlocked(obj); } @@ -324,16 +324,16 @@ drm_gem_handle_create(struct drm_file *file_priv, * Get the user-visible handle using idr. Preload and perform * allocation under our spinlock. */ + mutex_lock(&dev->object_name_lock); idr_preload(GFP_KERNEL); - spin_lock(&dev->object_name_lock); spin_lock(&file_priv->table_lock); ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT); drm_gem_object_reference(obj); obj->handle_count++; spin_unlock(&file_priv->table_lock); - spin_unlock(&dev->object_name_lock); idr_preload_end(); + mutex_unlock(&dev->object_name_lock); if (ret < 0) { drm_gem_object_handle_unreference_unlocked(obj); return ret; @@ -455,8 +455,8 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, if (obj == NULL) return -ENOENT; + mutex_lock(&dev->object_name_lock); idr_preload(GFP_KERNEL); - spin_lock(&dev->object_name_lock); /* prevent races with concurrent gem_close. */ if (obj->handle_count == 0) { ret = -ENOENT; @@ -478,8 +478,8 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, ret = 0; err: - spin_unlock(&dev->object_name_lock); idr_preload_end(); + mutex_unlock(&dev->object_name_lock); drm_gem_object_unreference_unlocked(obj); return ret; } @@ -502,11 +502,11 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data, if (!(dev->driver->driver_features & DRIVER_GEM)) return -ENODEV; - spin_lock(&dev->object_name_lock); + mutex_lock(&dev->object_name_lock); obj = idr_find(&dev->object_name_idr, (int) args->name); if (obj) drm_gem_object_reference(obj); - spin_unlock(&dev->object_name_lock); + mutex_unlock(&dev->object_name_lock); if (!obj) return -ENOENT; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f08bbcf..49e514b 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1218,7 +1218,7 @@ struct drm_device { /** \name GEM information */ /*@{ */ - spinlock_t object_name_lock; + struct mutex object_name_lock; struct idr object_name_idr; /*@} */ int switch_power_state; -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 14/20] drm/prime: clarify logic a bit in drm_gem_prime_fd_to_handle
if (!ret) implies that ret == 0, so no need to clear it again. And explicitly check for ret == 0 to indicate that we're checking an errno integer. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_prime.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index cb04516..3d57601 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -444,10 +444,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, ret = drm_prime_lookup_buf_handle(&file_priv->prime, dma_buf, handle); - if (!ret) { - ret = 0; + if (ret == 0) goto out_put; - } /* never seen this one, need to import */ obj = dev->driver->gem_prime_import(dev, dma_buf); -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 13/20] drm/prime: shrink critical section protected by prime lock
When exporting a gem object as a dma-buf the critical section for the per-fd prime lock is just the adding (and in case of errors, removing) of the handle to the per-fd lookup cache. So restrict the critical section to just that part of the function. This simplifies later reordering. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_prime.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index c2d6d54..cb04516 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -310,7 +310,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, if (!obj) return -ENOENT; - mutex_lock(&file_priv->prime.lock); /* re-export the original imported object */ if (obj->import_attach) { dmabuf = obj->import_attach->dmabuf; @@ -332,6 +331,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, } obj->export_dma_buf = dmabuf; + mutex_lock(&file_priv->prime.lock); /* if we've exported this buffer the cheat and add it to the import list * so we get the correct handle back */ @@ -363,13 +363,13 @@ out_have_obj: fail_rm_handle: drm_prime_remove_buf_handle_locked(&file_priv->prime, dmabuf); + mutex_unlock(&file_priv->prime.lock); fail_put_dmabuf: /* clear NOT to be checked when releasing dma_buf */ obj->export_dma_buf = NULL; dma_buf_put(dmabuf); out: drm_gem_object_unreference_unlocked(obj); - mutex_unlock(&file_priv->prime.lock); return ret; } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 12/20] drm/prime: use proper pointer in drm_gem_prime_handle_to_fd
Part of the function uses the properly-typed dmabuf variable, the other an untyped void *buf. Kill the later. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_prime.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 82cd83e..c2d6d54 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -303,7 +303,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, int *prime_fd) { struct drm_gem_object *obj; - void *buf; int ret = 0; struct dma_buf *dmabuf; @@ -323,15 +322,15 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, goto out_have_obj; } - buf = dev->driver->gem_prime_export(dev, obj, flags); - if (IS_ERR(buf)) { + dmabuf = dev->driver->gem_prime_export(dev, obj, flags); + if (IS_ERR(dmabuf)) { /* normally the created dma-buf takes ownership of the ref, * but if that fails then drop the ref */ - ret = PTR_ERR(buf); + ret = PTR_ERR(dmabuf); goto out; } - obj->export_dma_buf = buf; + obj->export_dma_buf = dmabuf; /* if we've exported this buffer the cheat and add it to the import list * so we get the correct handle back @@ -341,7 +340,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, if (ret) goto fail_put_dmabuf; - ret = dma_buf_fd(buf, flags); + ret = dma_buf_fd(dmabuf, flags); if (ret < 0) goto fail_rm_handle; @@ -362,11 +361,12 @@ out_have_obj: goto out; fail_rm_handle: - drm_prime_remove_buf_handle_locked(&file_priv->prime, buf); + drm_prime_remove_buf_handle_locked(&file_priv->prime, + dmabuf); fail_put_dmabuf: /* clear NOT to be checked when releasing dma_buf */ obj->export_dma_buf = NULL; - dma_buf_put(buf); + dma_buf_put(dmabuf); out: drm_gem_object_unreference_unlocked(obj); mutex_unlock(&file_priv->prime.lock); -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/20] drm/gem: create drm_gem_dumb_destroy
All the gem based kms drivers really want the same function to destroy a dumb framebuffer backing storage object. So give it to them and roll it out in all drivers. This still leaves the option open for kms drivers which don't use GEM for backing storage, but it does decently simplify matters for gem drivers. Cc: Inki Dae Cc: Laurent Pinchart Cc: Intel Graphics Development Cc: Ben Skeggs Cc: Rob Clark Cc: Alex Deucher Acked-by: Laurent Pinchart Reviewed-by: Rob Clark Acked-by: Inki Dae Signed-off-by: Daniel Vetter --- include/drm/drmP.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0eb4a37..f08bbcf 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1619,6 +1619,9 @@ int drm_gem_handle_create(struct drm_file *file_priv, u32 *handlep); int drm_gem_handle_delete(struct drm_file *filp, u32 handle); +int drm_gem_dumb_destroy(struct drm_file *file, +struct drm_device *dev, +uint32_t handle); void drm_gem_free_mmap_offset(struct drm_gem_object *obj); int drm_gem_create_mmap_offset(struct drm_gem_object *obj); -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/20] drm/gem: make drm_gem_object_handle_unreference_unlocked static
No one outside of drm should use this, the official interfaces are drm_gem_handle_create and drm_gem_handle_delete. The handle refcounting is purely an implementation detail of gem. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 2 +- include/drm/drmP.h| 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index c972a8f..b554154 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -231,7 +231,7 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) } } -void +static void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { if (WARN_ON(obj->handle_count == 0)) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 254f395..0eb4a37 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1619,7 +1619,6 @@ int drm_gem_handle_create(struct drm_file *file_priv, u32 *handlep); int drm_gem_handle_delete(struct drm_file *filp, u32 handle); -void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj); void drm_gem_free_mmap_offset(struct drm_gem_object *obj); int drm_gem_create_mmap_offset(struct drm_gem_object *obj); -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/20] drm/prime: fix error path in drm_gem_prime_fd_to_handle
handle_unreference only clears up the obj->name and the reference, but would leave a dangling handle in the idr. The right thing to do is to call handle_delete. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index f115962..82cd83e 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -476,7 +476,7 @@ fail: /* hmm, if driver attached, we are relying on the free-object path * to detach.. which seems ok.. */ - drm_gem_object_handle_unreference_unlocked(obj); + drm_gem_handle_delete(file_priv, *handle); out_put: dma_buf_put(dma_buf); mutex_unlock(&file_priv->prime.lock); -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/20] drm/gem: fix up flink name create race
This is the 2nd attempt, I've always been a bit dissatisified with the tricky nature of the first one: http://lists.freedesktop.org/archives/dri-devel/2012-July/025451.html The issue is that the flink ioctl can race with calling gem_close on the last gem handle. In that case we'll end up with a zero handle count, but an flink name (and it's corresponding reference). Which results in a neat space leak. In my first attempt I've solved this by rechecking the handle count. But fundamentally the issue is that ->handle_count isn't your usual refcount - it can be resurrected from 0 among other things. For those special beasts atomic_t often suggest way more ordering that it actually guarantees. To prevent being tricked by those hairy semantics take the easy way out and simply protect the handle with the existing dev->object_name_lock. With that change implemented it's dead easy to fix the flink vs. gem close reace: When we try to create the name we simply have to check whether there's still officially a gem handle around and if not refuse to create the flink name. Since the handle count decrement and flink name destruction is now also protected by that lock the reace is gone and we can't ever leak the flink reference again. Outside of the drm core only the exynos driver looks at the handle count, and tbh I have no idea why (it's just for debug dmesg output luckily). I've considered inlining the drm_gem_object_handle_free, but I plan to add more name-like things (like the exported dma_buf) to this scheme, so it's clearer to leave the handle freeing in its own function. This is exercised by the new gem_flink_race i-g-t testcase, which on my snb leaks gem objects at a rate of roughly 1k objects/s. v2: Fix up the error path handling in handle_create and make it more robust by simply calling object_handle_unreference. v3: Fix up the handle_unreference logic bug - atomic_dec_and_test retursn 1 for 0. Oops. v4: Squash in inlining of drm_gem_object_handle_reference as suggested by Dave Airlie and add a note that we now have a testcase. Cc: Dave Airlie Cc: Inki Dae Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 31 --- drivers/gpu/drm/drm_info.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +- include/drm/drmP.h | 19 ++- 4 files changed, 32 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 9118f5f..c972a8f 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -154,7 +154,7 @@ void drm_gem_private_object_init(struct drm_device *dev, obj->filp = NULL; kref_init(&obj->refcount); - atomic_set(&obj->handle_count, 0); + obj->handle_count = 0; obj->size = size; } EXPORT_SYMBOL(drm_gem_private_object_init); @@ -218,11 +218,9 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) struct drm_device *dev = obj->dev; /* Remove any name for this object */ - spin_lock(&dev->object_name_lock); if (obj->name) { idr_remove(&dev->object_name_idr, obj->name); obj->name = 0; - spin_unlock(&dev->object_name_lock); /* * The object name held a reference to this object, drop * that now. @@ -230,15 +228,13 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) * This cannot be the last reference, since the handle holds one too. */ kref_put(&obj->refcount, drm_gem_object_ref_bug); - } else - spin_unlock(&dev->object_name_lock); - + } } void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { - if (WARN_ON(atomic_read(&obj->handle_count) == 0)) + if (WARN_ON(obj->handle_count == 0)) return; /* @@ -247,8 +243,11 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) * checked for a name */ - if (atomic_dec_and_test(&obj->handle_count)) + spin_lock(&obj->dev->object_name_lock); + if (--obj->handle_count == 0) drm_gem_object_handle_free(obj); + spin_unlock(&obj->dev->object_name_lock); + drm_gem_object_unreference_unlocked(obj); } @@ -326,17 +325,21 @@ drm_gem_handle_create(struct drm_file *file_priv, * allocation under our spinlock. */ idr_preload(GFP_KERNEL); + spin_lock(&dev->object_name_lock); spin_lock(&file_priv->table_lock); ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT); - + drm_gem_object_reference(obj); + obj->handle_count++; spin_unlock(&file_priv->table_lock); + spin_unlock(&dev->object_name_lock); idr_preload_end(); - if (ret < 0) + if (ret < 0) { + drm_gem_object_handle_unreference_unlocke
[Intel-gfx] [PATCH 07/20] drm/gem: WARN about unbalanced handle refcounts
Trying to drop a reference we don't have is a pretty serious bug. Trying to paper over it is an even worse offense. So scream into dmesg with a big WARN in case that ever happens. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index b1d9e25..9118f5f 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -238,7 +238,7 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { - if (atomic_read(&obj->handle_count) == 0) + if (WARN_ON(atomic_read(&obj->handle_count) == 0)) return; /* -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/20] drm/gem: remove bogus NULL check from drm_gem_object_handle_unreference_unlocked
Calling this function with a NULL object is simply a bug, so papering over a NULL object not a good idea. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 77c7d19..b1d9e25 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -238,9 +238,6 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { - if (obj == NULL) - return; - if (atomic_read(&obj->handle_count) == 0) return; -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/20] drm/gem: move drm_gem_object_handle_unreference_unlocked into drm_gem.c
We have three callers of this function now and it's neither performance critical nor really small. So an inline function feels like overkill and unecessarily separates the different parts of the code. Since all callers of drm_gem_object_handle_free are now in drm_gem.c we can make that static (and remove the unused EXPORT_SYMBOL). To avoid a forward declaration move it (and drm_gem_object_free_bug) up a bit. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 89 --- include/drm/drmP.h| 21 +-- 2 files changed, 55 insertions(+), 55 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 9ab038c..77c7d19 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -201,6 +201,60 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) } } +static void drm_gem_object_ref_bug(struct kref *list_kref) +{ + BUG(); +} + +/** + * Called after the last handle to the object has been closed + * + * Removes any name for the object. Note that this must be + * called before drm_gem_object_free or we'll be touching + * freed memory + */ +static void drm_gem_object_handle_free(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + /* Remove any name for this object */ + spin_lock(&dev->object_name_lock); + if (obj->name) { + idr_remove(&dev->object_name_idr, obj->name); + obj->name = 0; + spin_unlock(&dev->object_name_lock); + /* +* The object name held a reference to this object, drop +* that now. + * + * This cannot be the last reference, since the handle holds one too. +*/ + kref_put(&obj->refcount, drm_gem_object_ref_bug); + } else + spin_unlock(&dev->object_name_lock); + +} + +void +drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) +{ + if (obj == NULL) + return; + + if (atomic_read(&obj->handle_count) == 0) + return; + + /* + * Must bump handle count first as this may be the last + * ref, in which case the object would disappear before we + * checked for a name + */ + + if (atomic_dec_and_test(&obj->handle_count)) + drm_gem_object_handle_free(obj); + drm_gem_object_unreference_unlocked(obj); +} + /** * Removes the mapping from handle to filp for this object. */ @@ -533,41 +587,6 @@ drm_gem_object_free(struct kref *kref) } EXPORT_SYMBOL(drm_gem_object_free); -static void drm_gem_object_ref_bug(struct kref *list_kref) -{ - BUG(); -} - -/** - * Called after the last handle to the object has been closed - * - * Removes any name for the object. Note that this must be - * called before drm_gem_object_free or we'll be touching - * freed memory - */ -void drm_gem_object_handle_free(struct drm_gem_object *obj) -{ - struct drm_device *dev = obj->dev; - - /* Remove any name for this object */ - spin_lock(&dev->object_name_lock); - if (obj->name) { - idr_remove(&dev->object_name_idr, obj->name); - obj->name = 0; - spin_unlock(&dev->object_name_lock); - /* -* The object name held a reference to this object, drop -* that now. - * - * This cannot be the last reference, since the handle holds one too. -*/ - kref_put(&obj->refcount, drm_gem_object_ref_bug); - } else - spin_unlock(&dev->object_name_lock); - -} -EXPORT_SYMBOL(drm_gem_object_handle_free); - void drm_gem_vm_open(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3711e97..91c8d05 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1572,7 +1572,6 @@ int drm_gem_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size); void drm_gem_private_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size); -void drm_gem_object_handle_free(struct drm_gem_object *obj); void drm_gem_vm_open(struct vm_area_struct *vma); void drm_gem_vm_close(struct vm_area_struct *vma); int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, @@ -1619,25 +1618,7 @@ drm_gem_object_handle_reference(struct drm_gem_object *obj) atomic_inc(&obj->handle_count); } -static inline void -drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) -{ - if (obj == NULL) - return; - - if (atomic_read(&obj->handle_count) == 0) - return; - - /* - * Must bump handle count first as this may be the last - * ref, in which case the object would
[Intel-gfx] [PATCH 03/20] drm/prime: remove cargo-cult locking from map_sg helper
I've checked both implementations (radeon/nouveau) and they both grab the page array from ttm simply by dereferencing it and then wrapping it up with drm_prime_pages_to_sg in the callback and map it with dma_map_sg (in the helper). Only the grabbing of the underlying page array is anything we need to be concerned about, and either those pages are pinned independently, or we're screwed no matter what. And indeed, nouveau/radeon pin the backing storage in their attach/detach functions. Since I've created this patch cma prime support for dma_buf was added. drm_gem_cma_prime_get_sg_table only calls kzalloc and the creates&maps the sg table with dma_get_sgtable. It doesn't touch any gem object state otherwise. So the cma helpers also look safe. The only thing we might claim it does is prevent concurrent mapping of dma_buf attachments. But a) that's not allowed and b) the current code is racy already since it checks whether the sg mapping exists _before_ grabbing the lock. So the dev->struct_mutex locking here does absolutely nothing useful, but only distracts. Remove it. This should also help Maarten's work to eventually pin the backing storage more dynamically by preventing locking inversions around dev->struct_mutex. v2: Add analysis for recently added cma helper prime code. Cc: Laurent Pinchart Cc: Maarten Lankhorst Acked-by: Laurent Pinchart Acked-by: Maarten Lankhorst Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_prime.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index a35f206..f115962 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -167,8 +167,6 @@ static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, if (WARN_ON(prime_attach->dir != DMA_NONE)) return ERR_PTR(-EBUSY); - mutex_lock(&obj->dev->struct_mutex); - sgt = obj->dev->driver->gem_prime_get_sg_table(obj); if (!IS_ERR(sgt)) { @@ -182,7 +180,6 @@ static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, } } - mutex_unlock(&obj->dev->struct_mutex); return sgt; } -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/20] drm/prime: add a bit of documentation about gem_obj->import_attach
Lifetime rules seem to be solid around ->import_attach. So this patch just properly documents them. Note that pointing directly at the attachment might have issues for devices that have multiple struct device *dev parts constituting the logical gpu and so might need multiple attachment points. Similarly for drm devices which don't need a dma attachment at all (like udl). But fixing that up is material for different patches. Reviewed-by: Rob Clark Signed-off-by: Daniel Vetter --- include/drm/drmP.h | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 016e75e..3711e97 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -678,7 +678,16 @@ struct drm_gem_object { /* dma buf exported from this GEM object */ struct dma_buf *export_dma_buf; - /* dma buf attachment backing this object */ + /** +* import_attach - dma buf attachment backing this object +* +* Any foreign dma_buf imported as a gem object has this set to the +* attachment point for the device. This is invariant over the lifetime +* of a gem object. +* +* The driver's ->gem_free_object callback is responsible for cleaning +* up the dma_buf attachment and references acquired at import time. +*/ struct dma_buf_attachment *import_attach; }; -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/20] drm/exynos: explicit store base gem object in dma_buf->priv
From: Inki Dae Signed-off-by: Inki Dae Signed-off-by: Kyungmin Park Signed-off-by: Daniel Vetter --- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index 3cd56e1..fd76449 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -22,6 +22,11 @@ struct exynos_drm_dmabuf_attachment { bool is_mapped; }; +static struct exynos_drm_gem_obj *dma_buf_to_obj(struct dma_buf *buf) +{ + return to_exynos_gem_obj(buf->priv); +} + static int exynos_gem_attach_dma_buf(struct dma_buf *dmabuf, struct device *dev, struct dma_buf_attachment *attach) @@ -63,7 +68,7 @@ static struct sg_table * enum dma_data_direction dir) { struct exynos_drm_dmabuf_attachment *exynos_attach = attach->priv; - struct exynos_drm_gem_obj *gem_obj = attach->dmabuf->priv; + struct exynos_drm_gem_obj *gem_obj = dma_buf_to_obj(attach->dmabuf); struct drm_device *dev = gem_obj->base.dev; struct exynos_drm_gem_buf *buf; struct scatterlist *rd, *wr; @@ -180,7 +185,7 @@ struct dma_buf *exynos_dmabuf_prime_export(struct drm_device *drm_dev, { struct exynos_drm_gem_obj *exynos_gem_obj = to_exynos_gem_obj(obj); - return dma_buf_export(exynos_gem_obj, &exynos_dmabuf_ops, + return dma_buf_export(obj, &exynos_dmabuf_ops, exynos_gem_obj->base.size, flags); } @@ -198,8 +203,7 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev, if (dma_buf->ops == &exynos_dmabuf_ops) { struct drm_gem_object *obj; - exynos_gem_obj = dma_buf->priv; - obj = &exynos_gem_obj->base; + obj = dma_buf->priv; /* is it from our device? */ if (obj->dev == drm_dev) { -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/20] drm: use common drm_gem_dmabuf_release in i915/exynos drivers
Note that this is slightly tricky since both drivers store their native objects in dma_buf->priv. But both also embed the base drm_gem_object at the first position, so the implicit cast is ok. To use the release helper we need to export it, too. Cc: Inki Dae Cc: Intel Graphics Development Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_prime.c| 3 ++- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 23 +-- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 13 + include/drm/drmP.h | 1 + 4 files changed, 5 insertions(+), 35 deletions(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 85e450e..a35f206 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -192,7 +192,7 @@ static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach, /* nothing to be done here */ } -static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) +void drm_gem_dmabuf_release(struct dma_buf *dma_buf) { struct drm_gem_object *obj = dma_buf->priv; @@ -202,6 +202,7 @@ static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) drm_gem_object_unreference_unlocked(obj); } } +EXPORT_SYMBOL(drm_gem_dmabuf_release); static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index a0f997e..3cd56e1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -127,27 +127,6 @@ static void exynos_gem_unmap_dma_buf(struct dma_buf_attachment *attach, /* Nothing to do. */ } -static void exynos_dmabuf_release(struct dma_buf *dmabuf) -{ - struct exynos_drm_gem_obj *exynos_gem_obj = dmabuf->priv; - - /* -* exynos_dmabuf_release() call means that file object's -* f_count is 0 and it calls drm_gem_object_handle_unreference() -* to drop the references that these values had been increased -* at drm_prime_handle_to_fd() -*/ - if (exynos_gem_obj->base.export_dma_buf == dmabuf) { - exynos_gem_obj->base.export_dma_buf = NULL; - - /* -* drop this gem object refcount to release allocated buffer -* and resources. -*/ - drm_gem_object_unreference_unlocked(&exynos_gem_obj->base); - } -} - static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf, unsigned long page_num) { @@ -193,7 +172,7 @@ static struct dma_buf_ops exynos_dmabuf_ops = { .kunmap = exynos_gem_dmabuf_kunmap, .kunmap_atomic = exynos_gem_dmabuf_kunmap_atomic, .mmap = exynos_gem_dmabuf_mmap, - .release= exynos_dmabuf_release, + .release= drm_gem_dmabuf_release, }; struct dma_buf *exynos_dmabuf_prime_export(struct drm_device *drm_dev, diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index f7af7e4..e918b05 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -103,17 +103,6 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, mutex_unlock(&obj->base.dev->struct_mutex); } -static void i915_gem_dmabuf_release(struct dma_buf *dma_buf) -{ - struct drm_i915_gem_object *obj = dma_buf->priv; - - if (obj->base.export_dma_buf == dma_buf) { - /* drop the reference on the export fd holds */ - obj->base.export_dma_buf = NULL; - drm_gem_object_unreference_unlocked(&obj->base); - } -} - static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) { struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); @@ -224,7 +213,7 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size static const struct dma_buf_ops i915_dmabuf_ops = { .map_dma_buf = i915_gem_map_dma_buf, .unmap_dma_buf = i915_gem_unmap_dma_buf, - .release = i915_gem_dmabuf_release, + .release = drm_gem_dmabuf_release, .kmap = i915_gem_dmabuf_kmap, .kmap_atomic = i915_gem_dmabuf_kmap_atomic, .kunmap = i915_gem_dmabuf_kunmap, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3ecdde6..016e75e 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1495,6 +1495,7 @@ extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf); extern int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle); +extern void drm_gem_dmabuf_release(struct dma_buf *dma_buf); extern int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, struct dr
[Intel-gfx] [PATCH 00/20] prime patches
Hi all, So I've finally tracked down the last thing which I didn't really understand in my earlier patches and made sure it won't ever break again by writing testcases. The one thing still left to do (but I have tests for it already) is to make sure we don't duplicate buffers when importing foreign buffers on two open fds. This is the use-case for which the exynos guys recently posted a few hacky patches. I've already merged the i915 patches from this series. Since there's no real functional depency all the patches here can go through drm-next without issues. Comments&flames highly welcome. Cheers, Daniel Daniel Vetter (19): drm: use common drm_gem_dmabuf_release in i915/exynos drivers drm/prime: remove cargo-cult locking from map_sg helper drm/prime: add a bit of documentation about gem_obj->import_attach drm/gem: move drm_gem_object_handle_unreference_unlocked into drm_gem.c drm/gem: remove bogus NULL check from drm_gem_object_handle_unreference_unlocked drm/gem: WARN about unbalanced handle refcounts drm/gem: fix up flink name create race drm/prime: fix error path in drm_gem_prime_fd_to_handle drm/gem: make drm_gem_object_handle_unreference_unlocked static drm/gem: create drm_gem_dumb_destroy drm/prime: use proper pointer in drm_gem_prime_handle_to_fd drm/prime: shrink critical section protected by prime lock drm/prime: clarify logic a bit in drm_gem_prime_fd_to_handle drm/gem: switch dev->object_name_lock to a mutex drm/gem: completely close gem_open vs. gem_close races drm/prime: proper locking+refcounting for obj->dma_buf link drm/prime: Simplify drm_gem_remove_prime_handles drm/prime: make drm_prime_lookup_buf_handle static drm/prime: Always add exported buffers to the handle cache Inki Dae (1): drm/exynos: explicit store base gem object in dma_buf->priv drivers/gpu/drm/drm_fops.c | 1 + drivers/gpu/drm/drm_gem.c | 178 ++- drivers/gpu/drm/drm_info.c | 2 +- drivers/gpu/drm/drm_prime.c| 190 ++--- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 35 ++ drivers/gpu/drm/exynos/exynos_drm_gem.c| 2 +- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 13 +- include/drm/drmP.h | 79 ++-- 8 files changed, 297 insertions(+), 203 deletions(-) -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] intel-gpu-tools: --disable-tests option should make cairo-check obsolete
On Wed, Aug 14, 2013 at 11:25 PM, Chris Wilson wrote: > On Wed, Aug 14, 2013 at 11:14:05PM +0200, Sedat Dilek wrote: >> On Wed, Aug 14, 2013 at 11:03 PM, Chris Wilson >> wrote: >> > On Wed, Aug 14, 2013 at 10:33:42PM +0200, Sedat Dilek wrote: >> >> Hi, >> >> >> >> I am here on Ubuntu/precise and wanted to avoid to upgrade to a higher >> >> cairo-version. >> > >> > Daniel's position is that he wants to make it hard for QA to build the >> > tests incorrectly and so skip half of them. I think defaulting to >> > erroring out at configure if cairo is too old, but allowing an explicit >> > --disable-cairo should make all us happy. >> >> Hmm, just curious how this should work? >> >> Beyond testdisplay... kms_flip and kms_render tests require cairo.h >> include - not sure which version of cairo is minimum. >> So, --disable-cairo configure-option should disable above tests? >> >> [ tests/Makefile.am ] >> ... >> testdisplay_SOURCES = \ >> testdisplay.c \ >> testdisplay.h \ >> testdisplay_hotplug.c \ >> $(NULL) >> >> TESTS_progs += testdisplay >> LDADD += $(CAIRO_LIBS) $(LIBUDEV_LIBS) $(GLIB_LIBS) >> AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS) $(GLIB_CFLAGS) >> ... >> >> $ grep -i cairo.h -nr ./ >> ./lib/drmtest.h:34:#include >> ./tests/kms_flip.c:28:#include >> ./tests/kms_render.c:27:#include >> ./tests/testdisplay.c:52:#include > > First you would need to split the cairo portion of drmtest.[ch] into its > own files and then conditionally compile those, along with the cairo > dependent tests. As said I am not an autotools expert. But I was interested in the tools not building the tests. That's why I asked why I am forced to upgrade to a higher cairo release. Why does tools require $(CAIRO_CFLAGS) and $(CAIRO_LIBS)? Hmm, looks like intel_perf_counters and intel_l3_parity do... tools/Makefile.am:40:AM_CFLAGS = $(DRM_CFLAGS) $(PCIACCESS_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) tools/Makefile.am:41:LDADD = $(top_builddir)/lib/libintel_tools.la $(DRM_LIBS) $(PCIACCESS_LIBS) $(CAIRO_LIBS) tools/intel_perf_counters.c:47:#include "drmtest.h" tools/intel_l3_parity.c:38:#include "drmtest.h" --disable-cairo might be a good idea, but I cannot help here. - Sedat - ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915: explicit store base gem object in dma_buf->priv
On Thu, Aug 08, 2013 at 09:10:38AM +0200, Daniel Vetter wrote: > Makes it more obviously correct what tricks we play by reusing the drm > prime release helper. > > Reviewed-by: Chris Wilson > Signed-off-by: Daniel Vetter Ok, to get things going I've merged the two i915 patches to dinq. -Daniel > --- > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 21 - > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index f7e1682..e918b05 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -27,10 +27,15 @@ > #include "i915_drv.h" > #include > > +static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) > +{ > + return to_intel_bo(buf->priv); > +} > + > static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment > *attachment, >enum dma_data_direction dir) > { > - struct drm_i915_gem_object *obj = attachment->dmabuf->priv; > + struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); > struct sg_table *st; > struct scatterlist *src, *dst; > int ret, i; > @@ -85,7 +90,7 @@ static void i915_gem_unmap_dma_buf(struct > dma_buf_attachment *attachment, > struct sg_table *sg, > enum dma_data_direction dir) > { > - struct drm_i915_gem_object *obj = attachment->dmabuf->priv; > + struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); > > mutex_lock(&obj->base.dev->struct_mutex); > > @@ -100,7 +105,7 @@ static void i915_gem_unmap_dma_buf(struct > dma_buf_attachment *attachment, > > static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) > { > - struct drm_i915_gem_object *obj = dma_buf->priv; > + struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); > struct drm_device *dev = obj->base.dev; > struct sg_page_iter sg_iter; > struct page **pages; > @@ -148,7 +153,7 @@ error: > > static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) > { > - struct drm_i915_gem_object *obj = dma_buf->priv; > + struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); > struct drm_device *dev = obj->base.dev; > int ret; > > @@ -191,7 +196,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, > struct vm_area_struct * > > static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, > size_t length, enum dma_data_direction direction) > { > - struct drm_i915_gem_object *obj = dma_buf->priv; > + struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); > struct drm_device *dev = obj->base.dev; > int ret; > bool write = (direction == DMA_BIDIRECTIONAL || direction == > DMA_TO_DEVICE); > @@ -222,9 +227,7 @@ static const struct dma_buf_ops i915_dmabuf_ops = { > struct dma_buf *i915_gem_prime_export(struct drm_device *dev, > struct drm_gem_object *gem_obj, int flags) > { > - struct drm_i915_gem_object *obj = to_intel_bo(gem_obj); > - > - return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, flags); > + return dma_buf_export(gem_obj, &i915_dmabuf_ops, gem_obj->size, flags); > } > > static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) > @@ -261,7 +264,7 @@ struct drm_gem_object *i915_gem_prime_import(struct > drm_device *dev, > > /* is this one of own objects? */ > if (dma_buf->ops == &i915_dmabuf_ops) { > - obj = dma_buf->priv; > + obj = dma_buf_to_obj(dma_buf); > /* is it from our device? */ > if (obj->base.dev == dev) { > /* > -- > 1.8.3.2 > -- 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
[Intel-gfx] [PATCH] assembler: error for the wrong syntax of SEND instruction on GEN6+
From: "Xiang, Haihao" predicate SEND execsize dst sendleadreg payload directsrcoperand instoptions predicate SEND execsize dst sendleadreg payload imm32reg instoptions predicate SEND execsize dst sendleadreg payload sndopr imm32reg instoptions predicate SEND execsize dst sendleadreg payload exp directsrcoperand instoptions The above four syntaxes are only used on legacy platforms which support implied move from payload to dst. Signed-off-by: Xiang, Haihao Signed-off-by: Ben Widawsky --- Somehow this ended up in our internal IGT. Anyone have an issue with me pushing it to igt proper? --- assembler/gram.y | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/assembler/gram.y b/assembler/gram.y index e58c1fe..8795797 100644 --- a/assembler/gram.y +++ b/assembler/gram.y @@ -1215,6 +1215,9 @@ sendinstruction: predicate sendop execsize exp post_dst payload msgtarget } | predicate sendop execsize dst sendleadreg payload directsrcoperand instoptions { + if (IS_GENp(6)) + error(&@2, "the syntax of send instruction\n"); + memset(&$$, 0, sizeof($$)); set_instruction_opcode(&$$, $2); GEN(&$$)->header.destreg__conditionalmod = $5.nr; /* msg reg index */ @@ -1233,6 +1236,9 @@ sendinstruction: predicate sendop execsize exp post_dst payload msgtarget } | predicate sendop execsize dst sendleadreg payload imm32reg instoptions { + if (IS_GENp(6)) + error(&@2, "the syntax of send instruction\n"); + if ($7.reg.type != BRW_REGISTER_TYPE_UD && $7.reg.type != BRW_REGISTER_TYPE_D && $7.reg.type != BRW_REGISTER_TYPE_V) { @@ -1336,6 +1342,9 @@ sendinstruction: predicate sendop execsize exp post_dst payload msgtarget } | predicate sendop execsize dst sendleadreg payload sndopr imm32reg instoptions { + if (IS_GENp(6)) + error(&@2, "the syntax of send instruction\n"); + if ($8.reg.type != BRW_REGISTER_TYPE_UD && $8.reg.type != BRW_REGISTER_TYPE_D && $8.reg.type != BRW_REGISTER_TYPE_V) { @@ -1355,15 +1364,16 @@ sendinstruction: predicate sendop execsize exp post_dst payload msgtarget if (set_instruction_src1(&$$, &$8, &@8) != 0) YYERROR; - if (IS_GENp(8)) { - gen8_set_eot(GEN8(&$$), !!($7 & EX_DESC_EOT_MASK)); - } else if (IS_GENx(5)) { + if (IS_GENx(5)) { GEN(&$$)->bits2.send_gen5.sfid = ($7 & EX_DESC_SFID_MASK); GEN(&$$)->bits3.generic_gen5.end_of_thread = !!($7 & EX_DESC_EOT_MASK); } } | predicate sendop execsize dst sendleadreg payload exp directsrcoperand instoptions { + if (IS_GENp(6)) + error(&@2, "the syntax of send instruction\n"); + memset(&$$, 0, sizeof($$)); set_instruction_opcode(&$$, $2); GEN(&$$)->header.destreg__conditionalmod = $5.nr; /* msg reg index */ -- 1.8.3.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] intel-gpu-tools: --disable-tests option should make cairo-check obsolete
On Wed, Aug 14, 2013 at 11:03 PM, Chris Wilson wrote: > On Wed, Aug 14, 2013 at 10:33:42PM +0200, Sedat Dilek wrote: >> Hi, >> >> I am here on Ubuntu/precise and wanted to avoid to upgrade to a higher >> cairo-version. > > Daniel's position is that he wants to make it hard for QA to build the > tests incorrectly and so skip half of them. I think defaulting to > erroring out at configure if cairo is too old, but allowing an explicit > --disable-cairo should make all us happy. Hmm, just curious how this should work? Beyond testdisplay... kms_flip and kms_render tests require cairo.h include - not sure which version of cairo is minimum. So, --disable-cairo configure-option should disable above tests? [ tests/Makefile.am ] ... testdisplay_SOURCES = \ testdisplay.c \ testdisplay.h \ testdisplay_hotplug.c \ $(NULL) TESTS_progs += testdisplay LDADD += $(CAIRO_LIBS) $(LIBUDEV_LIBS) $(GLIB_LIBS) AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS) $(GLIB_CFLAGS) ... $ grep -i cairo.h -nr ./ ./lib/drmtest.h:34:#include ./tests/kms_flip.c:28:#include ./tests/kms_render.c:27:#include ./tests/testdisplay.c:52:#include Thanks in advance. - Sedat - ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only unmask required PM interrupts
On Wed, Aug 14, 2013 at 09:01:22PM +, Azad, Vinit wrote: > Our interrupt handler isn't being fired since we do set the IER bits > properly (IIR bits aren't set). The overhead isn't because our driver is > reacting to these interrupts, but because hardware keeps generating > internal messages when PMINTRMSK doesn't mask out the up/down EI > interrupts (which happen periodically). Ah, that makes more sense. Added to the commit message, please elaborate more precisely for the next patch what exactly the effects are. > Unfortunately, the default value of PMINTRMSK register is 0 instead of > 0x as it is for other IMR, so we have to mask out the interrupts > manually. > > -Original Message- > From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of > Daniel Vetter > Sent: Wednesday, August 14, 2013 13:47 > To: Azad, Vinit > Cc: intel-gfx > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Only unmask required PM interrupts > > On Wed, Aug 14, 2013 at 10:34 PM, Vinit Azad wrote: > > Un-masking all PM interrupts causes hardware to generate interrupts > > regardless of whether the interrupts are enabled on the DE side. Since > > turbo only need up/down threshold and > > rc6 timeout interrupt, mask all other interrupts bits to avoid > > unnecessary overhead/wake up. > > Just to clarify since I can't really believe this yet: Even though we > disable all other interrupt sources in PMIER and mask them in PMIMR hw > still manages to fire off our interrupt handler? Do those interrupts end > up setting PMIIR? > > Thanks, Daniel > > > > Change-Id: I6c947df6fd5f60584d39b9e8b8c89faa51a5e827 > > Signed-off-by: Vinit Azad Patch merged to dinq with a bit of manual fixup. Please base patches on top of my drm-intel-nightly tree if possible, only really critical fixes (for hangs, black screens, ...) should be pased on -fixes. Thanks for the patch, -Daniel > > --- > > drivers/gpu/drm/i915/intel_pm.c |8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c index 8e9ce07..17a0dae 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3328,8 +3328,8 @@ static void gen6_enable_rps(struct drm_device *dev) > > I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS); > > I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS); > > spin_unlock_irq(&dev_priv->rps.lock); > > - /* unmask all PM interrupts */ > > - I915_WRITE(GEN6_PMINTRMSK, 0); > > + /* only unmask PM interrupts we need. Mask all others. */ > > + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); > > > > rc6vids = 0; > > ret = sandybridge_pcode_read(dev_priv, > > GEN6_PCODE_READ_RC6VIDS, &rc6vids); @@ -3596,8 +3596,8 @@ static void > > valleyview_enable_rps(struct drm_device *dev) > > WARN_ON(dev_priv->rps.pm_iir != 0); > > I915_WRITE(GEN6_PMIMR, 0); > > spin_unlock_irq(&dev_priv->rps.lock); > > - /* enable all PM interrupts */ > > - I915_WRITE(GEN6_PMINTRMSK, 0); > > + /* enable only the PM interrupts we need. Mask everything else */ > > + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); > > > > gen6_gt_force_wake_put(dev_priv); } > > -- > > 1.7.9.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 -- 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] drm/i915: Only unmask required PM interrupts
Our interrupt handler isn't being fired since we do set the IER bits properly (IIR bits aren't set). The overhead isn't because our driver is reacting to these interrupts, but because hardware keeps generating internal messages when PMINTRMSK doesn't mask out the up/down EI interrupts (which happen periodically). Unfortunately, the default value of PMINTRMSK register is 0 instead of 0x as it is for other IMR, so we have to mask out the interrupts manually. -Original Message- From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 14, 2013 13:47 To: Azad, Vinit Cc: intel-gfx Subject: Re: [Intel-gfx] [PATCH] drm/i915: Only unmask required PM interrupts On Wed, Aug 14, 2013 at 10:34 PM, Vinit Azad wrote: > Un-masking all PM interrupts causes hardware to generate interrupts > regardless of whether the interrupts are enabled on the DE side. Since > turbo only need up/down threshold and > rc6 timeout interrupt, mask all other interrupts bits to avoid > unnecessary overhead/wake up. Just to clarify since I can't really believe this yet: Even though we disable all other interrupt sources in PMIER and mask them in PMIMR hw still manages to fire off our interrupt handler? Do those interrupts end up setting PMIIR? Thanks, Daniel > > Change-Id: I6c947df6fd5f60584d39b9e8b8c89faa51a5e827 > Signed-off-by: Vinit Azad > --- > drivers/gpu/drm/i915/intel_pm.c |8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c index 8e9ce07..17a0dae 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3328,8 +3328,8 @@ static void gen6_enable_rps(struct drm_device *dev) > I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS); > I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS); > spin_unlock_irq(&dev_priv->rps.lock); > - /* unmask all PM interrupts */ > - I915_WRITE(GEN6_PMINTRMSK, 0); > + /* only unmask PM interrupts we need. Mask all others. */ > + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); > > rc6vids = 0; > ret = sandybridge_pcode_read(dev_priv, > GEN6_PCODE_READ_RC6VIDS, &rc6vids); @@ -3596,8 +3596,8 @@ static void > valleyview_enable_rps(struct drm_device *dev) > WARN_ON(dev_priv->rps.pm_iir != 0); > I915_WRITE(GEN6_PMIMR, 0); > spin_unlock_irq(&dev_priv->rps.lock); > - /* enable all PM interrupts */ > - I915_WRITE(GEN6_PMINTRMSK, 0); > + /* enable only the PM interrupts we need. Mask everything else */ > + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); > > gen6_gt_force_wake_put(dev_priv); } > -- > 1.7.9.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] drm/i915: Only unmask required PM interrupts
On Wed, Aug 14, 2013 at 10:34 PM, Vinit Azad wrote: > Un-masking all PM interrupts causes hardware to generate > interrupts regardless of whether the interrupts are enabled > on the DE side. Since turbo only need up/down threshold and > rc6 timeout interrupt, mask all other interrupts bits to avoid > unnecessary overhead/wake up. Just to clarify since I can't really believe this yet: Even though we disable all other interrupt sources in PMIER and mask them in PMIMR hw still manages to fire off our interrupt handler? Do those interrupts end up setting PMIIR? Thanks, Daniel > > Change-Id: I6c947df6fd5f60584d39b9e8b8c89faa51a5e827 > Signed-off-by: Vinit Azad > --- > drivers/gpu/drm/i915/intel_pm.c |8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 8e9ce07..17a0dae 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3328,8 +3328,8 @@ static void gen6_enable_rps(struct drm_device *dev) > I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS); > I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS); > spin_unlock_irq(&dev_priv->rps.lock); > - /* unmask all PM interrupts */ > - I915_WRITE(GEN6_PMINTRMSK, 0); > + /* only unmask PM interrupts we need. Mask all others. */ > + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); > > rc6vids = 0; > ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, > &rc6vids); > @@ -3596,8 +3596,8 @@ static void valleyview_enable_rps(struct drm_device > *dev) > WARN_ON(dev_priv->rps.pm_iir != 0); > I915_WRITE(GEN6_PMIMR, 0); > spin_unlock_irq(&dev_priv->rps.lock); > - /* enable all PM interrupts */ > - I915_WRITE(GEN6_PMINTRMSK, 0); > + /* enable only the PM interrupts we need. Mask everything else */ > + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); > > gen6_gt_force_wake_put(dev_priv); > } > -- > 1.7.9.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
[Intel-gfx] [PATCH] drm/i915: Only unmask required PM interrupts
Un-masking all PM interrupts causes hardware to generate interrupts regardless of whether the interrupts are enabled on the DE side. Since turbo only need up/down threshold and rc6 timeout interrupt, mask all other interrupts bits to avoid unnecessary overhead/wake up. Change-Id: I6c947df6fd5f60584d39b9e8b8c89faa51a5e827 Signed-off-by: Vinit Azad --- drivers/gpu/drm/i915/intel_pm.c |8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 8e9ce07..17a0dae 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3328,8 +3328,8 @@ static void gen6_enable_rps(struct drm_device *dev) I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS); I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS); spin_unlock_irq(&dev_priv->rps.lock); - /* unmask all PM interrupts */ - I915_WRITE(GEN6_PMINTRMSK, 0); + /* only unmask PM interrupts we need. Mask all others. */ + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); rc6vids = 0; ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids); @@ -3596,8 +3596,8 @@ static void valleyview_enable_rps(struct drm_device *dev) WARN_ON(dev_priv->rps.pm_iir != 0); I915_WRITE(GEN6_PMIMR, 0); spin_unlock_irq(&dev_priv->rps.lock); - /* enable all PM interrupts */ - I915_WRITE(GEN6_PMINTRMSK, 0); + /* enable only the PM interrupts we need. Mask everything else */ + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); gen6_gt_force_wake_put(dev_priv); } -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] intel-gpu-tools: --disable-tests option should make cairo-check obsolete
Hi, I am here on Ubuntu/precise and wanted to avoid to upgrade to a higher cairo-version. [ configure.ac ] ... # for testdisplay PKG_CHECK_MODULES(CAIRO, [cairo >= 1.12.0]) PKG_CHECK_MODULES(LIBUDEV, [libudev], [udev=yes], [udev=no]) if test x"$udev" = xyes; then AC_DEFINE(HAVE_UDEV,1,[Enable udev-based monitor hotplug detection]) fi PKG_CHECK_MODULES(GLIB, glib-2.0) ... $ find ./ -name 'testdisplay*' ./tests/testdisplay.h ./tests/testdisplay.c ./tests/testdisplay_hotplug.c So, testdisplay is a member of tests, with --disable-tests I expect that any checks will be dropped - means no cairo-update for me. Can someone with better autotools skillz fix that? Thanks in advance. Regards, - Sedat - P.S.: BTW, I am only interested in building the tools only. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] VGA arbiter support for Intel HD?
On Wed, Aug 14, 2013 at 01:39:55PM -0600, Alex Williamson wrote: > On Wed, 2013-08-14 at 21:30 +0300, Ville Syrjälä wrote: > > On Wed, Aug 14, 2013 at 11:14:48AM -0600, Alex Williamson wrote: > > > On Wed, 2013-08-14 at 17:47 +0300, Ville Syrjälä wrote: > > > > On Wed, Aug 14, 2013 at 07:23:57AM -0600, Alex Williamson wrote: > > > > > Hi, > > > > > > > > > > I'm trying to add support for device assignment of PCI VGA devices > > > > > with > > > > > VFIO and QEMU. For normal, discrete discrete graphics the Linux VGA > > > > > arbiter works fairly well, disabling VGA on one bridge and adding it > > > > > to > > > > > another (though I wish all the kernel VGA drivers made use of it). > > > > > The > > > > > i915 driver only seems to support disabling VGA on really old GMCH > > > > > devices (see intel_modeset_vga_set_state). This means that if I boot > > > > > with IGD as the primary graphics and attempt to assign a discrete > > > > > graphics device, all the VGA range accesses are still routed to IGD, I > > > > > end up getting some error messages from the IGD interrupt handler, and > > > > > the discrete card never initializes. > > > > > > > > > > I spent some time looking through the Sand Bridge, Ivy Bridge, and > > > > > Haswell datasheets, and I'm a bit concerned whether the hardware even > > > > > provides a reasonable way to disable VGA anymore. Quoting 2.17 from > > > > > the > > > > > Haswell docs: > > > > > > > > > > Accesses to the VGA memory range are directed to IGD depend on > > > > > the configuration. The configuration is specified by: > > > > > * Internal graphics controller in Device 2 is enabled > > > > > (DEVEN.D2EN bit 4) > > > > > * Internal graphics VGA in Device 0 Function 0 is > > > > > enabled > > > > > through register GGC bit 1. > > > > > * IGD's memory accesses (PCICMD2 04 – 05h, MAE bit 1) in > > > > > Device 2 configuration space are enabled. > > > > > * VGA compatibility memory accesses (VGA Miscellaneous > > > > > Output register – MSR Register, bit 1) are enabled. > > > > > * Software sets the proper value for VGA Memory Map Mode > > > > > register (VGA GR06 Register, bits 3-2). See the > > > > > following table for translations. > > > > > > > > > > (There's a similar list for VGA I/O range) I've found that if I > > > > > disable > > > > > memory and I/O in the PCI command register for IGD then I do get VGA > > > > > routing to the PEG device and the discrete VBIOS works. This > > > > > obviously > > > > > isn't a good option for the VGA arbiter since it entirely disables > > > > > IGD. > > > > > > > > > > The GGC registers aren't meant for runtime switching and are actually > > > > > locked. Disabling IGD via the device 2 enable bit doesn't seem like > > > > > and > > > > > option. I don't quite understand the VGA miscellaneous output > > > > > register > > > > > and VGA memory map mode, but the table provided for the latter makes > > > > > me > > > > > think they just augment the VGA ranges and don't disable them. > > > > > > > > Bit 1 of MSR (0x3c2/0x3cc) should allow you to turn off VGA mem > > > > access while leaving other memory space access working. > > > > > > > > As for VGA I/O decode, IIRC there's no standard bit for that in VGA > > > > or PCI config registers, and I can't see any other bit for it in the > > > > docs. But I guess you could just turn off I/O space completely > > > > via the PCI_COMMAND register. We shouldn't need it for anything beyond > > > > i915_disable_vga() and that has the necessary vgaarb calls already. > > > > > > Thanks Ville. The MSR seems to work for VGA memory. Disabling I/O via > > > PCI_COMMAND does works, but something is re-enabling it after > > > intel_modeset_vga_set_state(). If I manually disable I/O with setpci > > > then I do have VGA routing to PEG and can still interact with the KMS > > > console on IGD. It's unfortunate that the MSR bit for I/O only disables > > > pieces of the range. If we have no other options, I'll try to hunt down > > > where I/O is being re-enabled and see how feasible it is to avoid. > > > Thanks, > > > > Hmm. Now that I look at vgaarb more it seems I misunderstood the way it > > works. Based on the code it looks like it will permanently remove the > > device from the arbiration if set_vga_decode indicates that it doesn't > > decode legacy resources. And it calls set_vga_decode w/ decode=false > > if there are more than two VGA cards in the system. That means > > s/more than two/two or more/ > > > i915_disable_vga() is actually broken whenever there is another > > VGA card in the system. > > I didn't follow why i915_disable_vga is broken. It seems like the > intention is to disable VGA regardless of how many VGA devices are > present. I suppose that's the original intention, but i915_d
Re: [Intel-gfx] VGA arbiter support for Intel HD?
On Wed, 2013-08-14 at 21:30 +0300, Ville Syrjälä wrote: > On Wed, Aug 14, 2013 at 11:14:48AM -0600, Alex Williamson wrote: > > On Wed, 2013-08-14 at 17:47 +0300, Ville Syrjälä wrote: > > > On Wed, Aug 14, 2013 at 07:23:57AM -0600, Alex Williamson wrote: > > > > Hi, > > > > > > > > I'm trying to add support for device assignment of PCI VGA devices with > > > > VFIO and QEMU. For normal, discrete discrete graphics the Linux VGA > > > > arbiter works fairly well, disabling VGA on one bridge and adding it to > > > > another (though I wish all the kernel VGA drivers made use of it). The > > > > i915 driver only seems to support disabling VGA on really old GMCH > > > > devices (see intel_modeset_vga_set_state). This means that if I boot > > > > with IGD as the primary graphics and attempt to assign a discrete > > > > graphics device, all the VGA range accesses are still routed to IGD, I > > > > end up getting some error messages from the IGD interrupt handler, and > > > > the discrete card never initializes. > > > > > > > > I spent some time looking through the Sand Bridge, Ivy Bridge, and > > > > Haswell datasheets, and I'm a bit concerned whether the hardware even > > > > provides a reasonable way to disable VGA anymore. Quoting 2.17 from the > > > > Haswell docs: > > > > > > > > Accesses to the VGA memory range are directed to IGD depend on > > > > the configuration. The configuration is specified by: > > > > * Internal graphics controller in Device 2 is enabled > > > > (DEVEN.D2EN bit 4) > > > > * Internal graphics VGA in Device 0 Function 0 is enabled > > > > through register GGC bit 1. > > > > * IGD's memory accesses (PCICMD2 04 – 05h, MAE bit 1) in > > > > Device 2 configuration space are enabled. > > > > * VGA compatibility memory accesses (VGA Miscellaneous > > > > Output register – MSR Register, bit 1) are enabled. > > > > * Software sets the proper value for VGA Memory Map Mode > > > > register (VGA GR06 Register, bits 3-2). See the > > > > following table for translations. > > > > > > > > (There's a similar list for VGA I/O range) I've found that if I disable > > > > memory and I/O in the PCI command register for IGD then I do get VGA > > > > routing to the PEG device and the discrete VBIOS works. This obviously > > > > isn't a good option for the VGA arbiter since it entirely disables IGD. > > > > > > > > The GGC registers aren't meant for runtime switching and are actually > > > > locked. Disabling IGD via the device 2 enable bit doesn't seem like and > > > > option. I don't quite understand the VGA miscellaneous output register > > > > and VGA memory map mode, but the table provided for the latter makes me > > > > think they just augment the VGA ranges and don't disable them. > > > > > > Bit 1 of MSR (0x3c2/0x3cc) should allow you to turn off VGA mem > > > access while leaving other memory space access working. > > > > > > As for VGA I/O decode, IIRC there's no standard bit for that in VGA > > > or PCI config registers, and I can't see any other bit for it in the > > > docs. But I guess you could just turn off I/O space completely > > > via the PCI_COMMAND register. We shouldn't need it for anything beyond > > > i915_disable_vga() and that has the necessary vgaarb calls already. > > > > Thanks Ville. The MSR seems to work for VGA memory. Disabling I/O via > > PCI_COMMAND does works, but something is re-enabling it after > > intel_modeset_vga_set_state(). If I manually disable I/O with setpci > > then I do have VGA routing to PEG and can still interact with the KMS > > console on IGD. It's unfortunate that the MSR bit for I/O only disables > > pieces of the range. If we have no other options, I'll try to hunt down > > where I/O is being re-enabled and see how feasible it is to avoid. > > Thanks, > > Hmm. Now that I look at vgaarb more it seems I misunderstood the way it > works. Based on the code it looks like it will permanently remove the > device from the arbiration if set_vga_decode indicates that it doesn't > decode legacy resources. And it calls set_vga_decode w/ decode=false > if there are more than two VGA cards in the system. That means s/more than two/two or more/ > i915_disable_vga() is actually broken whenever there is another > VGA card in the system. I didn't follow why i915_disable_vga is broken. It seems like the intention is to disable VGA regardless of how many VGA devices are present. > To make it work I think what we'd need to do is always return > VGA_RSRC_LEGACY_IO from i915_vga_set_decode(), which will leave the > PCI_COMMAND I/O bit in the hands of vgaarb, and then poke at the > MSR register to disable the VGA mem decode permanently. But to touch > MSR we actually need VGA I/O, so I guess we'd have to do that right > after registering w/ vgaarb. Doing it from i915_vg
Re: [Intel-gfx] [PATCH 6.3/9] drm/i915: merge HSW and SNB PM irq handlers
On Fri, Aug 09, 2013 at 05:04:37PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > Because hsw_pm_irq_handler does exactly what gen6_rps_irq_handler does > and also processes the 2 additional VEBOX bits. So merge those > functions and wrap the VEBOX bits on an IS_HASWELL check. This HSW > check isn't really necessary since the bits are reserved on > SNB/IVB/VLV, but it's a good documentation on who uses them. > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_irq.c | 50 > ++--- > 1 file changed, 12 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index d9ebfb6..8ba5d0a 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -939,28 +939,6 @@ static void snb_gt_irq_handler(struct drm_device *dev, > ivybridge_parity_error_irq_handler(dev); > } > > -/* Legacy way of handling PM interrupts */ > -static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, > - u32 pm_iir) > -{ > - /* > - * IIR bits should never already be set because IMR should > - * prevent an interrupt from being shown in IIR. The warning > - * displays a case where we've unsafely cleared > - * dev_priv->rps.pm_iir. Although missing an interrupt of the same > - * type is not a problem, it displays a problem in the logic. > - * > - * The mask bit in IMR is cleared by dev_priv->rps.work. > - */ > - > - spin_lock(&dev_priv->irq_lock); > - dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS; > - snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS); > - spin_unlock(&dev_priv->irq_lock); > - > - queue_work(dev_priv->wq, &dev_priv->rps.work); > -} > - > #define HPD_STORM_DETECT_PERIOD 1000 > #define HPD_STORM_THRESHOLD 5 > > @@ -1027,13 +1005,10 @@ static void dp_aux_irq_handler(struct drm_device *dev) > wake_up_all(&dev_priv->gmbus_wait_queue); > } > > -/* Unlike gen6_rps_irq_handler() from which this function is originally > derived, > - * we must be able to deal with other PM interrupts. This is complicated > because > - * of the way in which we use the masks to defer the RPS work (which for > - * posterity is necessary because of forcewake). > - */ > -static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv, > -u32 pm_iir) > +/* The RPS events need forcewake, so we add them to a work queue and mask > their > + * IMR bits until the work is done. Other interrupts can be processed without > + * the work queue. */ > +static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 > pm_iir) > { > if (pm_iir & GEN6_PM_RPS_EVENTS) { > spin_lock(&dev_priv->irq_lock); > @@ -1044,12 +1019,14 @@ static void hsw_pm_irq_handler(struct > drm_i915_private *dev_priv, > queue_work(dev_priv->wq, &dev_priv->rps.work); > } > > - if (pm_iir & PM_VEBOX_USER_INTERRUPT) > - notify_ring(dev_priv->dev, &dev_priv->ring[VECS]); > + if (IS_HASWELL(dev_priv->dev)) { > + if (pm_iir & PM_VEBOX_USER_INTERRUPT) > + notify_ring(dev_priv->dev, &dev_priv->ring[VECS]); Make this HAS_VEBOX() instead of IS_HASWELL() > > - if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) { > - DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir); > - i915_handle_error(dev_priv->dev, false); > + if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) { > + DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir); > + i915_handle_error(dev_priv->dev, false); > + } > } > } > > @@ -1424,10 +1401,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void > *arg) > if (INTEL_INFO(dev)->gen >= 6) { > u32 pm_iir = I915_READ(GEN6_PMIIR); > if (pm_iir) { > - if (IS_HASWELL(dev)) > - hsw_pm_irq_handler(dev_priv, pm_iir); > - else > - gen6_rps_irq_handler(dev_priv, pm_iir); > + gen6_rps_irq_handler(dev_priv, pm_iir); > I915_WRITE(GEN6_PMIIR, pm_iir); > ret = IRQ_HANDLED; > } With the couple of small comments, these 3 patches are: Reviewed-by: Ben Widawsky Though after the IRC discussion you may want to discount my review. -- Ben Widawsky, 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] HDMI 4k support v3
On Wed, Aug 14, 2013 at 06:19:03PM +0100, Damien Lespiau wrote: > Follow up on v2: > http://lists.freedesktop.org/archives/dri-devel/2013-August/043604.html > > With the quick and nice reviews from Ville and Simon, it's time for a v3: > - Fix embarrassing hmdi typo > - Fix the sending of vendor specific infoframes for side-by-side half modes > - Smaller changes here and there. Looking very good. Reviewed-by: Ville Syrjälä for the rest as well. -- 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 1/9] drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq
Reviewed-by: Rodrigo Vivi Thanks for pointing me the doc that explains why 800 MHz when using FCLK input. ;) On Tue, Aug 06, 2013 at 06:57:11PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > We already have code to disable LCPLL and switch to FCLK, so we need this too. > We still don't call the code to disable LCPLL, but we'll call it when we add > support for Package C8+. > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/intel_ddi.c | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index b8c096b..63aca49 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1139,10 +1139,13 @@ static void intel_disable_ddi(struct intel_encoder > *intel_encoder) > > int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv) > { > - if (I915_READ(HSW_FUSE_STRAP) & HSW_CDCLK_LIMIT) > + uint32_t lcpll = I915_READ(LCPLL_CTL); > + > + if (lcpll & LCPLL_CD_SOURCE_FCLK) > + return 80; > + else if (I915_READ(HSW_FUSE_STRAP) & HSW_CDCLK_LIMIT) > return 45; > - else if ((I915_READ(LCPLL_CTL) & LCPLL_CLK_FREQ_MASK) == > - LCPLL_CLK_FREQ_450) > + else if ((lcpll & LCPLL_CLK_FREQ_MASK) == LCPLL_CLK_FREQ_450) > return 45; > else if (IS_ULT(dev_priv->dev)) > return 337500; > -- > 1.8.1.2 > > ___ > 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 07/12] video/hdmi: Introduce helpers for the HDMI vendor specific infoframe
On Wed, Aug 14, 2013 at 06:19:10PM +0100, Damien Lespiau wrote: > Provide the same programming model than the other infoframe types. > > The generic _pack() function can't handle those yet as we need to move > the vendor OUI in the generic hdmi_vendor_infoframe structure to know > which kind of vendor infoframe we are dealing with. > > v2: Fix the value of Side-by-side (half), hmdi typo, pack 3D_Ext_Data > (Ville Syrjälä) > > Signed-off-by: Damien Lespiau > --- > drivers/video/hdmi.c | 88 > > include/linux/hdmi.h | 26 > 2 files changed, 114 insertions(+) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index ac84215..59c4748 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -286,6 +286,94 @@ ssize_t hdmi_audio_infoframe_pack(struct > hdmi_audio_infoframe *frame, > EXPORT_SYMBOL(hdmi_audio_infoframe_pack); > > /** > + * hdmi_hdmi_infoframe_init() - initialize an HDMI vendor infoframe > + * @frame: HDMI vendor infoframe > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame) > +{ > + memset(frame, 0, sizeof(*frame)); > + > + frame->type = HDMI_INFOFRAME_TYPE_VENDOR; > + frame->version = 1; > + > + /* 0 is a valid value for s3d_struct, so we use a special "not set" > + * value */ > + frame->s3d_struct = HDMI_3D_STRUCTURE_INVALID; > + > + return 0; > +} > +EXPORT_SYMBOL(hdmi_hdmi_infoframe_init); > + > +/** > + * hdmi_hdmi_infoframe_pack() - write a HDMI vendor infoframe to binary > buffer > + * @frame: HDMI infoframe > + * @buffer: destination buffer > + * @size: size of buffer > + * > + * Packs the information contained in the @frame structure into a binary > + * representation that can be written into the corresponding controller > + * registers. Also computes the checksum as required by section 5.3.5 of > + * the HDMI 1.4 specification. > + * > + * Returns the number of bytes packed into the binary buffer or a negative > + * error code on failure. > + */ > +ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, > + void *buffer, size_t size) > +{ > + u8 *ptr = buffer; > + size_t length; > + > + /* empty info frame */ > + if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID) > + return -EINVAL; > + > + /* only one of those can be supplied */ > + if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) > + return -EINVAL; > + > + /* for side by side (half) we also need to provide 3D_Ext_Data */ > + if (frame->s3d_struct == HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) Could be >= for a bit of future proofing since the spec says we should send 3d_ext_data even for the reserved > 8 values. > + frame->length = 6; > + else > + frame->length = 5; > + > + length = HDMI_INFOFRAME_HEADER_SIZE + frame->length; > + > + if (size < length) > + return -ENOSPC; > + > + memset(buffer, 0, size); > + > + ptr[0] = frame->type; > + ptr[1] = frame->version; > + ptr[2] = frame->length; > + ptr[3] = 0; /* checksum */ > + > + /* HDMI OUI */ > + ptr[4] = 0x03; > + ptr[5] = 0x0c; > + ptr[6] = 0x00; > + > + if (frame->vic) { > + ptr[7] = 0x1 << 5; /* video format */ > + ptr[8] = frame->vic; > + } else { > + ptr[7] = 0x2 << 5; /* video format */ > + ptr[8] = (frame->s3d_struct & 0xf) << 4; > + if (frame->s3d_struct == HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) Could be >= here too. But whether or not you make those changes: Reviewed-by: Ville Syrjälä > + ptr[9] = (frame->s3d_ext_data & 0xf) << 4; > + } > + > + hdmi_infoframe_checksum(buffer, length); > + > + return length; > +} > +EXPORT_SYMBOL(hdmi_hdmi_infoframe_pack); > + > +/** > * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary > *buffer > * @frame: HDMI vendor infoframe > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > index b98340b..e733252 100644 > --- a/include/linux/hdmi.h > +++ b/include/linux/hdmi.h > @@ -234,11 +234,37 @@ struct hdmi_vendor_infoframe { > ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > void *buffer, size_t size); > > +enum hdmi_3d_structure { > + HDMI_3D_STRUCTURE_INVALID = -1, > + HDMI_3D_STRUCTURE_FRAME_PACKING = 0, > + HDMI_3D_STRUCTURE_FIELD_ALTERNATIVE, > + HDMI_3D_STRUCTURE_LINE_ALTERNATIVE, > + HDMI_3D_STRUCTURE_SIDE_BY_SIDE_FULL, > + HDMI_3D_STRUCTURE_L_DEPTH, > + HDMI_3D_STRUCTURE_L_DEPTH_GFX_GFX_DEPTH, > + HDMI_3D_STRUCTURE_TOP_AND_BOTTOM, > + HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF = 8, > +}; > + > +struct hdmi_hdmi_infoframe { > + enu
Re: [Intel-gfx] [PATCH 6.1/9] drm/i915: don't queue PM events we won't process
On Fri, Aug 09, 2013 at 05:04:35PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > On SNB/IVB/VLV we only call gen6_rps_irq_handler if one of the IIR > bits set is part of GEN6_PM_RPS_EVENTS, but at gen6_rps_irq_handler we > add all the enabled IIR bits to the work queue, not only the ones that > are part of GEN6_PM_RPS_EVENTS. But then gen6_pm_rps_work only > processes GEN6_PM_RPS_EVENTS, so it's useless to add anything that's > not GEN6_PM_RPS_EVENTS to the work queue. > > As a bonus, gen6_rps_irq_handler looks more similar to > hsw_pm_irq_handler, so we may be able to merge them in the future. > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_irq.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 0f46d33..5b51c43 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -959,7 +959,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private > *dev_priv, >*/ > > spin_lock(&dev_priv->irq_lock); > - dev_priv->rps.pm_iir |= pm_iir; > + dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS; > snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir); > spin_unlock(&dev_priv->irq_lock); > > @@ -1128,7 +1128,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void > *arg) > if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS) > gmbus_irq_handler(dev); > > - if (pm_iir & GEN6_PM_RPS_EVENTS) > + if (pm_iir) > gen6_rps_irq_handler(dev_priv, pm_iir); > > I915_WRITE(GTIIR, gt_iir); > @@ -1433,7 +1433,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void > *arg) > if (pm_iir) { > if (IS_HASWELL(dev)) > hsw_pm_irq_handler(dev_priv, pm_iir); > - else if (pm_iir & GEN6_PM_RPS_EVENTS) > + else > gen6_rps_irq_handler(dev_priv, pm_iir); > I915_WRITE(GEN6_PMIIR, pm_iir); > ret = IRQ_HANDLED; Can you please add WARN_ON(pm_iir & ~GEN6_PM_RPS_EVENTS) somewhere in the code path to make me happy? Otherwise it's: Reviewed-by: Ben Widawsky -- Ben Widawsky, 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] VGA arbiter support for Intel HD?
On Wed, Aug 14, 2013 at 11:14:48AM -0600, Alex Williamson wrote: > On Wed, 2013-08-14 at 17:47 +0300, Ville Syrjälä wrote: > > On Wed, Aug 14, 2013 at 07:23:57AM -0600, Alex Williamson wrote: > > > Hi, > > > > > > I'm trying to add support for device assignment of PCI VGA devices with > > > VFIO and QEMU. For normal, discrete discrete graphics the Linux VGA > > > arbiter works fairly well, disabling VGA on one bridge and adding it to > > > another (though I wish all the kernel VGA drivers made use of it). The > > > i915 driver only seems to support disabling VGA on really old GMCH > > > devices (see intel_modeset_vga_set_state). This means that if I boot > > > with IGD as the primary graphics and attempt to assign a discrete > > > graphics device, all the VGA range accesses are still routed to IGD, I > > > end up getting some error messages from the IGD interrupt handler, and > > > the discrete card never initializes. > > > > > > I spent some time looking through the Sand Bridge, Ivy Bridge, and > > > Haswell datasheets, and I'm a bit concerned whether the hardware even > > > provides a reasonable way to disable VGA anymore. Quoting 2.17 from the > > > Haswell docs: > > > > > > Accesses to the VGA memory range are directed to IGD depend on > > > the configuration. The configuration is specified by: > > > * Internal graphics controller in Device 2 is enabled > > > (DEVEN.D2EN bit 4) > > > * Internal graphics VGA in Device 0 Function 0 is enabled > > > through register GGC bit 1. > > > * IGD's memory accesses (PCICMD2 04 – 05h, MAE bit 1) in > > > Device 2 configuration space are enabled. > > > * VGA compatibility memory accesses (VGA Miscellaneous > > > Output register – MSR Register, bit 1) are enabled. > > > * Software sets the proper value for VGA Memory Map Mode > > > register (VGA GR06 Register, bits 3-2). See the > > > following table for translations. > > > > > > (There's a similar list for VGA I/O range) I've found that if I disable > > > memory and I/O in the PCI command register for IGD then I do get VGA > > > routing to the PEG device and the discrete VBIOS works. This obviously > > > isn't a good option for the VGA arbiter since it entirely disables IGD. > > > > > > The GGC registers aren't meant for runtime switching and are actually > > > locked. Disabling IGD via the device 2 enable bit doesn't seem like and > > > option. I don't quite understand the VGA miscellaneous output register > > > and VGA memory map mode, but the table provided for the latter makes me > > > think they just augment the VGA ranges and don't disable them. > > > > Bit 1 of MSR (0x3c2/0x3cc) should allow you to turn off VGA mem > > access while leaving other memory space access working. > > > > As for VGA I/O decode, IIRC there's no standard bit for that in VGA > > or PCI config registers, and I can't see any other bit for it in the > > docs. But I guess you could just turn off I/O space completely > > via the PCI_COMMAND register. We shouldn't need it for anything beyond > > i915_disable_vga() and that has the necessary vgaarb calls already. > > Thanks Ville. The MSR seems to work for VGA memory. Disabling I/O via > PCI_COMMAND does works, but something is re-enabling it after > intel_modeset_vga_set_state(). If I manually disable I/O with setpci > then I do have VGA routing to PEG and can still interact with the KMS > console on IGD. It's unfortunate that the MSR bit for I/O only disables > pieces of the range. If we have no other options, I'll try to hunt down > where I/O is being re-enabled and see how feasible it is to avoid. > Thanks, Hmm. Now that I look at vgaarb more it seems I misunderstood the way it works. Based on the code it looks like it will permanently remove the device from the arbiration if set_vga_decode indicates that it doesn't decode legacy resources. And it calls set_vga_decode w/ decode=false if there are more than two VGA cards in the system. That means i915_disable_vga() is actually broken whenever there is another VGA card in the system. To make it work I think what we'd need to do is always return VGA_RSRC_LEGACY_IO from i915_vga_set_decode(), which will leave the PCI_COMMAND I/O bit in the hands of vgaarb, and then poke at the MSR register to disable the VGA mem decode permanently. But to touch MSR we actually need VGA I/O, so I guess we'd have to do that right after registering w/ vgaarb. Doing it from i915_vga_set_decode() doesn't look possible since there's no guarantee that VGA I/O would end up at the right device at the time that is called. So maybe the following patch might work (although maybe vgaarb itself should be extended a bit to properly support this use case). From 90070e547f1582f8e73d9221d6a31502dea8246d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?=
Re: [Intel-gfx] [PATCH 2/4] [v3] drm/i915: cleanup map&fence in bind
On Wed, Aug 14, 2013 at 10:27:42AM -0700, Ben Widawsky wrote: > On Wed, Aug 14, 2013 at 10:18:55AM +0200, Daniel Vetter wrote: > > On Tue, Aug 13, 2013 at 06:09:07PM -0700, Ben Widawsky wrote: > > > Cleanup the map and fenceable setting during bind to make more sense, > > > and not check i915_is_ggtt() 2 unnecessary times > > > > > > v2: Move the bools into the if block (Chris) - There are ways to tidy > > > this function (fence calculations for instance) even further, but they > > > are quite invasive, so I am punting on those unless specifically asked. > > > > > > v3: Add newline between variable declaration and logic (Chris) > > > > > > Recommended-by: Chris Wilson > > > Reviewed-by: Chris Wilson > > > Signed-off-by: Ben Widawsky > > > --- > > > drivers/gpu/drm/i915/i915_gem.c | 19 +-- > > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > b/drivers/gpu/drm/i915/i915_gem.c > > > index 4a58ead..01cc016 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -3106,7 +3106,6 @@ i915_gem_object_bind_to_vm(struct > > > drm_i915_gem_object *obj, > > > struct drm_device *dev = obj->base.dev; > > > drm_i915_private_t *dev_priv = dev->dev_private; > > > u32 size, fence_size, fence_alignment, unfenced_alignment; > > > - bool mappable, fenceable; > > > size_t gtt_max = > > > map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total; > > > struct i915_vma *vma; > > > @@ -3191,18 +3190,18 @@ search_free: > > > list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); > > > list_add_tail(&vma->mm_list, &vm->inactive_list); > > > > > > - fenceable = > > > - i915_is_ggtt(vm) && > > > - i915_gem_obj_ggtt_size(obj) == fence_size && > > > - (i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) == 0; > > > + if (i915_is_ggtt(vm)) { > > > + bool mappable, fenceable; > > > > > > - mappable = > > > - i915_is_ggtt(vm) && > > > - vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; > > > + fenceable = > > > + i915_gem_obj_ggtt_size(obj) == fence_size && > > > + (i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) > > > == 0; > > > > Why not go the full mounty and also use vma->node here? Would also make > > checkpatch a bit more happy. I'll do a follow-up commit. > > -Daniel > > > > You've already done it, so it's moot. The idea I had was to use "ggtt" > as much as possible when it can only every be ggtt. I think this would > be helpful for both clarity, and debug. I disagree here and I think the extra indirection hampers code readability - I always end up checking your little functions to make sure they actually fish out the right value from the right vma. So my plan is that once this all lands I'll fully rip them out. This wasn't ever about performance, although I admit that unnecessary looping in our gem code does irk me a bit ;-) But again mostly from a clarify pov. For marking ggtt-only paths I think Chris' suggestion to enclose such code in if (is_ggtt(vm)) {} blocks which you've implemented here is much better for clarity. Cheers, 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] drm/i915: Don't load context at driver init time on SNB
On Tue, Aug 13, 2013 at 1:11 AM, Ville Syrjälä wrote: > On Mon, Aug 12, 2013 at 10:33:37AM -0700, Stéphane Marchesin wrote: >> On Fri, Aug 9, 2013 at 9:55 PM, Ben Widawsky wrote: >> > On Fri, Aug 09, 2013 at 08:32:54PM -0700, Stéphane Marchesin wrote: >> >> This is a partial revert of b4ae3f22d238617ca11610b29fde16cf8c0bc6e0 >> >> (drm/i915: load boot context at driver init time) >> >> >> >> This bit breaks hardware video decode for me after resume. >> >> >> >> Signed-off-by: Stéphane Marchesin >> >> --- >> >> drivers/gpu/drm/i915/intel_pm.c | 4 >> >> 1 file changed, 4 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c >> >> b/drivers/gpu/drm/i915/intel_pm.c >> >> index f895d15..ffa4ab2 100644 >> >> --- a/drivers/gpu/drm/i915/intel_pm.c >> >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> >> @@ -4614,10 +4614,6 @@ static void gen6_init_clock_gating(struct >> >> drm_device *dev) >> >> ILK_DPARBUNIT_CLOCK_GATE_ENABLE | >> >> ILK_DPFDUNIT_CLOCK_GATE_ENABLE); >> >> >> >> - /* WaMbcDriverBootEnable:snb */ >> >> - I915_WRITE(GEN6_MBCTL, I915_READ(GEN6_MBCTL) | >> >> -GEN6_MBCTL_ENABLE_BOOT_FETCH); >> >> - >> >> g4x_disable_trickle_feed(dev); >> >> >> >> /* The default value should be 0x200 according to docs, but the two >> > >> > I was looking at this a bit with Stéphane. One thing we both see is that >> > the bit isn't sticking as it should. Clearly doing the write is having >> > an effect, but something is seriously wrong. Writing the bit manually >> > with IGT does make the bit stick. >> > >> > Stéphane, could you try to write the bit with IGT before and after >> > resume, and see if it helps? >> >> It doesn't seem to stick, and so seems to have no effect. >> >> The confusing thing is that video decode works fine before suspend, >> even though that reg is 0. And after resume, it is broken, and that >> reg is still 0. So something else is going on. Maybe this reg is >> write-once-ever? > > BSpec says "This Bit is cleared by the Hardware once the Boot fetch is > complete." So it seems like the boot fetch doesn't work correctly, but still clears the bit? Stéphane ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: enable the power well before module unload
From: Paulo Zanoni Our driver initialization doesn't seem to be ready to load when the power well is disabled: we hit a few "Unclaimed register" messages. So do just like we already do for the suspend/resume path: enable the power well before unloading. At some point we'll want to be able to survive suspend/resume and load/unload with the power well disabled, but for now let's just fix the regression. Regression introduced by the following commit: commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41 Author: Paulo Zanoni Date: Wed Jul 3 17:12:13 2013 -0300 drm/i915: switch disable_power_well default value to 1 Bug can be reproduced by running the "module_reload" script from intel-gpu-tools. Cc: sta...@vger.kernel.org Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813 Signed-off-by: Paulo Zanoni --- drivers/gpu/drm/i915/i915_dma.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) QA (the bug reporter) still didn't confirm this patch fixes the bug, but I can reproduce it and I can confirm it fixes the problem at least on my machine. Also please notice we have other problems that happen when we run module_reload (e.g., gpu hang, no backlight, lockdep error message, eventual oops), this patch fixes only the "Unclaimed register" errors. diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index b30404d..d2dc02b 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1688,8 +1688,13 @@ int i915_driver_unload(struct drm_device *dev) intel_gpu_ips_teardown(); - if (HAS_POWER_WELL(dev)) + if (HAS_POWER_WELL(dev)) { + /* The i915.ko module is still not prepared to be loaded when +* the power well is not enabled, so just enable it in case +* we're going to unload/reload. */ + intel_set_power_well(dev, true); i915_remove_power_well(dev); + } i915_teardown_sysfs(dev); -- 1.8.1.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: inline vma_create into lookup_or_create_vma
On Wed, Aug 14, 2013 at 06:47:00PM +0200, Daniel Vetter wrote: > On Wed, Aug 14, 2013 at 11:59:09AM +0200, Daniel Vetter wrote: > > In the execbuf code we don't clean up any vmas which ended up not > > getting bound for code simplicity. To make sure that we don't end up > > creating multiple vma for the same vm kill the somewhat dangerous > > vma_create function and inline it into lookup_or_create. > > > > This is just a safety measure to prevent surprises in the future. > > > > Also update the somewhat confused comment in the execbuf code and > > clarify what kind of magic is going on with a new one. > > > > Cc: Ben Widawsky > > Signed-off-by: Daniel Vetter > > --- > > > > That's the only concern I could come up with when reading the execbuf > > vma conversion patch. So looks good and I'll slurp it all in as soon > > as some more head scratching is done for the very first patch in this > > series about the vma_unbind fix to only call vma_destroy if the vma > > isn't bound. > > One thing I've noticed but forgot to mention here is that the reloc code > still uses obj_ggtt_size/offset. I guess that will be fixed later on? > -Daniel Yes. -- Ben Widawsky, 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 2/4] [v3] drm/i915: cleanup map&fence in bind
On Wed, Aug 14, 2013 at 10:18:55AM +0200, Daniel Vetter wrote: > On Tue, Aug 13, 2013 at 06:09:07PM -0700, Ben Widawsky wrote: > > Cleanup the map and fenceable setting during bind to make more sense, > > and not check i915_is_ggtt() 2 unnecessary times > > > > v2: Move the bools into the if block (Chris) - There are ways to tidy > > this function (fence calculations for instance) even further, but they > > are quite invasive, so I am punting on those unless specifically asked. > > > > v3: Add newline between variable declaration and logic (Chris) > > > > Recommended-by: Chris Wilson > > Reviewed-by: Chris Wilson > > Signed-off-by: Ben Widawsky > > --- > > drivers/gpu/drm/i915/i915_gem.c | 19 +-- > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > b/drivers/gpu/drm/i915/i915_gem.c > > index 4a58ead..01cc016 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3106,7 +3106,6 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object > > *obj, > > struct drm_device *dev = obj->base.dev; > > drm_i915_private_t *dev_priv = dev->dev_private; > > u32 size, fence_size, fence_alignment, unfenced_alignment; > > - bool mappable, fenceable; > > size_t gtt_max = > > map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total; > > struct i915_vma *vma; > > @@ -3191,18 +3190,18 @@ search_free: > > list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); > > list_add_tail(&vma->mm_list, &vm->inactive_list); > > > > - fenceable = > > - i915_is_ggtt(vm) && > > - i915_gem_obj_ggtt_size(obj) == fence_size && > > - (i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) == 0; > > + if (i915_is_ggtt(vm)) { > > + bool mappable, fenceable; > > > > - mappable = > > - i915_is_ggtt(vm) && > > - vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; > > + fenceable = > > + i915_gem_obj_ggtt_size(obj) == fence_size && > > + (i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) > > == 0; > > Why not go the full mounty and also use vma->node here? Would also make > checkpatch a bit more happy. I'll do a follow-up commit. > -Daniel > You've already done it, so it's moot. The idea I had was to use "ggtt" as much as possible when it can only every be ggtt. I think this would be helpful for both clarity, and debug. The extra indirection would be immeasurable. Again, you've already changed it, so meh. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 12/12] drm/i915/hdmi: Write HDMI vendor specific infoframes
With all the common infoframe bits now in place, we can finally write the vendor specific infoframes in our driver. Signed-off-by: Damien Lespiau Reviewed-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_reg.h | 2 ++ drivers/gpu/drm/i915/intel_hdmi.c | 28 2 files changed, 30 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 17f6252..33427fd1 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4157,6 +4157,8 @@ _TRANSCODER(trans, HSW_VIDEO_DIP_CTL_A, HSW_VIDEO_DIP_CTL_B) #define HSW_TVIDEO_DIP_AVI_DATA(trans) \ _TRANSCODER(trans, HSW_VIDEO_DIP_AVI_DATA_A, HSW_VIDEO_DIP_AVI_DATA_B) +#define HSW_TVIDEO_DIP_VS_DATA(trans) \ +_TRANSCODER(trans, HSW_VIDEO_DIP_VS_DATA_A, HSW_VIDEO_DIP_VS_DATA_B) #define HSW_TVIDEO_DIP_SPD_DATA(trans) \ _TRANSCODER(trans, HSW_VIDEO_DIP_SPD_DATA_A, HSW_VIDEO_DIP_SPD_DATA_B) #define HSW_TVIDEO_DIP_GCP(trans) \ diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index a619d94..4148cc8 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -74,6 +74,8 @@ static u32 g4x_infoframe_index(enum hdmi_infoframe_type type) return VIDEO_DIP_SELECT_AVI; case HDMI_INFOFRAME_TYPE_SPD: return VIDEO_DIP_SELECT_SPD; + case HDMI_INFOFRAME_TYPE_VENDOR: + return VIDEO_DIP_SELECT_VENDOR; default: DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); return 0; @@ -87,6 +89,8 @@ static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type) return VIDEO_DIP_ENABLE_AVI; case HDMI_INFOFRAME_TYPE_SPD: return VIDEO_DIP_ENABLE_SPD; + case HDMI_INFOFRAME_TYPE_VENDOR: + return VIDEO_DIP_ENABLE_VENDOR; default: DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); return 0; @@ -100,6 +104,8 @@ static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type) return VIDEO_DIP_ENABLE_AVI_HSW; case HDMI_INFOFRAME_TYPE_SPD: return VIDEO_DIP_ENABLE_SPD_HSW; + case HDMI_INFOFRAME_TYPE_VENDOR: + return VIDEO_DIP_ENABLE_VS_HSW; default: DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); return 0; @@ -114,6 +120,8 @@ static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type, return HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder); case HDMI_INFOFRAME_TYPE_SPD: return HSW_TVIDEO_DIP_SPD_DATA(cpu_transcoder); + case HDMI_INFOFRAME_TYPE_VENDOR: + return HSW_TVIDEO_DIP_VS_DATA(cpu_transcoder); default: DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); return 0; @@ -392,6 +400,21 @@ static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder) intel_write_infoframe(encoder, &frame); } +static void +intel_hdmi_set_hdmi_infoframe(struct drm_encoder *encoder, + struct drm_display_mode *adjusted_mode) +{ + union hdmi_infoframe frame; + int ret; + + ret = drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi, + adjusted_mode); + if (ret < 0) + return; + + intel_write_infoframe(encoder, &frame); +} + static void g4x_set_infoframes(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode) { @@ -454,6 +477,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder, intel_hdmi_set_avi_infoframe(encoder, adjusted_mode); intel_hdmi_set_spd_infoframe(encoder); + intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode); } static void ibx_set_infoframes(struct drm_encoder *encoder, @@ -515,6 +539,7 @@ static void ibx_set_infoframes(struct drm_encoder *encoder, intel_hdmi_set_avi_infoframe(encoder, adjusted_mode); intel_hdmi_set_spd_infoframe(encoder); + intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode); } static void cpt_set_infoframes(struct drm_encoder *encoder, @@ -550,6 +575,7 @@ static void cpt_set_infoframes(struct drm_encoder *encoder, intel_hdmi_set_avi_infoframe(encoder, adjusted_mode); intel_hdmi_set_spd_infoframe(encoder); + intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode); } static void vlv_set_infoframes(struct drm_encoder *encoder, @@ -584,6 +610,7 @@ static void vlv_set_infoframes(struct drm_encoder *encoder, intel_hdmi_set_avi_infoframe(encoder, adjusted_mode); intel_hdmi_set_spd_infoframe(encoder); + intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode); } static void hsw_set_infoframes(struct drm_encoder *encoder, @@ -611,6 +638,7 @@ static void hsw_set_infoframes(struct drm_enco
[Intel-gfx] [PATCH 11/12] drm: Add a helper to forge HDMI vendor infoframes
This can then be used by DRM drivers to setup their vendor infoframes. v2: Fix hmdi typo (Simon Farnsworth) Signed-off-by: Damien Lespiau Reviewed-by: Simon Farnsworth Reviewed-by: Ville Syrjälä --- drivers/gpu/drm/drm_edid.c | 36 include/drm/drm_edid.h | 4 2 files changed, 40 insertions(+) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 3aa653f..42c62d1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3263,3 +3263,39 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame, return 0; } EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode); + +/** + * drm_hdmi_vendor_infoframe_from_display_mode() - fill an HDMI infoframe with + * data from a DRM display mode + * @frame: HDMI vendor infoframe + * @mode: DRM display mode + * + * Note that there's is a need to send HDMI vendor infoframes only when using a + * 4k or stereoscopic 3D mode. So when giving any other mode as input this + * function will return -EINVAL, error that can be safely ignored. + * + * Returns 0 on success or a negative error code on failure. + */ +int +drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_hdmi_infoframe *frame, + const struct drm_display_mode *mode) +{ + int err; + u8 vic; + + if (!frame || !mode) + return -EINVAL; + + vic = drm_match_hdmi_mode(mode); + if (!vic) + return -EINVAL; + + err = hdmi_hdmi_infoframe_init(frame); + if (err < 0) + return err; + + frame->vic = vic; + + return 0; +} +EXPORT_SYMBOL(drm_hdmi_vendor_infoframe_from_display_mode); diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index fc481fc..a204e31 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -256,6 +256,7 @@ struct drm_encoder; struct drm_connector; struct drm_display_mode; struct hdmi_avi_infoframe; +struct hdmi_hdmi_infoframe; void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid); int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads); @@ -268,5 +269,8 @@ int drm_load_edid_firmware(struct drm_connector *connector); int drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame, const struct drm_display_mode *mode); +int +drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_hdmi_infoframe *frame, + const struct drm_display_mode *mode); #endif /* __DRM_EDID_H__ */ -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/12] drm/edid: Move HDMI_IDENTIFIER to hdmi.h
We'll need the HDMI OUI for the HDMI vendor infoframe data, so let's move the DRM one to hdmi.h, might as well use the hdmi header to store some hdmi defines. (Note that, in fact, infoframes are part of the CEA-861 standard, and only the HDMI vendor specific infoframe is special to HDMI, but details..) Signed-off-by: Damien Lespiau Reviewed-by: Ville Syrjälä --- drivers/gpu/drm/drm_edid.c | 1 - include/linux/hdmi.h | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index d76d608..3aa653f 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2317,7 +2317,6 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, return closure.modes; } -#define HDMI_IDENTIFIER 0x000C03 #define AUDIO_BLOCK0x01 #define VIDEO_BLOCK 0x02 #define VENDOR_BLOCK0x03 diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index e733252..37e0cd7 100644 --- a/include/linux/hdmi.h +++ b/include/linux/hdmi.h @@ -18,6 +18,7 @@ enum hdmi_infoframe_type { HDMI_INFOFRAME_TYPE_AUDIO = 0x84, }; +#define HDMI_IDENTIFIER 0x000c03 #define HDMI_INFOFRAME_HEADER_SIZE 4 #define HDMI_AVI_INFOFRAME_SIZE13 #define HDMI_SPD_INFOFRAME_SIZE25 -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/12] video/hdmi: Hook the HDMI vendor infoframe with the generic _pack()
With this last bit, hdmi_infoframe_pack() is now able to pack any infoframe we support. At the same time, because it's impractical to make two commits out of this, we get rid of the version that encourages the open coding of the vendor infoframe packing. We can do so because the only user of this API has been ported in: Author: Damien Lespiau Date: Mon Aug 12 18:08:37 2013 +0100 gpu: host1x: Port the HDMI vendor infoframe code the common helpers v2: Change oui to be an unsigned int (Ville Syrjälä) Signed-off-by: Damien Lespiau Reviewed-by: Ville Syrjälä --- drivers/video/hdmi.c | 45 + include/linux/hdmi.h | 24 2 files changed, 21 insertions(+), 48 deletions(-) diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index 59c4748..7ae4f80 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -298,6 +298,7 @@ int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame) frame->type = HDMI_INFOFRAME_TYPE_VENDOR; frame->version = 1; + frame->oui = HDMI_IDENTIFIER; /* 0 is a valid value for s3d_struct, so we use a special "not set" * value */ frame->s3d_struct = HDMI_3D_STRUCTURE_INVALID; @@ -373,46 +374,18 @@ ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, } EXPORT_SYMBOL(hdmi_hdmi_infoframe_pack); -/** - * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary - *buffer - * @frame: HDMI vendor infoframe - * @buffer: destination buffer - * @size: size of buffer - * - * Packs the information contained in the @frame structure into a binary - * representation that can be written into the corresponding controller - * registers. Also computes the checksum as required by section 5.3.5 of - * the HDMI 1.4 specification. - * - * Returns the number of bytes packed into the binary buffer or a negative - * error code on failure. +/* + * hdmi_vendor_infoframe_pack() - write a vendor infoframe to binary buffer */ -ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, - void *buffer, size_t size) +static ssize_t hdmi_vendor_infoframe_pack(union hdmi_vendor_infoframe *frame, + void *buffer, size_t size) { - u8 *ptr = buffer; - size_t length; - - length = HDMI_INFOFRAME_HEADER_SIZE + frame->length; - - if (size < length) - return -ENOSPC; - - memset(buffer, 0, size); - - ptr[0] = frame->type; - ptr[1] = frame->version; - ptr[2] = frame->length; - ptr[3] = 0; /* checksum */ - - memcpy(&ptr[HDMI_INFOFRAME_HEADER_SIZE], frame->data, frame->length); - - hdmi_infoframe_checksum(buffer, length); + /* we only know about HDMI vendor infoframes */ + if (frame->any.oui != HDMI_IDENTIFIER) + return -EINVAL; - return length; + return hdmi_hdmi_infoframe_pack(&frame->hdmi, buffer, size); } -EXPORT_SYMBOL(hdmi_vendor_infoframe_pack); /** * hdmi_infoframe_pack() - write a HDMI infoframe to binary buffer diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index 37e0cd7..e24d850 100644 --- a/include/linux/hdmi.h +++ b/include/linux/hdmi.h @@ -225,16 +225,6 @@ int hdmi_audio_infoframe_init(struct hdmi_audio_infoframe *frame); ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame, void *buffer, size_t size); -struct hdmi_vendor_infoframe { - enum hdmi_infoframe_type type; - unsigned char version; - unsigned char length; - u8 data[27]; -}; - -ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, - void *buffer, size_t size); - enum hdmi_3d_structure { HDMI_3D_STRUCTURE_INVALID = -1, HDMI_3D_STRUCTURE_FRAME_PACKING = 0, @@ -251,6 +241,7 @@ struct hdmi_hdmi_infoframe { enum hdmi_infoframe_type type; unsigned char version; unsigned char length; + unsigned int oui; u8 vic; enum hdmi_3d_structure s3d_struct; unsigned int s3d_ext_data; @@ -260,12 +251,21 @@ int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame); ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, void *buffer, size_t size); +union hdmi_vendor_infoframe { + struct { + enum hdmi_infoframe_type type; + unsigned char version; + unsigned char length; + unsigned int oui; + } any; + struct hdmi_hdmi_infoframe hdmi; +}; + union hdmi_infoframe { struct hdmi_any_infoframe any; struct hdmi_avi_infoframe avi; struct hdmi_spd_infoframe spd; - struct hdmi_vendor_infoframe vendor; - struct hdmi_hdmi_infoframe hdmi; + union hdmi_vendor_infoframe vendor; struct hdmi_audio_infof
[Intel-gfx] [PATCH 08/12] gpu: host1x: Port the HDMI vendor infoframe code the common helpers
I just wrote the bits to define and pack HDMI vendor specific infoframe. Port the host1x driver to use those so I can refactor the infoframe code a bit more. This changes the length of the infoframe payload from 6 to 5, which is enough for the "frame packing" stereo format. v2: Pimp up the commit message with the note about the length (Ville Syrjälä) Cc: Thierry Reding Cc: Terje Bergström Cc: linux-te...@vger.kernel.org Signed-off-by: Damien Lespiau --- drivers/gpu/host1x/drm/hdmi.c | 24 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/host1x/drm/hdmi.c b/drivers/gpu/host1x/drm/hdmi.c index 01097da..b548918 100644 --- a/drivers/gpu/host1x/drm/hdmi.c +++ b/drivers/gpu/host1x/drm/hdmi.c @@ -539,7 +539,7 @@ static void tegra_hdmi_setup_audio_infoframe(struct tegra_hdmi *hdmi) static void tegra_hdmi_setup_stereo_infoframe(struct tegra_hdmi *hdmi) { - struct hdmi_vendor_infoframe frame; + struct hdmi_hdmi_infoframe frame; unsigned long value; u8 buffer[10]; ssize_t err; @@ -551,26 +551,10 @@ static void tegra_hdmi_setup_stereo_infoframe(struct tegra_hdmi *hdmi) return; } - memset(&frame, 0, sizeof(frame)); + hdmi_hdmi_infoframe_init(&frame); + frame.s3d_struct = HDMI_3D_STRUCTURE_FRAME_PACKING; - frame.type = HDMI_INFOFRAME_TYPE_VENDOR; - frame.version = 0x01; - frame.length = 6; - - frame.data[0] = 0x03; /* regid0 */ - frame.data[1] = 0x0c; /* regid1 */ - frame.data[2] = 0x00; /* regid2 */ - frame.data[3] = 0x02 << 5; /* video format */ - - /* TODO: 74 MHz limit? */ - if (1) { - frame.data[4] = 0x00 << 4; /* 3D structure */ - } else { - frame.data[4] = 0x08 << 4; /* 3D structure */ - frame.data[5] = 0x00 << 4; /* 3D ext. data */ - } - - err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer)); + err = hdmi_hdmi_infoframe_pack(&frame, buffer, sizeof(buffer)); if (err < 0) { dev_err(hdmi->dev, "failed to pack vendor infoframe: %zd\n", err); -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/12] video/hdmi: Introduce helpers for the HDMI vendor specific infoframe
Provide the same programming model than the other infoframe types. The generic _pack() function can't handle those yet as we need to move the vendor OUI in the generic hdmi_vendor_infoframe structure to know which kind of vendor infoframe we are dealing with. v2: Fix the value of Side-by-side (half), hmdi typo, pack 3D_Ext_Data (Ville Syrjälä) Signed-off-by: Damien Lespiau --- drivers/video/hdmi.c | 88 include/linux/hdmi.h | 26 2 files changed, 114 insertions(+) diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index ac84215..59c4748 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -286,6 +286,94 @@ ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame, EXPORT_SYMBOL(hdmi_audio_infoframe_pack); /** + * hdmi_hdmi_infoframe_init() - initialize an HDMI vendor infoframe + * @frame: HDMI vendor infoframe + * + * Returns 0 on success or a negative error code on failure. + */ +int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame) +{ + memset(frame, 0, sizeof(*frame)); + + frame->type = HDMI_INFOFRAME_TYPE_VENDOR; + frame->version = 1; + + /* 0 is a valid value for s3d_struct, so we use a special "not set" +* value */ + frame->s3d_struct = HDMI_3D_STRUCTURE_INVALID; + + return 0; +} +EXPORT_SYMBOL(hdmi_hdmi_infoframe_init); + +/** + * hdmi_hdmi_infoframe_pack() - write a HDMI vendor infoframe to binary buffer + * @frame: HDMI infoframe + * @buffer: destination buffer + * @size: size of buffer + * + * Packs the information contained in the @frame structure into a binary + * representation that can be written into the corresponding controller + * registers. Also computes the checksum as required by section 5.3.5 of + * the HDMI 1.4 specification. + * + * Returns the number of bytes packed into the binary buffer or a negative + * error code on failure. + */ +ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, +void *buffer, size_t size) +{ + u8 *ptr = buffer; + size_t length; + + /* empty info frame */ + if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID) + return -EINVAL; + + /* only one of those can be supplied */ + if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) + return -EINVAL; + + /* for side by side (half) we also need to provide 3D_Ext_Data */ + if (frame->s3d_struct == HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) + frame->length = 6; + else + frame->length = 5; + + length = HDMI_INFOFRAME_HEADER_SIZE + frame->length; + + if (size < length) + return -ENOSPC; + + memset(buffer, 0, size); + + ptr[0] = frame->type; + ptr[1] = frame->version; + ptr[2] = frame->length; + ptr[3] = 0; /* checksum */ + + /* HDMI OUI */ + ptr[4] = 0x03; + ptr[5] = 0x0c; + ptr[6] = 0x00; + + if (frame->vic) { + ptr[7] = 0x1 << 5; /* video format */ + ptr[8] = frame->vic; + } else { + ptr[7] = 0x2 << 5; /* video format */ + ptr[8] = (frame->s3d_struct & 0xf) << 4; + if (frame->s3d_struct == HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) + ptr[9] = (frame->s3d_ext_data & 0xf) << 4; + } + + hdmi_infoframe_checksum(buffer, length); + + return length; +} +EXPORT_SYMBOL(hdmi_hdmi_infoframe_pack); + +/** * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary *buffer * @frame: HDMI vendor infoframe diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index b98340b..e733252 100644 --- a/include/linux/hdmi.h +++ b/include/linux/hdmi.h @@ -234,11 +234,37 @@ struct hdmi_vendor_infoframe { ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, void *buffer, size_t size); +enum hdmi_3d_structure { + HDMI_3D_STRUCTURE_INVALID = -1, + HDMI_3D_STRUCTURE_FRAME_PACKING = 0, + HDMI_3D_STRUCTURE_FIELD_ALTERNATIVE, + HDMI_3D_STRUCTURE_LINE_ALTERNATIVE, + HDMI_3D_STRUCTURE_SIDE_BY_SIDE_FULL, + HDMI_3D_STRUCTURE_L_DEPTH, + HDMI_3D_STRUCTURE_L_DEPTH_GFX_GFX_DEPTH, + HDMI_3D_STRUCTURE_TOP_AND_BOTTOM, + HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF = 8, +}; + +struct hdmi_hdmi_infoframe { + enum hdmi_infoframe_type type; + unsigned char version; + unsigned char length; + u8 vic; + enum hdmi_3d_structure s3d_struct; + unsigned int s3d_ext_data; +}; + +int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame); +ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, +void *buffer, size_t size); + union hdmi_infoframe { struct hdmi_any_infoframe any;
[Intel-gfx] [PATCH 06/12] video/hdmi: Derive the bar data valid bit from the bar data fields
Just like: Author: Damien Lespiau Date: Mon Aug 12 11:53:24 2013 +0100 video/hdmi: Don't let the user of this API create invalid infoframes But this time for the horizontal/vertical bar data present bits. Signed-off-by: Damien Lespiau Reviewed-by: Ville Syrjälä --- drivers/video/hdmi.c | 5 +++-- include/linux/hdmi.h | 2 -- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index e36da36..ac84215 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -101,10 +101,11 @@ ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer, if (frame->active_aspect & 0xf) ptr[0] |= BIT(4); - if (frame->horizontal_bar_valid) + /* Bit 3 and 2 indicate if we transmit horizontal/vertical bar data */ + if (frame->top_bar || frame->bottom_bar) ptr[0] |= BIT(3); - if (frame->vertical_bar_valid) + if (frame->left_bar || frame->right_bar) ptr[0] |= BIT(2); ptr[1] = ((frame->colorimetry & 0x3) << 6) | diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index 931474c6..b98340b 100644 --- a/include/linux/hdmi.h +++ b/include/linux/hdmi.h @@ -109,8 +109,6 @@ struct hdmi_avi_infoframe { unsigned char version; unsigned char length; enum hdmi_colorspace colorspace; - bool horizontal_bar_valid; - bool vertical_bar_valid; enum hdmi_scan_mode scan_mode; enum hdmi_colorimetry colorimetry; enum hdmi_picture_aspect picture_aspect; -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/12] video/hdmi: Don't let the user of this API create invalid infoframes
To set the active aspect ratio value in the AVI infoframe today, you not only have to set the active_aspect field, but also the active_info_valid bit. Out of the 1 user of this API, we had 100% misuse, forgetting the _valid bit. This was fixed in: Author: Damien Lespiau Date: Tue Aug 6 20:32:17 2013 +0100 drm: Don't generate invalid AVI infoframes for CEA modes We can do better and derive the _valid bit from the user wanting to set the active aspect ratio. Signed-off-by: Damien Lespiau Reviewed-by: Ville Syrjälä --- drivers/gpu/drm/drm_edid.c | 1 - drivers/video/hdmi.c | 4 +++- include/linux/hdmi.h | 1 - 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 2381abd..d76d608 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3259,7 +3259,6 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame, frame->video_code = drm_match_cea_mode(mode); frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE; - frame->active_info_valid = 1; frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE; return 0; diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index 635d569..e36da36 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -96,7 +96,9 @@ ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer, ptr[0] = ((frame->colorspace & 0x3) << 5) | (frame->scan_mode & 0x3); - if (frame->active_info_valid) + /* Data byte 1, bit 4 has to be set if we provide the active format +* aspect ratio */ + if (frame->active_aspect & 0xf) ptr[0] |= BIT(4); if (frame->horizontal_bar_valid) diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index bc6743e..931474c6 100644 --- a/include/linux/hdmi.h +++ b/include/linux/hdmi.h @@ -109,7 +109,6 @@ struct hdmi_avi_infoframe { unsigned char version; unsigned char length; enum hdmi_colorspace colorspace; - bool active_info_valid; bool horizontal_bar_valid; bool vertical_bar_valid; enum hdmi_scan_mode scan_mode; -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/12] drm: Add support for alternate clocks of 4k modes
v2: Fix hmdi typo (Simon Farnsworth, Ville Syrjälä) Suggested-by: Ville Syrjälä Signed-off-by: Damien Lespiau Reviewed-by: Ville Syrjälä Reviewed-by: Simon Farnsworth --- drivers/gpu/drm/drm_edid.c | 68 ++ 1 file changed, 62 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 9de573c..2381abd 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2409,6 +2409,54 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) } EXPORT_SYMBOL(drm_match_cea_mode); +/* + * Calculate the alternate clock for HDMI modes (those from the HDMI vendor + * specific block). + * + * It's almost like cea_mode_alternate_clock(), we just need to add an + * exception for the VIC 4 mode (4096x2160@24Hz): no alternate clock for this + * one. + */ +static unsigned int +hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode) +{ + if (hdmi_mode->vdisplay == 4096 && hdmi_mode->hdisplay == 2160) + return hdmi_mode->clock; + + return cea_mode_alternate_clock(hdmi_mode); +} + +/* + * drm_match_hdmi_mode - look for a HDMI mode matching given mode + * @to_match: display mode + * + * An HDMI mode is one defined in the HDMI vendor specific block. + * + * Returns the HDMI Video ID (VIC) of the mode or 0 if it isn't one. + */ +static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match) +{ + u8 mode; + + if (!to_match->clock) + return 0; + + for (mode = 0; mode < ARRAY_SIZE(edid_4k_modes); mode++) { + const struct drm_display_mode *hdmi_mode = &edid_4k_modes[mode]; + unsigned int clock1, clock2; + + /* Make sure to also match alternate clocks */ + clock1 = hdmi_mode->clock; + clock2 = hdmi_mode_alternate_clock(hdmi_mode); + + if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || +KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && + drm_mode_equal_no_clocks(to_match, hdmi_mode)) + return mode + 1; + } + return 0; +} + static int add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) { @@ -2426,18 +2474,26 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) * with the alternate clock for certain CEA modes. */ list_for_each_entry(mode, &connector->probed_modes, head) { - const struct drm_display_mode *cea_mode; + const struct drm_display_mode *cea_mode = NULL; struct drm_display_mode *newmode; - u8 cea_mode_idx = drm_match_cea_mode(mode) - 1; + u8 mode_idx = drm_match_cea_mode(mode) - 1; unsigned int clock1, clock2; - if (cea_mode_idx >= ARRAY_SIZE(edid_cea_modes)) - continue; + if (mode_idx < ARRAY_SIZE(edid_cea_modes)) { + cea_mode = &edid_cea_modes[mode_idx]; + clock2 = cea_mode_alternate_clock(cea_mode); + } else { + mode_idx = drm_match_hdmi_mode(mode) - 1; + if (mode_idx < ARRAY_SIZE(edid_4k_modes)) { + cea_mode = &edid_4k_modes[mode_idx]; + clock2 = hdmi_mode_alternate_clock(cea_mode); + } + } - cea_mode = &edid_cea_modes[cea_mode_idx]; + if (!cea_mode) + continue; clock1 = cea_mode->clock; - clock2 = cea_mode_alternate_clock(cea_mode); if (clock1 == clock2) continue; -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/12] drm/edid: Parse the HDMI CEA block and look for 4k modes
HDMI 1.4 adds 4 "4k x 2k" modes in the the CEA vendor specific block. With this commit, we now parse this block and expose the 4k modes that we find there. v2: Fix the "4096x2160" string (nice catch!), add comments about do_hdmi_vsdb_modes() arguments and make it clearer that offset is relative to the end of the required fields of the HDMI VSDB (Ville Syrjälä) v3: Fix 'Unknow' typo (Simon Farnsworth) Signed-off-by: Damien Lespiau Tested-by: Cancan Feng Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67030 Reviewed-by: Simon Farnsworth --- drivers/gpu/drm/drm_edid.c | 124 +++-- 1 file changed, 109 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index bb25ee2..9de573c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -931,6 +931,36 @@ static const struct drm_display_mode edid_cea_modes[] = { .vrefresh = 100, }, }; +/* + * HDMI 1.4 4k modes. + */ +static const struct drm_display_mode edid_4k_modes[] = { + /* 1 - 3840x2160@30Hz */ + { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, + 3840, 4016, 4104, 4400, 0, + 2160, 2168, 2178, 2250, 0, + DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC), + .vrefresh = 30, }, + /* 2 - 3840x2160@25Hz */ + { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, + 3840, 4896, 4984, 5280, 0, + 2160, 2168, 2178, 2250, 0, + DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC), + .vrefresh = 25, }, + /* 3 - 3840x2160@24Hz */ + { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, + 3840, 5116, 5204, 5500, 0, + 2160, 2168, 2178, 2250, 0, + DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC), + .vrefresh = 24, }, + /* 4 - 4096x2160@24Hz (SMPTE) */ + { DRM_MODE("4096x2160", DRM_MODE_TYPE_DRIVER, 297000, + 4096, 5116, 5204, 5500, 0, + 2160, 2168, 2178, 2250, 0, + DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC), + .vrefresh = 24, }, +}; + /*** DDC fetch and block validation ***/ static const u8 edid_header[] = { @@ -2465,6 +2495,68 @@ do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len) return modes; } +/* + * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block + * @connector: connector corresponding to the HDMI sink + * @db: start of the CEA vendor specific block + * @len: length of the CEA block payload, ie. one can access up to db[len] + * + * Parses the HDMI VSDB looking for modes to add to @connector. + */ +static int +do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len) +{ + struct drm_device *dev = connector->dev; + int modes = 0, offset = 0, i; + u8 vic_len; + + if (len < 8) + goto out; + + /* no HDMI_Video_Present */ + if (!(db[8] & (1 << 5))) + goto out; + + /* Latency_Fields_Present */ + if (db[8] & (1 << 7)) + offset += 2; + + /* I_Latency_Fields_Present */ + if (db[8] & (1 << 6)) + offset += 2; + + /* the declared length is not long enough for the 2 first bytes +* of additional video format capabilities */ + offset += 2; + if (len < (8 + offset)) + goto out; + + vic_len = db[8 + offset] >> 5; + + for (i = 0; i < vic_len && len >= (9 + offset + i); i++) { + struct drm_display_mode *newmode; + u8 vic; + + vic = db[9 + offset + i]; + + vic--; /* VICs start at 1 */ + if (vic >= ARRAY_SIZE(edid_4k_modes)) { + DRM_ERROR("Unknown HDMI VIC: %d\n", vic); + continue; + } + + newmode = drm_mode_duplicate(dev, &edid_4k_modes[vic]); + if (!newmode) + continue; + + drm_mode_probed_add(connector, newmode); + modes++; + } + +out: + return modes; +} + static int cea_db_payload_len(const u8 *db) { @@ -2496,6 +2588,21 @@ cea_db_offsets(const u8 *cea, int *start, int *end) return 0; } +static bool cea_db_is_hdmi_vsdb(const u8 *db) +{ + int hdmi_id; + + if (cea_db_tag(db) != VENDOR_BLOCK) + return false; + + if (cea_db_payload_len(db) < 5) + return false; + + hdmi_id = db[1] | (db[2] << 8) | (db[3] << 16); + + return hdmi_id == HDMI_IDENTIFIER; +} + #define for_each_cea_db(cea, i, start, end) \ for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1) @@ -2519,6 +2626,8 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid) if (cea_db_tag(db) == VIDE
[Intel-gfx] [PATCH 02/12] drm/edid: Fix add_cea_modes() style issues
A few styles issues have crept in here, fix them before touching this code again. v2: constify arguments that can be (Ville Syrjälä) v3: constify, but better (Ville Syrjälä) Signed-off-by: Damien Lespiau --- drivers/gpu/drm/drm_edid.c | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index e014785..bb25ee2 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2441,10 +2441,11 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) } static int -do_cea_modes (struct drm_connector *connector, u8 *db, u8 len) +do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len) { struct drm_device *dev = connector->dev; - u8 * mode, cea_mode; + const u8 *mode; + u8 cea_mode; int modes = 0; for (mode = db; mode < db + len; mode++) { @@ -2501,8 +2502,9 @@ cea_db_offsets(const u8 *cea, int *start, int *end) static int add_cea_modes(struct drm_connector *connector, struct edid *edid) { - u8 * cea = drm_find_cea_extension(edid); - u8 * db, dbl; + const u8 *cea = drm_find_cea_extension(edid); + const u8 *db; + u8 dbl; int modes = 0; if (cea && cea_revision(cea) >= 3) { @@ -2516,7 +2518,7 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid) dbl = cea_db_payload_len(db); if (cea_db_tag(db) == VIDEO_BLOCK) - modes += do_cea_modes (connector, db+1, dbl); + modes += do_cea_modes(connector, db + 1, dbl); } } -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/12] drm: Don't export drm_find_cea_extension() any more
This function is only used inside drm_edid.c. Signed-off-by: Damien Lespiau Reviewed-by: Ville Syrjälä --- drivers/gpu/drm/drm_edid.c | 5 ++--- include/drm/drm_crtc.h | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index dfc7a1b..e014785 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2298,10 +2298,10 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, #define EDID_CEA_YCRCB422 (1 << 4) #define EDID_CEA_VCDB_QS (1 << 6) -/** +/* * Search EDID for CEA extension block. */ -u8 *drm_find_cea_extension(struct edid *edid) +static u8 *drm_find_cea_extension(struct edid *edid) { u8 *edid_ext = NULL; int i; @@ -2322,7 +2322,6 @@ u8 *drm_find_cea_extension(struct edid *edid) return edid_ext; } -EXPORT_SYMBOL(drm_find_cea_extension); /* * Calculate the alternate clock for the CEA mode diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..f3ecc6f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1050,7 +1050,6 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern int drm_mode_gamma_set_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); -extern u8 *drm_find_cea_extension(struct edid *edid); extern u8 drm_match_cea_mode(const struct drm_display_mode *to_match); extern bool drm_detect_hdmi_monitor(struct edid *edid); extern bool drm_detect_monitor_audio(struct edid *edid); -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] HDMI 4k support v3
Follow up on v2: http://lists.freedesktop.org/archives/dri-devel/2013-August/043604.html With the quick and nice reviews from Ville and Simon, it's time for a v3: - Fix embarrassing hmdi typo - Fix the sending of vendor specific infoframes for side-by-side half modes - Smaller changes here and there. -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] VGA arbiter support for Intel HD?
On Wed, 2013-08-14 at 17:47 +0300, Ville Syrjälä wrote: > On Wed, Aug 14, 2013 at 07:23:57AM -0600, Alex Williamson wrote: > > Hi, > > > > I'm trying to add support for device assignment of PCI VGA devices with > > VFIO and QEMU. For normal, discrete discrete graphics the Linux VGA > > arbiter works fairly well, disabling VGA on one bridge and adding it to > > another (though I wish all the kernel VGA drivers made use of it). The > > i915 driver only seems to support disabling VGA on really old GMCH > > devices (see intel_modeset_vga_set_state). This means that if I boot > > with IGD as the primary graphics and attempt to assign a discrete > > graphics device, all the VGA range accesses are still routed to IGD, I > > end up getting some error messages from the IGD interrupt handler, and > > the discrete card never initializes. > > > > I spent some time looking through the Sand Bridge, Ivy Bridge, and > > Haswell datasheets, and I'm a bit concerned whether the hardware even > > provides a reasonable way to disable VGA anymore. Quoting 2.17 from the > > Haswell docs: > > > > Accesses to the VGA memory range are directed to IGD depend on > > the configuration. The configuration is specified by: > > * Internal graphics controller in Device 2 is enabled > > (DEVEN.D2EN bit 4) > > * Internal graphics VGA in Device 0 Function 0 is enabled > > through register GGC bit 1. > > * IGD's memory accesses (PCICMD2 04 – 05h, MAE bit 1) in > > Device 2 configuration space are enabled. > > * VGA compatibility memory accesses (VGA Miscellaneous > > Output register – MSR Register, bit 1) are enabled. > > * Software sets the proper value for VGA Memory Map Mode > > register (VGA GR06 Register, bits 3-2). See the > > following table for translations. > > > > (There's a similar list for VGA I/O range) I've found that if I disable > > memory and I/O in the PCI command register for IGD then I do get VGA > > routing to the PEG device and the discrete VBIOS works. This obviously > > isn't a good option for the VGA arbiter since it entirely disables IGD. > > > > The GGC registers aren't meant for runtime switching and are actually > > locked. Disabling IGD via the device 2 enable bit doesn't seem like and > > option. I don't quite understand the VGA miscellaneous output register > > and VGA memory map mode, but the table provided for the latter makes me > > think they just augment the VGA ranges and don't disable them. > > Bit 1 of MSR (0x3c2/0x3cc) should allow you to turn off VGA mem > access while leaving other memory space access working. > > As for VGA I/O decode, IIRC there's no standard bit for that in VGA > or PCI config registers, and I can't see any other bit for it in the > docs. But I guess you could just turn off I/O space completely > via the PCI_COMMAND register. We shouldn't need it for anything beyond > i915_disable_vga() and that has the necessary vgaarb calls already. Thanks Ville. The MSR seems to work for VGA memory. Disabling I/O via PCI_COMMAND does works, but something is re-enabling it after intel_modeset_vga_set_state(). If I manually disable I/O with setpci then I do have VGA routing to PEG and can still interact with the KMS console on IGD. It's unfortunate that the MSR bit for I/O only disables pieces of the range. If we have no other options, I'll try to hunt down where I/O is being re-enabled and see how feasible it is to avoid. Thanks, Alex ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: inline vma_create into lookup_or_create_vma
On Wed, Aug 14, 2013 at 11:59:09AM +0200, Daniel Vetter wrote: > In the execbuf code we don't clean up any vmas which ended up not > getting bound for code simplicity. To make sure that we don't end up > creating multiple vma for the same vm kill the somewhat dangerous > vma_create function and inline it into lookup_or_create. > > This is just a safety measure to prevent surprises in the future. > > Also update the somewhat confused comment in the execbuf code and > clarify what kind of magic is going on with a new one. > > Cc: Ben Widawsky > Signed-off-by: Daniel Vetter > --- > > That's the only concern I could come up with when reading the execbuf > vma conversion patch. So looks good and I'll slurp it all in as soon > as some more head scratching is done for the very first patch in this > series about the vma_unbind fix to only call vma_destroy if the vma > isn't bound. One thing I've noticed but forgot to mention here is that the reloc code still uses obj_ggtt_size/offset. I guess that will be fixed later on? -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
[Intel-gfx] Improved IGT support for text fixtures
Hi all, So while wreaking havoc with igt tests and adding some nice infrastructure to handle subtest status reporting a bit better I've also added support to annotate the common test fixture code. And rolled it out over all tests. I plan to do a decent write up on all the new infrastructure and how to use it when writing testcases, but the immediate benefit is that enumerating testcases now doesn't need to be done on a system with an intel gpu any more. Furthermore (and that's the reason I've done this) we can also finally enumerate the prime_nv subtests without the need for an nvidia gpu. Yang guang: Can you please check that everything works correctly on the QA setup? Right now we should have 303 igt tests when enumerating them with piglit (i.e. subtests included). Cheers, 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 1/2] drm/i915: check the power well when redisabling VGA
On Mon, Aug 05, 2013 at 05:46:12PM +0200, Daniel Vetter wrote: > On Fri, Aug 02, 2013 at 04:22:24PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni > > > > If the power well is disabled VGA is guaranteed to be disabled. > > > > This fixes unclaimed register messages that happen on suspend/resume. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67517 > > Signed-off-by: Paulo Zanoni > > Have you seen my other comment asking whether we could just force-enable > the power well a bit earlier in the resume sequence instead? Imo that > feels a notch more robust, since we didn't sprinkle tons of power well > checks in all the other hw readout paths either. Ok, I've merged this one here (plus comments and the follow-up patch) instead of the other one. -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] VGA arbiter support for Intel HD?
On Wed, Aug 14, 2013 at 07:23:57AM -0600, Alex Williamson wrote: > Hi, > > I'm trying to add support for device assignment of PCI VGA devices with > VFIO and QEMU. For normal, discrete discrete graphics the Linux VGA > arbiter works fairly well, disabling VGA on one bridge and adding it to > another (though I wish all the kernel VGA drivers made use of it). The > i915 driver only seems to support disabling VGA on really old GMCH > devices (see intel_modeset_vga_set_state). This means that if I boot > with IGD as the primary graphics and attempt to assign a discrete > graphics device, all the VGA range accesses are still routed to IGD, I > end up getting some error messages from the IGD interrupt handler, and > the discrete card never initializes. > > I spent some time looking through the Sand Bridge, Ivy Bridge, and > Haswell datasheets, and I'm a bit concerned whether the hardware even > provides a reasonable way to disable VGA anymore. Quoting 2.17 from the > Haswell docs: > > Accesses to the VGA memory range are directed to IGD depend on > the configuration. The configuration is specified by: > * Internal graphics controller in Device 2 is enabled > (DEVEN.D2EN bit 4) > * Internal graphics VGA in Device 0 Function 0 is enabled > through register GGC bit 1. > * IGD's memory accesses (PCICMD2 04 – 05h, MAE bit 1) in > Device 2 configuration space are enabled. > * VGA compatibility memory accesses (VGA Miscellaneous > Output register – MSR Register, bit 1) are enabled. > * Software sets the proper value for VGA Memory Map Mode > register (VGA GR06 Register, bits 3-2). See the > following table for translations. > > (There's a similar list for VGA I/O range) I've found that if I disable > memory and I/O in the PCI command register for IGD then I do get VGA > routing to the PEG device and the discrete VBIOS works. This obviously > isn't a good option for the VGA arbiter since it entirely disables IGD. > > The GGC registers aren't meant for runtime switching and are actually > locked. Disabling IGD via the device 2 enable bit doesn't seem like and > option. I don't quite understand the VGA miscellaneous output register > and VGA memory map mode, but the table provided for the latter makes me > think they just augment the VGA ranges and don't disable them. Bit 1 of MSR (0x3c2/0x3cc) should allow you to turn off VGA mem access while leaving other memory space access working. As for VGA I/O decode, IIRC there's no standard bit for that in VGA or PCI config registers, and I can't see any other bit for it in the docs. But I guess you could just turn off I/O space completely via the PCI_COMMAND register. We shouldn't need it for anything beyond i915_disable_vga() and that has the necessary vgaarb calls already. -- 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: VGA also requires the power well
On Mon, Aug 12, 2013 at 06:06:48PM +, Zanoni, Paulo R wrote: > > -Original Message- > > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel > > Vetter > > Sent: Sunday, August 04, 2013 4:50 PM > > To: Paulo Zanoni > > Cc: Ville Syrjälä; intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: VGA also requires the power well > > > > On Fri, Aug 02, 2013 at 02:17:53PM -0300, Paulo Zanoni wrote: > > > 2013/6/6 Ville Syrjälä : > > > > On Wed, Jun 05, 2013 at 06:05:51PM -0300, Paulo Zanoni wrote: > > > >> From: Paulo Zanoni > > > >> > > > >> So add a power domain and check for it before we try to read > > > >> VGA_CONTROL. > > > >> > > > >> This fixes unclaimed register messages that happen on > > suspend/resume. > > > >> > > > >> Signed-off-by: Paulo Zanoni > > > >> --- > > > >> drivers/gpu/drm/i915/i915_drv.h | 1 + > > > >> drivers/gpu/drm/i915/intel_display.c | 3 +++ > > > >> drivers/gpu/drm/i915/intel_pm.c | 1 + > > > >> 3 files changed, 5 insertions(+) > > > >> > > > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > > >> index 46b1f70..d51ce13 100644 > > > >> --- a/drivers/gpu/drm/i915/i915_drv.h > > > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > > > >> @@ -89,6 +89,7 @@ enum port { > > > >> #define port_name(p) ((p) + 'A') > > > >> > > > >> enum intel_display_power_domain { > > > >> + POWER_DOMAIN_VGA, > > > >> POWER_DOMAIN_PIPE_A, > > > >> POWER_DOMAIN_PIPE_B, > > > >> POWER_DOMAIN_PIPE_C, > > > >> diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > > >> index 4c8fcec..3719d99 100644 > > > >> --- a/drivers/gpu/drm/i915/intel_display.c > > > >> +++ b/drivers/gpu/drm/i915/intel_display.c > > > >> @@ -9950,6 +9950,9 @@ void i915_redisable_vga(struct drm_device > > *dev) > > > >> struct drm_i915_private *dev_priv = dev->dev_private; > > > >> u32 vga_reg = i915_vgacntrl_reg(dev); > > > >> > > > >> + if (!intel_display_power_enabled(dev, POWER_DOMAIN_VGA)) > > > >> + return; > > > >> + > > > > > > > > So it looks like you're essentially making intel_redisable_vga() a nop > > > > for HSW. Shouldn't we instead enable the power well during resume? > > > > > > We enable the power well during resume, but it's only after this > > > function... > > > > Hm, so better move the (temporary) power well enabling in the resume > > code > > up a bit to cover this? > > I don't think so. If you look at i915_redisable_vga and commit > 0fde901f1ddd2ce0e380a6444f1fb7ca555859e9, you will start thinking that > just closing/opening the lid could make the BIOS somehow enable the > power well and then reenable VGA, so moving the check to "earlier in the > resume sequence" won't solve any problems, as VGA could be reenabled > later. Well that's only for machines without opregion afaik. But we lack the check for that, which I didn't really realize. To avoid blocking this even longer I've just merged this patch here with a big note added to the commit message. 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 04/14] drm/i915: add MIPI DSI register definitions
On Tue, Aug 13, 2013 at 04:29:43PM +0300, Jani Nikula wrote: > Add definitions for VLV MIPI DSI registers. > > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/i915/i915_reg.h | 409 > +++ > 1 file changed, 409 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index aced53a..32e32b6 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -5104,4 +5104,413 @@ > #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, > _PIPE_B_CSC_POSTOFF_ME) > #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, > _PIPE_B_CSC_POSTOFF_LO) > > +/* VLV MIPI registers */ > + > +/* XXX: This register seems very messy. MIPIB has only enable and delay. */ > +#define _MIPIA_PORT_CTRL (VLV_DISPLAY_BASE + 0x61190) > +#define _MIPIB_PORT_CTRL (VLV_DISPLAY_BASE + 0x61700) > +#define MIPI_PORT_CTRL(pipe) _PIPE(pipe, _MIPIA_PORT_CTRL, > _MIPIB_PORT_CTRL) > +#define DPI_ENABLE (1 << 31) /* A + B */ > +#define MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT 27 > +#define MIPIA_MIPI4DPHY_DELAY_COUNT_MASK(0xf << 27) > +#define DUAL_LINK_MODE_MASK (1 << 26) > +#define DUAL_LINK_MODE_FRONT_BACK (0 << 26) > +#define DUAL_LINK_MODE_PIXEL_ALTERNATIVE(1 << 26) 25 is "Dither Enable" according to my docs. > +#define FLOPPED_HSTX(1 << 23) > +#define DE_INVERT (1 << 19) /* XXX */ One doc has DE_INVERT (and HS/VS invert and border enable bits), another has the FLISDSI stuff. I guess that's the XXX here. > +#define MIPIA_FLISDSI_DELAY_COUNT_SHIFT 18 > +#define MIPIA_FLISDSI_DELAY_COUNT_MASK (0xf << 18) > +#define AFE_LATCHOUT(1 << 17) 16 is "LPOutput Hold" > +#define MIPIB_FLISDSI_DELAY_COUNT_HIGH_SHIFT15 > +#define MIPIB_FLISDSI_DELAY_COUNT_HIGH_MASK (1 << 15) > +#define MIPIB_MIPI4DPHY_DELAY_COUNT_SHIFT 11 > +#define MIPIB_MIPI4DPHY_DELAY_COUNT_MASK(0xf << 11) > +#define CSB_SHIFT 9 > +#define CSB_MASK(3 << 9) > +#define CSB_20MHZ (0 << 9) > +#define CSB_10MHZ (1 << 9) > +#define CSB_40MHZ (2 << 9) > +#define BANDGAP_MASK(1 << 8) > +#define BANDGAP_PNW_CIRCUIT (0 << 8) > +#define BANDGAP_LNC_CIRCUIT (1 << 8) > +#define MIPIB_FLISDSI_DELAY_COUNT_LOW_SHIFT 5 > +#define MIPIB_FLISDSI_DELAY_COUNT_LOW_MASK (7 << 5) > +#define TEARING_EFFECT_DELAY(1 << 4) /* A + > B */ > +#define TEARING_EFFECT_SHIFT2 /* A + B */ > +#define TEARING_EFFECT_MASK (3 << 2) > +#define TEARING_EFFECT_OFF (0 << 2) > +#define TEARING_EFFECT_DSI (1 << 2) > +#define TEARING_EFFECT_GPIO (2 << 2) > +#define LANE_CONFIGURATION_SHIFT0 > +#define LANE_CONFIGURATION_MASK (3 << 0) > +#define LANE_CONFIGURATION_4LANE(0 << 0) > +#define LANE_CONFIGURATION_DUAL_LINK_A (1 << 0) > +#define LANE_CONFIGURATION_DUAL_LINK_B (2 << 0) > + > +#define _MIPIA_TEARING_CTRL (VLV_DISPLAY_BASE + 0x61194) > +#define _MIPIB_TEARING_CTRL (VLV_DISPLAY_BASE + 0x61704) > +#define MIPI_TEARING_CTRL(pipe) _PIPE(pipe, > _MIPIA_TEARING_CTRL, _MIPIB_TEARING_CTRL) > +#define TEARING_EFFECT_DELAY_SHIFT 0 > +#define TEARING_EFFECT_DELAY_MASK (0x << 0) > + > +/* XXX */ > +#define _MIPIA_AUTOPWG (VLV_DISPLAY_BASE + > 0x611a0) Is the XXX here because the spec says the contents are all "reserved"? > + > +/* MIPI DSI Controller and D-PHY registers */ > + > +#define _MIPIA_DEVICE_READY (VLV_DISPLAY_BASE + 0xb000) > +#define _MIPIB_DEVICE_READY (VLV_DISPLAY_BASE + 0xb800) > +#define MIPI_DEVICE_READY(pipe) _PIPE(pipe, > _MIPIA_DEVICE_READY, _MIPIB_DEVICE_READY) > +#define BUS_POSSESSION (1 << 3) /* set > to give bus to receiver */ > +#define ULPS_STATE_MASK (3 << 1) > +#define ULPS_STATE_ENTER(2 << 1) /* XXX */ > +#define ULPS_STATE_EXIT (1 << 1) /* XXX */ Maybe we should have the (0 << 1) "normal operation" listed here as well? > +#define DEVICE_READY
Re: [Intel-gfx] [PATCH] drm/i915: Dump the contents of the GTT
On Wed, Aug 14, 2013 at 3:44 PM, Chris Wilson wrote: > On Wed, Aug 14, 2013 at 03:02:37PM +0200, Sedat Dilek wrote: >> On Wed, Aug 14, 2013 at 3:01 PM, Sedat Dilek wrote: >> > On Wed, Aug 14, 2013 at 2:52 PM, Chris Wilson >> > wrote: >> >> On Wed, Aug 14, 2013 at 02:31:24PM +0200, Sedat Dilek wrote: >> >>> Requested output and dmesg files attached. >> >>> ( Should I attach the one with "BROKEN" display - I mean w/o doing a >> >>> re-login and display "REPAIRED"? ) >> >> >> >> Yes please. >> > >> > Attached tarball contains: >> > >> > 130413 Aug 14 14:52 >> > dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_0-before-relogin-lightdm-unity2d_VT-1.txt >> > 145973 Aug 14 14:53 >> > dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt >> > 199199 Aug 14 14:55 >> > dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt >> > >> > 79398 Aug 14 14:53 >> > i915_gem_gtt_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt >> > 108374 Aug 14 14:55 >> > i915_gem_gtt_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt > > The PTE values do correspond with the stolen addresses for the > framebuffer. So I am resonably confident that the driver is > self-consistent at least. DSPSURF points to the right location in the > GGTT and those entries point to the right location in stolen. The > display on the other hand seems to be continuing to read from GGTT > offset 0, and so the real framebuffer appears offset by a few lines. > > How about trying https://patchwork.kernel.org/patch/2841934/ ? The same screen corruption (see also attached tarball). - Sedat - for-ickle-3.tar.xz Description: Binary data for-ickle-3.tar.xz.sha256sum Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Dump the contents of the GTT
On Wed, Aug 14, 2013 at 03:02:37PM +0200, Sedat Dilek wrote: > On Wed, Aug 14, 2013 at 3:01 PM, Sedat Dilek wrote: > > On Wed, Aug 14, 2013 at 2:52 PM, Chris Wilson > > wrote: > >> On Wed, Aug 14, 2013 at 02:31:24PM +0200, Sedat Dilek wrote: > >>> Requested output and dmesg files attached. > >>> ( Should I attach the one with "BROKEN" display - I mean w/o doing a > >>> re-login and display "REPAIRED"? ) > >> > >> Yes please. > > > > Attached tarball contains: > > > > 130413 Aug 14 14:52 > > dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_0-before-relogin-lightdm-unity2d_VT-1.txt > > 145973 Aug 14 14:53 > > dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt > > 199199 Aug 14 14:55 > > dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt > > > > 79398 Aug 14 14:53 > > i915_gem_gtt_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt > > 108374 Aug 14 14:55 > > i915_gem_gtt_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt The PTE values do correspond with the stolen addresses for the framebuffer. So I am resonably confident that the driver is self-consistent at least. DSPSURF points to the right location in the GGTT and those entries point to the right location in stolen. The display on the other hand seems to be continuing to read from GGTT offset 0, and so the real framebuffer appears offset by a few lines. How about trying https://patchwork.kernel.org/patch/2841934/ ? -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: Dump the contents of the GTT
On Wed, Aug 14, 2013 at 3:02 PM, Sedat Dilek wrote: > On Wed, Aug 14, 2013 at 3:01 PM, Sedat Dilek wrote: >> On Wed, Aug 14, 2013 at 2:52 PM, Chris Wilson >> wrote: >>> On Wed, Aug 14, 2013 at 02:31:24PM +0200, Sedat Dilek wrote: Requested output and dmesg files attached. ( Should I attach the one with "BROKEN" display - I mean w/o doing a re-login and display "REPAIRED"? ) >>> >>> Yes please. >> >> Attached tarball contains: >> >> 130413 Aug 14 14:52 >> dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_0-before-relogin-lightdm-unity2d_VT-1.txt >> 145973 Aug 14 14:53 >> dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt >> 199199 Aug 14 14:55 >> dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt >> >> 79398 Aug 14 14:53 >> i915_gem_gtt_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt >> 108374 Aug 14 14:55 >> i915_gem_gtt_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt >> >> > > The real attachment. > A new tarball with... 1. booting into text-mode (no graphical mode, no X) 2. start lightdm service (upstart) 3. start unity-2d session and logout (re-enter lightdm greeter screen) - Sedat - for-ickle-2.tar.xz Description: Binary data for-ickle-2.tar.xz.sha256sum Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] VGA arbiter support for Intel HD?
Hi, I'm trying to add support for device assignment of PCI VGA devices with VFIO and QEMU. For normal, discrete discrete graphics the Linux VGA arbiter works fairly well, disabling VGA on one bridge and adding it to another (though I wish all the kernel VGA drivers made use of it). The i915 driver only seems to support disabling VGA on really old GMCH devices (see intel_modeset_vga_set_state). This means that if I boot with IGD as the primary graphics and attempt to assign a discrete graphics device, all the VGA range accesses are still routed to IGD, I end up getting some error messages from the IGD interrupt handler, and the discrete card never initializes. I spent some time looking through the Sand Bridge, Ivy Bridge, and Haswell datasheets, and I'm a bit concerned whether the hardware even provides a reasonable way to disable VGA anymore. Quoting 2.17 from the Haswell docs: Accesses to the VGA memory range are directed to IGD depend on the configuration. The configuration is specified by: * Internal graphics controller in Device 2 is enabled (DEVEN.D2EN bit 4) * Internal graphics VGA in Device 0 Function 0 is enabled through register GGC bit 1. * IGD's memory accesses (PCICMD2 04 – 05h, MAE bit 1) in Device 2 configuration space are enabled. * VGA compatibility memory accesses (VGA Miscellaneous Output register – MSR Register, bit 1) are enabled. * Software sets the proper value for VGA Memory Map Mode register (VGA GR06 Register, bits 3-2). See the following table for translations. (There's a similar list for VGA I/O range) I've found that if I disable memory and I/O in the PCI command register for IGD then I do get VGA routing to the PEG device and the discrete VBIOS works. This obviously isn't a good option for the VGA arbiter since it entirely disables IGD. The GGC registers aren't meant for runtime switching and are actually locked. Disabling IGD via the device 2 enable bit doesn't seem like and option. I don't quite understand the VGA miscellaneous output register and VGA memory map mode, but the table provided for the latter makes me think they just augment the VGA ranges and don't disable them. Is it possible to support the VGA arbiter with the latest IGD? Is anyone working on it? Thanks, Alex ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Dump the contents of the GTT
On Wed, Aug 14, 2013 at 3:01 PM, Sedat Dilek wrote: > On Wed, Aug 14, 2013 at 2:52 PM, Chris Wilson > wrote: >> On Wed, Aug 14, 2013 at 02:31:24PM +0200, Sedat Dilek wrote: >>> Requested output and dmesg files attached. >>> ( Should I attach the one with "BROKEN" display - I mean w/o doing a >>> re-login and display "REPAIRED"? ) >> >> Yes please. > > Attached tarball contains: > > 130413 Aug 14 14:52 > dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_0-before-relogin-lightdm-unity2d_VT-1.txt > 145973 Aug 14 14:53 > dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt > 199199 Aug 14 14:55 > dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt > > 79398 Aug 14 14:53 > i915_gem_gtt_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt > 108374 Aug 14 14:55 > i915_gem_gtt_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt > > The real attachment. - Sedat - for-ickle.tar.xz.sha256sum Description: Binary data for-ickle.tar.xz Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Dump the contents of the GTT
On Wed, Aug 14, 2013 at 2:52 PM, Chris Wilson wrote: > On Wed, Aug 14, 2013 at 02:31:24PM +0200, Sedat Dilek wrote: >> Requested output and dmesg files attached. >> ( Should I attach the one with "BROKEN" display - I mean w/o doing a >> re-login and display "REPAIRED"? ) > > Yes please. Attached tarball contains: 130413 Aug 14 14:52 dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_0-before-relogin-lightdm-unity2d_VT-1.txt 145973 Aug 14 14:53 dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt 199199 Aug 14 14:55 dmesg_3.11.0-rc5-next20130814-1-iniza-small_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt 79398 Aug 14 14:53 i915_gem_gtt_drm-debug-6_1-before-relogin-lightdm-unity2d_X.txt 108374 Aug 14 14:55 i915_gem_gtt_drm-debug-6_2_after-relogin-lightdm-unity2d_X.txt - Sedat - ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Dump the contents of the GTT
On Wed, Aug 14, 2013 at 02:31:24PM +0200, Sedat Dilek wrote: > Requested output and dmesg files attached. > ( Should I attach the one with "BROKEN" display - I mean w/o doing a > re-login and display "REPAIRED"? ) Yes please. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: inline vma_create into lookup_or_create_vma
In the execbuf code we don't clean up any vmas which ended up not getting bound for code simplicity. To make sure that we don't end up creating multiple vma for the same vm kill the somewhat dangerous vma_create function and inline it into lookup_or_create. This is just a safety measure to prevent surprises in the future. Also update the somewhat confused comment in the execbuf code and clarify what kind of magic is going on with a new one. v2: Keep the function separate as requested by Chris. But give it a __ prefix for paranoia and move it tighter together with the other vma stuff. Cc: Chris Wilson Cc: Ben Widawsky Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.h| 2 -- drivers/gpu/drm/i915/i915_gem.c| 50 +++--- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +-- drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- 4 files changed, 34 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index fc35ae3..b06a90f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1748,8 +1748,6 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, size_t size); void i915_gem_free_object(struct drm_gem_object *obj); -struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, -struct i915_address_space *vm); void i915_gem_vma_destroy(struct i915_vma *vma); int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 4be3be9..0832f61 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4121,9 +4121,20 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) i915_gem_object_free(obj); } -struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, struct i915_address_space *vm) { + struct i915_vma *vma; + list_for_each_entry(vma, &obj->vma_list, vma_link) + if (vma->vm == vm) + return vma; + + return NULL; +} + +static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj, + struct i915_address_space *vm) +{ struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL); if (vma == NULL) return ERR_PTR(-ENOMEM); @@ -4143,6 +4154,19 @@ struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, return vma; } +struct i915_vma * +i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj, + struct i915_address_space *vm) +{ + struct i915_vma *vma; + + vma = i915_gem_obj_to_vma(obj, vm); + if (!vma) + vma = __i915_gem_vma_create(obj, vm); + + return vma; +} + void i915_gem_vma_destroy(struct i915_vma *vma) { WARN_ON(vma->node.allocated); @@ -4869,27 +4893,3 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, return 0; } - -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, -struct i915_address_space *vm) -{ - struct i915_vma *vma; - list_for_each_entry(vma, &obj->vma_list, vma_link) - if (vma->vm == vm) - return vma; - - return NULL; -} - -struct i915_vma * -i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj, - struct i915_address_space *vm) -{ - struct i915_vma *vma; - - vma = i915_gem_obj_to_vma(obj, vm); - if (!vma) - vma = i915_gem_vma_create(obj, vm); - - return vma; -} diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index c3b36f5..9b3b5f8 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -123,11 +123,16 @@ eb_lookup_vmas(struct eb_vmas *eb, list_for_each_entry(obj, &objects, obj_exec_link) { struct i915_vma *vma; + /* +* NOTE: We can leak any vmas created here when something fails +* later on. But that's no issue since vma_unbind can deal with +* vmas which are not actually bound. And since only +* lookup_or_create exists as an interface to get at the vma +* from the (obj, vm) we don't run the risk of creating +* duplicated vmas for the same vm. +*/ vma = i915_gem_obj_lookup_or_create_vma(obj, vm); if (IS_ERR(vma)) { - /* XXX: We don't need an erro
Re: [Intel-gfx] [PATCH] drm/i915: inline vma_create into lookup_or_create_vma
On Wed, Aug 14, 2013 at 12:49:04PM +0100, Chris Wilson wrote: > On Wed, Aug 14, 2013 at 11:59:09AM +0200, Daniel Vetter wrote: > > In the execbuf code we don't clean up any vmas which ended up not > > getting bound for code simplicity. To make sure that we don't end up > > creating multiple vma for the same vm kill the somewhat dangerous > > vma_create function and inline it into lookup_or_create. > > Just make it static. It's a nice little standalone function that helps > the reader. I'm afraid that someone will use it eventually. I'll smash a __ prefix onto it to make it clear that people better don't touch it. -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 4/4] drm/i915: Convert execbuf code to use vmas
On Wed, Aug 14, 2013 at 11:38:36AM +0200, Daniel Vetter wrote: > From: Ben Widawsky > > + vma = i915_gem_obj_lookup_or_create_vma(obj, vm); > + if (IS_ERR(vma)) { > + /* XXX: We don't need an error path fro vma because if > + * the vma was created just for this execbuf, object > + * unreference should kill it off.*/ The XXX part here is that before the object is finally unreffed we have a dangling vma and so we need to take care else way not to assume that a vma is implicitly bound. -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: inline vma_create into lookup_or_create_vma
On Wed, Aug 14, 2013 at 11:59:09AM +0200, Daniel Vetter wrote: > In the execbuf code we don't clean up any vmas which ended up not > getting bound for code simplicity. To make sure that we don't end up > creating multiple vma for the same vm kill the somewhat dangerous > vma_create function and inline it into lookup_or_create. Just make it static. It's a nice little standalone function that helps the reader. -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: Dump the contents of the GTT
On Wed, Aug 14, 2013 at 1:35 PM, Chris Wilson wrote: > Chasing wild speculation that we may be writing the wrong addresses > into the GTT for stolen objects, I would like to inspect those values. > > Signed-off-by: Chris Wilson > Cc: Sedat Dilek > --- > > Sedak, can you please apply this patch and then attach the contents of > /sys/kernel/debug/dri/0/i915_gem_gtt with the broken display? > Compiling... next-20130814 with drm-intel-nightly (up to commit d93f59e86ae93066969fa8ae2a6c9ccc7fc4728d) plus this patch. How do you want your Doner-Kebap? (Going into the city...) - Sedat - > --- > drivers/gpu/drm/i915/i915_debugfs.c | 47 > - > 1 file changed, 46 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 10d864c..4edf65a 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -390,6 +390,51 @@ static int i915_gem_object_info(struct seq_file *m, > void* data) > return 0; > } > > +static int i915_gem_gtt_contents(struct seq_file *m, struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + gen6_gtt_pte_t __iomem *gtt_entries; > + gen6_gtt_pte_t scratch_pte; > + gen6_gtt_pte_t zero[8] = {}; > + int i, j, last_zero = 0; > + int ret; > + > + if (INTEL_INFO(dev)->gen < 6) > + return 0; > + > + ret = mutex_lock_interruptible(&dev->struct_mutex); > + if (ret) > + return ret; > + > + gtt_entries = (gen6_gtt_pte_t __iomem *)dev_priv->gtt.gsm; > + scratch_pte = > dev_priv->gtt.base.pte_encode(dev_priv->gtt.base.scratch.addr, > I915_CACHE_LLC); > + for (i = 0; i < gtt_total_entries(dev_priv->gtt); i += 8) { > + gen6_gtt_pte_t pte[8]; > + int this_zero; > + > + for (j = 0; j < 8; j++) { > + pte[j] = ioread32(>t_entries[i+j]); > + if (pte[j] == scratch_pte) > + pte[j] = 0; > + } > + > + this_zero = memcmp(pte, zero, sizeof(pte)) == 0; > + if (last_zero && this_zero) { > + if (last_zero++ == 1) > + seq_puts(m, "...\n"); > + continue; > + } > + > + seq_printf(m, "[%08x] %08x %08x %08x %08x %08x %08x %08x > %08x\n", > + i, pte[0], pte[1], pte[2], pte[3], pte[4], pte[5], > pte[6], pte[7]); > + last_zero = this_zero; > + } > + > + mutex_unlock(&dev->struct_mutex); > + > + return 0; > +} > + > static int i915_gem_gtt_info(struct seq_file *m, void *data) > { > struct drm_info_node *node = (struct drm_info_node *) m->private; > @@ -422,7 +467,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void > *data) > seq_printf(m, "Total %d objects, %zu bytes, %zu GTT size\n", >count, total_obj_size, total_gtt_size); > > - return 0; > + return i915_gem_gtt_contents(m, dev); > } > > static int i915_gem_pageflip_info(struct seq_file *m, void *data) > -- > 1.8.4.rc2 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Dump the contents of the GTT
Chasing wild speculation that we may be writing the wrong addresses into the GTT for stolen objects, I would like to inspect those values. Signed-off-by: Chris Wilson Cc: Sedat Dilek --- Sedak, can you please apply this patch and then attach the contents of /sys/kernel/debug/dri/0/i915_gem_gtt with the broken display? --- drivers/gpu/drm/i915/i915_debugfs.c | 47 - 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 10d864c..4edf65a 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -390,6 +390,51 @@ static int i915_gem_object_info(struct seq_file *m, void* data) return 0; } +static int i915_gem_gtt_contents(struct seq_file *m, struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + gen6_gtt_pte_t __iomem *gtt_entries; + gen6_gtt_pte_t scratch_pte; + gen6_gtt_pte_t zero[8] = {}; + int i, j, last_zero = 0; + int ret; + + if (INTEL_INFO(dev)->gen < 6) + return 0; + + ret = mutex_lock_interruptible(&dev->struct_mutex); + if (ret) + return ret; + + gtt_entries = (gen6_gtt_pte_t __iomem *)dev_priv->gtt.gsm; + scratch_pte = dev_priv->gtt.base.pte_encode(dev_priv->gtt.base.scratch.addr, I915_CACHE_LLC); + for (i = 0; i < gtt_total_entries(dev_priv->gtt); i += 8) { + gen6_gtt_pte_t pte[8]; + int this_zero; + + for (j = 0; j < 8; j++) { + pte[j] = ioread32(>t_entries[i+j]); + if (pte[j] == scratch_pte) + pte[j] = 0; + } + + this_zero = memcmp(pte, zero, sizeof(pte)) == 0; + if (last_zero && this_zero) { + if (last_zero++ == 1) + seq_puts(m, "...\n"); + continue; + } + + seq_printf(m, "[%08x] %08x %08x %08x %08x %08x %08x %08x %08x\n", + i, pte[0], pte[1], pte[2], pte[3], pte[4], pte[5], pte[6], pte[7]); + last_zero = this_zero; + } + + mutex_unlock(&dev->struct_mutex); + + return 0; +} + static int i915_gem_gtt_info(struct seq_file *m, void *data) { struct drm_info_node *node = (struct drm_info_node *) m->private; @@ -422,7 +467,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data) seq_printf(m, "Total %d objects, %zu bytes, %zu GTT size\n", count, total_obj_size, total_gtt_size); - return 0; + return i915_gem_gtt_contents(m, dev); } static int i915_gem_pageflip_info(struct seq_file *m, void *data) -- 1.8.4.rc2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] HDMI 4k support v2
On Wed, Aug 14, 2013 at 12:17:16AM +0100, Damien Lespiau wrote: > Following up the first instance of this series: > http://lists.freedesktop.org/archives/dri-devel/2013-August/043125.html > > Here is a v2 with Ville's review pass and a few additions: > - Alternate clock modes for 4k resolutions > - HDMI vendor specific infoframe support in drivers/video/hdmi.c > - Enabling of those vendor specific infoframes in the i915 driver The patches I didn't explicitly comment on (01,05,09,12 by my reckoning) are: Reviewed-by: Ville Syrjälä > Along the way, a tegra patch was needed for a small consolidation of the code > packing vendor specific infoframes. This patch has only been compile-tested. > > On Intel, it's possible to read back the programmed infoframe buffers with > intel-gpu-tools' intel_infoframe and this gives: > > Vendor InfoFrame: > - frequency: every vsync > - raw: > f4050181 000c0347 0320 > > 004c > > - vendor Id: 0x000c03 (HDMI) > - video format: 0x001 > - HDMI VIC: 3 > > after a $ xrandr --output HDMI1 --mode 3840x2160 -r 24 > > -- > Damien > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- 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 07/12] video/hdmi: Introduce helpers for the HDMI vendor specific infoframe
On Wed, Aug 14, 2013 at 12:17:23AM +0100, Damien Lespiau wrote: > Provide the programming model than the other infoframe types. > > The generic _pack() function can't handle those yet as we need to move > the vendor OUI in the generic hdmi_vendor_infoframe structure to know > which kind of vendor infoframe we are dealing with. > > Signed-off-by: Damien Lespiau > --- > drivers/video/hdmi.c | 82 > > include/linux/hdmi.h | 25 > 2 files changed, 107 insertions(+) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index ac84215..2059f7b 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -286,6 +286,88 @@ ssize_t hdmi_audio_infoframe_pack(struct > hdmi_audio_infoframe *frame, > EXPORT_SYMBOL(hdmi_audio_infoframe_pack); > > /** > + * hdmi_hdmi_infoframe_init() - initialize an HDMI vendor infoframe > + * @frame: HDMI vendor infoframe > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame) > +{ > + memset(frame, 0, sizeof(*frame)); > + > + frame->type = HDMI_INFOFRAME_TYPE_VENDOR; > + frame->version = 1; > + frame->length = 5; /* we can hardcode the size for now as we don't > + support neither 3D_Ext_Data nor 3D_Metadata_* */ > + > + /* 0 is a valid value for s3d_struct, so we use a special "not set" > + * value */ > + frame->s3d_struct = HDMI_3D_STRUCTURE_INVALID; > + > + return 0; > +} > +EXPORT_SYMBOL(hdmi_hdmi_infoframe_init); > + > +/** > + * hdmi_hmdi_infoframe_pack() - write a HDMI vendor infoframe to binary > buffer Another hmdi typo that wasn't spotted yet :) > + * @frame: HDMI infoframe > + * @buffer: destination buffer > + * @size: size of buffer > + * > + * Packs the information contained in the @frame structure into a binary > + * representation that can be written into the corresponding controller > + * registers. Also computes the checksum as required by section 5.3.5 of > + * the HDMI 1.4 specification. > + * > + * Returns the number of bytes packed into the binary buffer or a negative > + * error code on failure. > + */ > +ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, > + void *buffer, size_t size) > +{ > + u8 *ptr = buffer; > + size_t length; > + > + /* empty info frame */ > + if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID) > + return -EINVAL; > + > + /* only one of those can be supplied */ > + if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) > + return -EINVAL; > + > + length = HDMI_INFOFRAME_HEADER_SIZE + frame->length; > + > + if (size < length) > + return -ENOSPC; > + > + memset(buffer, 0, size); > + > + ptr[0] = frame->type; > + ptr[1] = frame->version; > + ptr[2] = frame->length; > + ptr[3] = 0; /* checksum */ > + > + /* HDMI OUI */ > + ptr[4] = 0x03; > + ptr[5] = 0x0c; > + ptr[6] = 0x00; > + > + if (frame->vic) { > + ptr[7] = 0x1 << 5; /* video format */ > + ptr[8] = frame->vic; > + } else { > + ptr[7] = 0x2 << 5; /* video format */ > + ptr[8] = (frame->s3d_struct & 0xf) << 4; > + } > + > + hdmi_infoframe_checksum(buffer, length); > + > + return length; > +} > +EXPORT_SYMBOL(hdmi_hdmi_infoframe_pack); > + > +/** > * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary > *buffer > * @frame: HDMI vendor infoframe > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > index b98340b..f5098a8 100644 > --- a/include/linux/hdmi.h > +++ b/include/linux/hdmi.h > @@ -234,11 +234,36 @@ struct hdmi_vendor_infoframe { > ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > void *buffer, size_t size); > > +enum hdmi_3d_structure { > + HDMI_3D_STRUCTURE_INVALID = -1, > + HDMI_3D_STRUCTURE_FRAME_PACKING = 0, > + HDMI_3D_STRUCTURE_FIELD_ALTERNATIVE, > + HDMI_3D_STRUCTURE_LINE_ALTERNATIVE, > + HDMI_3D_STRUCTURE_SIDE_BY_SIDE_FULL, > + HDMI_3D_STRUCTURE_L_DEPTH, > + HDMI_3D_STRUCTURE_L_DEPTH_GFX_GFX_DEPTH, > + HDMI_3D_STRUCTURE_TOP_BOTTOM, > + HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF, > +}; > + > +struct hdmi_hdmi_infoframe { > + enum hdmi_infoframe_type type; > + unsigned char version; > + unsigned char length; > + u8 vic; > + enum hdmi_3d_structure s3d_struct; > +}; > + > +int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame); > +ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, > + void *buffer, size_t size); > + > union hdmi_infoframe { > struct hdmi_any_infoframe any; > struct hdmi_avi_infoframe avi; > struct hdmi_spd_i
Re: [Intel-gfx] [PATCH 08/12] gpu: host1x: Port the HDMI vendor infoframe code the common helpers
On Wed, Aug 14, 2013 at 12:17:24AM +0100, Damien Lespiau wrote: > I just wrote the bits to define and pack HDMI vendor specific infoframe. > Port the host1x driver to use those so I can refactor the infoframe code > a bit more. > > Cc: Thierry Reding > Cc: Terje Bergström > Cc: linux-te...@vger.kernel.org > > Signed-off-by: Damien Lespiau > --- > drivers/gpu/host1x/drm/hdmi.c | 24 > 1 file changed, 4 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/host1x/drm/hdmi.c b/drivers/gpu/host1x/drm/hdmi.c > index 01097da..b548918 100644 > --- a/drivers/gpu/host1x/drm/hdmi.c > +++ b/drivers/gpu/host1x/drm/hdmi.c > @@ -539,7 +539,7 @@ static void tegra_hdmi_setup_audio_infoframe(struct > tegra_hdmi *hdmi) > > static void tegra_hdmi_setup_stereo_infoframe(struct tegra_hdmi *hdmi) > { > - struct hdmi_vendor_infoframe frame; > + struct hdmi_hdmi_infoframe frame; > unsigned long value; > u8 buffer[10]; > ssize_t err; > @@ -551,26 +551,10 @@ static void tegra_hdmi_setup_stereo_infoframe(struct > tegra_hdmi *hdmi) > return; > } > > - memset(&frame, 0, sizeof(frame)); > + hdmi_hdmi_infoframe_init(&frame); > + frame.s3d_struct = HDMI_3D_STRUCTURE_FRAME_PACKING; > > - frame.type = HDMI_INFOFRAME_TYPE_VENDOR; > - frame.version = 0x01; > - frame.length = 6; This changes the length of the infoframe from 6 to 5, which is enough for "frame packing", but maybe the commit msg should still mention that small detail. > - > - frame.data[0] = 0x03; /* regid0 */ > - frame.data[1] = 0x0c; /* regid1 */ > - frame.data[2] = 0x00; /* regid2 */ > - frame.data[3] = 0x02 << 5; /* video format */ > - > - /* TODO: 74 MHz limit? */ > - if (1) { > - frame.data[4] = 0x00 << 4; /* 3D structure */ > - } else { > - frame.data[4] = 0x08 << 4; /* 3D structure */ > - frame.data[5] = 0x00 << 4; /* 3D ext. data */ > - } > - > - err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer)); > + err = hdmi_hdmi_infoframe_pack(&frame, buffer, sizeof(buffer)); > if (err < 0) { > dev_err(hdmi->dev, "failed to pack vendor infoframe: %zd\n", > err); > -- > 1.8.3.1 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- 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 10/12] video/hdmi: Hook the HDMI vendor infoframe with the generic _pack()
On Wed, Aug 14, 2013 at 12:17:26AM +0100, Damien Lespiau wrote: > With this last bit, hdmi_infoframe_pack() is now able to pack any > infoframe we support. > > At the same time, because it's impractical to make two commits out of > this, we get rid of the version that encourages the open coding of the > vendor infoframe packing. We can do so because the only user of this API > has been ported in: > > Author: Damien Lespiau > Date: Mon Aug 12 18:08:37 2013 +0100 > > gpu: host1x: Port the HDMI vendor infoframe code the common helpers > > Signed-off-by: Damien Lespiau > --- > drivers/video/hdmi.c | 45 + > include/linux/hdmi.h | 24 > 2 files changed, 21 insertions(+), 48 deletions(-) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index 2059f7b..073f005 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -300,6 +300,7 @@ int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe > *frame) > frame->length = 5; /* we can hardcode the size for now as we don't > support neither 3D_Ext_Data nor 3D_Metadata_* */ > > + frame->oui = HDMI_IDENTIFIER; > /* 0 is a valid value for s3d_struct, so we use a special "not set" >* value */ > frame->s3d_struct = HDMI_3D_STRUCTURE_INVALID; > @@ -367,46 +368,18 @@ ssize_t hdmi_hdmi_infoframe_pack(struct > hdmi_hdmi_infoframe *frame, > } > EXPORT_SYMBOL(hdmi_hdmi_infoframe_pack); > > -/** > - * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary > - *buffer > - * @frame: HDMI vendor infoframe > - * @buffer: destination buffer > - * @size: size of buffer > - * > - * Packs the information contained in the @frame structure into a binary > - * representation that can be written into the corresponding controller > - * registers. Also computes the checksum as required by section 5.3.5 of > - * the HDMI 1.4 specification. > - * > - * Returns the number of bytes packed into the binary buffer or a negative > - * error code on failure. > +/* > + * hdmi_vendor_infoframe_pack() - write a vendor infoframe to binary buffer > */ > -ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > -void *buffer, size_t size) > +static ssize_t hdmi_vendor_infoframe_pack(union hdmi_vendor_infoframe *frame, > + void *buffer, size_t size) > { > - u8 *ptr = buffer; > - size_t length; > - > - length = HDMI_INFOFRAME_HEADER_SIZE + frame->length; > - > - if (size < length) > - return -ENOSPC; > - > - memset(buffer, 0, size); > - > - ptr[0] = frame->type; > - ptr[1] = frame->version; > - ptr[2] = frame->length; > - ptr[3] = 0; /* checksum */ > - > - memcpy(&ptr[HDMI_INFOFRAME_HEADER_SIZE], frame->data, frame->length); > - > - hdmi_infoframe_checksum(buffer, length); > + /* we only know about HDMI vendor infoframes */ > + if (frame->any.oui != HDMI_IDENTIFIER) > + return -EINVAL; > > - return length; > + return hdmi_hdmi_infoframe_pack(&frame->hdmi, buffer, size); > } > -EXPORT_SYMBOL(hdmi_vendor_infoframe_pack); > > /** > * hdmi_infoframe_pack() - write a HDMI infoframe to binary buffer > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > index 53bbf0d..5c572e9 100644 > --- a/include/linux/hdmi.h > +++ b/include/linux/hdmi.h > @@ -225,16 +225,6 @@ int hdmi_audio_infoframe_init(struct > hdmi_audio_infoframe *frame); > ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame, > void *buffer, size_t size); > > -struct hdmi_vendor_infoframe { > - enum hdmi_infoframe_type type; > - unsigned char version; > - unsigned char length; > - u8 data[27]; > -}; > - > -ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > -void *buffer, size_t size); > - > enum hdmi_3d_structure { > HDMI_3D_STRUCTURE_INVALID = -1, > HDMI_3D_STRUCTURE_FRAME_PACKING = 0, > @@ -251,6 +241,7 @@ struct hdmi_hdmi_infoframe { > enum hdmi_infoframe_type type; > unsigned char version; > unsigned char length; > + int oui; unsigned perhaps? Same deal below in the 'any' struct. Doesn't really matter I guess, so w/ or w/o that change: Reviewed-by: Ville Syrjälä > u8 vic; > enum hdmi_3d_structure s3d_struct; > }; > @@ -259,12 +250,21 @@ int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe > *frame); > ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, >void *buffer, size_t size); > > +union hdmi_vendor_infoframe { > + struct { > + enum hdmi_infoframe_type type; > + unsigned char version; > + unsigned char length; > + int oui; > + } any; > + struct hdmi_hdmi_infofr
Re: [Intel-gfx] [PATCH] Correct misspelled caching
On Wed, Aug 14, 2013 at 10:13 AM, Chris Wilson wrote: > On Wed, Aug 14, 2013 at 10:09:23AM +0200, Sedat Dilek wrote: >> On Wed, Aug 14, 2013 at 10:05 AM, Chris Wilson >> wrote: >> > On Wed, Aug 14, 2013 at 10:01:13AM +0200, Sedat Dilek wrote: >> >> Signed-off-by: Sedat Dilek >> > >> > Am I the only one who reads that as a hard 'ch' without the 'e'? >> >> I am not an English native or teacher: But from what I know it's... to >> cache | caching | cached. >> Google gives me no hits for cach*e*ing. >> Anyway, the spelling should be consistent in kernel-drm | libdrm | >> intel-ddx | etc. > > It was. I blame Ben. I can't say :-). Anyway, what's next with this patch? - Sedat - ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl
By exporting the ability to map user address and inserting PTEs representing their backing pages into the GTT, we can exploit UMA in order to utilize normal application data as a texture source or even as a render target (depending upon the capabilities of the chipset). This has a number of uses, with zero-copy downloads to the GPU and efficient readback making the intermixed streaming of CPU and GPU operations fairly efficient. This ability has many widespread implications from faster rendering of client-side software rasterisers (chromium), mitigation of stalls due to read back (firefox) and to faster pipelining of texture data (such as pixel buffer objects in GL or data blobs in CL). v2: Compile with CONFIG_MMU_NOTIFIER v3: We can sleep while performing invalidate-range, which we can utilise to drop our page references prior to the kernel manipulating the vma (for either discard or cloning) and so protect normal users. v4: Only run the invalidate notifier if the range intercepts the bo. v5: Prevent userspace from attempting to GTT mmap non-page aligned buffers v6: Recheck after reacquire mutex for lost mmu. v7: Fix implicit padding of ioctl struct by rounding to next 64bit boundary. v8: Fix rebasing error after forwarding porting the back port. v9: Limit the userptr to page aligned entries. We now expect userspace to handle all the offset-in-page adjustments itself. v10: Prevent vma from being copied across fork to avoid issues with cow. NB: We still have performance concerns over the use of the linear lists and unfiltered notifies in mmu_notifier which do not scale to our use case, where we may have many thousands of objects being tracked. Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/Makefile |1 + drivers/gpu/drm/i915/i915_dma.c |1 + drivers/gpu/drm/i915/i915_drv.h | 18 +- drivers/gpu/drm/i915/i915_gem.c |3 + drivers/gpu/drm/i915/i915_gem_userptr.c | 416 +++ drivers/gpu/drm/i915/i915_gpu_error.c |2 + include/uapi/drm/i915_drm.h | 16 ++ 7 files changed, 456 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/i915_gem_userptr.c diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 9d498e5..2369cfe 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -16,6 +16,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \ i915_gem_gtt.o \ i915_gem_stolen.o \ i915_gem_tiling.o \ + i915_gem_userptr.o \ i915_sysfs.o \ i915_trace_points.o \ i915_ums.o \ diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3cab741..23a9374 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1894,6 +1894,7 @@ const struct drm_ioctl_desc i915_ioctls[] = { DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(I915_GEM_CREATE2, i915_gem_create2_ioctl, DRM_AUTH|DRM_UNLOCKED), + DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_UNLOCKED), }; int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c2da7e9..d6c6626 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -43,6 +43,7 @@ #include #include #include +#include /* General customization: */ @@ -320,6 +321,7 @@ struct drm_i915_error_state { u32 tiling:2; u32 dirty:1; u32 purgeable:1; + u32 userptr:1; s32 ring:4; u32 cache_level:2; } **active_bo, **pinned_bo; @@ -1296,6 +1298,7 @@ struct drm_i915_gem_object_ops { */ int (*get_pages)(struct drm_i915_gem_object *); void (*put_pages)(struct drm_i915_gem_object *); + void (*release)(struct drm_i915_gem_object *); }; struct drm_i915_gem_object { @@ -1425,9 +1428,20 @@ struct drm_i915_gem_object { /** for phy allocated objects */ struct drm_i915_gem_phys_object *phys_obj; + + union { + struct i915_gem_userptr { + uintptr_t ptr; + bool read_only; + + struct mm_struct *mm; +#if defined(CONFIG_MMU_NOTIFIER) + struct mmu_notifier mn; +#endif + } userptr; + }; }; #define to_gem_object(obj) (&((struct drm_i915_gem_object *)(obj))->base) - #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) /** @@ -1733,6 +1747,8 @@ int i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); int i915_gem_get_tiling_ioctl(struct drm_device *dev, void *data,
Re: [Intel-gfx] [PATCH 07/12] video/hdmi: Introduce helpers for the HDMI vendor specific infoframe
On Wed, Aug 14, 2013 at 12:17:23AM +0100, Damien Lespiau wrote: > Provide the programming model than the other infoframe types. > > The generic _pack() function can't handle those yet as we need to move > the vendor OUI in the generic hdmi_vendor_infoframe structure to know > which kind of vendor infoframe we are dealing with. > > Signed-off-by: Damien Lespiau > --- > drivers/video/hdmi.c | 82 > > include/linux/hdmi.h | 25 > 2 files changed, 107 insertions(+) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index ac84215..2059f7b 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -286,6 +286,88 @@ ssize_t hdmi_audio_infoframe_pack(struct > hdmi_audio_infoframe *frame, > EXPORT_SYMBOL(hdmi_audio_infoframe_pack); > > /** > + * hdmi_hdmi_infoframe_init() - initialize an HDMI vendor infoframe > + * @frame: HDMI vendor infoframe > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame) > +{ > + memset(frame, 0, sizeof(*frame)); > + > + frame->type = HDMI_INFOFRAME_TYPE_VENDOR; > + frame->version = 1; > + frame->length = 5; /* we can hardcode the size for now as we don't > + support neither 3D_Ext_Data nor 3D_Metadata_* */ > + > + /* 0 is a valid value for s3d_struct, so we use a special "not set" > + * value */ > + frame->s3d_struct = HDMI_3D_STRUCTURE_INVALID; > + > + return 0; > +} > +EXPORT_SYMBOL(hdmi_hdmi_infoframe_init); > + > +/** > + * hdmi_hmdi_infoframe_pack() - write a HDMI vendor infoframe to binary > buffer > + * @frame: HDMI infoframe > + * @buffer: destination buffer > + * @size: size of buffer > + * > + * Packs the information contained in the @frame structure into a binary > + * representation that can be written into the corresponding controller > + * registers. Also computes the checksum as required by section 5.3.5 of > + * the HDMI 1.4 specification. > + * > + * Returns the number of bytes packed into the binary buffer or a negative > + * error code on failure. > + */ > +ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, > + void *buffer, size_t size) > +{ > + u8 *ptr = buffer; > + size_t length; > + > + /* empty info frame */ > + if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID) > + return -EINVAL; > + > + /* only one of those can be supplied */ > + if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) > + return -EINVAL; > + > + length = HDMI_INFOFRAME_HEADER_SIZE + frame->length; > + > + if (size < length) > + return -ENOSPC; > + > + memset(buffer, 0, size); > + > + ptr[0] = frame->type; > + ptr[1] = frame->version; > + ptr[2] = frame->length; > + ptr[3] = 0; /* checksum */ > + > + /* HDMI OUI */ > + ptr[4] = 0x03; > + ptr[5] = 0x0c; > + ptr[6] = 0x00; > + > + if (frame->vic) { > + ptr[7] = 0x1 << 5; /* video format */ > + ptr[8] = frame->vic; > + } else { > + ptr[7] = 0x2 << 5; /* video format */ > + ptr[8] = (frame->s3d_struct & 0xf) << 4; > + } > + > + hdmi_infoframe_checksum(buffer, length); > + > + return length; > +} > +EXPORT_SYMBOL(hdmi_hdmi_infoframe_pack); > + > +/** > * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary > *buffer > * @frame: HDMI vendor infoframe > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > index b98340b..f5098a8 100644 > --- a/include/linux/hdmi.h > +++ b/include/linux/hdmi.h > @@ -234,11 +234,36 @@ struct hdmi_vendor_infoframe { > ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > void *buffer, size_t size); > > +enum hdmi_3d_structure { > + HDMI_3D_STRUCTURE_INVALID = -1, > + HDMI_3D_STRUCTURE_FRAME_PACKING = 0, > + HDMI_3D_STRUCTURE_FIELD_ALTERNATIVE, > + HDMI_3D_STRUCTURE_LINE_ALTERNATIVE, > + HDMI_3D_STRUCTURE_SIDE_BY_SIDE_FULL, > + HDMI_3D_STRUCTURE_L_DEPTH, > + HDMI_3D_STRUCTURE_L_DEPTH_GFX_GFX_DEPTH, > + HDMI_3D_STRUCTURE_TOP_BOTTOM, TOP_AND_BOTTOM maybe? > + HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF, The value of this should be 8. Also according to the spec we must send 3d_ext_data when when 3d_structure >= 8, so either we need a bit more code, or we need to disallow 3d_structure >= 8 for the time being. > +}; > + > +struct hdmi_hdmi_infoframe { > + enum hdmi_infoframe_type type; > + unsigned char version; > + unsigned char length; > + u8 vic; > + enum hdmi_3d_structure s3d_struct; > +}; > + > +int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame); > +ssize_t hdmi_hdmi_infoframe_pack(struct hdmi_hdmi_infoframe *frame, > +
Re: [Intel-gfx] [PATCH 04/12] drm: Add support for alternate clocks of 4k modes
A few minor things: On Wednesday 14 August 2013 00:17:20 Damien Lespiau wrote: > Suggested-by: Ville Syrjälä > Signed-off-by: Damien Lespiau > --- > drivers/gpu/drm/drm_edid.c | 68 > ++ > 1 file changed, 62 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 0faa08e..606335f 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2409,6 +2409,54 @@ u8 drm_match_cea_mode(const struct drm_display_mode > *to_match) > } > EXPORT_SYMBOL(drm_match_cea_mode); > > +/* > + * Calculate the alternate clock for HDMI modes (those from the HDMI vendor > + * specific block). > + * > + * It's almost like cea_mode_alternate_clock(), we just need to add an > + * exception for the VIC 4 mode (4096x2160@24Hz): no alternate clock for this > + * one. > + */ > +static unsigned int > +hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode) > +{ > + if (hdmi_mode->vdisplay == 4096 && hdmi_mode->hdisplay == 2160) > + return hdmi_mode->clock; > + > + return cea_mode_alternate_clock(hdmi_mode); > +} > + > +/* > + * drm_match_cea_mode - look for a HDMI mode matching given mode Here we document drm_match_cea_mode, but the function is drm_match_hdmi_mode. > + * @to_match: display mode > + * > + * An HDMI mode is one defined in the HDMI vendor specific block. > + * > + * Returns the HDMI Video ID (VIC) of the mode or 0 if it isn't one. > + */ > +static u8 drm_match_hmdi_mode(const struct drm_display_mode *to_match) Surely should be hdmi, not hmdi? > +{ > + u8 mode; > + > + if (!to_match->clock) > + return 0; > + > + for (mode = 0; mode < ARRAY_SIZE(edid_4k_modes); mode++) { > + const struct drm_display_mode *hdmi_mode = &edid_4k_modes[mode]; > + unsigned int clock1, clock2; > + > + /* Make sure to also match alternate clocks */ > + clock1 = hdmi_mode->clock; > + clock2 = hdmi_mode_alternate_clock(hdmi_mode); > + > + if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || > + KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && > + drm_mode_equal_no_clocks(to_match, hdmi_mode)) > + return mode + 1; > + } > + return 0; > +} > + > static int > add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) > { > @@ -2426,18 +2474,26 @@ add_alternate_cea_modes(struct drm_connector > *connector, struct edid *edid) >* with the alternate clock for certain CEA modes. >*/ > list_for_each_entry(mode, &connector->probed_modes, head) { > - const struct drm_display_mode *cea_mode; > + const struct drm_display_mode *cea_mode = NULL; > struct drm_display_mode *newmode; > - u8 cea_mode_idx = drm_match_cea_mode(mode) - 1; > + u8 mode_idx = drm_match_cea_mode(mode) - 1; > unsigned int clock1, clock2; > > - if (cea_mode_idx >= ARRAY_SIZE(edid_cea_modes)) > - continue; > + if (mode_idx < ARRAY_SIZE(edid_cea_modes)) { > + cea_mode = &edid_cea_modes[mode_idx]; > + clock2 = cea_mode_alternate_clock(cea_mode); > + } else { > + mode_idx = drm_match_hmdi_mode(mode) - 1; Same typo here - hmdi for hdmi. > + if (mode_idx < ARRAY_SIZE(edid_4k_modes)) { > + cea_mode = &edid_4k_modes[mode_idx]; > + clock2 = hdmi_mode_alternate_clock(cea_mode); > + } > + } > > - cea_mode = &edid_cea_modes[cea_mode_idx]; > + if (!cea_mode) > + continue; > > clock1 = cea_mode->clock; > - clock2 = cea_mode_alternate_clock(cea_mode); > > if (clock1 == clock2) > continue; > -- Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com 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 06/12] video/hdmi: Derive the bar data valid bit from the bar data fields
On Wed, Aug 14, 2013 at 12:17:22AM +0100, Damien Lespiau wrote: > Just like: > > Author: Damien Lespiau > Date: Mon Aug 12 11:53:24 2013 +0100 > > video/hdmi: Don't let the user of this API create invalid infoframes > > But this time for the horizontal/vertical bar data present bits. > > Signed-off-by: Damien Lespiau > --- > drivers/video/hdmi.c | 5 +++-- > include/linux/hdmi.h | 2 -- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index e36da36..ac84215 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -101,10 +101,11 @@ ssize_t hdmi_avi_infoframe_pack(struct > hdmi_avi_infoframe *frame, void *buffer, > if (frame->active_aspect & 0xf) > ptr[0] |= BIT(4); > > - if (frame->horizontal_bar_valid) > + /* Bit 3 and 2 indicate if we transmit horizontal/vertical bar data */ > + if (frame->top_bar || frame->bottom_bar) > ptr[0] |= BIT(3); > > - if (frame->vertical_bar_valid) > + if (frame->left_bar || frame->right_bar) > ptr[0] |= BIT(2); Technically top=0,bottom=0 or left=0,right=0 is a valid bar setup, but it would indicate that the entire picture is made up of the bottom/right bar. I guess no one would really want to use such a setup, and even if they do, they could just use some N!=0 for both bars to achieve the same effect. So we don't seem to lose anything by doing this. Reviewed-by: Ville Syrjälä > > ptr[1] = ((frame->colorimetry & 0x3) << 6) | > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > index 931474c6..b98340b 100644 > --- a/include/linux/hdmi.h > +++ b/include/linux/hdmi.h > @@ -109,8 +109,6 @@ struct hdmi_avi_infoframe { > unsigned char version; > unsigned char length; > enum hdmi_colorspace colorspace; > - bool horizontal_bar_valid; > - bool vertical_bar_valid; > enum hdmi_scan_mode scan_mode; > enum hdmi_colorimetry colorimetry; > enum hdmi_picture_aspect picture_aspect; > -- > 1.8.3.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/12] drm: Add support for alternate clocks of 4k modes
On Wed, Aug 14, 2013 at 12:17:20AM +0100, Damien Lespiau wrote: > Suggested-by: Ville Syrjälä > Signed-off-by: Damien Lespiau > --- > drivers/gpu/drm/drm_edid.c | 68 > ++ > 1 file changed, 62 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 0faa08e..606335f 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2409,6 +2409,54 @@ u8 drm_match_cea_mode(const struct drm_display_mode > *to_match) > } > EXPORT_SYMBOL(drm_match_cea_mode); > > +/* > + * Calculate the alternate clock for HDMI modes (those from the HDMI vendor > + * specific block). > + * > + * It's almost like cea_mode_alternate_clock(), we just need to add an > + * exception for the VIC 4 mode (4096x2160@24Hz): no alternate clock for this > + * one. > + */ > +static unsigned int > +hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode) > +{ > + if (hdmi_mode->vdisplay == 4096 && hdmi_mode->hdisplay == 2160) > + return hdmi_mode->clock; > + > + return cea_mode_alternate_clock(hdmi_mode); > +} > + > +/* > + * drm_match_cea_mode - look for a HDMI mode matching given mode ^^^ Fix that, and you get: Reviewed-by: Ville Syrjälä > + * @to_match: display mode > + * > + * An HDMI mode is one defined in the HDMI vendor specific block. > + * > + * Returns the HDMI Video ID (VIC) of the mode or 0 if it isn't one. > + */ > +static u8 drm_match_hmdi_mode(const struct drm_display_mode *to_match) > +{ > + u8 mode; > + > + if (!to_match->clock) > + return 0; > + > + for (mode = 0; mode < ARRAY_SIZE(edid_4k_modes); mode++) { > + const struct drm_display_mode *hdmi_mode = &edid_4k_modes[mode]; > + unsigned int clock1, clock2; > + > + /* Make sure to also match alternate clocks */ > + clock1 = hdmi_mode->clock; > + clock2 = hdmi_mode_alternate_clock(hdmi_mode); > + > + if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || > + KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && > + drm_mode_equal_no_clocks(to_match, hdmi_mode)) > + return mode + 1; > + } > + return 0; > +} > + > static int > add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) > { > @@ -2426,18 +2474,26 @@ add_alternate_cea_modes(struct drm_connector > *connector, struct edid *edid) >* with the alternate clock for certain CEA modes. >*/ > list_for_each_entry(mode, &connector->probed_modes, head) { > - const struct drm_display_mode *cea_mode; > + const struct drm_display_mode *cea_mode = NULL; > struct drm_display_mode *newmode; > - u8 cea_mode_idx = drm_match_cea_mode(mode) - 1; > + u8 mode_idx = drm_match_cea_mode(mode) - 1; > unsigned int clock1, clock2; > > - if (cea_mode_idx >= ARRAY_SIZE(edid_cea_modes)) > - continue; > + if (mode_idx < ARRAY_SIZE(edid_cea_modes)) { > + cea_mode = &edid_cea_modes[mode_idx]; > + clock2 = cea_mode_alternate_clock(cea_mode); > + } else { > + mode_idx = drm_match_hmdi_mode(mode) - 1; > + if (mode_idx < ARRAY_SIZE(edid_4k_modes)) { > + cea_mode = &edid_4k_modes[mode_idx]; > + clock2 = hdmi_mode_alternate_clock(cea_mode); > + } > + } > > - cea_mode = &edid_cea_modes[cea_mode_idx]; > + if (!cea_mode) > + continue; > > clock1 = cea_mode->clock; > - clock2 = cea_mode_alternate_clock(cea_mode); > > if (clock1 == clock2) > continue; > -- > 1.8.3.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/12] drm/edid: Parse the HDMI CEA block and look for 4k modes
Minor typo - feel free to ignore: On Wednesday 14 August 2013 00:17:19 Damien Lespiau wrote: > HDMI 1.4 adds 4 "4k x 2k" modes in the the CEA vendor specific block. > > With this commit, we now parse this block and expose the 4k modes that > we find there. > > v2: Fix the "4096x2160" string (nice catch!), add comments about > do_hdmi_vsdb_modes() arguments and make it clearer that offset is > relative to the end of the required fields of the HDMI VSDB > (Ville Syrjälä) > > Signed-off-by: Damien Lespiau > Tested-by: Cancan Feng > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67030 > --- > drivers/gpu/drm/drm_edid.c | 124 > +++-- > 1 file changed, 109 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 9e9b6ed..0faa08e 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2465,6 +2495,68 @@ do_cea_modes(struct drm_connector *connector, const u8 > *db, u8 len) > return modes; > } > > +/* > + * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block > + * @connector: connector corresponding to the HDMI sink > + * @db: start of the CEA vendor specific block > + * @len: length of the CEA block payload, ie. one can access up to db[len] > + * > + * Parses the HDMI VSDB looking for modes to add to @connector. > + */ > +static int > +do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len) > +{ > + struct drm_device *dev = connector->dev; > + int modes = 0, offset = 0, i; > + u8 vic_len; > + > + if (len < 8) > + goto out; > + > + /* no HDMI_Video_Present */ > + if (!(db[8] & (1 << 5))) > + goto out; > + > + /* Latency_Fields_Present */ > + if (db[8] & (1 << 7)) > + offset += 2; > + > + /* I_Latency_Fields_Present */ > + if (db[8] & (1 << 6)) > + offset += 2; > + > + /* the declared length is not long enough for the 2 first bytes > + * of additional video format capabilities */ > + offset += 2; > + if (len < (8 + offset)) > + goto out; > + > + vic_len = db[8 + offset] >> 5; > + > + for (i = 0; i < vic_len && len >= (9 + offset + i); i++) { > + struct drm_display_mode *newmode; > + u8 vic; > + > + vic = db[9 + offset + i]; > + > + vic--; /* VICs start at 1 */ > + if (vic >= ARRAY_SIZE(edid_4k_modes)) { > + DRM_ERROR("Unknow HDMI VIC: %d\n", vic); ^^ Missing "n" - should be Unknown > + continue; > + } > + > + newmode = drm_mode_duplicate(dev, &edid_4k_modes[vic]); > + if (!newmode) > + continue; > + > + drm_mode_probed_add(connector, newmode); > + modes++; > + } > + > +out: > + return modes; > +} > + > static int > cea_db_payload_len(const u8 *db) > { -- Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com 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 02/12] drm/edid: Fix add_cea_modes() style issues
On Wed, Aug 14, 2013 at 12:17:18AM +0100, Damien Lespiau wrote: > A few styles issues have crept in here, fix them before touching this > code again. > > v2: constify arguments that can be (Ville Syrjälä) > > Signed-off-by: Damien Lespiau > --- > drivers/gpu/drm/drm_edid.c | 15 --- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index e014785..9e9b6ed 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2301,7 +2301,7 @@ add_detailed_modes(struct drm_connector *connector, > struct edid *edid, > /* > * Search EDID for CEA extension block. > */ > -static u8 *drm_find_cea_extension(struct edid *edid) > +static u8 *drm_find_cea_extension(const struct edid *edid) This guy still casts away the const internally. Not sure if something will complain about that. So maybe just drop this bit for now. A long time ago I posted a big contify everything in drm_edid.c patch, and it did in a way that didn't ad new warnings even w/ -Wcast-qual. Maybe I should resurrect it. > { > u8 *edid_ext = NULL; > int i; > @@ -2441,10 +2441,11 @@ add_alternate_cea_modes(struct drm_connector > *connector, struct edid *edid) > } > > static int > -do_cea_modes (struct drm_connector *connector, u8 *db, u8 len) > +do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len) > { > struct drm_device *dev = connector->dev; > - u8 * mode, cea_mode; > + const u8 *mode; > + u8 cea_mode; > int modes = 0; > > for (mode = db; mode < db + len; mode++) { > @@ -2499,10 +2500,10 @@ cea_db_offsets(const u8 *cea, int *start, int *end) > for ((i) = (start); (i) < (end) && (i) + > cea_db_payload_len(&(cea)[(i)]) < (end); (i) += > cea_db_payload_len(&(cea)[(i)]) + 1) > > static int > -add_cea_modes(struct drm_connector *connector, struct edid *edid) > +add_cea_modes(struct drm_connector *connector, const struct edid *edid) > { > - u8 * cea = drm_find_cea_extension(edid); > - u8 * db, dbl; > + u8 *cea = drm_find_cea_extension(edid); > + u8 *db, dbl; cea and db could be const here. > int modes = 0; > > if (cea && cea_revision(cea) >= 3) { > @@ -2516,7 +2517,7 @@ add_cea_modes(struct drm_connector *connector, struct > edid *edid) > dbl = cea_db_payload_len(db); > > if (cea_db_tag(db) == VIDEO_BLOCK) > - modes += do_cea_modes (connector, db+1, dbl); > + modes += do_cea_modes(connector, db + 1, dbl); > } > } > > -- > 1.8.3.1 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- 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: Drop the overzealous warning from i915_gem_set_cache_level
On Wed, Aug 14, 2013 at 09:54:05AM +0100, Chris Wilson wrote: > On Wed, Aug 14, 2013 at 10:49:11AM +0200, Daniel Vetter wrote: > > On Tue, Aug 13, 2013 at 03:37:56PM +0300, Ville Syrjälä wrote: > > > On Tue, Aug 13, 2013 at 01:20:13PM +0100, Chris Wilson wrote: > > > > On Tue, Aug 13, 2013 at 03:12:59PM +0300, Ville Syrjälä wrote: > > > > > Thinking about this stuff a bit, I think I actually came up with a > > > > > scenario where we would currently fail to invalidate the CPU cache > > > > > between non-snooped GPU/GTT access and CPU access: > > > > > > > > > > 1. make bo non-snooped w/ pin_display=true (wd=0, rd|=gtt) > > > > > 2. set to CPU read domain (wd=0 rd|=cpu) > > > > > 3. set to GTT (or GPU) write domain (wd=gtt, rd=gtt) -> CPU cache is > > > > > stale after this point > > > > > 4. make bo snooped -> pin_display=true still so we directly set > > > > > (wd=cpu, rd=cpu) > > > > > 5. set to CPU domain -> CPU cache is still stale > > > > > > > > You will also find the scanout reads stale data as well. > > > > > > Well, assuming you actually write something to the bo w/ the CPU. If > > > not, then it keeps scanning out the correct data. > > > > I think an if (obj->pin_display) return -EBUSY; in the set_caching ioctl > > would be good to fix this. > > And we already do that check (as a result of obj->pin_count). > Sorted. Indeed. Patch merged to dinq (with a pimped commit message), 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
[Intel-gfx] [PATCH] drm/i915: inline vma_create into lookup_or_create_vma
In the execbuf code we don't clean up any vmas which ended up not getting bound for code simplicity. To make sure that we don't end up creating multiple vma for the same vm kill the somewhat dangerous vma_create function and inline it into lookup_or_create. This is just a safety measure to prevent surprises in the future. Also update the somewhat confused comment in the execbuf code and clarify what kind of magic is going on with a new one. Cc: Ben Widawsky Signed-off-by: Daniel Vetter --- That's the only concern I could come up with when reading the execbuf vma conversion patch. So looks good and I'll slurp it all in as soon as some more head scratching is done for the very first patch in this series about the vma_unbind fix to only call vma_destroy if the vma isn't bound. -Daniel --- drivers/gpu/drm/i915/i915_drv.h| 2 -- drivers/gpu/drm/i915/i915_gem.c| 41 +- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +--- drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- 4 files changed, 26 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index fc35ae3..b06a90f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1748,8 +1748,6 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, size_t size); void i915_gem_free_object(struct drm_gem_object *obj); -struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, -struct i915_address_space *vm); void i915_gem_vma_destroy(struct i915_vma *vma); int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 4be3be9..bb029a4 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4121,28 +4121,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) i915_gem_object_free(obj); } -struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, -struct i915_address_space *vm) -{ - struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL); - if (vma == NULL) - return ERR_PTR(-ENOMEM); - - INIT_LIST_HEAD(&vma->vma_link); - INIT_LIST_HEAD(&vma->mm_list); - INIT_LIST_HEAD(&vma->exec_list); - vma->vm = vm; - vma->obj = obj; - - /* Keep GGTT vmas first to make debug easier */ - if (i915_is_ggtt(vm)) - list_add(&vma->vma_link, &obj->vma_list); - else - list_add_tail(&vma->vma_link, &obj->vma_list); - - return vma; -} - void i915_gem_vma_destroy(struct i915_vma *vma) { WARN_ON(vma->node.allocated); @@ -4888,8 +4866,23 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj, struct i915_vma *vma; vma = i915_gem_obj_to_vma(obj, vm); - if (!vma) - vma = i915_gem_vma_create(obj, vm); + if (!vma) { + vma = kzalloc(sizeof(*vma), GFP_KERNEL); + if (vma == NULL) + return ERR_PTR(-ENOMEM); + + INIT_LIST_HEAD(&vma->vma_link); + INIT_LIST_HEAD(&vma->mm_list); + INIT_LIST_HEAD(&vma->exec_list); + vma->vm = vm; + vma->obj = obj; + + /* Keep GGTT vmas first to make debug easier */ + if (i915_is_ggtt(vm)) + list_add(&vma->vma_link, &obj->vma_list); + else + list_add_tail(&vma->vma_link, &obj->vma_list); + } return vma; } diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index c3b36f5..9b3b5f8 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -123,11 +123,16 @@ eb_lookup_vmas(struct eb_vmas *eb, list_for_each_entry(obj, &objects, obj_exec_link) { struct i915_vma *vma; + /* +* NOTE: We can leak any vmas created here when something fails +* later on. But that's no issue since vma_unbind can deal with +* vmas which are not actually bound. And since only +* lookup_or_create exists as an interface to get at the vma +* from the (obj, vm) we don't run the risk of creating +* duplicated vmas for the same vm. +*/ vma = i915_gem_obj_lookup_or_create_vma(obj, vm); if (IS_ERR(vma)) { - /* XXX: We don't need an error path fro vma because if -* the vma was created just for this execbuf, object -* unreference should kill it off.*/
Re: [Intel-gfx] [PATCH igt] intel_infoframes: Add support for decoding HDMI VICs
Reviewed-by: Simon Farnsworth On Wednesday 14 August 2013 00:19:14 Damien Lespiau wrote: > The HDMI vendor infoframe can contain a HDMI VIC (as of HDMI 1.4, only > used for 4k formats). > > Signed-off-by: Damien Lespiau > --- > tools/intel_infoframes.c | 26 ++ > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/tools/intel_infoframes.c b/tools/intel_infoframes.c > index 09fdcb9..b6d289f 100644 > --- a/tools/intel_infoframes.c > +++ b/tools/intel_infoframes.c > @@ -184,8 +184,13 @@ typedef union { > uint8_t Rsvd0:5; > uint8_t video_format :3; > > - uint8_t Rsvd1 :4; > - uint8_t s3d_structure :4; > + union { > + uint8_t vic; > + struct { > + uint8_t Rsvd1 :4; > + uint8_t s3d_structure :4; > + } s3d; > + } pb5; > > uint8_t Rsvd2:4; > uint8_t s3d_ext_data :4; > @@ -467,13 +472,26 @@ static const char *s3d_structure_to_string(int format) > > static void dump_vendor_hdmi(DipInfoFrame *frame) > { > + int vic_present = frame->vendor.video_format & 0x1; > int s3d_present = frame->vendor.video_format & 0x2; > > printf("- video format: 0x%03x %s\n", frame->vendor.video_format, > s3d_present ? "(3D)" : ""); > - if (s3d_present) > + > + if (vic_present && s3d_present) { > + printf("Error: HDMI VIC and S3D bits set. Only one of those " > +" at a time is valid\n"); > + return; > + } > + > + if (vic_present) > + printf("- HDMI VIC: %d\n", frame->vendor.pb5.vic); > + else if (s3d_present) { > + int s3d_structure = frame->vendor.pb5.s3d.s3d_structure; > + > printf("- 3D Format: %s\n", > -s3d_structure_to_string(frame->vendor.s3d_structure)); > +s3d_structure_to_string(s3d_structure)); > + } > } > > static void dump_vendor_info(Transcoder transcoder) > -- Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com 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] HDMI 4k support v2
On Wednesday 14 August 2013 00:17:16 Damien Lespiau wrote: > Following up the first instance of this series: > http://lists.freedesktop.org/archives/dri-devel/2013-August/043125.html > > Here is a v2 with Ville's review pass and a few additions: > - Alternate clock modes for 4k resolutions > - HDMI vendor specific infoframe support in drivers/video/hdmi.c > - Enabling of those vendor specific infoframes in the i915 driver > > Along the way, a tegra patch was needed for a small consolidation of the code > packing vendor specific infoframes. This patch has only been compile-tested. > > On Intel, it's possible to read back the programmed infoframe buffers with > intel-gpu-tools' intel_infoframe and this gives: > > Vendor InfoFrame: > - frequency: every vsync > - raw: > f4050181 000c0347 0320 > > 004c > > - vendor Id: 0x000c03 (HDMI) > - video format: 0x001 > - HDMI VIC: 3 > > after a $ xrandr --output HDMI1 --mode 3840x2160 -r 24 > > With the typos fixed, feel free to add: Reviewed-by: Simon Farnsworth -- Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com 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 11/12] drm: Add a helper to forge HDMI vendor infoframes
More typo fallout: On Wednesday 14 August 2013 00:17:27 Damien Lespiau wrote: > This can then be used by DRM drivers to setup their vendor infoframes. > > Signed-off-by: Damien Lespiau > --- > drivers/gpu/drm/drm_edid.c | 36 > include/drm/drm_edid.h | 4 > 2 files changed, 40 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 9a07a33..83e1202 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3262,3 +3262,39 @@ drm_hdmi_avi_infoframe_from_display_mode(struct > hdmi_avi_infoframe *frame, > return 0; > } > EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode); > + > +/** > + * drm_hdmi_vendor_infoframe_from_display_mode() - fill an HDMI infoframe > with > + * data from a DRM display mode > + * @frame: HDMI vendor infoframe > + * @mode: DRM display mode > + * > + * Note that there's is a need to send HDMI vendor infoframes only when > using a > + * 4k or stereoscopic 3D mode. So when giving any other mode as input this > + * function will return -EINVAL, error that can be safely ignored. > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +int > +drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_hdmi_infoframe > *frame, > + const struct drm_display_mode *mode) > +{ > + int err; > + u8 vic; > + > + if (!frame || !mode) > + return -EINVAL; > + > + vic = drm_match_hmdi_mode(mode); Should be hdmi again. > + if (!vic) > + return -EINVAL; > + > + err = hdmi_hdmi_infoframe_init(frame); > + if (err < 0) > + return err; > + > + frame->vic = vic; > + > + return 0; > +} > +EXPORT_SYMBOL(drm_hdmi_vendor_infoframe_from_display_mode); -- Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com 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