[Intel-gfx] [PATCH] drm/i915: Add GEN7_GPGPU_DISPATCHDIMX/Y/Z to the register whitelist

[Jordan Justen]

This is required to support glDispatchComputeIndirect for gen7.

Signed-off-by: Jordan Justen 
Reviewed-by: Kristian Høgsberg 
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 6 +-
 drivers/gpu/drm/i915/i915_reg.h| 4 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c 
b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 09932ca..db58c8d 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -448,6 +448,9 @@ static const struct drm_i915_reg_descriptor 
gen7_render_regs[] = {
REG32(GEN7_3DPRIM_INSTANCE_COUNT),
REG32(GEN7_3DPRIM_START_INSTANCE),
REG32(GEN7_3DPRIM_BASE_VERTEX),
+   REG32(GEN7_GPGPU_DISPATCHDIMX),
+   REG32(GEN7_GPGPU_DISPATCHDIMY),
+   REG32(GEN7_GPGPU_DISPATCHDIMZ),
REG64(GEN7_SO_NUM_PRIMS_WRITTEN(0)),
REG64(GEN7_SO_NUM_PRIMS_WRITTEN(1)),
REG64(GEN7_SO_NUM_PRIMS_WRITTEN(2)),
@@ -1214,6 +1217,7 @@ int i915_cmd_parser_get_version(void)
 *MI_PREDICATE_SRC1 registers.
 * 3. Allow access to the GPGPU_THREADS_DISPATCHED register.
 * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3.
+* 5. GPGPU dispatch compute indirect registers.
 */
-   return 4;
+   return 5;
 }
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 56157eb..f2b6425 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -536,6 +536,10 @@
 #define GEN7_3DPRIM_START_INSTANCE  0x243C
 #define GEN7_3DPRIM_BASE_VERTEX 0x2440
 
+#define GEN7_GPGPU_DISPATCHDIMX 0x2500
+#define GEN7_GPGPU_DISPATCHDIMY 0x2504
+#define GEN7_GPGPU_DISPATCHDIMZ 0x2508
+
 #define OACONTROL 0x2360
 
 #define _GEN7_PIPEA_DE_LOAD_SL 0x70068
-- 
2.5.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: prevent out of range pt in the PDE macros (take 3)

[Chris Wilson]

On Fri, Oct 02, 2015 at 01:47:03PM +0100, Michel Thierry wrote:
> On 10/1/2015 5:09 PM, Chris Wilson wrote:
> >On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:
> >>---
> >>  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++
> >>  1 file changed, 10 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
> >>b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>index 9fbb07d..94f8344 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>@@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
> >>   */
> >>  #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
> >>for (iter = gen6_pde_index(start); \
> >>-pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
> >>+pt = (length > 0 && iter < I915_PDES) ? \
> >>+   (pd)->page_table[iter] : NULL, \
> >>+length > 0 && iter < I915_PDES; \
> >
> >length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]) : 0,
> >
> >as the compiler wouldn't be able to CSE it otherwise (I think).
> 
> Even after that change, the compiler keeps doing an optimization
> when page_table[iter] is null (takes the null assignment as the
> break condition).
> 
> I've been playing with these examples
> http://paste.ubuntu.com/12638106/
> 
> Only the 1st example (a) iterates over all elements, b & c stop
> after the 1st run.

Forgot that was the condition you wanted to change.

length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]), 1 : 0,

Would be nice to prove that length > 0 implies iter < I915_PDES. If only
we had smart tools :)
-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] [BXT MIPI PATCH v3 13/14] drm/i915/bxt: Remove DSP CLK_GATE programming for BXT

[Daniel Vetter]

On Fri, Sep 18, 2015 at 04:38:27PM +0300, Jani Nikula wrote:
> On Tue, 01 Sep 2015, Uma Shankar  wrote:
> > DSP CLK_GATE registers are specific to BYT and CHT.
> > Avoid programming the same for BXT platform.
> >
> > v2: Rebased on latest drm nightly branch.
> >
> > v3: Fixed Jani's review comments
> >
> > Signed-off-by: Uma Shankar 
> 
> Reviewed-by: Jani Nikula 

Merged everything in this series to dinq, with the exception of two
backlight-related patches which seem superseeded.

I'll expect follow-up work to clean up the encoder callback code
organization.
-Daniel

> 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dsi.c |8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c 
> > b/drivers/gpu/drm/i915/intel_dsi.c
> > index 6a0071f..08bade2 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -631,9 +631,11 @@ static void intel_dsi_post_disable(struct 
> > intel_encoder *encoder)
> >  
> > intel_dsi_clear_device_ready(encoder);
> >  
> > -   val = I915_READ(DSPCLK_GATE_D);
> > -   val &= ~DPOUNIT_CLOCK_GATE_DISABLE;
> > -   I915_WRITE(DSPCLK_GATE_D, val);
> > +   if (!IS_BROXTON(dev_priv->dev)) {
> > +   val = I915_READ(DSPCLK_GATE_D);
> > +   val &= ~DPOUNIT_CLOCK_GATE_DISABLE;
> > +   I915_WRITE(DSPCLK_GATE_D, val);
> > +   }
> >  
> > drm_panel_unprepare(intel_dsi->panel);
> >  
> > -- 
> > 1.7.9.5
> >
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Allow minimum brightness upon backlight enable

[Chris Wilson]

On Fri, Oct 02, 2015 at 10:50:50AM +0300, Jani Nikula wrote:
> On Fri, 02 Oct 2015, clinton.a.tay...@intel.com wrote:
> > From: Clint Taylor 
> >
> > backlight minimum is a valid value so you don't need to set maximum.
> >
> > Signed-off-by: Clint Taylor 
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> > b/drivers/gpu/drm/i915/intel_panel.c
> > index 4d28c7b..d509904 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1069,7 +1069,7 @@ void intel_panel_enable_backlight(struct 
> > intel_connector *connector)
> >  
> > WARN_ON(panel->backlight.max == 0);
> >  
> > -   if (panel->backlight.level <= panel->backlight.min) {
> > +   if (panel->backlight.level < panel->backlight.min) {
> > panel->backlight.level = panel->backlight.max;
> > if (panel->backlight.device)
> > panel->backlight.device->props.brightness =
> 
> This is policy in action in the kernel. We have this whole if block here
> because some, uh, misguided userspace sets brightness to minimum before
> switching the backlight off, and assumes enabling backlight cranks up
> the brightness as well. (*)

If you mean X, no. It has to set the backlight because on some intel
models the backlight is not routed through i915 and so it needs to
explicitly control it. On resume, it sets the previous value. This is
just a broken kernel.
-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] [BXT MIPI PATCH v5 11/14] drm/i915/bxt: Modify BXT BLC according to VBT changes

[Daniel Vetter]

On Wed, Sep 30, 2015 at 10:34:57PM +0530, Uma Shankar wrote:
> From: Sunil Kamath 
> 
> Latest VBT mentions which set of registers will be used for BLC,
> as controller number field. Making use of this field in BXT
> BLC implementation. Also, the registers are used in case control
> pin indicates display DDI. Adding a check for this.
> According to Bspec, BLC_PWM_*_2 uses the display utility pin for output.
> To use backlight 2, enable the utility pin with mode = PWM
>v2: Jani's review comments
>addressed
>- Add a prefix _ to BXT BLC registers definitions.
>- Add "bxt only" comment for u8 controller
>- Remove control_pin check for DDI controller
>- Check for valid controller values
>- Set pipe bits in UTIL_PIN_CTL
>- Enable/Disable UTIL_PIN_CTL in enable/disable_backlight()
>- If BLC 2 is used, read active_low_pwm from UTIL_PIN polarity
>Satheesh's review comment addressed
>- If UTIL PIN is already enabled, BIOS would have programmed it. No
>need to disable and enable again.
>v3: Jani's review comments
>- add UTIL_PIN_PIPE_MASK and UTIL_PIN_MODE_MASK
>- Disable UTIL_PIN if controller 1 is used
>- Mask out UTIL_PIN_PIPE_MASK and UTIL_PIN_MODE_MASK before enabling
>UTIL_PIN
>- check valid controller value in intel_bios.c
>- add backlight.util_pin_active_low
>- disable util pin before enabling
>v4: Change for BXT-PO branch:
>Stubbed unwanted definition which was existing before
>because of DC6 patch.
>UTIL_PIN_MODE_PWM (0x1b << 24)
> 
> v2: Fixed Jani's review comment.
> 
> v3: Split the backight PWM frequency programming into separate patch,
> in cases BIOS doesn't initializes it.
> 
> v4: Starting afresh and not modifying existing state for backlight, as
> per Jani's recommendation.
> 
> v5: Fixed Jani's review comment wrt util pin enable
> 
> Signed-off-by: Vandana Kannan 
> Signed-off-by: Sunil Kamath 
> Signed-off-by: Uma Shankar 
> ---
>  drivers/gpu/drm/i915/i915_reg.h|   28 
>  drivers/gpu/drm/i915/intel_drv.h   |2 +
>  drivers/gpu/drm/i915/intel_panel.c |   83 
> 
>  3 files changed, 88 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 88a16e2..519f764 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3512,17 +3512,29 @@ enum skl_disp_power_wells {
>  #define UTIL_PIN_CTL 0x48400
>  #define   UTIL_PIN_ENABLE(1 << 31)
>  
> +#define   UTIL_PIN_PIPE(x) ((x) << 29)
> +#define   UTIL_PIN_PIPE_MASK   (3 << 29)
> +#define   UTIL_PIN_MODE_PWM(1 << 24)
> +#define   UTIL_PIN_MODE_MASK   (0xf << 24)
> +#define   UTIL_PIN_POLARITY(1 << 22)
> +
>  /* BXT backlight register definition. */
> -#define BXT_BLC_PWM_CTL1 0xC8250
> +#define _BXT_BLC_PWM_CTL10xC8250
>  #define   BXT_BLC_PWM_ENABLE (1 << 31)
>  #define   BXT_BLC_PWM_POLARITY   (1 << 29)
> -#define BXT_BLC_PWM_FREQ10xC8254
> -#define BXT_BLC_PWM_DUTY10xC8258
> -
> -#define BXT_BLC_PWM_CTL2 0xC8350
> -#define BXT_BLC_PWM_FREQ20xC8354
> -#define BXT_BLC_PWM_DUTY20xC8358
> -
> +#define _BXT_BLC_PWM_FREQ1   0xC8254
> +#define _BXT_BLC_PWM_DUTY1   0xC8258
> +
> +#define _BXT_BLC_PWM_CTL20xC8350
> +#define _BXT_BLC_PWM_FREQ2   0xC8354
> +#define _BXT_BLC_PWM_DUTY2   0xC8358
> +
> +#define BXT_BLC_PWM_CTL(controller)_PIPE(controller, \
> + _BXT_BLC_PWM_CTL1, _BXT_BLC_PWM_CTL2)
> +#define BXT_BLC_PWM_FREQ(controller)   _PIPE(controller, \
> + _BXT_BLC_PWM_FREQ1, _BXT_BLC_PWM_FREQ2)
> +#define BXT_BLC_PWM_DUTY(controller)   _PIPE(controller, \
> + _BXT_BLC_PWM_DUTY1, _BXT_BLC_PWM_DUTY2)
>  
>  #define PCH_GTC_CTL  0xe7000
>  #define   PCH_GTC_ENABLE (1 << 31)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 1059283..d8ca075 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -182,7 +182,9 @@ struct intel_panel {
>   bool enabled;
>   bool combination_mode;  /* gen 2/4 only */
>   bool active_low_pwm;
> + bool util_pin_active_low;   /* bxt+ */
>   struct backlight_device *device;
> + u8 controller;  /* bxt+ only */
>   } backlight;
>  
>   void (*backlight_power)(struct intel_connector *, bool enable);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> 

[Intel-gfx] [PATCH 2/2] drm/i915/gen9: Add HDC_CHICKEN1 to HW whitelist

[Arun Siluvery]

Required for WaAllowUMDToModifyHDCChicken1:skl,bxt

Cc: Mika Kuoppala 
Cc: Nick Hoath 
Signed-off-by: Arun Siluvery 
---
 drivers/gpu/drm/i915/i915_reg.h | 2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 797c74e..dc84072 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5937,6 +5937,8 @@ enum skl_disp_power_wells {
 #define  HDC_FORCE_NON_COHERENT(1<<4)
 #define  HDC_BARRIER_PERFORMANCE_DISABLE   (1<<10)
 
+#define HDC_CHICKEN1   0x7304
+
 /* GEN9 chicken */
 #define SLICE_ECO_CHICKEN0 0x7308
 #define   PIXEL_MASK_CAMMING_DISABLE   (1 << 14)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c 
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 64b2754..a091e9e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -921,6 +921,7 @@ static int gen9_init_workarounds(struct intel_engine_cs 
*ring)
struct drm_device *dev = ring->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
uint32_t tmp;
+   int ret;
 
/* WaDisablePartialInstShootdown:skl,bxt */
WA_SET_BIT_MASKED(GEN8_ROW_CHICKEN,
@@ -979,6 +980,11 @@ static int gen9_init_workarounds(struct intel_engine_cs 
*ring)
tmp |= HDC_FORCE_CSR_NON_COHERENT_OVR_DISABLE;
WA_SET_BIT_MASKED(HDC_CHICKEN0, tmp);
 
+   /* WaAllowUMDToModifyHDCChicken1:skl,bxt */
+   ret = wa_ring_whitelist_reg(ring, HDC_CHICKEN1);
+   if (ret)
+   return ret;
+
/* WaDisableSamplerPowerBypassForSOPingPong:skl,bxt */
if (IS_SKYLAKE(dev) ||
(IS_BROXTON(dev) && INTEL_REVID(dev) <= BXT_REVID_B0)) {
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice

[Daniel Vetter]

On Fri, Oct 02, 2015 at 12:44:45PM +0300, Imre Deak wrote:
> On Thu, 2015-10-01 at 16:56 -0700, Ben Widawsky wrote:
> > On Wed, Sep 30, 2015 at 11:00:46PM +0300, Imre Deak wrote:
> > > From: Ben Widawsky 
> > > 
> > > Signed-off-by: Ben Widawsky 
> > > 
> > > ---
> > > Changes (Imre):
> > > - use the new INSTDONE capturing by default on new GENs (On Ben's request)
> > > - keep printing the render ring INSTDONE to dmesg
> > > - don't hard code the extra_instdone array sizes
> > > - fix typo in GEN8_MCR_SLICE/GEN8_MCR_SUBSLICE
> > > - fix typo when capturing to extra->row
> > > - warn if the MCR selectors are non-zero
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h   |  6 ++--
> > >  drivers/gpu/drm/i915/i915_gpu_error.c | 62 
> > > +++
> > >  drivers/gpu/drm/i915/i915_irq.c   |  4 +--
> > >  drivers/gpu/drm/i915/i915_reg.h   |  5 +++
> > >  4 files changed, 66 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 621acf1..d1b4011 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -513,10 +513,12 @@ struct drm_i915_error_state {
> > >   u32 gam_ecochk;
> > >   u32 gab_ctl;
> > >   u32 gfx_mode;
> > > +#define INSTDONE_SLICE_NUM 3
> > > +#define INSTDONE_SUBSLICE_NUM 3
> > >   struct extra_instdone {
> > >   u32 slice_common;
> > > - u32 sampler;
> > > - u32 row;
> > > + u32 sampler[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > > + u32 row[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > >   } extra_instdone;
> > >  
> > >   u64 fence[I915_MAX_NUM_FENCES];
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
> > > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index e78e512..c6d1cbc 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -336,7 +336,7 @@ int i915_error_state_to_str(struct 
> > > drm_i915_error_state_buf *m,
> > >   struct drm_i915_private *dev_priv = dev->dev_private;
> > >   struct drm_i915_error_state *error = error_priv->error;
> > >   struct drm_i915_error_object *obj;
> > > - int i, j, offset, elt;
> > > + int i, j, slice, subslice, offset, elt;
> > >   int max_hangcheck_score;
> > >  
> > >   if (!error) {
> > > @@ -385,9 +385,17 @@ int i915_error_state_to_str(struct 
> > > drm_i915_error_state_buf *m,
> > >  
> > >   err_printf(m, "  SC_INSTDONE (slice common): 0x%08x\n",
> > >  error->extra_instdone.slice_common);
> > > - err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > > -error->extra_instdone.sampler);
> > > - err_printf(m, "  ROW_INSTDONE: 0x%08x\n", error->extra_instdone.row);
> > > + for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> > > + for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > > +  subslice++) {
> > > + struct extra_instdone *extra = >extra_instdone;
> > > +
> > > + err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > > +extra->sampler[slice][subslice]);
> > > + err_printf(m, "  ROW_INSTDONE: 0x%08x\n",
> > > +extra->row[slice][subslice]);
> > > + }
> > > + }
> > >  
> > >   if (INTEL_INFO(dev)->gen >= 6) {
> > >   err_printf(m, "ERROR: 0x%08x\n", error->error);
> > > @@ -1383,11 +1391,35 @@ const char *i915_cache_level_str(struct 
> > > drm_i915_private *i915, int type)
> > >   }
> > >  }
> > >  
> > > -/* NB: please notice the memset */
> > > +static inline uint32_t instdone_read(struct drm_i915_private *dev_priv, 
> > > int
> > > +  slice, int subslice, uint32_t offset) {
> > > + /*
> > > +  * XXX: The MCR register should be locked, but since we are only using
> > > +  * it for debug/error state, it's not terribly important to
> > > +  * synchronize it properly.
> > > +  */
> > > + uint32_t tmp = I915_READ(GEN8_MCR_SELECTOR);
> > > + uint32_t ret;
> > > +
> > > + /*
> > > +  * The HW expects the slice and sublice selectors to be reset to 0
> > > +  * after reading out the registers.
> > > +  */
> > > + WARN_ON_ONCE(tmp & (GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK));
> > > +
> > > + I915_WRITE(GEN8_MCR_SELECTOR, tmp | GEN8_MCR_SLICE(slice) |
> > > + GEN8_MCR_SUBSLICE(subslice));
> > > + ret = I915_READ(offset);
> > > + I915_WRITE(GEN8_MCR_SELECTOR, tmp);
> > > +
> > > + return ret;
> > > +}
> > 
> > Hmm. I have second thoughts on this. We should take struct_mutex to prevent 
> > any
> > reads/writes while we're doing this.
> 
> Right. I guess taking mutex_lock on the error path would be frowned
> upon. So how about extending uncore.lock from I915_WRITE to the whole
> sequence instead?

Don't grab locks from the error handling code. And it looks like we're
doing funky stuff here anyway, so I'd 

[Intel-gfx] [PATCH v2] drm/i915: prevent out of range pt in the PDE macros (take 3)

[Michel Thierry]

We tried to fix this in commit fdc454c1484a ("drm/i915: Prevent out of
range pt in gen6_for_each_pde").

But the static analyzer still complains that, just before we break due
to "iter < I915_PDES", we do "pt = (pd)->page_table[iter]" with an
iter value that is bigger than I915_PDES. Of course, this isn't really
a problem since no one uses pt outside the macro. Still, every single
new usage of the macro will create a new issue for us to mark as a
false positive.

Also, Paulo re-started the discussion a while ago [1], but didn't end up
implemented.

In order to "solve" this "problem", this patch takes the ideas from
Chris and Dave, but that check would change the desired behavior of the
code, because the object (for example pdp->page_directory[iter]) can be
null during init/alloc, and C would take this as false, breaking the for
loop immediately.

This has been already verified with "static analysis tools".

[1]http://lists.freedesktop.org/archives/intel-gfx/2015-June/068548.html

v2: Make it a single statement, while preventing the common subexpression
elimination (Chris)

Cc: Paulo Zanoni 
Cc: Chris Wilson 
Cc: Dave Gordon 
Signed-off-by: Michel Thierry 
---
 drivers/gpu/drm/i915/i915_gem_gtt.h | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 9fbb07d..a216397 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -394,7 +394,8 @@ struct i915_hw_ppgtt {
  */
 #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
for (iter = gen6_pde_index(start); \
-pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
+length > 0 && iter < I915_PDES ? \
+   (pt = (pd)->page_table[iter]), 1 : 0; \
 iter++, \
 temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
 temp = min_t(unsigned, temp, length), \
@@ -459,7 +460,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
  */
 #define gen8_for_each_pde(pt, pd, start, length, temp, iter)   \
for (iter = gen8_pde_index(start); \
-pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES;   
\
+length > 0 && iter < I915_PDES ? \
+   (pt = (pd)->page_table[iter]), 1 : 0; \
 iter++,\
 temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start,\
 temp = min(temp, length),  \
@@ -467,8 +469,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
 
 #define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter) \
for (iter = gen8_pdpe_index(start); \
-pd = (pdp)->page_directory[iter], \
-length > 0 && (iter < I915_PDPES_PER_PDP(dev)); \
+length > 0 && (iter < I915_PDPES_PER_PDP(dev)) ? \
+   (pd = (pdp)->page_directory[iter]), 1 : 0; \
 iter++,\
 temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start,   \
 temp = min(temp, length),  \
@@ -476,8 +478,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
 
 #define gen8_for_each_pml4e(pdp, pml4, start, length, temp, iter)  \
for (iter = gen8_pml4e_index(start);\
-pdp = (pml4)->pdps[iter], \
-length > 0 && iter < GEN8_PML4ES_PER_PML4; \
+length > 0 && iter < GEN8_PML4ES_PER_PML4 ? \
+   (pdp = (pml4)->pdps[iter]), 1 : 0; \
 iter++,\
 temp = ALIGN(start+1, 1ULL << GEN8_PML4E_SHIFT) - start,   \
 temp = min(temp, length),  \
-- 
2.6.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: prevent out of range pt in the PDE macros (take 3)

[Michel Thierry]

On 10/1/2015 5:09 PM, Chris Wilson wrote:

On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:

---
  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 9fbb07d..94f8344 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
   */
  #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
for (iter = gen6_pde_index(start); \
-pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
+pt = (length > 0 && iter < I915_PDES) ? \
+   (pd)->page_table[iter] : NULL, \
+length > 0 && iter < I915_PDES; \


length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]) : 0,

as the compiler wouldn't be able to CSE it otherwise (I think).


Even after that change, the compiler keeps doing an optimization when 
page_table[iter] is null (takes the null assignment as the break condition).


I've been playing with these examples
http://paste.ubuntu.com/12638106/

Only the 1st example (a) iterates over all elements, b & c stop after 
the 1st run.


Thanks,

-Michel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: prevent out of range pt in the PDE macros (take 3)

[Michel Thierry]

On 10/2/2015 1:58 PM, Chris Wilson wrote:

On Fri, Oct 02, 2015 at 01:47:03PM +0100, Michel Thierry wrote:

On 10/1/2015 5:09 PM, Chris Wilson wrote:

On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:

---
  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 9fbb07d..94f8344 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
   */
  #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
for (iter = gen6_pde_index(start); \
-pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
+pt = (length > 0 && iter < I915_PDES) ? \
+   (pd)->page_table[iter] : NULL, \
+length > 0 && iter < I915_PDES; \


length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]) : 0,

as the compiler wouldn't be able to CSE it otherwise (I think).


Even after that change, the compiler keeps doing an optimization
when page_table[iter] is null (takes the null assignment as the
break condition).

I've been playing with these examples
http://paste.ubuntu.com/12638106/

Only the 1st example (a) iterates over all elements, b & c stop
after the 1st run.


Forgot that was the condition you wanted to change.

length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]), 1 : 0,

Would be nice to prove that length > 0 implies iter < I915_PDES. If only
we had smart tools :)


Thanks, that made it.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/2] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Arun Siluvery]

Some of the HW registers are privileged and cannot be written to from a
non-privileged batch buffers coming from userspace unless they are on whitelist.
Userspace need write access to them to implement mainly preemption related WA 
and
also some general WA.

The reason for using this approach is, the register bits that control preemption
granularity at the HW level are not context save/restored; so even if we set 
these
bits always in kernel they are going to change once the context is switched out.
We can consider making them non-privileged by default but these registers also
contain other chicken bits which should not be allowed to be modified.

In the later revisions controlling bits are save/restored at context level but
in the existing revisions these are exported via other debug registers and 
should
be on the whitelist. This patch adds changes to provide HW with a list of 
registers
to be whitelisted. HW checks this list during execution and provides access 
accordingly.

HW imposes a limit on the number of registers on whitelist and it is per-engine.
At this point we are only enabling whitelist for RCS and we don't foresee any
requirement for other engines.

The registers to be whitelisted are added using generic workaround list 
mechanism,
even these are only enablers for userspace workarounds. But by sharing this
mechanism we get some test assets without additional cost (Mika).

Cc: Mika Kuoppala 
Cc: Nick Hoath 
Signed-off-by: Arun Siluvery 
---
 drivers/gpu/drm/i915/i915_drv.h |  9 -
 drivers/gpu/drm/i915/i915_reg.h |  2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9c10270..44fbd1d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1674,11 +1674,18 @@ struct i915_wa_reg {
u32 mask;
 };
 
-#define I915_MAX_WA_REGS 16
+/*
+ * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
+ * allowing it for RCS as we don't foresee any requirement of having
+ * a whitelist for other engines. When it is really required for
+ * other engines then the limit need to be increased.
+ */
+#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
 
 struct i915_workarounds {
struct i915_wa_reg reg[I915_MAX_WA_REGS];
u32 count;
+   u32 hw_whitelist_index[I915_NUM_RINGS];
 };
 
 struct i915_virtual_gpu {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 05c8621..797c74e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1578,6 +1578,8 @@ enum skl_disp_power_wells {
 #define   RING_WAIT_I8XX   (1<<0) /* gen2, PRBx_HEAD */
 #define   RING_WAIT(1<<11) /* gen3+, PRBx_CTL */
 #define   RING_WAIT_SEMAPHORE  (1<<10) /* gen6+ */
+#define RING_FORCE_TO_NONPRIV(base) ((base)+0x4D0)
+#define   RING_MAX_NONPRIV_SLOTS  12
 
 #define GEN7_TLB_RD_ADDR   0x4700
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c 
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c82c74c..64b2754 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -800,6 +800,22 @@ static int wa_add(struct drm_i915_private *dev_priv,
 
 #define WA_WRITE(addr, val) WA_REG(addr, 0x, val)
 
+static inline int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
+uint32_t reg_addr)
+{
+   struct drm_i915_private *dev_priv = ring->dev->dev_private;
+   struct i915_workarounds *wa = _priv->workarounds;
+   const uint32_t index = wa->hw_whitelist_index[ring->id];
+
+   if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
+   return -EINVAL;
+
+   WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base) + index * 4, reg_addr);
+   wa->hw_whitelist_index[ring->id]++;
+
+   return 0;
+}
+
 static int gen8_init_workarounds(struct intel_engine_cs *ring)
 {
struct drm_device *dev = ring->dev;
@@ -1094,6 +1110,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
WARN_ON(ring->id != RCS);
 
dev_priv->workarounds.count = 0;
+   dev_priv->workarounds.hw_whitelist_index[ring->id] = 0;
 
if (IS_BROADWELL(dev))
return bdw_init_workarounds(ring);
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/sysfs: Grab lock for edid/modes_show

[Daniel Vetter]

We chase pointers/lists without taking the locks protecting them,
which isn't that good.

Fix it.

v2: Actually unlock properly, spotted by Julia.

Cc: Julia Lawall 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_sysfs.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 7506de0a75b4..9bffa63fe849 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -261,23 +261,29 @@ static ssize_t edid_show(struct file *filp, struct 
kobject *kobj,
struct drm_connector *connector = to_drm_connector(connector_dev);
unsigned char *edid;
size_t size;
+   ssize_t ret = 0;
 
+   mutex_lock(>dev->mode_config.mutex);
if (!connector->edid_blob_ptr)
-   return 0;
+   goto unlock;
 
edid = connector->edid_blob_ptr->data;
size = connector->edid_blob_ptr->length;
if (!edid)
-   return 0;
+   goto unlock;
 
if (off >= size)
-   return 0;
+   goto unlock;
 
if (off + count > size)
count = size - off;
memcpy(buf, edid + off, count);
 
-   return count;
+   ret = count;
+   mutex_unlock(>dev->mode_config.mutex);
+unlock:
+
+   return ret;
 }
 
 static ssize_t modes_show(struct device *device,
@@ -288,10 +294,12 @@ static ssize_t modes_show(struct device *device,
struct drm_display_mode *mode;
int written = 0;
 
+   mutex_lock(>dev->mode_config.mutex);
list_for_each_entry(mode, >modes, head) {
written += snprintf(buf + written, PAGE_SIZE - written, "%s\n",
mode->name);
}
+   mutex_unlock(>dev->mode_config.mutex);
 
return written;
 }
-- 
2.5.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 1/1 v2] drm/i915: Add scheduling priority to per-context parameters

[Chris Wilson]

On Thu, Oct 01, 2015 at 04:56:26PM +0100, Dave Gordon wrote:
> Hmmm ... the email seems to have been damaged during composition :(
> I probably shouldn't try to use vi(1) [where '~' means
> toggle-letter-case] over an ssh link [where '~' is an escape, of
> sorts] from another Linux machine inside a PuTTY terminal under
> Windows [where various keys send escape sequences containing '~'] :(
> Anyway, this
> version has the #defines as they actually appeared in the source,
> i.e. starting with UPPERCASE 'I' and not lowercase 'i'!
> 
> The next use for the i915 get/set per-context parameters ioctl,
> ahead of the introduction of the forthcoming GPU scheduler.
> 
> Signed-off-by: Dave Gordon 
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 28 
>  drivers/gpu/drm/i915/i915_gem_context.c | 17 +
>  include/uapi/drm/i915_drm.h |  1 +
>  3 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 279e258..104b711 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -850,6 +850,33 @@ struct i915_ctx_hang_stats {
>   bool banned;
>  };
> 
> +/*
> + * User-settable GFX scheduler priorities are on a scale of 1 (lowest
> + * priority) to 1023 (highest priority). The special value 0 means
> + * "let the system decide my priority automatically"; this is the
> + * default if the user process does not explicitly request a different
> + * priority. Any process may decrease its scheduling priority, but
> + * only a sufficiently-privileged process may increase it. However,
> + * it is always permissible to reset it to "system default", even if
> + * is currently lower than that. Thus, if the system-assigned default
> + * were, say, 256, a process could decrease it to 128, and then to 64.
> + * It could NOT then increase it to 128 again, but COULD request a
> + * priority of 0 -- which would actually reset it to 256, allowing
> + * the process to then request 128 again. (This avoids the issue with
> + * nice(2) priorities, namely that non-super-users can not increase
> + * scheduling priorities of their own processes, even if they were the
> + * ones that decreased the priorities in the first place).

I would prefer not to couple it in such a way. Have a continuous range
-1024,1024 (default 0), but only allow a privileged process to request
a positive priority value. So any process can set any negative/zero value
at any time (thereby getting a small boost in priority at times), but only
the select few can completely gazzump them by setting a positive value.

That is a little more intuitive from my perspective. Have you considered
skipping the nice() step entirely and jump to a setpriority() model?
-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 igt 2/8] lib/igt_core: Add igt_assert_fd

[Morton, Derek J]

>
>
>-Original Message-
>From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of 
>Daniel Vetter
>Sent: Friday, October 2, 2015 9:02 AM
>To: Daniel Stone
>Cc: Vetter, Daniel; intel-gfx@lists.freedesktop.org; Wood, Thomas
>Subject: Re: [Intel-gfx] [PATCH igt 2/8] lib/igt_core: Add igt_assert_fd
>
>On Thu, Oct 01, 2015 at 05:34:02PM +0100, Daniel Stone wrote:
>> Skip open-coding and assert that fds are valid.
>> 
>> Signed-off-by: Daniel Stone 
>> ---
>>  lib/igt_core.h | 11 +++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/lib/igt_core.h b/lib/igt_core.h index 9a5d9c5..8f93e8e 
>> 100644
>> --- a/lib/igt_core.h
>> +++ b/lib/igt_core.h
>> @@ -507,6 +507,17 @@ void igt_exit(void) __attribute__((noreturn));  
>> #define igt_assert_lt(n1, n2) igt_assert_cmpint(n1, <, >=, n2)
>>  
>>  /**
>> + * igt_assert_fd:
>> + * @fd: file descriptor
>> + *
>> + * Fails (sub-) test if the given file descriptor is not valid.
>> + *
>> + * Like igt_assert(), but displays the values being compared on 
>> +failure instead
>> + * of simply printing the stringified expression.
>> + */
>> +#define igt_assert_fd(fd) igt_assert_lte(0, fd)
>
I think this will assert for valid fd's and not invalid ones.
From igt_core.h:
igt_assert_lte
Fails (sub-)test if the second integers is greater than the first.

>If we do this then I think some more verbose output would be nice, like
>
>#igt_assert_fd(fd) igt_assert_f(fd >= 0, "file descriptor " #fd " failed\n");
>
>Large-scale sed would be even more awesome ;-) -Daniel
>
>> +
>> +/**
>>   * igt_require:
>>   * @expr: condition to test
>>   *
>> --
>> 2.5.0
>> 
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>___
>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] drm/i915: prevent out of range pt in the PDE macros (take 3)

[Daniel Vetter]

On Fri, Oct 02, 2015 at 09:58:08AM +0100, Chris Wilson wrote:
> On Fri, Oct 02, 2015 at 09:58:05AM +0200, Daniel Vetter wrote:
> > On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:
> > > We tried to fix this in commit fdc454c1484a ("drm/i915: Prevent out of
> > > range pt in gen6_for_each_pde").
> > > 
> > > But the static analyzer still complains that, just before we break due
> > > to "iter < I915_PDES", we do "pt = (pd)->page_table[iter]" with an
> > > iter value that is bigger than I915_PDES. Of course, this isn't really
> > > a problem since no one uses pt outside the macro. Still, every single
> > > new usage of the macro will create a new issue for us to mark as a
> > > false positive.
> > > 
> > > Also, Paulo re-started the discussion a while ago [1], but didn't end up
> > > implemented.
> > > 
> > > In order to "solve" this "problem", this patch takes the ideas from
> > > Chris and Dave, but that check would change the desired behavior of the
> > > code, because the object (for example pdp->page_directory[iter]) can be
> > > null during init/alloc, and C would take this as false, breaking the for
> > > loop immediately.
> > > 
> > > This has been already verified with "static analysis tools".
> > > 
> > > [1]http://lists.freedesktop.org/archives/intel-gfx/2015-June/068548.html
> > > 
> > > Cc: Paulo Zanoni 
> > > Cc: Chris Wilson 
> > > Cc: Dave Gordon 
> > > Signed-off-by: Michel Thierry 
> > 
> > So maybe I'm dense and not seeing what's really going on, but the only
> > thing we seem to be doing is create a pointer to arr[SIZE], i.e. a pointer
> > to the element right after the last valid one. Pointer arithmetic and
> > comparison are explicitly allowed by the C standard on such a pointer. The
> > only thing not allowed is dereference it (which we don't seem to be doing
> > here).
> 
> You're thinking of &(pd)->page_table[iter] (i.e. (pd)->page_table +
> iter). There is an apparent dereference here of (pd)->page_table[ITER_SIZE].

Oh right.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
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 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice

[Imre Deak]

On Thu, 2015-10-01 at 16:00 -0700, Ben Widawsky wrote:
> On Wed, Sep 30, 2015 at 11:00:46PM +0300, Imre Deak wrote:
> > From: Ben Widawsky 
> > 
> > Signed-off-by: Ben Widawsky 
> > 
> > ---
> > Changes (Imre):
> > - use the new INSTDONE capturing by default on new GENs (On Ben's request)
> > - keep printing the render ring INSTDONE to dmesg
> > - don't hard code the extra_instdone array sizes
> > - fix typo in GEN8_MCR_SLICE/GEN8_MCR_SUBSLICE
> > - fix typo when capturing to extra->row
> > - warn if the MCR selectors are non-zero
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h   |  6 ++--
> >  drivers/gpu/drm/i915/i915_gpu_error.c | 62 
> > +++
> >  drivers/gpu/drm/i915/i915_irq.c   |  4 +--
> >  drivers/gpu/drm/i915/i915_reg.h   |  5 +++
> >  4 files changed, 66 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 621acf1..d1b4011 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -513,10 +513,12 @@ struct drm_i915_error_state {
> > u32 gam_ecochk;
> > u32 gab_ctl;
> > u32 gfx_mode;
> > +#define INSTDONE_SLICE_NUM 3
> > +#define INSTDONE_SUBSLICE_NUM 3
> > struct extra_instdone {
> > u32 slice_common;
> > -   u32 sampler;
> > -   u32 row;
> > +   u32 sampler[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > +   u32 row[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > } extra_instdone;
> >  
> > u64 fence[I915_MAX_NUM_FENCES];
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
> > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index e78e512..c6d1cbc 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -336,7 +336,7 @@ int i915_error_state_to_str(struct 
> > drm_i915_error_state_buf *m,
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_error_state *error = error_priv->error;
> > struct drm_i915_error_object *obj;
> > -   int i, j, offset, elt;
> > +   int i, j, slice, subslice, offset, elt;
> > int max_hangcheck_score;
> >  
> > if (!error) {
> > @@ -385,9 +385,17 @@ int i915_error_state_to_str(struct 
> > drm_i915_error_state_buf *m,
> >  
> > err_printf(m, "  SC_INSTDONE (slice common): 0x%08x\n",
> >error->extra_instdone.slice_common);
> > -   err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > -  error->extra_instdone.sampler);
> > -   err_printf(m, "  ROW_INSTDONE: 0x%08x\n", error->extra_instdone.row);
> > +   for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> 
> Ideally we should only be doing this for the active slices. 

Looking at it now the spec is not too clear to me:
"""
When slice 0 is disabled (when fuse reflection MMADR 0x9120[25] = 0),
this field must be set to a valid slice (slice 1 or slice 2) before
issuing a read to a register in a slice unit.
"""

Why is slice 0 special? I would understand the above if it just said
"the field must be set to a valid slice", without mentioning specific
slices.

But yes, I think it would be safer to access only active slices. I would
also limit the readout to active subslices, although there is no similar
note about it in bspec.

> I was hoping you'd have a chance to look into that.

Ok, I'll resend this patch with the above changed.

>  Do we know if this works on SKUs with fewer slices?

It worked on CHV and BXT.

> > +   for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > +subslice++) {
> > +   struct extra_instdone *extra = >extra_instdone;
> > +
> > +   err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > +  extra->sampler[slice][subslice]);
> > +   err_printf(m, "  ROW_INSTDONE: 0x%08x\n",
> > +  extra->row[slice][subslice]);
> > +   }
> > +   }
> >  
> > if (INTEL_INFO(dev)->gen >= 6) {
> > err_printf(m, "ERROR: 0x%08x\n", error->error);
> > @@ -1383,11 +1391,35 @@ const char *i915_cache_level_str(struct 
> > drm_i915_private *i915, int type)
> > }
> >  }
> >  
> > -/* NB: please notice the memset */
> > +static inline uint32_t instdone_read(struct drm_i915_private *dev_priv, int
> > +slice, int subslice, uint32_t offset) {
> > +   /*
> > +* XXX: The MCR register should be locked, but since we are only using
> > +* it for debug/error state, it's not terribly important to
> > +* synchronize it properly.
> > +*/
> > +   uint32_t tmp = I915_READ(GEN8_MCR_SELECTOR);
> > +   uint32_t ret;
> > +
> > +   /*
> > +* The HW expects the slice and sublice selectors to be reset to 0
> > +* after reading out the registers.
> > +*/
> > +   WARN_ON_ONCE(tmp & (GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK));
> > +
> > +   

[Intel-gfx] [PATCH] drm/sysfs: Grab lock for edid/modes_show

[Daniel Vetter]

We chase pointers/lists without taking the locks protecting them,
which isn't that good.

Fix it.

v2: Actually unlock properly, spotted by Julia.

v3: Put the label _before_ the mutex_unlock (Emil)

Cc: Emil Velikov 
Cc: Julia Lawall 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_sysfs.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 7506de0a75b4..8ceb1cb6166a 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -261,23 +261,29 @@ static ssize_t edid_show(struct file *filp, struct 
kobject *kobj,
struct drm_connector *connector = to_drm_connector(connector_dev);
unsigned char *edid;
size_t size;
+   ssize_t ret = 0;
 
+   mutex_lock(>dev->mode_config.mutex);
if (!connector->edid_blob_ptr)
-   return 0;
+   goto unlock;
 
edid = connector->edid_blob_ptr->data;
size = connector->edid_blob_ptr->length;
if (!edid)
-   return 0;
+   goto unlock;
 
if (off >= size)
-   return 0;
+   goto unlock;
 
if (off + count > size)
count = size - off;
memcpy(buf, edid + off, count);
 
-   return count;
+   ret = count;
+unlock:
+   mutex_unlock(>dev->mode_config.mutex);
+
+   return ret;
 }
 
 static ssize_t modes_show(struct device *device,
@@ -288,10 +294,12 @@ static ssize_t modes_show(struct device *device,
struct drm_display_mode *mode;
int written = 0;
 
+   mutex_lock(>dev->mode_config.mutex);
list_for_each_entry(mode, >modes, head) {
written += snprintf(buf + written, PAGE_SIZE - written, "%s\n",
mode->name);
}
+   mutex_unlock(>dev->mode_config.mutex);
 
return written;
 }
-- 
2.5.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PULL] drm-intel-next

[Daniel Vetter]

Hi Dave,

drm-intel-next-2015-09-28:
- fastboot by default for some systems (Maarten Lankhorts)
- piles of workarounds for bxt and skl
- more fbc work from Paulo
- fix hdmi hotplug detection (Sonika)
- first few patches from Ville to parametrize register macros, prep work for
  typesafe mmio functions
- prep work for nv12 rotation (Tvrtko Ursulin)
- various other bugfixes and improvements all over

I have another backmerge here since things became messy and I didn't
realize you resolved some of them already (usually you complain when
there's a conflict ...).

For 4.4 I plan one more feature round after this and then that's it.

Cheers, Daniel


The following changes since commit 2d4df13c0f9ef56452b1d9a9016cb3946e17bfe5:

  Merge tag 'topic/drm-misc-2015-09-25' of 
git://anongit.freedesktop.org/drm-intel into drm-next (2015-09-30 08:35:45 
+1000)

are available in the git repository at:

  git://anongit.freedesktop.org/drm-intel tags/drm-intel-next-2015-09-28-merged

for you to fetch changes up to 44cc6c08da0b6c8321c6740bbb6a0c6feb45b2c2:

  Merge remote-tracking branch 'airlied/drm-next' into drm-intel-next 
(2015-09-30 08:47:41 +0200)


Andrzej Hajda (1):
  drm/i915: fix handling gen8_emit_flush_coherentl3_wa result

Animesh Manna (3):
  drm/i915/bxt: Path added of dmc firmware ver1 for BXT.
  drm/i915/bxt: Stepping info added for bxt.
  drm/i915/bxt: Modified HAS_CSR, added support for BXT

Arun Siluvery (3):
  drm/i915/gen9: Add WaDisableSamplerPowerBypassForSOPingPong
  drm/i915/bxt: Add WaSetClckGatingDisableMedia
  drm/i915/bxt: Update revision id for BXT C0

Bob Paauwe (1):
  drm/i915/skl: Don't clear all watermarks when updating. (v2)

Chris Wilson (1):
  drm/i915: Defer adding preallocated stolen objects to the VM list

Damien Lespiau (1):
  drm/i915/bxt: Fix wrongly placed ')' in I915_READ()

Daniel Vetter (4):
  Merge remote-tracking branch 'drm-intel/drm-intel-next-queued' into 
drm-intel-next-queued
  drm/i915: Mark debug mod options as _unsafe
  drm/i915: Update DRIVER_DATE to 20150928
  Merge remote-tracking branch 'airlied/drm-next' into drm-intel-next

Dongwon Kim (1):
  drm/i915: Do not hardcode s_max, ss_max and eu_mask for BXT

Egbert Eich (1):
  drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

Geliang Tang (2):
  drm/i915: fix kernel-doc warnings in i915_gem.c
  drm/i915: fix task reference leak in i915_debugfs.c

Jani Nikula (1):
  drm/i915/skl: handle port E in cpt_digital_port_connected

Jesse Barnes (5):
  drm/i915: make CSR firmware messages less verbose
  drm/i915: don't try to load GuC fw on pre-gen9
  drm/i915: add more debug info for when atomic updates fail v3
  drm/i915: cleanup pipe_update trace functions with new crtc debug info v3
  drm/i915: fix crash in error state readout on non-execlist platforms v2

Lukas Wunner (1):
  drm/i915: Spell vga_switcheroo consistently

Maarten Lankhorst (6):
  drm/i915: Set csc coefficients in update_pipe_size.
  drm/i915: Remove references to crtc->active from intel_fbdev.c
  drm/i915: Always try to inherit the initial fb.
  drm/i915: Make updating pipe without modeset atomic.
  drm/i915: skip modeset if compatible for everyone.
  drm/i915: Fix fastboot scalers for skylake.

Masanari Iida (1):
  drm/i915: Fix warnings while make xmldocs caused by intel_lrc.c

Matt Roper (1):
  drm/i915: Don't leak VBT mode data

Michał Winiarski (1):
  drm/i915/gtt: Do not initialize drm_mm twice.

Michel Thierry (2):
  drm/i915: WaEnableForceRestoreInCtxtDescForVCS is for video engines only
  drm/i915/lrc: Prevent preemption when lite-restore is disabled

Nick Hoath (3):
  drm/i915/gen9: Add WaDisableMinuteIaClockGating
  drm/i915: Split alloc from init for lrc
  drm/i915: Remove extraneous request cancel.

Paulo Zanoni (9):
  drm/i915: fix the FBC work allocation failure path
  drm/i915: check for the supported strides on HSW+ FBC
  drm/i915: avoid the last 8mb of stolen on BDW/SKL
  drm/i915: print the correct amount of bytes allocated for the CFB
  drm/i915: don't enable FBC when pixel rate exceeds 95% on HSW/BDW
  drm/i915: apply WaFbcAsynchFlipDisableFbcQueue earlier
  drm/i915: don't apply WaFbcAsynchFlipDisableFbcQueue on SKL
  drm/i915: reject invalid formats for FBC
  drm/i915: fix FBC for cases where crtc->base.y is non-zero

Robert Beckett (1):
  drm/i915/gen9: WA ST Unit Power Optimization Disable

Sagar Arun Kamble (8):
  drm/i915: Fix fb object's frontbuffer-bits
  drm/i915/bxt: WaGsvDisableTurbo
  drm/i915: Increase maximum polling time to 50ms for forcewake 
request/clear ack
  drm/i915: Add IS_SKL_GT3 and IS_SKL_GT4 macro.
  drm/i915: WaRsDisableCoarsePowerGating
  drm/i915: WaRsUseTimeoutMode
  drm/i915: 

Re: [Intel-gfx] [BXT MIPI PATCH v5 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Jani Nikula]

On Thu, 01 Oct 2015, Uma Shankar  wrote:
> From: Shashank Sharma 
>
> SKL and BXT qualifies the HAS_DDI() check, and hence haswell
> modeset functions are re-used for modeset sequence. But DDI
> interface doesn't include support for DSI.
> This patch adds:
> 1. cases for DSI encoder, in those modeset functions and allows
>a CRTC modeset
> 2. Adds call to pre_pll enabled from CRTC modeset function. Nothing
>needs to be done as such in CRTC for DSI encoder, as PLL, clock
>and and transcoder programming will be taken care in encoder's
>pre_enable and pre_pll_enable function.
>
> v2: Fixed Jani's review comments. Added INVALID_PORT for non DDI
> encoder like DSI for platforms having HAS_DDI as true.
>
> v3: Rebased on latest drm-nightly branch. Added a WARN_ON for invalid
> encoder.
>
> v4: WARN_ON for invalid encoder is refactored as per Jani's suggestion.
> Fixed the sequence for pre_pll_enable.
>
> v5: Protected DDI code paths in case of DSI encoder calls.
>
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Uma Shankar 

Reviewed-by: Jani Nikula 

> ---
>  drivers/gpu/drm/i915/intel_ddi.c  |7 +--
>  drivers/gpu/drm/i915/intel_display.c  |   21 +++--
>  drivers/gpu/drm/i915/intel_opregion.c |9 +++--
>  3 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> b/drivers/gpu/drm/i915/intel_ddi.c
> index cacb07b..7b7f544 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -390,8 +390,10 @@ void intel_prepare_ddi(struct drm_device *dev)
>   enum port port;
>   bool supports_hdmi;
>  
> - ddi_get_encoder_port(intel_encoder, _dig_port, );
> + if (intel_encoder->type == INTEL_OUTPUT_DSI)
> + continue;
>  
> + ddi_get_encoder_port(intel_encoder, _dig_port, );
>   if (visited[port])
>   continue;
>  
> @@ -1779,7 +1781,8 @@ bool intel_ddi_get_hw_state(struct intel_encoder 
> *encoder,
>  void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)
>  {
>   struct drm_crtc *crtc = _crtc->base;
> - struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> + struct drm_device *dev = crtc->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>   enum port port = intel_ddi_get_encoder_port(intel_encoder);
>   enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b8e0310..ea0f533 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4991,6 +4991,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   struct intel_encoder *encoder;
> + bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
>   int pipe = intel_crtc->pipe;
>  
>   WARN_ON(!crtc->state->enable);
> @@ -5023,9 +5024,12 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>   intel_crtc->active = true;
>  
>   intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> + for_each_encoder_on_crtc(dev, crtc, encoder) {
> + if (encoder->pre_pll_enable)
> + encoder->pre_pll_enable(encoder);
>   if (encoder->pre_enable)
>   encoder->pre_enable(encoder);
> + }
>  
>   if (intel_crtc->config->has_pch_encoder) {
>   intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> @@ -5033,7 +5037,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>   dev_priv->display.fdi_link_train(crtc);
>   }
>  
> - intel_ddi_enable_pipe_clock(intel_crtc);
> + if (!is_dsi)
> + intel_ddi_enable_pipe_clock(intel_crtc);
>  
>   if (INTEL_INFO(dev)->gen == 9)
>   skylake_pfit_update(intel_crtc, 1);
> @@ -5049,7 +5054,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>   intel_crtc_load_lut(crtc);
>  
>   intel_ddi_set_pipe_settings(crtc);
> - intel_ddi_enable_transcoder_func(crtc);
> + if (!is_dsi)
> + intel_ddi_enable_transcoder_func(crtc);
>  
>   intel_update_watermarks(crtc);
>   intel_enable_pipe(intel_crtc);
> @@ -5057,7 +5063,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>   if (intel_crtc->config->has_pch_encoder)
>   lpt_pch_enable(crtc);
>  
> - if (intel_crtc->config->dp_encoder_is_mst)
> + if (intel_crtc->config->dp_encoder_is_mst && !is_dsi)
>   

Re: [Intel-gfx] [PATCH] drm/sysfs: Grab lock for edid/modes_show

[Emil Velikov]

On 2 October 2015 at 12:01, Daniel Vetter  wrote:
> We chase pointers/lists without taking the locks protecting them,
> which isn't that good.
>
> Fix it.
>
> v2: Actually unlock properly, spotted by Julia.
>
> v3: Put the label _before_ the mutex_unlock (Emil)
>
> Cc: Emil Velikov 
> Cc: Julia Lawall 
> Signed-off-by: Daniel Vetter 
Patch does exactly what it says on the tin.

Reviewed-by: Emil Velikov 

-Emil
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: prevent out of range pt in the PDE macros (take 3)

[Chris Wilson]

On Fri, Oct 02, 2015 at 09:58:05AM +0200, Daniel Vetter wrote:
> On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:
> > We tried to fix this in commit fdc454c1484a ("drm/i915: Prevent out of
> > range pt in gen6_for_each_pde").
> > 
> > But the static analyzer still complains that, just before we break due
> > to "iter < I915_PDES", we do "pt = (pd)->page_table[iter]" with an
> > iter value that is bigger than I915_PDES. Of course, this isn't really
> > a problem since no one uses pt outside the macro. Still, every single
> > new usage of the macro will create a new issue for us to mark as a
> > false positive.
> > 
> > Also, Paulo re-started the discussion a while ago [1], but didn't end up
> > implemented.
> > 
> > In order to "solve" this "problem", this patch takes the ideas from
> > Chris and Dave, but that check would change the desired behavior of the
> > code, because the object (for example pdp->page_directory[iter]) can be
> > null during init/alloc, and C would take this as false, breaking the for
> > loop immediately.
> > 
> > This has been already verified with "static analysis tools".
> > 
> > [1]http://lists.freedesktop.org/archives/intel-gfx/2015-June/068548.html
> > 
> > Cc: Paulo Zanoni 
> > Cc: Chris Wilson 
> > Cc: Dave Gordon 
> > Signed-off-by: Michel Thierry 
> 
> So maybe I'm dense and not seeing what's really going on, but the only
> thing we seem to be doing is create a pointer to arr[SIZE], i.e. a pointer
> to the element right after the last valid one. Pointer arithmetic and
> comparison are explicitly allowed by the C standard on such a pointer. The
> only thing not allowed is dereference it (which we don't seem to be doing
> here).

You're thinking of &(pd)->page_table[iter] (i.e. (pd)->page_table +
iter). There is an apparent dereference here of (pd)->page_table[ITER_SIZE].
-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 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice

[Imre Deak]

On Thu, 2015-10-01 at 16:56 -0700, Ben Widawsky wrote:
> On Wed, Sep 30, 2015 at 11:00:46PM +0300, Imre Deak wrote:
> > From: Ben Widawsky 
> > 
> > Signed-off-by: Ben Widawsky 
> > 
> > ---
> > Changes (Imre):
> > - use the new INSTDONE capturing by default on new GENs (On Ben's request)
> > - keep printing the render ring INSTDONE to dmesg
> > - don't hard code the extra_instdone array sizes
> > - fix typo in GEN8_MCR_SLICE/GEN8_MCR_SUBSLICE
> > - fix typo when capturing to extra->row
> > - warn if the MCR selectors are non-zero
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h   |  6 ++--
> >  drivers/gpu/drm/i915/i915_gpu_error.c | 62 
> > +++
> >  drivers/gpu/drm/i915/i915_irq.c   |  4 +--
> >  drivers/gpu/drm/i915/i915_reg.h   |  5 +++
> >  4 files changed, 66 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 621acf1..d1b4011 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -513,10 +513,12 @@ struct drm_i915_error_state {
> > u32 gam_ecochk;
> > u32 gab_ctl;
> > u32 gfx_mode;
> > +#define INSTDONE_SLICE_NUM 3
> > +#define INSTDONE_SUBSLICE_NUM 3
> > struct extra_instdone {
> > u32 slice_common;
> > -   u32 sampler;
> > -   u32 row;
> > +   u32 sampler[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > +   u32 row[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > } extra_instdone;
> >  
> > u64 fence[I915_MAX_NUM_FENCES];
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
> > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index e78e512..c6d1cbc 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -336,7 +336,7 @@ int i915_error_state_to_str(struct 
> > drm_i915_error_state_buf *m,
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_error_state *error = error_priv->error;
> > struct drm_i915_error_object *obj;
> > -   int i, j, offset, elt;
> > +   int i, j, slice, subslice, offset, elt;
> > int max_hangcheck_score;
> >  
> > if (!error) {
> > @@ -385,9 +385,17 @@ int i915_error_state_to_str(struct 
> > drm_i915_error_state_buf *m,
> >  
> > err_printf(m, "  SC_INSTDONE (slice common): 0x%08x\n",
> >error->extra_instdone.slice_common);
> > -   err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > -  error->extra_instdone.sampler);
> > -   err_printf(m, "  ROW_INSTDONE: 0x%08x\n", error->extra_instdone.row);
> > +   for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> > +   for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > +subslice++) {
> > +   struct extra_instdone *extra = >extra_instdone;
> > +
> > +   err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > +  extra->sampler[slice][subslice]);
> > +   err_printf(m, "  ROW_INSTDONE: 0x%08x\n",
> > +  extra->row[slice][subslice]);
> > +   }
> > +   }
> >  
> > if (INTEL_INFO(dev)->gen >= 6) {
> > err_printf(m, "ERROR: 0x%08x\n", error->error);
> > @@ -1383,11 +1391,35 @@ const char *i915_cache_level_str(struct 
> > drm_i915_private *i915, int type)
> > }
> >  }
> >  
> > -/* NB: please notice the memset */
> > +static inline uint32_t instdone_read(struct drm_i915_private *dev_priv, int
> > +slice, int subslice, uint32_t offset) {
> > +   /*
> > +* XXX: The MCR register should be locked, but since we are only using
> > +* it for debug/error state, it's not terribly important to
> > +* synchronize it properly.
> > +*/
> > +   uint32_t tmp = I915_READ(GEN8_MCR_SELECTOR);
> > +   uint32_t ret;
> > +
> > +   /*
> > +* The HW expects the slice and sublice selectors to be reset to 0
> > +* after reading out the registers.
> > +*/
> > +   WARN_ON_ONCE(tmp & (GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK));
> > +
> > +   I915_WRITE(GEN8_MCR_SELECTOR, tmp | GEN8_MCR_SLICE(slice) |
> > +   GEN8_MCR_SUBSLICE(subslice));
> > +   ret = I915_READ(offset);
> > +   I915_WRITE(GEN8_MCR_SELECTOR, tmp);
> > +
> > +   return ret;
> > +}
> 
> Hmm. I have second thoughts on this. We should take struct_mutex to prevent 
> any
> reads/writes while we're doing this.

Right. I guess taking mutex_lock on the error path would be frowned
upon. So how about extending uncore.lock from I915_WRITE to the whole
sequence instead?

> > +
> >  void i915_get_extra_instdone(struct drm_device *dev,
> >  struct extra_instdone *extra)
> >  {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > +   int slice, subslice;
> >  
> > /*
> >  * The render INSTDONE register (GEN2_INSTDONE, 

[Intel-gfx] [PATCH i-g-t] lib/core: Fix docs for igt_assert_lt(e)

[Daniel Vetter]

Logical negation is hard.

Cc: "Morton, Derek J" 
Signed-off-by: Daniel Vetter 
---
 lib/igt_core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_core.h b/lib/igt_core.h
index f8bfbf0145e1..293f691d0025 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -458,7 +458,7 @@ void igt_exit(void) __attribute__((noreturn));
  * @n1: first integer
  * @n2: second integer
  *
- * Fails (sub-)test if the second integers is greater than the first.
+ * Fails (sub-)test if the second integers is strictly smaller than the first.
  * Beware that for now this only works on integers.
  *
  * Like igt_assert(), but displays the values being compared on failure instead
@@ -471,7 +471,7 @@ void igt_exit(void) __attribute__((noreturn));
  * @n1: first integer
  * @n2: second integer
  *
- * Fails (sub-)test if the second integers is strictly smaller than the first.
+ * Fails (sub-)test if the second integers is smaller than or equal to the 
first.
  * Beware that for now this only works on integers.
  *
  * Like igt_assert(), but displays the values being compared on failure instead
-- 
2.5.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BXT MIPI PATCH v5 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Daniel Vetter]

On Thu, Oct 01, 2015 at 10:23:49PM +0530, Uma Shankar wrote:
> From: Shashank Sharma 
> 
> SKL and BXT qualifies the HAS_DDI() check, and hence haswell
> modeset functions are re-used for modeset sequence. But DDI
> interface doesn't include support for DSI.
> This patch adds:
> 1. cases for DSI encoder, in those modeset functions and allows
>a CRTC modeset
> 2. Adds call to pre_pll enabled from CRTC modeset function. Nothing
>needs to be done as such in CRTC for DSI encoder, as PLL, clock
>and and transcoder programming will be taken care in encoder's
>pre_enable and pre_pll_enable function.
> 
> v2: Fixed Jani's review comments. Added INVALID_PORT for non DDI
> encoder like DSI for platforms having HAS_DDI as true.
> 
> v3: Rebased on latest drm-nightly branch. Added a WARN_ON for invalid
> encoder.
> 
> v4: WARN_ON for invalid encoder is refactored as per Jani's suggestion.
> Fixed the sequence for pre_pll_enable.
> 
> v5: Protected DDI code paths in case of DSI encoder calls.
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Uma Shankar 

Ok, after this patch we get stuff like this:

for_each_encoder_on_crtc(dev, crtc, encoder) {
if (encoder->pre_pll_enable)
encoder->pre_pll_enable(encoder);
if (encoder->pre_enable)
encoder->pre_enable(encoder);
}

if (intel_crtc->config->has_pch_encoder) {
intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
  true);
dev_priv->display.fdi_link_train(crtc);
}

if (!is_dsi)
intel_ddi_enable_pipe_clock(intel_crtc);

1. Please remove pre_pll_enable again, we don't need 2 callbacks in
exactly the same spot. Yes this might mean that you need special bxt_
versions of that in the dsi encoder, we have that everywhere.

2. the has_pch_encoder is already something encoder-specific (it's
exclusively used by the HSW LPT CRT encoder). Now we have another one of
those for the !is_dsi case. These special-cases should be moved into the
encoder->pre_enable callbacks, that's what they're for.

I'm not going to block these patches are (18months is already ridiculous),
but I want this cleanup done. Uma, can you pls own this? If you can't do
it yourself please escalate to Indranil so he can find someone.

Thanks, Daniel


> ---
>  drivers/gpu/drm/i915/intel_ddi.c  |7 +--
>  drivers/gpu/drm/i915/intel_display.c  |   21 +++--
>  drivers/gpu/drm/i915/intel_opregion.c |9 +++--
>  3 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> b/drivers/gpu/drm/i915/intel_ddi.c
> index cacb07b..7b7f544 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -390,8 +390,10 @@ void intel_prepare_ddi(struct drm_device *dev)
>   enum port port;
>   bool supports_hdmi;
>  
> - ddi_get_encoder_port(intel_encoder, _dig_port, );
> + if (intel_encoder->type == INTEL_OUTPUT_DSI)
> + continue;
>  
> + ddi_get_encoder_port(intel_encoder, _dig_port, );
>   if (visited[port])
>   continue;
>  
> @@ -1779,7 +1781,8 @@ bool intel_ddi_get_hw_state(struct intel_encoder 
> *encoder,
>  void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)
>  {
>   struct drm_crtc *crtc = _crtc->base;
> - struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> + struct drm_device *dev = crtc->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>   enum port port = intel_ddi_get_encoder_port(intel_encoder);
>   enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b8e0310..ea0f533 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4991,6 +4991,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   struct intel_encoder *encoder;
> + bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
>   int pipe = intel_crtc->pipe;
>  
>   WARN_ON(!crtc->state->enable);
> @@ -5023,9 +5024,12 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>   intel_crtc->active = true;
>  
>   intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> + for_each_encoder_on_crtc(dev, crtc, encoder) {
> + if (encoder->pre_pll_enable)
> + 

[Intel-gfx] [patch] drm/i915: unlock on error in i915_ppgtt_info()

[Dan Carpenter]

We need to call intel_runtime_pm_put() and mutex_unlock() before
returning.

Fixes: 7cb5dff8d59d ('drm/i915: fix task reference leak in i915_debugfs.c')
Signed-off-by: Dan Carpenter 

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index afa7982..dcc50f3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2294,18 +2294,21 @@ static int i915_ppgtt_info(struct seq_file *m, void 
*data)
struct task_struct *task;
 
task = get_pid_task(file->pid, PIDTYPE_PID);
-   if (!task)
-   return -ESRCH;
+   if (!task) {
+   ret = -ESRCH;
+   goto out_put;
+   }
seq_printf(m, "\nproc: %s\n", task->comm);
put_task_struct(task);
idr_for_each(_priv->context_idr, per_file_ctx,
 (void *)(unsigned long)m);
}
 
+out_put:
intel_runtime_pm_put(dev_priv);
mutex_unlock(>struct_mutex);
 
-   return 0;
+   return ret;
 }
 
 static int count_irq_waiters(struct drm_i915_private *i915)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Add GEN7_GPGPU_DISPATCHDIMX/Y/Z to the register whitelist

[Jordan Justen]

On 2015-10-02 01:06:01, Daniel Vetter wrote:
> On Thu, Oct 01, 2015 at 11:09:58PM -0700, Jordan Justen wrote:
> > This is required to support glDispatchComputeIndirect for gen7.
> > 
> > Signed-off-by: Jordan Justen 
> > Reviewed-by: Kristian Høgsberg 
> 
> Do you have a link to the mesa patches? As soon as those are r-b'ed I'll
> pull this in.

The patch that uses these registers in mesa is already upstream.

http://cgit.freedesktop.org/mesa/mesa/commit/?id=ebbe6cdad7ab082d2b191fe6c7c0eaa6921d55de

-Jordan

> > ---
> >  drivers/gpu/drm/i915/i915_cmd_parser.c | 6 +-
> >  drivers/gpu/drm/i915/i915_reg.h| 4 
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c 
> > b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index 09932ca..db58c8d 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -448,6 +448,9 @@ static const struct drm_i915_reg_descriptor 
> > gen7_render_regs[] = {
> >   REG32(GEN7_3DPRIM_INSTANCE_COUNT),
> >   REG32(GEN7_3DPRIM_START_INSTANCE),
> >   REG32(GEN7_3DPRIM_BASE_VERTEX),
> > + REG32(GEN7_GPGPU_DISPATCHDIMX),
> > + REG32(GEN7_GPGPU_DISPATCHDIMY),
> > + REG32(GEN7_GPGPU_DISPATCHDIMZ),
> >   REG64(GEN7_SO_NUM_PRIMS_WRITTEN(0)),
> >   REG64(GEN7_SO_NUM_PRIMS_WRITTEN(1)),
> >   REG64(GEN7_SO_NUM_PRIMS_WRITTEN(2)),
> > @@ -1214,6 +1217,7 @@ int i915_cmd_parser_get_version(void)
> >*MI_PREDICATE_SRC1 registers.
> >* 3. Allow access to the GPGPU_THREADS_DISPATCHED register.
> >* 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3.
> > +  * 5. GPGPU dispatch compute indirect registers.
> >*/
> > - return 4;
> > + return 5;
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 56157eb..f2b6425 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -536,6 +536,10 @@
> >  #define GEN7_3DPRIM_START_INSTANCE  0x243C
> >  #define GEN7_3DPRIM_BASE_VERTEX 0x2440
> >  
> > +#define GEN7_GPGPU_DISPATCHDIMX 0x2500
> > +#define GEN7_GPGPU_DISPATCHDIMY 0x2504
> > +#define GEN7_GPGPU_DISPATCHDIMZ 0x2508
> > +
> >  #define OACONTROL 0x2360
> >  
> >  #define _GEN7_PIPEA_DE_LOAD_SL   0x70068
> > -- 
> > 2.5.1
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/sysfs: Grab lock for edid/modes_show

[Julia Lawall]

On Fri, 2 Oct 2015, Daniel Vetter wrote:

> We chase pointers/lists without taking the locks protecting them,
> which isn't that good.
>
> Fix it.
>
> v2: Actually unlock properly, spotted by Julia.

The unlock is still on top of the unlock label?

julia

>
> Cc: Julia Lawall 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_sysfs.c | 16 
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 7506de0a75b4..9bffa63fe849 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -261,23 +261,29 @@ static ssize_t edid_show(struct file *filp, struct 
> kobject *kobj,
>   struct drm_connector *connector = to_drm_connector(connector_dev);
>   unsigned char *edid;
>   size_t size;
> + ssize_t ret = 0;
>
> + mutex_lock(>dev->mode_config.mutex);
>   if (!connector->edid_blob_ptr)
> - return 0;
> + goto unlock;
>
>   edid = connector->edid_blob_ptr->data;
>   size = connector->edid_blob_ptr->length;
>   if (!edid)
> - return 0;
> + goto unlock;
>
>   if (off >= size)
> - return 0;
> + goto unlock;
>
>   if (off + count > size)
>   count = size - off;
>   memcpy(buf, edid + off, count);
>
> - return count;
> + ret = count;
> + mutex_unlock(>dev->mode_config.mutex);
> +unlock:
> +
> + return ret;
>  }
>
>  static ssize_t modes_show(struct device *device,
> @@ -288,10 +294,12 @@ static ssize_t modes_show(struct device *device,
>   struct drm_display_mode *mode;
>   int written = 0;
>
> + mutex_lock(>dev->mode_config.mutex);
>   list_for_each_entry(mode, >modes, head) {
>   written += snprintf(buf + written, PAGE_SIZE - written, "%s\n",
>   mode->name);
>   }
> + mutex_unlock(>dev->mode_config.mutex);
>
>   return written;
>  }
> --
> 2.5.1
>
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice

[Ben Widawsky]

On Fri, Oct 02, 2015 at 01:58:13PM +0300, Imre Deak wrote:
> On Thu, 2015-10-01 at 16:00 -0700, Ben Widawsky wrote:
> > On Wed, Sep 30, 2015 at 11:00:46PM +0300, Imre Deak wrote:
> > > From: Ben Widawsky 
> > > 
> > > Signed-off-by: Ben Widawsky 
> > > 
> > > ---
> > > Changes (Imre):
> > > - use the new INSTDONE capturing by default on new GENs (On Ben's request)
> > > - keep printing the render ring INSTDONE to dmesg
> > > - don't hard code the extra_instdone array sizes
> > > - fix typo in GEN8_MCR_SLICE/GEN8_MCR_SUBSLICE
> > > - fix typo when capturing to extra->row
> > > - warn if the MCR selectors are non-zero
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h   |  6 ++--
> > >  drivers/gpu/drm/i915/i915_gpu_error.c | 62 
> > > +++
> > >  drivers/gpu/drm/i915/i915_irq.c   |  4 +--
> > >  drivers/gpu/drm/i915/i915_reg.h   |  5 +++
> > >  4 files changed, 66 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 621acf1..d1b4011 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -513,10 +513,12 @@ struct drm_i915_error_state {
> > >   u32 gam_ecochk;
> > >   u32 gab_ctl;
> > >   u32 gfx_mode;
> > > +#define INSTDONE_SLICE_NUM 3
> > > +#define INSTDONE_SUBSLICE_NUM 3
> > >   struct extra_instdone {
> > >   u32 slice_common;
> > > - u32 sampler;
> > > - u32 row;
> > > + u32 sampler[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > > + u32 row[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > >   } extra_instdone;
> > >  
> > >   u64 fence[I915_MAX_NUM_FENCES];
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
> > > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index e78e512..c6d1cbc 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -336,7 +336,7 @@ int i915_error_state_to_str(struct 
> > > drm_i915_error_state_buf *m,
> > >   struct drm_i915_private *dev_priv = dev->dev_private;
> > >   struct drm_i915_error_state *error = error_priv->error;
> > >   struct drm_i915_error_object *obj;
> > > - int i, j, offset, elt;
> > > + int i, j, slice, subslice, offset, elt;
> > >   int max_hangcheck_score;
> > >  
> > >   if (!error) {
> > > @@ -385,9 +385,17 @@ int i915_error_state_to_str(struct 
> > > drm_i915_error_state_buf *m,
> > >  
> > >   err_printf(m, "  SC_INSTDONE (slice common): 0x%08x\n",
> > >  error->extra_instdone.slice_common);
> > > - err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > > -error->extra_instdone.sampler);
> > > - err_printf(m, "  ROW_INSTDONE: 0x%08x\n", error->extra_instdone.row);
> > > + for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> > 
> > Ideally we should only be doing this for the active slices. 
> 
> Looking at it now the spec is not too clear to me:
> """
> When slice 0 is disabled (when fuse reflection MMADR 0x9120[25] = 0),
> this field must be set to a valid slice (slice 1 or slice 2) before
> issuing a read to a register in a slice unit.
> """
> 

The default is to go to slice 0. So if you have slice 0 disabled (which I
believe only exists in special fused configs) you will end up reading garbage on
the MMIO. This is true of all MMIO, not just INSTDONE. In other words, if we
actually have such things shipping, they surely aren't working today.

> Why is slice 0 special? I would understand the above if it just said
> "the field must be set to a valid slice", without mentioning specific
> slices.
> 
> But yes, I think it would be safer to access only active slices. I would
> also limit the readout to active subslices, although there is no similar
> note about it in bspec.
> 
> > I was hoping you'd have a chance to look into that.
> 
> Ok, I'll resend this patch with the above changed.
> 
> >  Do we know if this works on SKUs with fewer slices?
> 
> It worked on CHV and BXT.
> 
> > > + for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > > +  subslice++) {
> > > + struct extra_instdone *extra = >extra_instdone;
> > > +
> > > + err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > > +extra->sampler[slice][subslice]);
> > > + err_printf(m, "  ROW_INSTDONE: 0x%08x\n",
> > > +extra->row[slice][subslice]);
> > > + }
> > > + }
> > >  
> > >   if (INTEL_INFO(dev)->gen >= 6) {
> > >   err_printf(m, "ERROR: 0x%08x\n", error->error);
> > > @@ -1383,11 +1391,35 @@ const char *i915_cache_level_str(struct 
> > > drm_i915_private *i915, int type)
> > >   }
> > >  }
> > >  
> > > -/* NB: please notice the memset */
> > > +static inline uint32_t instdone_read(struct drm_i915_private *dev_priv, 
> > > int
> > > +  slice, int subslice, uint32_t offset) {
> > > + 

Re: [Intel-gfx] [PATCH v3] drm/i915/guc: Add host2guc notification for suspend and resume

[Kamble, Sagar A]

Reviewed-by: Sagar Arun Kamble 

On 9/30/2015 10:16 PM, yu@intel.com wrote:

From: Alex Dai 

Add host2guc interface to notify GuC power state changes when
enter or resume from power saving state.

v3: Move intel_guc_suspend to i915_drm_suspend for consistency.

v2: Add GuC suspend/resume to runtime suspend/resume too

v1: Change to a more flexible way when fill host to GuC scratch
data in order to remove hard coding.

Signed-off-by: Alex Dai 
---
  drivers/gpu/drm/i915/i915_drv.c|  8 +
  drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++
  drivers/gpu/drm/i915/intel_guc.h   |  2 ++
  drivers/gpu/drm/i915/intel_guc_fwif.h  |  8 +
  drivers/gpu/drm/i915/intel_guc_loader.c|  4 ++-
  5 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1cb6b82..760e0ce 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -638,6 +638,8 @@ static int i915_drm_suspend(struct drm_device *dev)
return error;
}
  
+	intel_guc_suspend(dev);

+
intel_suspend_gt_powersave(dev);
  
  	/*

@@ -767,6 +769,8 @@ static int i915_drm_resume(struct drm_device *dev)
}
mutex_unlock(>struct_mutex);
  
+	intel_guc_resume(dev);

+
intel_modeset_init_hw(dev);
  
  	spin_lock_irq(_priv->irq_lock);

@@ -1500,6 +1504,8 @@ static int intel_runtime_suspend(struct device *device)
i915_gem_release_all_mmaps(dev_priv);
mutex_unlock(>struct_mutex);
  
+	intel_guc_suspend(dev);

+
intel_suspend_gt_powersave(dev);
intel_runtime_pm_disable_interrupts(dev_priv);
  
@@ -1559,6 +1565,8 @@ static int intel_runtime_resume(struct device *device)

intel_opregion_notify_adapter(dev, PCI_D0);
dev_priv->pm.suspended = false;
  
+	intel_guc_resume(dev);

+
if (IS_GEN6(dev_priv))
intel_init_pch_refclk(dev);
  
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c

index 0b1797f..036b42b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -923,3 +923,53 @@ void i915_guc_submission_fini(struct drm_device *dev)
gem_release_guc_obj(guc->ctx_pool_obj);
guc->ctx_pool_obj = NULL;
  }
+
+/**
+ * intel_guc_suspend() - notify GuC entering suspend state
+ * @dev:   drm device
+ */
+int intel_guc_suspend(struct drm_device *dev)
+{
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct intel_guc *guc = _priv->guc;
+   struct intel_context *ctx;
+   u32 data[3];
+
+   if (!i915.enable_guc_submission)
+   return 0;
+
+   ctx = dev_priv->ring[RCS].default_context;
+
+   data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
+   /* any value greater than GUC_POWER_D0 */
+   data[1] = GUC_POWER_D1;
+   /* first page is shared data with GuC */
+   data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state);
+
+   return host2guc_action(guc, data, ARRAY_SIZE(data));
+}
+
+
+/**
+ * intel_guc_resume() - notify GuC resuming from suspend state
+ * @dev:   drm device
+ */
+int intel_guc_resume(struct drm_device *dev)
+{
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct intel_guc *guc = _priv->guc;
+   struct intel_context *ctx;
+   u32 data[3];
+
+   if (!i915.enable_guc_submission)
+   return 0;
+
+   ctx = dev_priv->ring[RCS].default_context;
+
+   data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
+   data[1] = GUC_POWER_D0;
+   /* first page is shared data with GuC */
+   data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state);
+
+   return host2guc_action(guc, data, ARRAY_SIZE(data));
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 63e73f3..5ba5866 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -116,6 +116,8 @@ extern void intel_guc_ucode_init(struct drm_device *dev);
  extern int intel_guc_ucode_load(struct drm_device *dev);
  extern void intel_guc_ucode_fini(struct drm_device *dev);
  extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
+extern int intel_guc_suspend(struct drm_device *dev);
+extern int intel_guc_resume(struct drm_device *dev);
  
  /* i915_guc_submission.c */

  int i915_guc_submission_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h 
b/drivers/gpu/drm/i915/intel_guc_fwif.h
index f0a9e82..d25c5b7 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -293,12 +293,20 @@ struct guc_context_desc {
  #define GUC_FORCEWAKE_RENDER  (1 << 0)
  #define GUC_FORCEWAKE_MEDIA   (1 << 1)
  
+#define GUC_POWER_UNSPECIFIED	0

+#define GUC_POWER_D0   1
+#define GUC_POWER_D1   2

Re: [Intel-gfx] [patch] drm/i915: unlock on error in i915_ppgtt_info()

[Geliang Tang]

On Fri, Oct 02, 2015 at 06:14:22PM +0300, Dan Carpenter wrote:
> We need to call intel_runtime_pm_put() and mutex_unlock() before
> returning.
> 
> Fixes: 7cb5dff8d59d ('drm/i915: fix task reference leak in i915_debugfs.c')
> Signed-off-by: Dan Carpenter 

Acked-by: Geliang Tang 

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 3/8] drm/i915/ivb: Move WaCxSRDisabledForSpriteScaling w/a to atomic check

[Egbert Eich]

Regarding commit 7809e5ae35b9d8d0710f0874b2e3f10be144e38b

On Wed, Jul 01, 2015 at 07:25:56PM -0700, Matt Roper wrote:
> Determine whether we need to apply this workaround at atomic check time
> and just set a flag that will be used by the main watermark update
> routine.
> 
> Moving this workaround into the atomic framework reduces
> ilk_update_sprite_wm() to just a standard watermark update, so drop it
> completely and just ensure that ilk_update_wm() is called whenever a
> sprite plane is updated in a way that would affect watermarks.
> 
> Signed-off-by: Matt Roper 


This patch causes intel_update_watermarks() to be called much more
frequently although the watermark values don't change. 
The change responsible for this is:

> index 5de1ded..46ef981 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11560,23 +11560,38 @@ retry:
>  static bool intel_wm_need_update(struct drm_plane *plane,
>struct drm_plane_state *state)
>  {
> - /* Update watermarks on tiling changes. */
> + struct intel_plane_state *new = to_intel_plane_state(state);
> + struct intel_plane_state *cur = to_intel_plane_state(plane->state);
> +
> + /* Update watermarks on tiling or size changes. */
>   if (!plane->state->fb || !state->fb ||
>   plane->state->fb->modifier[0] != state->fb->modifier[0] ||
> - plane->state->rotation != state->rotation)
> - return true;
> -

> - if (plane->state->crtc_w != state->crtc_w)

A quick look thru intel_pm.c reveals that this is  relevant for
WM caluclations for gen=4 and any chipsets for which is_g4x is true.
Should this really be removed?

> + plane->state->rotation != state->rotation ||

> + drm_rect_width(>src) != drm_rect_width(>src) ||
> + drm_rect_height(>src) != drm_rect_height(>src) ||
> + drm_rect_width(>dst) != drm_rect_width(>dst) ||
> + drm_rect_height(>dst) != drm_rect_height(>dst))

these values seem to be used only for watermark calculations on gen < 9 when
HAS_PCH_SPLIT() is true.

Still these are responsible for most of the watermark recalculations (when the 
mouse
cursor is moved towards the edge for example). On the system I'm looking at the 
moment
(Q35) changes in these values don't change the WMs.

Since WM calculation is very chip generation specific and differs considerably 
between
generations, wouldn't it make sense to either have chipset specific functions 
to determin
whether a recalculation is needed - or even perfrom this in the update_wm() 
function
itself?

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] fixup! drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v3)

[Zanoni, Paulo R]

Em Qui, 2015-10-01 às 16:03 -0700, Matt Roper escreveu:
> Cc: Paulo Zanoni 
> Signed-off-by: Matt Roper 
> ---
> Paulo, I'm not positive that this is the cause of the issues you're
> seeing, but
> I did find this unwanted behavior change while re-reading all the SKL
> watermark
> code.  Could you give this a try and see if it improves your
> situation at all?

Thanks for the patch, but unfortunately this doesn't solve the problems
I'm seeing.

For my normal work activities I'm carrying a patch that reverts the
following commits:

drm/i915: Calculate watermark configuration during atomic check (v2)
drm/i915: Don't set plane visible during HW readout if CRTC is off
drm/i915: Calculate ILK-style watermarks during atomic check (v3)
drm/i915: Calculate pipe watermarks into CRTC state (v3)
drm/i915: Refactor ilk_update_wm (v3)
drm/i915: Drop intel_update_sprite_watermarks

So I guess the sprite update thing is very likely the first bad commit.
I'm also noticing that the screen stays black for _way_ too much time
during boot, but I'm not sure it's caused by the watermark series:
might be something else on -nightly.

Thanks,
Paulo

> 
>  drivers/gpu/drm/i915/intel_pm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 3857592..22c97f2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2951,6 +2951,9 @@ skl_get_total_relative_data_rate(const struct
> intel_crtc_state *cstate)
>   if (pstate->fb == NULL)
>   continue;
>  
> + if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> + continue;
> +
>   /* packed/uv */
>   total_data_rate +=
> skl_plane_relative_data_rate(cstate,
>   psta
> te,
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH libdrm v4 0/2] 48-bit virtual address support in i915

[Emil Velikov]

HI all,

On 3 September 2015 at 15:23, Michel Thierry  wrote:
> This new version picks the suggestion from Michał Winiarski related to v3 [1].
>
> 48-bit virtual address range will be enabled in i915 soon, but some objects
> must be referenced by 32-bit offsets. These patches use a new kernel flag to
> specify if this restriction applies or not.
>
> I'm sending these patches to comply with the i915 merge process.
> Once the kernel patch is merged, I'll make a new libdrm release and address
> the mesa build dependency.
>
> [1] http://lists.freedesktop.org/archives/dri-devel/2015-August/087837.html
>
> Michel Thierry (2):
>   intel: 48b ppgtt support (EXEC_OBJECT_SUPPORTS_48B_ADDRESS flag)
>   intel: add drm_intel_bo_use_48b_address_range to symbol-check test
>
Can we get some eyes on these and the mesa patches ?

Michel,
Afaics Kristian had some concerns wrt the mesa patches. Did you
address these and forgot to keep mesa-dev in the CC list ?

-Emil

P.S. Michel, lets keep patch discussion on the ML rather than bugzilla ;-)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] Approximately every other day video will freeze for no reason at all and no specific time between freezes.

[Chris]

To elaborate on this. The video is frozen to include the clock however I
can move the mouse cursor around the screen. I can not however switch
between window desktops. This happened on 28 Sept 2015 after an uptime
of 6d20h6m, on 30 Sept after an uptime of 1d23h10m and today after an
uptime of 1d20h12m. I have reported this bug at the Ubuntu Launchpad
site and per an individual there I was instructed to go from running
kernel 4.0.0-997-generic #201503310205 SMP Tue Mar
31 02:07:04 UTC 2015 (I could not report this bug because apport-collect
would error with 'non supported kernel') so he had me switch to kernel
3.19.0-30-generic #33~14.04.1-Ubuntu SMP Tue Sep 22 09:27:00 UTC 2015.
System info is Ubuntu 14.04.3 LTS Gnome-Shell 3.12.2. Computer is a Dell
Optiplex 780 with Bios A15. There are no errors shown in my syslog as in
previous problems where "Hangcheck...render ring idle" would show up.
All background processes such as Fetchmail, Procmail, Spam Assassin,
ClamAv and cron jobs continue to run normally.

Any assistance would be appreciated.

Chris

-- 
Chris
KeyID 0xE372A7DA98E6705C
31.11°N 97.89°W (Elev. 1092 ft)
15:31:29 up 21 min, 1 user, load average: 0.54, 0.35, 0.40
Ubuntu 14.04.3 LTS,

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Boyer, Wayne]

On 8/26/15, 1:23 AM, Deepak  wrote:


>
>
>On 08/25/2015 10:18 PM, Chris Wilson wrote:
>> A long time ago (before 3.14) we relied on a permanent pinning of the
>> ifbdev to lock the fb in place inside the GGTT. However, the
>> introduction of stealing the BIOS framebuffer and reusing its address in
>> the GGTT for the fbdev has muddied waters and we use an inherited fb.
>> However, the inherited fb is only pinned whilst it is active and we no
>> longer have an explicit pin for the info->system_base mmapping used by
>> the fbdev. The result is that after some aperture pressure the fbdev may
>> be evicted, but we continue to write the fbcon into the same GGTT
>> address - overwriting anything else that may be put into that offset.
>> The effect is most pronounced across suspend/resume as
>> intel_fbdev_set_suspend() does a full clear over the whole scanout.
>
>Yup this is a critical fix :) by keeping the internal FB pinned we avoid
>alloc of buffer within same FB GTT offset
>Reviewed-by: Deepak S 

Daniel,

Is there anything else that needs to happen before this gets pulled in?

Thanks,
Wayne

>
>> Signed-off-by: Chris Wilson 
>> Cc: "Goel, Akash" 
>> Cc: Daniel Vetter 
>> Cc: Jesse Barnes 
>> Cc: sta...@vger.kernel.org
>> ---
>>   drivers/gpu/drm/i915/intel_fbdev.c | 15 +++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
>>b/drivers/gpu/drm/i915/intel_fbdev.c
>> index 96476d7d7ed2..082f2938ec97 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper
>>*helper,
>>  obj = intel_fb->obj;
>>  size = obj->base.size;
>>   
>> +/* The fb constructor will have already pinned us (or inherited a
>> + * GGTT region from the BIOS) suitable for a scanout, so
>> + * this should just be a no-op and increment the pin count for the
>> + * fbdev mmapping. It does have a useful side-effect of validating
>> + * the pin for fbdev's use via a GGTT mmapping.
>> + */
>> +ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);
>> +if (ret)
>> +goto out_unlock;
>> +
>>  info = drm_fb_helper_alloc_fbi(helper);
>>  if (IS_ERR(info)) {
>>  ret = PTR_ERR(info);
>> @@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper
>>*helper,
>>   out_destroy_fbi:
>>  drm_fb_helper_release_fbi(helper);
>>   out_unpin:
>> +/* Once for info->screen_base mmaping... */
>> +i915_gem_object_ggtt_unpin(obj);
>> +/* ...and once for the intel_fb */
>>  i915_gem_object_ggtt_unpin(obj);
>>  drm_gem_object_unreference(>base);
>>   out_unlock:
>> @@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs
>>intel_fb_helper_funcs = {
>>   static void intel_fbdev_destroy(struct drm_device *dev,
>>  struct intel_fbdev *ifbdev)
>>   {
>> +/* Release the pinning for the info->screen_base mmaping. */
>> +i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
>>   
>>  drm_fb_helper_unregister_fbi(>helper);
>>  drm_fb_helper_release_fbi(>helper);
>
>___
>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 i-g-t] benchmarks: Fix build errors on Android M-Dessert

[Daniel Vetter]

On Thu, Oct 01, 2015 at 04:09:02PM +0100, Derek Morton wrote:
> Android M-Dessert treats implicit declaration of function warnings
> as errors resulting in igt failing to build.
> 
> This patch fixes the errors by including missing header files as
> required. Mostly this involved including igt.h in the benchmarks.
> 
> Signed-off-by: Derek Morton 

Applied, thanks.
-Daniel

> ---
>  benchmarks/intel_upload_blit_large.c | 7 +--
>  benchmarks/intel_upload_blit_large_gtt.c | 8 +---
>  benchmarks/intel_upload_blit_large_map.c | 8 +---
>  benchmarks/intel_upload_blit_small.c | 8 +---
>  tools/intel_reg_decode.c | 1 +
>  5 files changed, 5 insertions(+), 27 deletions(-)
> 
> diff --git a/benchmarks/intel_upload_blit_large.c 
> b/benchmarks/intel_upload_blit_large.c
> index 1984bfd..12bbae3 100644
> --- a/benchmarks/intel_upload_blit_large.c
> +++ b/benchmarks/intel_upload_blit_large.c
> @@ -44,6 +44,7 @@
>   * The current workload doing this path is pixmap upload for non-KMS.
>   */
>  
> +#include "igt.h"
>  #include 
>  #include 
>  #include 
> @@ -57,12 +58,6 @@
>  #include 
>  #include 
>  
> -#include "drmtest.h"
> -#include "intel_bufmgr.h"
> -#include "intel_batchbuffer.h"
> -#include "intel_io.h"
> -#include "intel_chipset.h"
> -
>  #define OBJECT_WIDTH 1280
>  #define OBJECT_HEIGHT720
>  
> diff --git a/benchmarks/intel_upload_blit_large_gtt.c 
> b/benchmarks/intel_upload_blit_large_gtt.c
> index d62a01e..0b704b5 100644
> --- a/benchmarks/intel_upload_blit_large_gtt.c
> +++ b/benchmarks/intel_upload_blit_large_gtt.c
> @@ -44,6 +44,7 @@
>   * The current workload doing this path is pixmap upload in 2D with KMS.
>   */
>  
> +#include "igt.h"
>  #include 
>  #include 
>  #include 
> @@ -53,13 +54,6 @@
>  #include 
>  #include 
>  #include 
> -#include "drm.h"
> -#include "i915_drm.h"
> -#include "drmtest.h"
> -#include "intel_bufmgr.h"
> -#include "intel_batchbuffer.h"
> -#include "intel_io.h"
> -#include "intel_chipset.h"
>  
>  #define OBJECT_WIDTH 1280
>  #define OBJECT_HEIGHT720
> diff --git a/benchmarks/intel_upload_blit_large_map.c 
> b/benchmarks/intel_upload_blit_large_map.c
> index 03bf760..ae05434 100644
> --- a/benchmarks/intel_upload_blit_large_map.c
> +++ b/benchmarks/intel_upload_blit_large_map.c
> @@ -47,6 +47,7 @@
>   * suitable)
>   */
>  
> +#include "igt.h"
>  #include 
>  #include 
>  #include 
> @@ -56,13 +57,6 @@
>  #include 
>  #include 
>  #include 
> -#include "drm.h"
> -#include "i915_drm.h"
> -#include "drmtest.h"
> -#include "intel_bufmgr.h"
> -#include "intel_batchbuffer.h"
> -#include "intel_io.h"
> -#include "intel_chipset.h"
>  
>  #define OBJECT_WIDTH 1280
>  #define OBJECT_HEIGHT720
> diff --git a/benchmarks/intel_upload_blit_small.c 
> b/benchmarks/intel_upload_blit_small.c
> index ac557a4..7e3346e 100644
> --- a/benchmarks/intel_upload_blit_small.c
> +++ b/benchmarks/intel_upload_blit_small.c
> @@ -40,6 +40,7 @@
>   * frame.
>   */
>  
> +#include "igt.h"
>  #include 
>  #include 
>  #include 
> @@ -49,13 +50,6 @@
>  #include 
>  #include 
>  #include 
> -#include "drm.h"
> -#include "i915_drm.h"
> -#include "drmtest.h"
> -#include "intel_bufmgr.h"
> -#include "intel_batchbuffer.h"
> -#include "intel_io.h"
> -#include "intel_chipset.h"
>  
>  /* Happens to be 128k, the size of the VBOs used by i965's Mesa driver. */
>  #define OBJECT_WIDTH 256
> diff --git a/tools/intel_reg_decode.c b/tools/intel_reg_decode.c
> index 4f97d99..bb8f5b3 100644
> --- a/tools/intel_reg_decode.c
> +++ b/tools/intel_reg_decode.c
> @@ -31,6 +31,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "intel_chipset.h"
>  #include "intel_reg.h"
> -- 
> 1.9.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Allow minimum brightness upon backlight enable

[Jani Nikula]

On Fri, 02 Oct 2015, clinton.a.tay...@intel.com wrote:
> From: Clint Taylor 
>
> backlight minimum is a valid value so you don't need to set maximum.
>
> Signed-off-by: Clint Taylor 
> ---
>  drivers/gpu/drm/i915/intel_panel.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> b/drivers/gpu/drm/i915/intel_panel.c
> index 4d28c7b..d509904 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1069,7 +1069,7 @@ void intel_panel_enable_backlight(struct 
> intel_connector *connector)
>  
>   WARN_ON(panel->backlight.max == 0);
>  
> - if (panel->backlight.level <= panel->backlight.min) {
> + if (panel->backlight.level < panel->backlight.min) {
>   panel->backlight.level = panel->backlight.max;
>   if (panel->backlight.device)
>   panel->backlight.device->props.brightness =

This is policy in action in the kernel. We have this whole if block here
because some, uh, misguided userspace sets brightness to minimum before
switching the backlight off, and assumes enabling backlight cranks up
the brightness as well. (*)

The condition used to be (level == 0), but since the minimum can now
also be non-zero, we check (level <= min). If we changed this to (level
< min), we'd regress the misguided userspaces, especially if min == 0.

See

commit 13f3fbe827d09e3182023c8c54058cbf97aa146e
Author: Jeremiah Mahler 
Date:   Mon Jan 12 11:01:03 2015 -0800

drm/i915: fix inconsistent brightness after resume

I'd really love to to just nuke the if block completely, but no
regressions and all.


BR,
Jani.



(*) Imagine an amplifier that would set the volume to max if it was
powered on with the volume knob turned to zero...



-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: prevent out of range pt in the PDE macros (take 3)

[Daniel Vetter]

On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:
> We tried to fix this in commit fdc454c1484a ("drm/i915: Prevent out of
> range pt in gen6_for_each_pde").
> 
> But the static analyzer still complains that, just before we break due
> to "iter < I915_PDES", we do "pt = (pd)->page_table[iter]" with an
> iter value that is bigger than I915_PDES. Of course, this isn't really
> a problem since no one uses pt outside the macro. Still, every single
> new usage of the macro will create a new issue for us to mark as a
> false positive.
> 
> Also, Paulo re-started the discussion a while ago [1], but didn't end up
> implemented.
> 
> In order to "solve" this "problem", this patch takes the ideas from
> Chris and Dave, but that check would change the desired behavior of the
> code, because the object (for example pdp->page_directory[iter]) can be
> null during init/alloc, and C would take this as false, breaking the for
> loop immediately.
> 
> This has been already verified with "static analysis tools".
> 
> [1]http://lists.freedesktop.org/archives/intel-gfx/2015-June/068548.html
> 
> Cc: Paulo Zanoni 
> Cc: Chris Wilson 
> Cc: Dave Gordon 
> Signed-off-by: Michel Thierry 

So maybe I'm dense and not seeing what's really going on, but the only
thing we seem to be doing is create a pointer to arr[SIZE], i.e. a pointer
to the element right after the last valid one. Pointer arithmetic and
comparison are explicitly allowed by the C standard on such a pointer. The
only thing not allowed is dereference it (which we don't seem to be doing
here).

The reason for this is exactly this case here, not allowing this means
checking whether you've run off the end of an array often becomes very
unnatural.

So if there's nothing else going on I think the right choice is to mark
them all up as false positives - the tool is wrong.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
> b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 9fbb07d..94f8344 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
>   */
>  #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
>   for (iter = gen6_pde_index(start); \
> -  pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
> +  pt = (length > 0 && iter < I915_PDES) ? \
> + (pd)->page_table[iter] : NULL, \
> +  length > 0 && iter < I915_PDES; \
>iter++, \
>temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
>temp = min_t(unsigned, temp, length), \
> @@ -459,7 +461,9 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>   */
>  #define gen8_for_each_pde(pt, pd, start, length, temp, iter) \
>   for (iter = gen8_pde_index(start); \
> -  pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES;   
> \
> +  pt = (length > 0 && iter < I915_PDES) ? \
> + (pd)->page_table[iter] : NULL, \
> +  length > 0 && iter < I915_PDES;\
>iter++,\
>temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start,\
>temp = min(temp, length),  \
> @@ -467,7 +471,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>  
>  #define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter)   \
>   for (iter = gen8_pdpe_index(start); \
> -  pd = (pdp)->page_directory[iter], \
> +  pd = (length > 0 && (iter < I915_PDPES_PER_PDP(dev))) ? \
> + (pdp)->page_directory[iter] : NULL, \
>length > 0 && (iter < I915_PDPES_PER_PDP(dev)); \
>iter++,\
>temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start,   \
> @@ -476,7 +481,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>  
>  #define gen8_for_each_pml4e(pdp, pml4, start, length, temp, iter)\
>   for (iter = gen8_pml4e_index(start);\
> -  pdp = (pml4)->pdps[iter], \
> +  pdp = (length > 0 && iter < GEN8_PML4ES_PER_PML4) ? \
> + (pml4)->pdps[iter] : NULL, \
>length > 0 && iter < GEN8_PML4ES_PER_PML4; \
>iter++,\
>temp = ALIGN(start+1, 1ULL << GEN8_PML4E_SHIFT) - start,   \
> -- 
> 2.6.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

Re: [Intel-gfx] [PATCH igt 2/8] lib/igt_core: Add igt_assert_fd

[Daniel Vetter]

On Thu, Oct 01, 2015 at 05:34:02PM +0100, Daniel Stone wrote:
> Skip open-coding and assert that fds are valid.
> 
> Signed-off-by: Daniel Stone 
> ---
>  lib/igt_core.h | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 9a5d9c5..8f93e8e 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -507,6 +507,17 @@ void igt_exit(void) __attribute__((noreturn));
>  #define igt_assert_lt(n1, n2) igt_assert_cmpint(n1, <, >=, n2)
>  
>  /**
> + * igt_assert_fd:
> + * @fd: file descriptor
> + *
> + * Fails (sub-) test if the given file descriptor is not valid.
> + *
> + * Like igt_assert(), but displays the values being compared on failure 
> instead
> + * of simply printing the stringified expression.
> + */
> +#define igt_assert_fd(fd) igt_assert_lte(0, fd)

If we do this then I think some more verbose output would be nice, like

#igt_assert_fd(fd) igt_assert_f(fd >= 0, "file descriptor " #fd " failed\n");

Large-scale sed would be even more awesome ;-)
-Daniel

> +
> +/**
>   * igt_require:
>   * @expr: condition to test
>   *
> -- 
> 2.5.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH igt 7/8] tests: Add blob-property test

[Daniel Vetter]

On Thu, Oct 01, 2015 at 05:42:55PM +0100, Daniel Stone wrote:
> Exercises the new blob-creation ioctl, testing lifetimes and behaviour
> of user-created blobs, as well as exercising all the invariant
> conditions we guarantee from modes exposed as blob properties.
> 
> v2: Renamed to core_prop_blob, skip test if blob not available.
> v3: No changes.
> v4: Consistently return 0/errno.
> v5: Use do_ioctl_err and igt_assert_fd.
> Use igt_assert_*() helper macros rather than direct igt_assert().
> 
> Signed-off-by: Daniel Stone 

Reviewed-by: Daniel Vetter 

> ---
>  tests/.gitignore   |   1 +
>  tests/Makefile.sources |   1 +
>  tests/core_prop_blob.c | 228 
> +
>  3 files changed, 230 insertions(+)
>  create mode 100644 tests/core_prop_blob.c
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index dc8bb53..beda511 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -3,6 +3,7 @@ core_get_client_auth
>  core_getclient
>  core_getstats
>  core_getversion
> +core_prop_blob
>  drm_auth
>  drm_import_export
>  drm_read
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 2e2e088..ac731f9 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -99,6 +99,7 @@ TESTS_progs = \
>   core_getclient \
>   core_getstats \
>   core_getversion \
> + core_prop_blob \
>   drm_auth \
>   drm_import_export \
>   drm_read \
> diff --git a/tests/core_prop_blob.c b/tests/core_prop_blob.c
> new file mode 100644
> index 000..d704158
> --- /dev/null
> +++ b/tests/core_prop_blob.c
> @@ -0,0 +1,228 @@
> +/*
> + * Copyright © 2014-2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *   Damien Lespiau 
> + *   Daniel Stone 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "drmtest.h"
> +#include "igt_debugfs.h"
> +#include "igt_kms.h"
> +#include "igt_aux.h"
> +
> +IGT_TEST_DESCRIPTION("Tests behaviour of mass-data 'blob' properties.");
> +
> +static const struct drm_mode_modeinfo test_mode_valid = {
> + .clock = 1234,
> + .hdisplay = 640,
> + .hsync_start = 641,
> + .hsync_end = 642,
> + .htotal = 643,
> + .hskew = 0,
> + .vdisplay = 480,
> + .vsync_start = 481,
> + .vsync_end = 482,
> + .vtotal = 483,
> + .vscan = 0,
> + .vrefresh = 6,
> + .flags = 0,
> + .type = 0,
> + .name = "FROMUSER",
> +};
> +
> +
> +#define ioctl_or_ret_errno(fd, ioc, ioc_data) { \
> + if (drmIoctl(fd, ioc, ioc_data) != 0) \
> + return errno; \
> +}
> +
> +static int
> +validate_prop(int fd, uint32_t prop_id)
> +{
> + struct drm_mode_get_blob get;
> + struct drm_mode_modeinfo ret_mode;
> +
> + get.blob_id = prop_id;
> + get.length = 0;
> + get.data = (uintptr_t) 0;
> + ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_GETPROPBLOB, );
> +
> + if (get.length != sizeof(test_mode_valid))
> + return ENOMEM;
> +
> + get.data = (uintptr_t) _mode;
> + ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_GETPROPBLOB, );
> +
> + if (memcmp(_mode, _mode_valid, sizeof(test_mode_valid)) != 0)
> + return EINVAL;
> +
> + return 0;
> +}
> +
> +static uint32_t
> +create_prop(int fd)
> +{
> + struct drm_mode_create_blob create;
> +
> + create.length = sizeof(test_mode_valid);
> + create.data = (uintptr_t) _mode_valid;
> +
> + do_ioctl(fd, DRM_IOCTL_MODE_CREATEPROPBLOB, );
> + igt_assert_neq_u32(create.blob_id, 0);
> +
> + return create.blob_id;
> +}
> +
> +static int
> +destroy_prop(int fd, uint32_t prop_id)
> +{
> + struct drm_mode_destroy_blob destroy;
> +
> + destroy.blob_id = prop_id;
> + ioctl_or_ret_errno(fd, 

Re: [Intel-gfx] [PATCH] drm/i915: Add GEN7_GPGPU_DISPATCHDIMX/Y/Z to the register whitelist

[Daniel Vetter]

On Thu, Oct 01, 2015 at 11:09:58PM -0700, Jordan Justen wrote:
> This is required to support glDispatchComputeIndirect for gen7.
> 
> Signed-off-by: Jordan Justen 
> Reviewed-by: Kristian Høgsberg 

Do you have a link to the mesa patches? As soon as those are r-b'ed I'll
pull this in.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 6 +-
>  drivers/gpu/drm/i915/i915_reg.h| 4 
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c 
> b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 09932ca..db58c8d 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -448,6 +448,9 @@ static const struct drm_i915_reg_descriptor 
> gen7_render_regs[] = {
>   REG32(GEN7_3DPRIM_INSTANCE_COUNT),
>   REG32(GEN7_3DPRIM_START_INSTANCE),
>   REG32(GEN7_3DPRIM_BASE_VERTEX),
> + REG32(GEN7_GPGPU_DISPATCHDIMX),
> + REG32(GEN7_GPGPU_DISPATCHDIMY),
> + REG32(GEN7_GPGPU_DISPATCHDIMZ),
>   REG64(GEN7_SO_NUM_PRIMS_WRITTEN(0)),
>   REG64(GEN7_SO_NUM_PRIMS_WRITTEN(1)),
>   REG64(GEN7_SO_NUM_PRIMS_WRITTEN(2)),
> @@ -1214,6 +1217,7 @@ int i915_cmd_parser_get_version(void)
>*MI_PREDICATE_SRC1 registers.
>* 3. Allow access to the GPGPU_THREADS_DISPATCHED register.
>* 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3.
> +  * 5. GPGPU dispatch compute indirect registers.
>*/
> - return 4;
> + return 5;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 56157eb..f2b6425 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -536,6 +536,10 @@
>  #define GEN7_3DPRIM_START_INSTANCE  0x243C
>  #define GEN7_3DPRIM_BASE_VERTEX 0x2440
>  
> +#define GEN7_GPGPU_DISPATCHDIMX 0x2500
> +#define GEN7_GPGPU_DISPATCHDIMY 0x2504
> +#define GEN7_GPGPU_DISPATCHDIMZ 0x2508
> +
>  #define OACONTROL 0x2360
>  
>  #define _GEN7_PIPEA_DE_LOAD_SL   0x70068
> -- 
> 2.5.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx