Re: [Intel-gfx] [PATCH 4/4] drm/i915/gen12: Invalidate aux table entries forcibly

[Rafael Antognolli]

On Wed, May 06, 2020 at 04:32:02PM +0100, Chris Wilson wrote:
> Quoting Mika Kuoppala (2020-05-06 16:20:22)
> > Chris Wilson  writes:
> > 
> > > Quoting Mika Kuoppala (2020-05-06 15:47:34)
> > >> Aux table invalidation can fail on update. So
> > >> next access may cause memory access to be into stale entry.
> > >> 
> > >> Proposed workaround is to invalidate entries between
> > >> all batchbuffers.
> > >
> > > This sounds like it should have a sunset clause. Do we anticipate being
> > > able to drop this w/a in future?
> > 
> > Rafael kindly pointed out that Mesa already does this:
> > https://gitlab.freedesktop.org/mesa/mesa/-/blob/master/src/gallium/drivers/iris/iris_state.c#L5131
> > So I would say we can drop this patch.
> 
> Is the false hit self-contained? Is it caused by PTE update of the
> address or by a 3D state change i.e. is it a potential isolation issue?
> -Chris

There's no 3D state for the aux table. What we do in Iris is:
 - allocate buffers and cpu map them
 - write the multiple levels of the table (main buffers -> CCS buffers)
   and keep track of when it changes
 - whenever it changes, we emit LRI to 0x4208 to invalidate the cache.

Anv does something similar except it writes to the table from the GPU.

I tried removing the heuristics and invalidating the table on the
beginning of every batch, but it doesn't help. We can probably get
preempted mid-batch and then another context with a different aux table
and the wrong cache is probably causing the hang. The kernel
invalidating it seems to fix the problem.

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

Re: [Intel-gfx] [PATCH] drm/i915/tgl: Make Wa_14010229206 permanent

[Rafael Antognolli]

On Thu, Mar 26, 2020 at 04:49:55PM -0700, Swathi Dhanavanthri wrote:
> This workaround now applies to all steppings, not just A0.
> Wa_1409085225 is a temporary A0-only W/A however it is
> identical to Wa_14010229206 and hence the combined workaround
> is made permanent.
> Bspec: 52890

FYI, this patch fixes some corruption I was seeing.

Tested-by: Rafael Antognolli 

> Signed-off-by: Swathi Dhanavanthri 
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index e96cc7fa0936..c3c42cf614a9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1380,12 +1380,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, 
> struct i915_wa_list *wal)
>   GEN7_FF_THREAD_MODE,
>   GEN12_FF_TESSELATION_DOP_GATE_DISABLE);
>  
> - /*
> -  * Wa_1409085225:tgl
> -  * Wa_14010229206:tgl
> -  */
> - wa_masked_en(wal, GEN9_ROW_CHICKEN4, GEN12_DISABLE_TDL_PUSH);
> -
>   /* Wa_1408615072:tgl */
>   wa_write_or(wal, UNSLICE_UNIT_LEVEL_CLKGATE2,
>   VSUNIT_CLKGATE_DIS_TGL);
> @@ -1403,6 +1397,11 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, 
> struct i915_wa_list *wal)
>   wa_masked_en(wal,
>GEN9_CS_DEBUG_MODE1,
>FF_DOP_CLOCK_GATE_DISABLE);
> + /*
> +  * Wa_1409085225:tgl
> +  * Wa_14010229206:tgl
> +  */
> + wa_masked_en(wal, GEN9_ROW_CHICKEN4, GEN12_DISABLE_TDL_PUSH);
>   }
>  
>   if (IS_GEN(i915, 11)) {
> -- 
> 2.20.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/tgl: WaDisableGPGPUMidThreadPreemption

[Rafael Antognolli]

On Wed, Mar 04, 2020 at 04:24:13PM +, Tvrtko Ursulin wrote:
> 
> On 04/03/2020 16:02, Rafael Antognolli wrote:
> > On Wed, Mar 04, 2020 at 03:31:44PM +, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin 
> > > 
> > > Enable FtrPerCtxtPreemptionGranularityControl bit and select thread-
> > > group as the default preemption level.
> > > 
> > > v2:
> > >   * Remove register whitelisting (Rafael, Tony).
> > > 
> > > Signed-off-by: Tvrtko Ursulin 
> > > Cc: Michał Winiarski 
> > > Cc: Joonas Lahtinen 
> > > Cc: piotr.zdunow...@intel.com
> > > Cc: michal.mro...@intel.com
> > > Cc: Tony Ye 
> > > Cc: Rafael Antognolli 
> > 
> > Thanks for CC'ing me. I also saw a reply from Jason yesterday, but I
> > don't see it in the list now (though my mail client a mess lately).
> 
> I saw nothing from Jason, but there was an email from you asking about
> interface descriptors and whitelisting which is why I copied you.
> 
> > But he asked whether it's possible for Media and OpenCL drivers to
> > also disable mid-thread preemption through the
> > INTERFACE_DESCRIPTOR_DATA, instead of from the kernel side, so we could
> > try to experiment with it in the future.
> > 
> > Also, do you have an idea of how broken it is? Or is it just not tested
> > because no driver is currently implementing it? And do you know if the
> > windows 3D drivers implement it at all? I see code in the driver that
> > seems to me that it's only disabled in certain cases...
> > 
> > To summarize, I think we should either:
> > 1) Disable mid-thread preemption from the kernel and not whitelist
> > the register (just like you do in this patch); or
> > 2) Not do anything at all from the kernel, and let userspace disable
> > it if needed.
> > 
> > I think 2) is better, if it's not an issue to the other userspace
> > drivers (OpenCL and Media).
> 
> I know it is somewhat broken like in
> https://gitlab.freedesktop.org/drm/intel/issues/1293.
> 
> And I know OpenCL and Media would prefer i915 to handle it, but that's
> always the case. :) OpenCL and Media folks are on the thread so can comment
> if they are okay with handling this themselves.
> 
> Indeed a blanket ban in i915 means no one can try it out later without
> further kernel changes.

Well, based on your comment from the previous patch:

"General thinking is, since MTP is considered not validated / broken /
dangerous, i915 should default it off. But yes, whitelisting or not on
top is open."

Maybe we should simply ban it and be done. So this patch is:

Acked-by: Rafael Antognolli 

> Regards,
> 
> Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/tgl: WaDisableGPGPUMidThreadPreemption

[Rafael Antognolli]

On Wed, Mar 04, 2020 at 03:31:44PM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> Enable FtrPerCtxtPreemptionGranularityControl bit and select thread-
> group as the default preemption level.
> 
> v2:
>  * Remove register whitelisting (Rafael, Tony).
> 
> Signed-off-by: Tvrtko Ursulin 
> Cc: Michał Winiarski 
> Cc: Joonas Lahtinen 
> Cc: piotr.zdunow...@intel.com
> Cc: michal.mro...@intel.com
> Cc: Tony Ye 
> Cc: Rafael Antognolli 

Thanks for CC'ing me. I also saw a reply from Jason yesterday, but I
don't see it in the list now (though my mail client a mess lately).

But he asked whether it's possible for Media and OpenCL drivers to
also disable mid-thread preemption through the
INTERFACE_DESCRIPTOR_DATA, instead of from the kernel side, so we could
try to experiment with it in the future.

Also, do you have an idea of how broken it is? Or is it just not tested
because no driver is currently implementing it? And do you know if the
windows 3D drivers implement it at all? I see code in the driver that
seems to me that it's only disabled in certain cases...

To summarize, I think we should either:
   1) Disable mid-thread preemption from the kernel and not whitelist
   the register (just like you do in this patch); or
   2) Not do anything at all from the kernel, and let userspace disable
   it if needed.

I think 2) is better, if it's not an issue to the other userspace
drivers (OpenCL and Media).

--
Rafael

> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index cb7d85c42f13..7be71a1a5719 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -601,6 +601,11 @@ static void tgl_ctx_workarounds_init(struct 
> intel_engine_cs *engine,
>*/
>   wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK,
>  FF_MODE2_TDS_TIMER_128, 0);
> +
> + /* WaDisableGPGPUMidThreadPreemption:tgl */
> + WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1,
> + GEN9_PREEMPT_GPGPU_LEVEL_MASK,
> + GEN9_PREEMPT_GPGPU_THREAD_GROUP_LEVEL);
>  }
>  
>  static void
> @@ -1475,8 +1480,8 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, 
> struct i915_wa_list *wal)
>PSDUNIT_CLKGATE_DIS);
>   }
>  
> - if (IS_GEN_RANGE(i915, 9, 11)) {
> - /* 
> FtrPerCtxtPreemptionGranularityControl:skl,bxt,kbl,cfl,cnl,icl */
> + if (IS_GEN_RANGE(i915, 9, 12)) {
> + /* 
> FtrPerCtxtPreemptionGranularityControl:skl,bxt,kbl,cfl,cnl,icl,tgl */
>   wa_masked_en(wal,
>GEN7_FF_SLICE_CS_CHICKEN1,
>GEN9_FFSC_PERCTX_PREEMPT_CTRL);
> -- 
> 2.20.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 11/11] drm/i915/tgl: Implement Wa_1407901919

[Rafael Antognolli]

On Fri, Feb 28, 2020 at 02:10:50PM -0800, Souza, Jose wrote:
> Can you guys help in this one? Check Matt comment bellow.
> 
> On Fri, 2020-02-28 at 14:07 -0800, Matt Roper wrote:
> > On Thu, Feb 27, 2020 at 02:01:01PM -0800, José Roberto de Souza
> > wrote:
> > > This will fix a memory coherence issue.
> > > 
> > > v3: using whitespace to make easy to read WA (Chris)
> > > 
> > > BSpec: 52890
> > > Cc: Chris Wilson 
> > > Signed-off-by: José Roberto de Souza 
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c |  8 
> > >  drivers/gpu/drm/i915/i915_reg.h | 20 +++
> > > -
> > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index 3e375a3b7714..c59e1a604ab8 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -601,6 +601,14 @@ static void tgl_ctx_workarounds_init(struct
> > > intel_engine_cs *engine,
> > >*/
> > >   wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK,
> > >  FF_MODE2_TDS_TIMER_128, 0);
> > > +
> > > + /* Wa_1407901919:tgl */
> > > + wa_add(wal, ICL_HDC_MODE,
> > > +HDC_COHERENT_ACCESS_L1_CACHE_DIS |
> > > +HDC_DIS_L1_INVAL_FOR_NON_L1_CACHEABLE_W,
> > 
> > I'm not sure if this is what the workaround is asking for.  The way I
> > understood the workaround, the 2-dword STATE_COMPUTE_MODE instruction
> > has a couple bits that must be left at 0.  STATE_COMPUTE_MODE is
> > basically how we ultimately load the HDC_MODE registers (rather than
> > using a simple LRI like we do for a bunch of other registers), but
> > the
> > workaround isn't asking us to worry about bits 13+14 in the HDC_MODE
> > register itself, but rather those flags bits on the instruction that
> > manipulates the register.
> > 
> > Every time there's a context switch, the hardware will generate a
> > copy
> > of this instruction as part of the context image in writes to RAM;
> > I'm
> > assuming these bits aren't set on those hardware-created
> > instructions?
> > Assuming that's true, then I think this workaround would just be
> > userspace's responsibility --- if they submit an explicit
> > STATE_COMPUTE_MODE instruction that isn't just part of the context
> > image, they need to follow the workaround guidance here and leave two
> > of
> > those bits set to 0.

Hmmm... IMHO the workaround description doesn't make it very clear if
it's talking about the register itself, or the STATE_COMPUTE_MODE
instruction to set it. But the comment in the bug about it seems to
suggest what Matt described.

In any case, it should just be left as 0, and as Matt said, userspace
shouldn't set it to 1. So I agree, there's nothing to be done in the
kernel.

--
Rafael

> > 
> > Matt
> > 
> > > +0,
> > > +HDC_COHERENT_ACCESS_L1_CACHE_DIS |
> > > +HDC_DIS_L1_INVAL_FOR_NON_L1_CACHEABLE_W);
> > >  }
> > >  
> > >  static void
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 80cf02a6eec1..28822585537b 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7883,15 +7883,17 @@ enum {
> > >  #define  GEN8_LQSC_FLUSH_COHERENT_LINES  (1 << 21)
> > >  
> > >  /* GEN8 chicken */
> > > -#define HDC_CHICKEN0 _MMIO(0x7300)
> > > -#define CNL_HDC_CHICKEN0 _MMIO(0xE5F0)
> > > -#define ICL_HDC_MODE _MMIO(0xE5F4)
> > > -#define  HDC_FORCE_CSR_NON_COHERENT_OVR_DISABLE  (1 << 15)
> > > -#define  HDC_FENCE_DEST_SLM_DISABLE  (1 << 14)
> > > -#define  HDC_DONOT_FETCH_MEM_WHEN_MASKED (1 << 11)
> > > -#define  HDC_FORCE_CONTEXT_SAVE_RESTORE_NON_COHERENT (1 <<
> > > 5)
> > > -#define  HDC_FORCE_NON_COHERENT  (1 << 4)
> > > -#define  HDC_BARRIER_PERFORMANCE_DISABLE (1 << 10)
> > > +#define HDC_CHICKEN0 _MMIO(0
> > > x7300)
> > > +#define CNL_HDC_CHICKEN0 _MMIO(0xE5F0)
> > > +#define ICL_HDC_MODE _MMIO(0
> > > xE5F4)
> > > +#define  HDC_FORCE_CSR_NON_COHERENT_OVR_DISABLE  REG_BIT
> > > (15)
> > > +#define  HDC_FENCE_DEST_SLM_DISABLE  REG_BIT
> > > (14)
> > > +#define  HDC_DIS_L1_INVAL_FOR_NON_L1_CACHEABLE_W REG_BIT(13)
> > > +#define  HDC_COHERENT_ACCESS_L1_CACHE_DISREG_BIT(12)
> > > +#define  HDC_DONOT_FETCH_MEM_WHEN_MASKED REG_BIT(11)
> > > +#define  HDC_FORCE_CONTEXT_SAVE_RESTORE_NON_COHERENT REG_BIT
> > > (5)
> > > +#define  HDC_FORCE_NON_COHERENT  REG_BIT
> > > (4)
> > > +#define  HDC_BARRIER_PERFORMANCE_DISABLE REG_BIT(10)
> > >  
> > >  #define GEN8_HDC_CHICKEN1_MMIO(0x7304)
> > >  
> > > -- 
> > > 2.25.1
> > > 
> > > 

Re: [Intel-gfx] [PATCH] drm/i915/gt/tgl: implement Wa_1409085225

[Rafael Antognolli]

On Tue, Feb 18, 2020 at 03:44:49PM -0800, Rafael Antognolli wrote:
> On Tue, Feb 18, 2020 at 02:47:10PM -0500, Matt Atwood wrote:
> > Disable Push Constant buffer addition for A0, which can cause FIFO
> > underruns.
> > 
> > Fix a minor white space issue while we're here.
> > 
> > Bspec: 52890
> > Cc: Rafael Antognolli 
> > Signed-off-by: Matt Atwood 
> > ---
> >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 ++
> >  drivers/gpu/drm/i915/i915_reg.h |  3 +++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
> > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 887e0dc701f7..9bbd28aa9bde 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -580,6 +580,7 @@ static void icl_ctx_workarounds_init(struct 
> > intel_engine_cs *engine,
> >  static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> >  struct i915_wa_list *wal)
> >  {
> > +   struct drm_i915_private *i915 = engine->i915;
> > u32 val;
> >  
> > /* Wa_1409142259:tgl */
> > @@ -590,6 +591,7 @@ static void tgl_ctx_workarounds_init(struct 
> > intel_engine_cs *engine,
> > val = intel_uncore_read(engine->uncore, FF_MODE2);
> > val &= ~FF_MODE2_TDS_TIMER_MASK;
> > val |= FF_MODE2_TDS_TIMER_128;
> > +
> > /*
> >  * FIXME: FF_MODE2 register is not readable till TGL B0. We can
> >  * enable verification of WA from the later steppings, which enables
> > @@ -598,6 +600,14 @@ static void tgl_ctx_workarounds_init(struct 
> > intel_engine_cs *engine,
> > wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
> >IS_TGL_REVID(engine->i915, TGL_REVID_A0, TGL_REVID_A0) ? 0 :
> > FF_MODE2_TDS_TIMER_MASK);
> > +
> > +   /* Wa_1409085225:tgl
> > +*
> > +* Push Constant Buffer can case FIFO underruns on A0
> > +*/
> > +   if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> 
> Is this really only applicable to A stepping?

Hmmm... it looks like there is another workaround that's almost the
same: 14010229206 <-- this one seems to be permanent.

The one you implemented is indeed only A stepping. I'm not sure which
one is right, though.

> > +   WA_SET_BIT_MASKED(GEN9_ROW_CHICKEN4,
> > + GEN12_DISABLE_TDL_PUSH);
> >  }
> >  
> >  static void
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index b09c1d6dc0aa..a75a27ed63ce 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9153,6 +9153,9 @@ enum {
> >  #define   PUSH_CONSTANT_DEREF_DISABLE  (1 << 8)
> >  #define   GEN11_TDL_CLOCK_GATING_FIX_DISABLE   (1 << 1)
> >  
> > +#define GEN9_ROW_CHICKEN4  _MMIO(0x48c)
> 
> s/0x48c/0xe48c/ ?
> 
> > +#define  GEN12_DISABLE_TDL_PUSH(1 << 9)
> > +
> >  #define HSW_ROW_CHICKEN3   _MMIO(0xe49c)
> >  #define  HSW_ROW_CHICKEN3_L3_GLOBAL_ATOMICS_DISABLE(1 << 6)
> >  
> > -- 
> > 2.21.1
> > 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gt/tgl: implement Wa_1409085225

[Rafael Antognolli]

On Tue, Feb 18, 2020 at 02:47:10PM -0500, Matt Atwood wrote:
> Disable Push Constant buffer addition for A0, which can cause FIFO
> underruns.
> 
> Fix a minor white space issue while we're here.
> 
> Bspec: 52890
> Cc: Rafael Antognolli 
> Signed-off-by: Matt Atwood 
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 ++
>  drivers/gpu/drm/i915/i915_reg.h |  3 +++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 887e0dc701f7..9bbd28aa9bde 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -580,6 +580,7 @@ static void icl_ctx_workarounds_init(struct 
> intel_engine_cs *engine,
>  static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
>struct i915_wa_list *wal)
>  {
> + struct drm_i915_private *i915 = engine->i915;
>   u32 val;
>  
>   /* Wa_1409142259:tgl */
> @@ -590,6 +591,7 @@ static void tgl_ctx_workarounds_init(struct 
> intel_engine_cs *engine,
>   val = intel_uncore_read(engine->uncore, FF_MODE2);
>   val &= ~FF_MODE2_TDS_TIMER_MASK;
>   val |= FF_MODE2_TDS_TIMER_128;
> +
>   /*
>* FIXME: FF_MODE2 register is not readable till TGL B0. We can
>* enable verification of WA from the later steppings, which enables
> @@ -598,6 +600,14 @@ static void tgl_ctx_workarounds_init(struct 
> intel_engine_cs *engine,
>   wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
>  IS_TGL_REVID(engine->i915, TGL_REVID_A0, TGL_REVID_A0) ? 0 :
>   FF_MODE2_TDS_TIMER_MASK);
> +
> + /* Wa_1409085225:tgl
> +  *
> +  * Push Constant Buffer can case FIFO underruns on A0
> +  */
> + if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))

Is this really only applicable to A stepping?

> + WA_SET_BIT_MASKED(GEN9_ROW_CHICKEN4,
> +   GEN12_DISABLE_TDL_PUSH);
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b09c1d6dc0aa..a75a27ed63ce 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9153,6 +9153,9 @@ enum {
>  #define   PUSH_CONSTANT_DEREF_DISABLE(1 << 8)
>  #define   GEN11_TDL_CLOCK_GATING_FIX_DISABLE (1 << 1)
>  
> +#define GEN9_ROW_CHICKEN4_MMIO(0x48c)

s/0x48c/0xe48c/ ?

> +#define  GEN12_DISABLE_TDL_PUSH  (1 << 9)
> +
>  #define HSW_ROW_CHICKEN3 _MMIO(0xe49c)
>  #define  HSW_ROW_CHICKEN3_L3_GLOBAL_ATOMICS_DISABLE(1 << 6)
>  
> -- 
> 2.21.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/tgl: Add Wa_1808121037 to tgl.

[Rafael Antognolli]

On Fri, Feb 14, 2020 at 09:10:38AM -0800, Matt Roper wrote:
> On Wed, Feb 12, 2020 at 11:17:28AM -0800, Rafael Antognolli wrote:
> > It's not clear whether this workaround is final yet, but the BSpec
> > indicates that userspace needs to set bit 9 of this register on demand:
> > 
> >"To avoid sporadic corruptions “Set 0x7010[9] when Depth Buffer
> >Surface Format is D16_UNORM , surface type is not NULL & 1X_MSAA"
> > 
> > BugLink: https://gitlab.freedesktop.org/mesa/mesa/issues/2501
> > Signed-off-by: Rafael Antognolli 
> 
> Seems like the right register to whitelist to allow userspace to apply
> the workaround.
> 
> Reviewed-by: Matt Roper 
> 
> I think we can drop the "Allow userpace to implement this workaround"
> part of the comment; that part is self-explanatory given that it's a
> whitelist entry.  Do you mind if we just tweak the comment while
> applying?  It looks like the CI shards queue is massive right now so
> it's already going to take a long time to get the full results back for
> this patch; no need to make it even longer by resubmitting for a trivial
> comment shortening.

I don't mind it at all, feel free to change it however you want.

Thanks!
Rafael

> 
> Matt
> 
> > ---
> >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
> > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 62b43f538a56..57b9685d9347 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -1264,6 +1264,12 @@ static void tgl_whitelist_build(struct 
> > intel_engine_cs *engine)
> > whitelist_reg_ext(w, PS_INVOCATION_COUNT,
> >   RING_FORCE_TO_NONPRIV_ACCESS_RD |
> >   RING_FORCE_TO_NONPRIV_RANGE_4);
> > +
> > +   /* Wa_1808121037:tgl
> > +*
> > +* Allow userpace to implement this workaround.
> > +*/
> > +   whitelist_reg(w, GEN7_COMMON_SLICE_CHICKEN1);
> > break;
> > default:
> > break;
> > -- 
> > 2.25.0
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/tgl: Add Wa_1808121037 to tgl.

[Rafael Antognolli]

It's not clear whether this workaround is final yet, but the BSpec
indicates that userspace needs to set bit 9 of this register on demand:

   "To avoid sporadic corruptions “Set 0x7010[9] when Depth Buffer
   Surface Format is D16_UNORM , surface type is not NULL & 1X_MSAA"

BugLink: https://gitlab.freedesktop.org/mesa/mesa/issues/2501
Signed-off-by: Rafael Antognolli 
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 62b43f538a56..57b9685d9347 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1264,6 +1264,12 @@ static void tgl_whitelist_build(struct intel_engine_cs 
*engine)
whitelist_reg_ext(w, PS_INVOCATION_COUNT,
  RING_FORCE_TO_NONPRIV_ACCESS_RD |
  RING_FORCE_TO_NONPRIV_RANGE_4);
+
+   /* Wa_1808121037:tgl
+*
+* Allow userpace to implement this workaround.
+*/
+   whitelist_reg(w, GEN7_COMMON_SLICE_CHICKEN1);
break;
default:
break;
-- 
2.25.0

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

Re: [Intel-gfx] [PATCH 2/2] drm/i915/icl: Apply WaEnablePreemptionGranularityControlByUMD

[Rafael Antognolli]

On Tue, Mar 05, 2019 at 01:48:27PM +0100, Michał Winiarski wrote:
> There are still some cases where userspace needs to change the
> preemption granularity for compute workloads. Let's whitelist the
> per-ctx granularity control register to allow it.

We are not planning to enable it just yet but would be good to easily
enable it in the future.

Acked-by: Rafael Antognolli 

> Signed-off-by: Michał Winiarski 
> Cc: Anuj Phogat 
> Cc: Joonas Lahtinen 
> Cc: Matt Roper 
> Cc: Rafael Antognolli 
> Cc: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/intel_workarounds.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c 
> b/drivers/gpu/drm/i915/intel_workarounds.c
> index 2fba33509f4e..582f554e53f4 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> @@ -1053,6 +1053,9 @@ static void icl_whitelist_build(struct i915_wa_list *w)
>  
>   /* WaAllowUMDToModifySamplerMode:icl */
>   whitelist_reg(w, GEN10_SAMPLER_MODE);
> +
> + /* WaEnablePreemptionGranularityControlByUMD:icl */
> + whitelist_reg(w, GEN8_CS_CHICKEN1);
>  }
>  
>  void intel_engine_init_whitelist(struct intel_engine_cs *engine)
> -- 
> 2.20.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/icl: Default to Thread Group preemption for compute workloads

[Rafael Antognolli]

On Tue, Mar 05, 2019 at 01:48:26PM +0100, Michał Winiarski wrote:
> We assumed that the default preemption granularity is fine for ICL.
> Unfortunately, it turns out that some drivers don't support mid-thread
> preemption for compute workloads.
> If a workload that doesn't support mid-thread preemption gets mid-thread
> preempted, we're going to observe a GPU hang.
> While I'm here, let's also update the "workaround" naming.

Yeah, in Mesa we are not implementing the SIP, so we can't do
thread-level preemption yet and need the granularity to be no higher
than thread group level.

Acked-by: Rafael Antognolli 

> Signed-off-by: Michał Winiarski 
> Cc: Anuj Phogat 
> Cc: Joonas Lahtinen 
> Cc: Matt Roper 
> Cc: Rafael Antognolli 
> Tested-by: Anuj Phogat 
> Reviewed-by: Rodrigo Vivi 
> ---
>  drivers/gpu/drm/i915/intel_workarounds.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c 
> b/drivers/gpu/drm/i915/intel_workarounds.c
> index 89b4007d5200..2fba33509f4e 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> @@ -555,6 +555,11 @@ static void icl_ctx_workarounds_init(struct 
> intel_engine_cs *engine)
>  GEN10_CACHE_MODE_SS,
>  0, /* write-only, so skip validation */
>  _MASKED_BIT_ENABLE(FLOAT_BLEND_OPTIMIZATION_ENABLE));
> +
> + /* WaDisableGPGPUMidThreadPreemption:icl */
> + WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1,
> + GEN9_PREEMPT_GPGPU_LEVEL_MASK,
> + GEN9_PREEMPT_GPGPU_THREAD_GROUP_LEVEL);
>  }
>  
>  void intel_engine_init_ctx_wa(struct intel_engine_cs *engine)
> @@ -1162,8 +1167,8 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, 
> struct i915_wa_list *wal)
>   GEN7_DISABLE_SAMPLER_PREFETCH);
>   }
>  
> - if (IS_GEN(i915, 9) || IS_CANNONLAKE(i915)) {
> - /* 
> WaEnablePreemptionGranularityControlByUMD:skl,bxt,kbl,cfl,cnl */
> + if (IS_GEN_RANGE(i915, 9, 11)) {
> + /* 
> FtrPerCtxtPreemptionGranularityControl:skl,bxt,kbl,cfl,cnl,icl */
>   wa_masked_en(wal,
>GEN7_FF_SLICE_CS_CHICKEN1,
>GEN9_FFSC_PERCTX_PREEMPT_CTRL);
> -- 
> 2.20.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/icl: Apply WaEnablePreemptionGranularityControlByUMD

[Rafael Antognolli]

On Tue, Jan 08, 2019 at 12:32:05PM -0800, Kenneth Graunke wrote:
> On Tuesday, January 8, 2019 7:53:05 AM PST Joonas Lahtinen wrote:
> > + Ken/Jason for Mesa
> > Quoting Matt Roper (2019-01-07 21:19:31)
> > > On Mon, Jan 07, 2019 at 01:23:50PM +0100, Michał Winiarski wrote:
> > > > On Mon, Jan 07, 2019 at 01:01:16PM +0200, Joonas Lahtinen wrote:
> > > > > Quoting José Roberto de Souza (2019-01-04 19:37:00)
> > > > > > According to Workaround database ICL also needs
> > > > > > WaEnablePreemptionGranularityControlByUMD, to allow userspace to do
> > > > > > fine-granularity preemptions per-context.
> > > > > 
> > > > > I must wonder where is the userspace component that needs this, and 
> > > > > why
> > > > > it hasn't been noticed earlier?
> > > > > 
> > > > > Or is this one more of the cases when no userspace actually uses the
> > > > > register?
> > > > 
> > > > It's used:
> > > > https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/mesa/drivers/dri/i965/brw_state_upload.c#L64
> > > > 
> > > > -Michał
> > > 
> > > Wasn't this just an artificial i915-only workaround that was added to
> > > prevent breakage of pre-preemption UMD's?  Initial gen9 driver releases
> > > didn't support preemption, so when preemption support did get added to
> > > i915, the kernel had to force object-level off by default at context
> > > creation to avoid breaking old userspace that didn't build batch buffers
> > > with all the necessary preemption workarounds.  This CS_CHICKEN1
> > > register was then exposed to userspace so that newer, preemption-aware
> > > userspace could opt back in if it properly supported preemption.

It's not only that userspace didn't build proper batch buffers with the
necessary workarounds, but that most of the workarounds required
disabling preemption depending on the type of primitive being drawn. So
userspace needed access to CS_CHICKEN1 to be able to enable/disable
preemption for those.

> > > For gen11, there shouldn't be any "old" userspace around that doesn't
> > > support preemption, so shouldn't the kernel just leave object-level
> > > preemption enabled by default (meaning there's no need to expose this
> > > register to userspace to allow it to explicitly opt-in)?
> > 
> > Makes sense to me. We should have known by know if somebody expects to
> > control the register, because they would be failing to do so.
> > 
> > Mesa could also drop the register load for Gen11+
> > 
> > Regards, Joonas
> 
> + Rafael, as he's done all the preemption work in Mesa.
> 
> That seems reasonable to me.  It looks like i965 always enables
> mid-object preemption (sets CS_CHICKEN1 bit 0) on Gen10+, and never
> disables it.  You can probably safely turn it on by default, and we
> can stop writing the register altogether.

Yeah, I noticed this after re-reading some other thread, right after we
got the preemption patches merged. On gen11, we have some workarounds
but they don't require us to disable preemption through CS_CHICKEN1, so
it should be safe for the kernel to not whitelist or disable it.

Thanks,
Rafael
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/cnl: Add Wa_2201832410

[Rafael Antognolli]

Matches bspec.

Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com>

On Wed, Mar 07, 2018 at 02:09:12PM -0800, Rodrigo Vivi wrote:
> "Clock gating bug in GWL may not clear barrier state when an EOT
> is received, causing a hang the next time that barrier is used."
> 
> HSDES: 2201832410
> 
> Cc: Rafael Antognolli <rafael.antogno...@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 3 +++
>  drivers/gpu/drm/i915/intel_pm.c | 5 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4787d9bf58b9..e6a8c0ee7df1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3965,6 +3965,9 @@ enum {
>  #define  SARBUNIT_CLKGATE_DIS(1 << 5)
>  #define  RCCUNIT_CLKGATE_DIS (1 << 7)
>  
> +#define SUBSLICE_UNIT_LEVEL_CLKGATE  _MMIO(0x9524)
> +#define  GWUNIT_CLKGATE_DIS  (1 << 16)
> +
>  #define UNSLICE_UNIT_LEVEL_CLKGATE   _MMIO(0x9434)
>  #define  VFUNIT_CLKGATE_DIS  (1 << 20)
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6cab20ce167a..b8da4dcdd584 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8522,6 +8522,11 @@ static void cnl_init_clock_gating(struct 
> drm_i915_private *dev_priv)
>   val |= SARBUNIT_CLKGATE_DIS;
>   I915_WRITE(SLICE_UNIT_LEVEL_CLKGATE, val);
>  
> + /* Wa_2201832410:cnl */
> + val = I915_READ(SUBSLICE_UNIT_LEVEL_CLKGATE);
> + val |= GWUNIT_CLKGATE_DIS;
> + I915_WRITE(SUBSLICE_UNIT_LEVEL_CLKGATE, val);
> +
>   /* WaDisableVFclkgate:cnl */
>   /* WaVFUnitClockGatingDisable:cnl */
>   val = I915_READ(UNSLICE_UNIT_LEVEL_CLKGATE);
> -- 
> 2.13.6
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/cnl: document WaVFUnitClockGatingDisable

[Rafael Antognolli]

On Mon, Mar 05, 2018 at 05:20:00PM -0800, Rodrigo Vivi wrote:
> No functional change. WA is already properly applied.
> but in different databases it has different names.
> Let's document all of them to avoid future confusion.

Works for me.

Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com>

> Cc: Rafael Antognolli <rafael.antogno...@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index be01012bb65f..c0253e42a280 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8528,6 +8528,7 @@ static void cnl_init_clock_gating(struct 
> drm_i915_private *dev_priv)
>   I915_WRITE(SUBSLICE_UNIT_LEVEL_CLKGATE, val);
>  
>   /* WaDisableVFclkgate:cnl */
> + /* WaVFUnitClockGatingDisable:cnl */
>   val = I915_READ(UNSLICE_UNIT_LEVEL_CLKGATE);
>   val |= VFUNIT_CLKGATE_DIS;
>   I915_WRITE(UNSLICE_UNIT_LEVEL_CLKGATE, val);
> -- 
> 2.13.6
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915/cnl: WaPipeControlBefore3DStateSamplePattern

[Rafael Antognolli]

This workaround should prevent a bug that can be hit on a context
restore. To avoid the issue, we must emit a PIPE_CONTROL with CS stall
(0x7a04 0x0010 0x 0x) followed by 12DW's of
NOOP(0x0) in the indirect context batch buffer, to ensure the engine is
idle prior to programming 3DSTATE_SAMPLE_PATTERN.

It's also not clear whether we should add those extra dwords because of
the workaround itself, or if that's just padding for the WA BB (and next
commands could come right after the PIPE_CONTROL). We keep them for now.

References: HSD#1939868

 v2: More descriptive changelog and comments.
 v3: Explain that PIPE_CONTROL is actually 6 dwords, and that we advance
 10 more dwords because of that.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Acked-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 38 +-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index adf257dfa5e0..4bca4bc47cca 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1328,6 +1328,40 @@ static u32 *gen9_init_indirectctx_bb(struct 
intel_engine_cs *engine, u32 *batch)
return batch;
 }
 
+static u32 *gen10_init_indirectctx_bb(struct intel_engine_cs *engine, u32 
*batch)
+{
+   int i;
+
+   /*
+* WaPipeControlBefore3DStateSamplePattern: cnl
+*
+* Ensure the engine is idle prior to programming a
+* 3DSTATE_SAMPLE_PATTERN during a context restore.
+*/
+   batch = gen8_emit_pipe_control(batch,
+  PIPE_CONTROL_CS_STALL,
+  0);
+   /*
+* WaPipeControlBefore3DStateSamplePattern says we need 4 dwords for
+* the PIPE_CONTROL followed by 12 dwords of 0x0, so 16 dwords in
+* total. However, a PIPE_CONTROL is 6 dwords long, not 4, which is
+* confusing. Since gen8_emit_pipe_control() already advances the
+* batch by 6 dwords, we advance the other 10 here, completing a
+* cacheline. It's not clear if the workaround requires this padding
+* before other commands, or if it's just the regular padding we would
+* already have for the workaround bb, so leave it here for now.
+*/
+   for (i = 0; i < 10; i++)
+   *batch++ = MI_NOOP;
+
+   /* Pad to end of cacheline */
+   while ((unsigned long)batch % CACHELINE_BYTES)
+   *batch++ = MI_NOOP;
+
+   return batch;
+}
+
+
 #define CTX_WA_BB_OBJ_SIZE (PAGE_SIZE)
 
 static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
@@ -1381,7 +1415,9 @@ static int intel_init_workaround_bb(struct 
intel_engine_cs *engine)
 
switch (INTEL_GEN(engine->i915)) {
case 10:
-   return 0;
+   wa_bb_fn[0] = gen10_init_indirectctx_bb;
+   wa_bb_fn[1] = NULL;
+   break;
case 9:
wa_bb_fn[0] = gen9_init_indirectctx_bb;
wa_bb_fn[1] = NULL;
-- 
2.14.3

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

Re: [Intel-gfx] [PATCH v2] drm/i915/cnl: WaPipeControlBefore3DStateSamplePattern

[Rafael Antognolli]

On Fri, Jan 26, 2018 at 11:17:01PM +, Chris Wilson wrote:
> Quoting Rafael Antognolli (2018-01-26 23:05:24)
> > This workaround should prevent a bug that can be hit on a context
> > restore. To avoid the issue, we must emit a PIPE_CONTROL with CS stall
> > (0x7a04 0x0010 0x 0x) followed by 12DW's of
> > NOOP(0x0) in the indirect context batch buffer, to ensure the engine is
> > idle prior to programming 3DSTATE_SAMPLE_PATTERN.
> > 
> > References: HSD#1939868
> > 
> >  v2:
> >- More descriptive changelog and comments.
> >- Fix math when counting dwords.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 34 +-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index 51e61b04a555..46c130d106d7 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1366,6 +1366,36 @@ static u32 *gen9_init_indirectctx_bb(struct 
> > intel_engine_cs *engine, u32 *batch)
> > return batch;
> >  }
> >  
> > +static u32 *gen10_init_indirectctx_bb(struct intel_engine_cs *engine, u32 
> > *batch)
> > +{
> > +   int i;
> > +
> > +   /*
> > +* WaPipeControlBefore3DStateSamplePattern: cnl
> > +*
> > +* Ensure the engine is idle prior to programming a
> > +* 3DSTATE_SAMPLE_PATTERN during a context restore.
> > +*/
> > +   batch = gen8_emit_pipe_control(batch,
> > +  PIPE_CONTROL_CS_STALL,
> > +  0);
> > +   /*
> > +* WaPipeControlBefore3DStateSamplePattern says we need 4 dwords for
> > +* the PIPE_CONTROL followed by 12 dwords of 0x0, so 16 dwords in
> > +* total. Since gen8_emit_pipe_control() already advances the batch 
> > by
> > +* 6 dwords, we advance the other 10 here.
> > +*/
> > +   for (i = 0; i < 10; i++)
> > +   *batch++ = MI_NOOP;
> 
> If the spec says emit 12 nops, emit 12 nops. Otherwise if we add another
> w/a here it may break this magic.

So, in the bspec, there's the following text:

"The address above needs to be a GGTT address and contain a pipe control
with CS stall(0x7a04 0x0010 0x 0x)followed by
12DW’s of NOOP(0x)"

while on an internal ticket pointed by it, it is:

"The address above needs to be a GGTT address.. It would contain the
following value: 7A04 0010 followed by 14 DW’s of zero."

That's why I think it is really 16 DW's in total. And
gen8_emit_pipe_control() already emits 6 of them. How are we supposed to
emit 12 after that?

I don't know if they expect the pipe control to be cacheline aligned,
but I can ask and try to find out.

> I am not going to ask what the magic nops do, it just gives a 72 cycle
> delay in the CS parser. My guess is that they are trying to isolate a
> cacheline, in which case they are expecting the pipecontrol to be
> cacheline aligned. (I would get that clarified and add an assert to
> document such a requirement.) Or they just mean that the indirect ctx is
> cacheline aligned and the nops are just the normal padding and nothing
> to do with the w/a.
> 
> s/10/12/ here and tweak the comment to remove the assumed cacheline
> tail, but please see if someone can clarify if the nops are indeed part
> of the w/a or just the normal filler.
> With s/10/12/,
> Acked-by: Chris Wilson <ch...@chris-wilson.co.uk>
> -Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915/cnl: WaPipeControlBefore3DStateSamplePattern

[Rafael Antognolli]

This workaround should prevent a bug that can be hit on a context
restore. To avoid the issue, we must emit a PIPE_CONTROL with CS stall
(0x7a04 0x0010 0x 0x) followed by 12DW's of
NOOP(0x0) in the indirect context batch buffer, to ensure the engine is
idle prior to programming 3DSTATE_SAMPLE_PATTERN.

References: HSD#1939868

 v2:
   - More descriptive changelog and comments.
   - Fix math when counting dwords.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 34 +-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 51e61b04a555..46c130d106d7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1366,6 +1366,36 @@ static u32 *gen9_init_indirectctx_bb(struct 
intel_engine_cs *engine, u32 *batch)
return batch;
 }
 
+static u32 *gen10_init_indirectctx_bb(struct intel_engine_cs *engine, u32 
*batch)
+{
+   int i;
+
+   /*
+* WaPipeControlBefore3DStateSamplePattern: cnl
+*
+* Ensure the engine is idle prior to programming a
+* 3DSTATE_SAMPLE_PATTERN during a context restore.
+*/
+   batch = gen8_emit_pipe_control(batch,
+  PIPE_CONTROL_CS_STALL,
+  0);
+   /*
+* WaPipeControlBefore3DStateSamplePattern says we need 4 dwords for
+* the PIPE_CONTROL followed by 12 dwords of 0x0, so 16 dwords in
+* total. Since gen8_emit_pipe_control() already advances the batch by
+* 6 dwords, we advance the other 10 here.
+*/
+   for (i = 0; i < 10; i++)
+   *batch++ = MI_NOOP;
+
+   /* Pad to end of cacheline */
+   while ((unsigned long)batch % CACHELINE_BYTES)
+   *batch++ = MI_NOOP;
+
+   return batch;
+}
+
+
 #define CTX_WA_BB_OBJ_SIZE (PAGE_SIZE)
 
 static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
@@ -1419,7 +1449,9 @@ static int intel_init_workaround_bb(struct 
intel_engine_cs *engine)
 
switch (INTEL_GEN(engine->i915)) {
case 10:
-   return 0;
+   wa_bb_fn[0] = gen10_init_indirectctx_bb;
+   wa_bb_fn[1] = NULL;
+   break;
case 9:
wa_bb_fn[0] = gen9_init_indirectctx_bb;
wa_bb_fn[1] = NULL;
-- 
2.14.3

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

Re: [Intel-gfx] [PATCH] drm/i915/cnl: WaPipeControlBefore3DStateSamplePattern

[Rafael Antognolli]

On Fri, Jan 26, 2018 at 09:55:58AM -0800, Rafael Antognolli wrote:
> On Fri, Jan 26, 2018 at 08:23:13AM +, Chris Wilson wrote:
> > Quoting Rafael Antognolli (2018-01-26 01:26:34)
> > > Write a PIPE_CONTROL with CS stall followed by 14 dwords of 0 in the
> > > indirect context wa bb.
> > 
> > 14 MI_NOOPS following? That isn't what you wrote in the code, but the
> 
> Agreed, sorry. The workarounds says:
> 
> 0x7a04 0x0010 + 14 dwords of 0. I counted 6 dwords from
> gen8_emit_pipe_control() and figured "just 8 more to 14". I think I had
> it right on my first version of this patch, but...
> 
> > main thing you haven't explained is why. A normal batch will already
> > have a flush in its setup, but more to the point would be the only
> > reason this is required is because of an implicit 3DSTATE inside the
> > context image on preemption. Right? Otherwise it seems to be a purely
> > userspace problem.
> 
> This is the text from the workaround:
> 
> "This bug can be hit on a context restore.  To avoid the issue the
> following must be programmed by SW to ensure the engine is idle prior to
> programming 3DSTATE_SAMPLE_PATTERN:
> 
> With RS context enabled: 0x21c8 = 0x85c0
> 
> With RS context disabled:0x21c8 = 0x1ac0
> 
> The above program specifes the offset to insert driver programmed
> commands
> 
> 0x21c4[31:6] = 0x
> 
> 0x21c4[5:0] = 0x
> 
> N=Size of CL needed to fit Workaround
> 
> The above programming is the GGTT address of the driver programmed
> commands and the size(# of CL) to execute.
> 
> The address above needs to be a GGTT address and contain a pipe control
> with CS stall(0x7a04 0x0010 0x 0x)followed by
> 12DW’s of NOOP(0x)"
> 
> Since it needs to be in a GGTT address, and it's specifically talking
> about the Indirect Context Pointer, we figured it should be in the
> kernel. I can update the commit message with the above text if it helps.
> 
> I originally implemented this trying to fix my GPU hang, but it turns
> out the issue was something else and this commit doesn't help at all.
> Still, I see no reason to not have it there just in case it prevent any
> future hangs...

Oh, and this workaround is actually a little longer, and there is a part
that describe a similar PIPE_CONTROL before 3DSTATE_SAMPLE_PATTERN. That
part is already implemented in userspace.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/cnl: WaPipeControlBefore3DStateSamplePattern

[Rafael Antognolli]

On Fri, Jan 26, 2018 at 08:23:13AM +, Chris Wilson wrote:
> Quoting Rafael Antognolli (2018-01-26 01:26:34)
> > Write a PIPE_CONTROL with CS stall followed by 14 dwords of 0 in the
> > indirect context wa bb.
> 
> 14 MI_NOOPS following? That isn't what you wrote in the code, but the

Agreed, sorry. The workarounds says:

0x7a04 0x0010 + 14 dwords of 0. I counted 6 dwords from
gen8_emit_pipe_control() and figured "just 8 more to 14". I think I had
it right on my first version of this patch, but...

> main thing you haven't explained is why. A normal batch will already
> have a flush in its setup, but more to the point would be the only
> reason this is required is because of an implicit 3DSTATE inside the
> context image on preemption. Right? Otherwise it seems to be a purely
> userspace problem.

This is the text from the workaround:

"This bug can be hit on a context restore.  To avoid the issue the
following must be programmed by SW to ensure the engine is idle prior to
programming 3DSTATE_SAMPLE_PATTERN:

With RS context enabled: 0x21c8 = 0x85c0

With RS context disabled:0x21c8 = 0x1ac0

The above program specifes the offset to insert driver programmed
commands

0x21c4[31:6] = 0x

0x21c4[5:0] = 0x

N=Size of CL needed to fit Workaround

The above programming is the GGTT address of the driver programmed
commands and the size(# of CL) to execute.

The address above needs to be a GGTT address and contain a pipe control
with CS stall(0x7a04 0x0010 0x 0x)followed by
12DW’s of NOOP(0x)"

Since it needs to be in a GGTT address, and it's specifically talking
about the Indirect Context Pointer, we figured it should be in the
kernel. I can update the commit message with the above text if it helps.

I originally implemented this trying to fix my GPU hang, but it turns
out the issue was something else and this commit doesn't help at all.
Still, I see no reason to not have it there just in case it prevent any
future hangs...

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

[Intel-gfx] [PATCH] drm/i915/cnl: WaPipeControlBefore3DStateSamplePattern

[Rafael Antognolli]

Write a PIPE_CONTROL with CS stall followed by 14 dwords of 0 in the
indirect context wa bb.

References: HSD#1939868

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 51e61b04a555..fd7e7ffde570 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1366,6 +1366,25 @@ static u32 *gen9_init_indirectctx_bb(struct 
intel_engine_cs *engine, u32 *batch)
return batch;
 }
 
+static u32 *gen10_init_indirectctx_bb(struct intel_engine_cs *engine, u32 
*batch)
+{
+   int i;
+
+   /* WaPipeControlBefore3DStateSamplePattern: cnl */
+   batch = gen8_emit_pipe_control(batch,
+  PIPE_CONTROL_CS_STALL,
+  0);
+   for (i = 0; i < 8; i++)
+   *batch++ = MI_NOOP;
+
+   /* Pad to end of cacheline */
+   while ((unsigned long)batch % CACHELINE_BYTES)
+   *batch++ = MI_NOOP;
+
+   return batch;
+}
+
+
 #define CTX_WA_BB_OBJ_SIZE (PAGE_SIZE)
 
 static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
@@ -1419,7 +1438,9 @@ static int intel_init_workaround_bb(struct 
intel_engine_cs *engine)
 
switch (INTEL_GEN(engine->i915)) {
case 10:
-   return 0;
+   wa_bb_fn[0] = gen10_init_indirectctx_bb;
+   wa_bb_fn[1] = NULL;
+   break;
case 9:
wa_bb_fn[0] = gen9_init_indirectctx_bb;
wa_bb_fn[1] = NULL;
-- 
2.14.3

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

[Intel-gfx] [PATCH 1/2] drm/i915: Implement WaDisableVFclkgate.

[Rafael Antognolli]

This workaround supposedly fixes some hangs in the VF unit.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
Reviewed-by: Lucas De Marchi <lucas.demar...@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 3 +++
 drivers/gpu/drm/i915/intel_pm.c | 5 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 09bf043c1c2e..a9a6d6698bb1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3875,6 +3875,9 @@ enum {
 #define  SARBUNIT_CLKGATE_DIS  (1 << 5)
 #define  RCCUNIT_CLKGATE_DIS   (1 << 7)
 
+#define UNSLICE_UNIT_LEVEL_CLKGATE _MMIO(0x9434)
+#define  VFUNIT_CLKGATE_DIS(1 << 20)
+
 /*
  * Display engine regs
  */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a349c4fd51ff..e33842d6bb14 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -8448,6 +8448,11 @@ static void cnl_init_clock_gating(struct 
drm_i915_private *dev_priv)
if (IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
val |= SARBUNIT_CLKGATE_DIS;
I915_WRITE(SLICE_UNIT_LEVEL_CLKGATE, val);
+
+   /* WaDisableVFclkgate:cnl */
+   val = I915_READ(UNSLICE_UNIT_LEVEL_CLKGATE);
+   val |= VFUNIT_CLKGATE_DIS;
+   I915_WRITE(UNSLICE_UNIT_LEVEL_CLKGATE, val);
 }
 
 static void cfl_init_clock_gating(struct drm_i915_private *dev_priv)
-- 
2.14.3

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

[Intel-gfx] [PATCH 2/2] drm/i915: Implement WaDisableEarlyEOT.

[Rafael Antognolli]

There seems to be another clock gating issue which the workaround is
described as:

   "WA: Set 0xE4F0[1] = 1 to disable Early EOT of thread."

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
Reviewed-by: Lucas De Marchi <lucas.demar...@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h| 1 +
 drivers/gpu/drm/i915/intel_engine_cs.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index a9a6d6698bb1..8c13d3c89107 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8145,6 +8145,7 @@ enum {
 #define   PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE(1<<8)
 #define   STALL_DOP_GATING_DISABLE (1<<5)
 #define   THROTTLE_12_5(7<<2)
+#define   DISABLE_EARLY_EOT(1<<1)
 
 #define GEN7_ROW_CHICKEN2  _MMIO(0xe4f4)
 #define GEN7_ROW_CHICKEN2_GT2  _MMIO(0xf4f4)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index 510e0bc3a377..8b9892947e39 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1272,6 +1272,9 @@ static int cnl_init_workarounds(struct intel_engine_cs 
*engine)
if (ret)
return ret;
 
+   /* WaDisableEarlyEOT:cnl */
+   WA_SET_BIT_MASKED(GEN8_ROW_CHICKEN, DISABLE_EARLY_EOT);
+
return 0;
 }
 
-- 
2.14.3

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

[Intel-gfx] [PATCH 2/2] drm/i915: Implement WaDisableEarlyEOT.

[Rafael Antognolli]

There seems to be another clock gating issue which the workaround is
described as:

   "WA: Set 0xE4F0[1] = 1 to disable Early EOT of thread."

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h| 1 +
 drivers/gpu/drm/i915/intel_engine_cs.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1358ce1513f6..0f07f5900a6f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8145,6 +8145,7 @@ enum {
 #define   PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE(1<<8)
 #define   STALL_DOP_GATING_DISABLE (1<<5)
 #define   THROTTLE_12_5(7<<2)
+#define   DISABLE_EARLY_EOT(1<<1)
 
 #define GEN7_ROW_CHICKEN2  _MMIO(0xe4f4)
 #define GEN7_ROW_CHICKEN2_GT2  _MMIO(0xf4f4)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index 86d4c85c8725..bf581a0bb789 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1272,6 +1272,9 @@ static int cnl_init_workarounds(struct intel_engine_cs 
*engine)
if (ret)
return ret;
 
+   /* WaDisableEarlyEOT:cnl */
+   WA_SET_BIT_MASKED(GEN8_ROW_CHICKEN, DISABLE_EARLY_EOT);
+
return 0;
 }
 
-- 
2.13.6

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

[Intel-gfx] [PATCH 1/2] drm/i915: Implement WaDisableVFclkgate.

[Rafael Antognolli]

This workaround supposedly fixes some hangs in the VF unit.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 3 +++
 drivers/gpu/drm/i915/intel_pm.c | 5 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 09bf043c1c2e..1358ce1513f6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3875,6 +3875,9 @@ enum {
 #define  SARBUNIT_CLKGATE_DIS  (1 << 5)
 #define  RCCUNIT_CLKGATE_DIS   (1 << 7)
 
+#define UNSLICE_UNIT_LEVEL_CLOCK   _MMIO(0x9434)
+#define  VFUNIT_CLKGATE_DIS(1 << 20)
+
 /*
  * Display engine regs
  */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 67f326230a7e..87b9bdee48e5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -8446,6 +8446,11 @@ static void cnl_init_clock_gating(struct 
drm_i915_private *dev_priv)
if (IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
val |= SARBUNIT_CLKGATE_DIS;
I915_WRITE(SLICE_UNIT_LEVEL_CLKGATE, val);
+
+   /* WaDisableVFclkgate:cnl */
+   val = I915_READ(UNSLICE_UNIT_LEVEL_CLOCK);
+   val |= VFUNIT_CLKGATE_DIS;
+   I915_WRITE(UNSLICE_UNIT_LEVEL_CLOCK, val);
 }
 
 static void cfl_init_clock_gating(struct drm_i915_private *dev_priv)
-- 
2.13.6

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

Re: [Intel-gfx] [PATCH] drm/i915: Unify SLICE_UNIT_LEVEL_CLKGATE w/a for cnl

[Rafael Antognolli]

On Sat, Nov 11, 2017 at 10:03:36AM +, Chris Wilson wrote:
> gem_workarounds reports that the SLICE_UNIT_LEVEL_CLKGATE write isn't
> sticking. Commit 0a60797a0efb ("drm/i915: Implement
> ReadHitWriteOnlyDisable.") presumes that SLICE_UNIT_LEVEL_CLKGATE is a
> masked register in the context image, but commit 90007bca6162
> ("drm/i915/cnl: Introduce initial Cannonlake Workarounds.") lists it as
> an ordering unmasked register. The masked write will be losing the
> default settings if we trust the original commit. That gem_workarounds
> reports the value is lost entirely is more worrying though -- but it
> clearly suggests that it is not a masked register in the context image,
> so unify both w/a to use the original rmw.

Thanks for fixing this.

Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com>

> Fixes: 0a60797a0efb ("drm/i915: Implement ReadHitWriteOnlyDisable.")
> References: 90007bca6162 ("drm/i915/cnl: Introduce initial Cannonlake 
> Workarounds.")
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Rafael Antognolli <rafael.antogno...@intel.com>
> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> Cc: Oscar Mateo <oscar.ma...@intel.com>
> Cc: Mika Kuoppala <mika.kuopp...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 3 ---
>  drivers/gpu/drm/i915/intel_pm.c| 8 +---
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 23694916662f..125e4d90c5f7 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1324,9 +1324,6 @@ static int cnl_init_workarounds(struct intel_engine_cs 
> *engine)
>   WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_GPGPU_LEVEL_MASK,
>   GEN9_PREEMPT_GPGPU_COMMAND_LEVEL);
>  
> - /* ReadHitWriteOnlyDisable: cnl */
> - WA_SET_BIT_MASKED(SLICE_UNIT_LEVEL_CLKGATE, RCCUNIT_CLKGATE_DIS);
> -
>   /* WaEnablePreemptionGranularityControlByUMD:cnl */
>   I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1,
>  _MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL));
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c1a56809f143..6dee6b15f726 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8474,11 +8474,13 @@ static void cnl_init_clock_gating(struct 
> drm_i915_private *dev_priv)
>   I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) |
>  DISP_FBC_MEMORY_WAKE);
>  
> + val = I915_READ(SLICE_UNIT_LEVEL_CLKGATE);
> + /* ReadHitWriteOnlyDisable:cnl */
> + val |= RCCUNIT_CLKGATE_DIS;
>   /* WaSarbUnitClockGatingDisable:cnl (pre-prod) */
>   if (IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
> - I915_WRITE(SLICE_UNIT_LEVEL_CLKGATE,
> -I915_READ(SLICE_UNIT_LEVEL_CLKGATE) |
> -SARBUNIT_CLKGATE_DIS);
> + val |= SARBUNIT_CLKGATE_DIS;
> + I915_WRITE(SLICE_UNIT_LEVEL_CLKGATE, val);
>  
>   /* Display WA #1133: WaFbcSkipSegments:cnl */
>   val = I915_READ(ILK_DPFC_CHICKEN);
> -- 
> 2.15.0
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Implement ReadHitWriteOnlyDisable.

[Rafael Antognolli]

On Fri, Nov 10, 2017 at 09:21:51PM +, Chris Wilson wrote:
> Quoting Rafael Antognolli (2017-11-03 18:30:27)
> > The workaround for this is described as:
> > 
> > "if RenderSurfaceState.Num_Multisamples > 1, disable RCC clock gating if
> > RenderSurfaceState.Num_Multisamples == 1, set 0x7010[14] = 1"
> > 
> > Further documentation in the internal bug referenced by the bspec
> > suggest that any of the above suggestions should suffice to fix the
> > issue. We are going with disabling RCC clock gating.
> > 
> > Unfortunately, what we are doing doesn't match the name of the
> > workaround, but at least it matches its description.
> > 
> > This change improves CNL stability by avoiding some of the hangs seen in
> > the platform.
> > 
> > v2: Only disable RCC clock gating.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h| 1 +
> >  drivers/gpu/drm/i915/intel_engine_cs.c | 3 +++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 8c775e96b4e4..bd36ec9bc93f 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3837,6 +3837,7 @@ enum {
> >   */
> >  #define SLICE_UNIT_LEVEL_CLKGATE   _MMIO(0x94d4)
> >  #define  SARBUNIT_CLKGATE_DIS  (1 << 5)
> > +#define  RCCUNIT_CLKGATE_DIS   (1 << 7)
> >  
> >  /*
> >   * Display engine regs
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> > b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index f31f2d6384c3..3af0dcb91e9c 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -1320,6 +1320,9 @@ static int cnl_init_workarounds(struct 
> > intel_engine_cs *engine)
> > WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_GPGPU_LEVEL_MASK,
> > GEN9_PREEMPT_GPGPU_COMMAND_LEVEL);
> >  
> > +   /* ReadHitWriteOnlyDisable: cnl */
> > +   WA_SET_BIT_MASKED(SLICE_UNIT_LEVEL_CLKGATE, RCCUNIT_CLKGATE_DIS);
> 
> This is not sticking. Why are we applying SLICE_UNIT_LEVEL_CLKGATE as a
> context register here, and as an ordinary unmasked register over in
> cnl_init_clock_gating() ?

Ugh, I didn't know there was a better place for this. Feel free to move it to
cnl_init_clock_gating(), or I can send a patch soon. Sorry for the mess.

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

Re: [Intel-gfx] [PATCH v2 6/9] drm/i915: expose command stream timestamp frequency to userspace

[Rafael Antognolli]

This patch, along with the respective ones for Mesa, does fix the gl
timestamp query piglit failures on CNL. So it is

Tested-by: Rafael Antognolli <rafael.antogno...@intel.com>

On Thu, Nov 02, 2017 at 04:29:46PM +, Lionel Landwerlin wrote:
> We use to have this fixed per generation, but starting with CNL userspace
> cannot tell just off the PCI ID. Let's make this information available. This
> is particularly useful for performance monitoring where much of the
> normalization work is done using those timestamps (this include pipeline
> statistics in both GL & Vulkan as well as OA reports).
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwer...@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +
>  drivers/gpu/drm/i915/i915_drv.c  |  3 +
>  drivers/gpu/drm/i915/i915_drv.h  |  2 +
>  drivers/gpu/drm/i915/i915_reg.h  | 21 +++
>  drivers/gpu/drm/i915/intel_device_info.c | 99 
> 
>  include/uapi/drm/i915_drm.h  |  6 ++
>  6 files changed, 133 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 39883cd915db..0897fd616a1f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3246,6 +3246,8 @@ static int i915_engine_info(struct seq_file *m, void 
> *unused)
>  yesno(dev_priv->gt.awake));
>   seq_printf(m, "Global active requests: %d\n",
>  dev_priv->gt.active_requests);
> + seq_printf(m, "CS timestamp frequency: %llu\n",
> +dev_priv->info.cs_timestamp_frequency);
>  
>   p = drm_seq_file_printer(m);
>   for_each_engine(engine, dev_priv, id)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e7e9e061073b..fdd23e79fb46 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -416,6 +416,9 @@ static int i915_getparam(struct drm_device *dev, void 
> *data,
>   if (!value)
>   return -ENODEV;
>   break;
> + case I915_PARAM_CS_TIMESTAMP_FREQUENCY:
> + value = INTEL_INFO(dev_priv)->cs_timestamp_frequency;
> + break;
>   default:
>   DRM_DEBUG("Unknown parameter %d\n", param->param);
>   return -EINVAL;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6cb7cd7f9420..4e804aaeaae1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -886,6 +886,8 @@ struct intel_device_info {
>   /* Slice/subslice/EU info */
>   struct sseu_dev_info sseu;
>  
> + uint64_t cs_timestamp_frequency;
> +
>   struct color_luts {
>   u16 degamma_lut_size;
>   u16 gamma_lut_size;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a2223f01ee2a..f392f28f2cfa 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1119,9 +1119,24 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  
>  /* RPM unit config (Gen8+) */
>  #define RPM_CONFIG0  _MMIO(0x0D00)
> +#define  GEN9_RPM_CONFIG0_CRYSTAL_CLOCK_FREQ_SHIFT   3
> +#define  GEN9_RPM_CONFIG0_CRYSTAL_CLOCK_FREQ_MASK(1 << 
> GEN9_RPM_CONFIG0_CRYSTAL_CLOCK_FREQ_SHIFT)
> +#define  GEN9_RPM_CONFIG0_CRYSTAL_CLOCK_FREQ_19_2_MHZ0
> +#define  GEN9_RPM_CONFIG0_CRYSTAL_CLOCK_FREQ_24_MHZ  1
> +#define  GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_SHIFT 1
> +#define  GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK  (0x3 << 
> GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_SHIFT)
> +
>  #define RPM_CONFIG1  _MMIO(0x0D04)
>  #define  GEN10_GT_NOA_ENABLE  (1 << 9)
>  
> +/* GPM unit config (assuming Gen8+, documentation is fuzzy...) */
> +#define GEN8_CTC_MODE_MMIO(0xA26C)
> +#define  GEN8_CTC_SOURCE_PARAMETER_MASK 1
> +#define  GEN8_CTC_SOURCE_CRYSTAL_CLOCK   0
> +#define  GEN8_CTC_SOURCE_DIVIDE_LOGIC1
> +#define  GEN8_CTC_SHIFT_PARAMETER_SHIFT  1
> +#define  GEN8_CTC_SHIFT_PARAMETER_MASK   (0x3 << 
> GEN8_CTC_SHIFT_PARAMETER_SHIFT)
> +
>  /* RPC unit config (Gen8+) */
>  #define RPC_CONFIG   _MMIO(0x0D08)
>  
> @@ -8865,6 +8880,12 @@ enum skl_power_gate {
>  #define ILK_TIMESTAMP_HI _MMIO(0x70070)
>  #define IVB_TIMESTAMP_CTR_MMIO(0x44070)
>  
> +#define GEN8_TIMESTAMP_OVERRIDE  _MMIO(0x44074)
> +#define  GEN8_TIMESTAMP_OVERRIDE_US_COUNTER_SHIFT0
> +#define  GEN8_TIMESTAMP_OVERRIDE_US_COUNTER_MASK 0x

[Intel-gfx] [PATCH v2] drm/i915: Implement ReadHitWriteOnlyDisable.

[Rafael Antognolli]

The workaround for this is described as:

"if RenderSurfaceState.Num_Multisamples > 1, disable RCC clock gating if
RenderSurfaceState.Num_Multisamples == 1, set 0x7010[14] = 1"

Further documentation in the internal bug referenced by the bspec
suggest that any of the above suggestions should suffice to fix the
issue. We are going with disabling RCC clock gating.

Unfortunately, what we are doing doesn't match the name of the
workaround, but at least it matches its description.

This change improves CNL stability by avoiding some of the hangs seen in
the platform.

v2: Only disable RCC clock gating.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h| 1 +
 drivers/gpu/drm/i915/intel_engine_cs.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8c775e96b4e4..bd36ec9bc93f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3837,6 +3837,7 @@ enum {
  */
 #define SLICE_UNIT_LEVEL_CLKGATE   _MMIO(0x94d4)
 #define  SARBUNIT_CLKGATE_DIS  (1 << 5)
+#define  RCCUNIT_CLKGATE_DIS   (1 << 7)
 
 /*
  * Display engine regs
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index f31f2d6384c3..3af0dcb91e9c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1320,6 +1320,9 @@ static int cnl_init_workarounds(struct intel_engine_cs 
*engine)
WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_GPGPU_LEVEL_MASK,
GEN9_PREEMPT_GPGPU_COMMAND_LEVEL);
 
+   /* ReadHitWriteOnlyDisable: cnl */
+   WA_SET_BIT_MASKED(SLICE_UNIT_LEVEL_CLKGATE, RCCUNIT_CLKGATE_DIS);
+
/* WaEnablePreemptionGranularityControlByUMD:cnl */
I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1,
   _MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL));
-- 
2.13.6

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

Re: [Intel-gfx] [PATCH] drm/i915: Implement ReadHitWriteOnlyDisable.

[Rafael Antognolli]

On Wed, Nov 01, 2017 at 02:11:05PM -0700, Rodrigo Vivi wrote:
> On Wed, Nov 01, 2017 at 04:32:35PM +0000, Rafael Antognolli wrote:
> > The workaround for this is described as:
> > 
> > "if RenderSurfaceState.Num_Multisamples > 1, disable RCC clock gating if
> > RenderSurfaceState.Num_Multisamples == 1, set 0x7010[14] = 1"
> > 
> > So it looks like the userspace should be responsible for setting these,
> > based on the number of multisamples dependency. However, the register
> > that controls RCC clock gating is not a context register, and cannot be
> > set by userspace.
> > 
> > Since we would end up setting one or another based on the number of
> > multisamples anyway, it seems harmless to just set both all the time.
> > 
> > This change (specially the GEN10_READ_HIT_WRITEONLY_DISABLE bit)
> 
> I wonder if we shouldn't stay only with this bit. For me it looks like
> one or another.

I would say we need at least the GEN10_READ_HIT_WRITEONLY_DISABLE bit, and then
we can decide if we are adding the other one or not. Within the same
program (I think sometimes even within a single draw call), it's
possible that we have some surfaces that are multisampled while others
are not, so in that case we would probably have to set both anyway.

However, I don't have a test case that proves that the workaround for
the multisampled case is required...

> > improves CNL stability by avoiding some of the hangs seen in the
> > platform.
> 
> But this is what matters. If this is the safest option for us
> let's do it.
> 
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h| 2 ++
> >  drivers/gpu/drm/i915/intel_engine_cs.c | 5 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 8c775e96b4e4..d9a65cebefaa 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3837,6 +3837,7 @@ enum {
> >   */
> >  #define SLICE_UNIT_LEVEL_CLKGATE   _MMIO(0x94d4)
> >  #define  SARBUNIT_CLKGATE_DIS  (1 << 5)
> > +#define  RCCUNIT_CLKGATE_DIS   (1 << 7)
> >  
> >  /*
> >   * Display engine regs
> > @@ -7016,6 +7017,7 @@ enum {
> >  #define GEN7_COMMON_SLICE_CHICKEN1 _MMIO(0x7010)
> >  # define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC ((1<<10) | (1<<26))
> >  # define GEN9_RHWO_OPTIMIZATION_DISABLE(1<<14)
> > +# define GEN10_READ_HIT_WRITEONLY_DISABLE  (1<<14)
> 
> I don't believe you need to redefine this.
> It is same as GEN9_RHWO_OPTIMIZATION_DISABLE.
> 
> RCC Read Hit Write Only Optimization Disabled, SKL+ o spec.

Oh, I haven't noticed that! Will fix it in the next version...

> >  #define COMMON_SLICE_CHICKEN2  _MMIO(0x7014)
> >  # define GEN9_PBE_COMPRESSED_HASH_SELECTION(1<<13)
> >  # define GEN9_DISABLE_GATHER_AT_SET_SHADER_COMMON_SLICE (1<<12)
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> > b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index f31f2d6384c3..0d8e25a4623a 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -1320,6 +1320,11 @@ static int cnl_init_workarounds(struct 
> > intel_engine_cs *engine)
> > WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_GPGPU_LEVEL_MASK,
> > GEN9_PREEMPT_GPGPU_COMMAND_LEVEL);
> >  
> > +   /* ReadHitWriteOnlyDisable: cnl */
> 
> I was going to complain about the name but I saw on bspec it is really
> ReadHitWriteOnlyDisable while on wa_database it is WaReadHitWriteOnlyDisable
> 
> I would tend to prefer the second one, but with the first one the search on 
> Bspec works
> and search on wa_database also works... while second one it would be found on 
> BSpec.
> So let it be: ReadHitWriteOnlyDisable
> 
> Thanks,
> Rodrigo.
> 
> > +   WA_SET_BIT_MASKED(GEN7_COMMON_SLICE_CHICKEN1,
> > + GEN10_READ_HIT_WRITEONLY_DISABLE);
> > +   WA_SET_BIT_MASKED(SLICE_UNIT_LEVEL_CLKGATE, RCCUNIT_CLKGATE_DIS);
> > +
> 
> > /* WaEnablePreemptionGranularityControlByUMD:cnl */
> > I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1,
> >_MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL));
> > -- 
> > 2.13.6
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Implement ReadHitWriteOnlyDisable.

[Rafael Antognolli]

The workaround for this is described as:

"if RenderSurfaceState.Num_Multisamples > 1, disable RCC clock gating if
RenderSurfaceState.Num_Multisamples == 1, set 0x7010[14] = 1"

So it looks like the userspace should be responsible for setting these,
based on the number of multisamples dependency. However, the register
that controls RCC clock gating is not a context register, and cannot be
set by userspace.

Since we would end up setting one or another based on the number of
multisamples anyway, it seems harmless to just set both all the time.

This change (specially the GEN10_READ_HIT_WRITEONLY_DISABLE bit)
improves CNL stability by avoiding some of the hangs seen in the
platform.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h| 2 ++
 drivers/gpu/drm/i915/intel_engine_cs.c | 5 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8c775e96b4e4..d9a65cebefaa 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3837,6 +3837,7 @@ enum {
  */
 #define SLICE_UNIT_LEVEL_CLKGATE   _MMIO(0x94d4)
 #define  SARBUNIT_CLKGATE_DIS  (1 << 5)
+#define  RCCUNIT_CLKGATE_DIS   (1 << 7)
 
 /*
  * Display engine regs
@@ -7016,6 +7017,7 @@ enum {
 #define GEN7_COMMON_SLICE_CHICKEN1 _MMIO(0x7010)
 # define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC ((1<<10) | (1<<26))
 # define GEN9_RHWO_OPTIMIZATION_DISABLE(1<<14)
+# define GEN10_READ_HIT_WRITEONLY_DISABLE  (1<<14)
 #define COMMON_SLICE_CHICKEN2  _MMIO(0x7014)
 # define GEN9_PBE_COMPRESSED_HASH_SELECTION(1<<13)
 # define GEN9_DISABLE_GATHER_AT_SET_SHADER_COMMON_SLICE (1<<12)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index f31f2d6384c3..0d8e25a4623a 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1320,6 +1320,11 @@ static int cnl_init_workarounds(struct intel_engine_cs 
*engine)
WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_GPGPU_LEVEL_MASK,
GEN9_PREEMPT_GPGPU_COMMAND_LEVEL);
 
+   /* ReadHitWriteOnlyDisable: cnl */
+   WA_SET_BIT_MASKED(GEN7_COMMON_SLICE_CHICKEN1,
+ GEN10_READ_HIT_WRITEONLY_DISABLE);
+   WA_SET_BIT_MASKED(SLICE_UNIT_LEVEL_CLKGATE, RCCUNIT_CLKGATE_DIS);
+
/* WaEnablePreemptionGranularityControlByUMD:cnl */
I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1,
   _MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL));
-- 
2.13.6

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

Re: [Intel-gfx] [PATCH] drm/i915/cnl: WaSendPushConstantsFromMMIO / WaDisableGatherAtSetShaderCommonSlice

[Rafael Antognolli]

I can confirm this fixes my CNL issues, and it's what we could
understand from the sparse documentation.

Tested-by: Rafael Antognolli <rafael.antogno...@intel.com>

On Thu, Oct 05, 2017 at 03:26:40PM -0700, Rodrigo Vivi wrote:
> WaSendPushConstantsFromMMIO: "If not using RS, we must send two
> MMIO registers at context create to trigger push constants at
> 3D primitive"
> 
> There is almost no documentation on Spec or wa_database about
> this WaSendPushConstantsFromMMIO. So we also found out in another
> place that WaSendPushConstantsFromMMIO was only adding
> COMMON_SLICE_CHICKEN2 in a white list.
> 
> So we looked to the programming notes of COMMON_SLICE_CHICKEN2
> and we notice that this bit 12 is marked in association with
> 2 other MMIO registers for SKL+. Apparently for SKL+ we should
> check few MMIOs to decide for set or reset of this bit 12.
> 
> Also "gather" is related to gather and packing of constants elements
> into "push constants".
> 
> Mesa has no plans of using RS so there is no need to whitelist
> and we only need to initialize the context image with that bit clear.
> 
> v2: Move to cnl_init_workarounds as Chris suggested.
> Add both Wa names and improve commit message as Rafael suggested.
> 
> Cc: Rafael Antognolli <rafael.antogno...@intel.com>
> Cc: Mika Kuoppala <mika.kuopp...@intel.com>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 807a7aafc089..3beb7d327785 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1296,6 +1296,11 @@ static int cnl_init_workarounds(struct intel_engine_cs 
> *engine)
>   WA_SET_BIT_MASKED(COMMON_SLICE_CHICKEN2,
> GEN8_CSC2_SBE_VUE_CACHE_CONSERVATIVE);
>  
> + /* WaDisableGatherAtSetShaderCommonSlice:cnl */
> + /* WaSendPushConstantsFromMMIO:cnl */
> + WA_CLR_BIT_MASKED(COMMON_SLICE_CHICKEN2,
> +   GEN9_DISABLE_GATHER_AT_SET_SHADER_COMMON_SLICE);
> +
>   /* WaInPlaceDecompressionHang:cnl */
>   I915_WRITE(GEN9_GAMT_ECO_REG_RW_IA,
>  (I915_READ(GEN9_GAMT_ECO_REG_RW_IA) |
> -- 
> 2.13.5
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] aubdump: Don't bail if a GEM handle of 0 is passed into execbuf

[Rafael Antognolli]

Patch is
Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com>

On Fri, Mar 24, 2017 at 04:45:01PM -0700, Jason Ekstrand wrote:
> A gem handle of 0 can be used to check for whether or not 48-bit
> addressing is available.  This keeps aubdump from failing on you if
> you try to do the check.
> ---
>  tools/aubdump.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/aubdump.c b/tools/aubdump.c
> index 8a89b8c..3aca7eb 100644
> --- a/tools/aubdump.c
> +++ b/tools/aubdump.c
> @@ -131,7 +131,6 @@ get_bo(uint32_t handle)
>  
>   fail_if(handle >= MAX_BO_COUNT, "bo handle too large\n");
>   bo = [handle];
> - fail_if(bo->size == 0, "invalid bo handle (%d) in execbuf\n", handle);
>  
>   return bo;
>  }
> @@ -442,7 +441,7 @@ dump_execbuffer2(int fd, struct drm_i915_gem_execbuffer2 
> *execbuffer2)
>   offset = align_u32(offset + bo->size + 4095, 4096);
>   }
>  
> - if (bo->map == NULL)
> + if (bo->map == NULL && bo->size > 0)
>   bo->map = gem_mmap(fd, obj->handle, 0, bo->size);
>   fail_if(bo->map == MAP_FAILED, "intel_aubdump: bo mmap 
> failed\n");
>   }
> @@ -583,7 +582,7 @@ maybe_init(void)
>   }
>   fclose(config);
>  
> - bos = malloc(MAX_BO_COUNT * sizeof(bos[0]));
> + bos = calloc(MAX_BO_COUNT, sizeof(bos[0]));
>   fail_if(bos == NULL, "intel_aubdump: out of memory\n");
>  }
>  
> -- 
> 2.5.0.400.gff86faf
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] DP Aux interfaces inquiry

[Rafael Antognolli]

Hi Mario, please see below...

On Wed, Jan 25, 2017 at 04:43:58PM +, mario.limoncie...@dell.com wrote:
> Thanks for your comments.  Some nested below.
> 
> > -Original Message-
> > From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> > Sent: Wednesday, January 25, 2017 9:57 AM
> > To: Limonciello, Mario 
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] DP Aux interfaces inquiry
> > 
> > On Tue, Jan 24, 2017 at 07:51:28PM +, mario.limoncie...@dell.com
> > wrote:
> > > Hi,
> > >
> > > Recently Synaptics collaborated with Dell on a plugin [1] for fwupd that
> > allows flashing Synaptics MST hubs using the DP aux interface to manipulate
> > the DPCD [2].
> > > Currently the plugin hardcodes the max number of DP aux devices to look 
> > > for
> > to 3 (as that's what we've seen so far on HW), but we were wondering:
> > >
> > >   1) If there is a way to query the number of devices that the kernel is
> > going to be creating?
> > >   2) Are there any instances of more than 3 devices in the wild today that
> > anyone is aware of?
> > 
> > These depend on the board you're dealing with, and on the number of gpus you
> > have in the system (and whether they actually have drivers loaded for them).
> 
> OK, that's what I was suspecting.
> 
> > 
> > We should also perhaps expose the aux device node for MST devices. At which
> > point the number of aux nodes could change dynamically when you plug MST
> > devices in/out.
> > 
> 
> Hmm, for the devices themselves?  The way Synaptics handles cascaded MST 
> devices
> today is a remote control mechanism.  They're able to turn on remote control
> for one MST hub, and it will forward control commands (and payloads) to the 
> next
> cascaded hub.
> 
> If/when you do this can you send a uevent up to userspace?  It would be good
> for fwupd to be able to listen to it and refresh devices based upon what 
> happened
> from nodes coming and going.
> 
> > So I don't think you should be making any assumptions on the number/order of
> > these device nodes.
> > 
> 
> From userspace would it be better to just scan /dev for /dev/drm_dp_aux# 
> nodes?
> No assumptions about the order of them, just look for all the ones with that 
> prefix?
> 
> If not, do you have a better recommendation on how to do this?

Alternatively, you can also scan /sys/class/drm_dp_aux_dev/. The number
of the files that you have there should match what you have inside /dev.
It's not necessarily better though.

Regards,
Rafael
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 14/14] drm/i915: Support explicit fencing for execbuf

[Rafael Antognolli]

Hi Chris,

I'm not sure you are waiting for this kind of feedback, but I've managed
to test this particular patch with Mesa, and I have some piglit tests
for it too. They are waiting for review, but at least the feature is
working as expected:

https://github.com/rantogno/piglit/commits/review/fences-v02

I used mesa and libdrm patches from more than a single set, but in case
you want to look they are here:

https://github.com/rantogno/mesa/commits/wip-fence
https://github.com/rantogno/libdrm/commits/wip-fence

And my kernel tree when I tested it:

https://github.com/rantogno/linux/commits/wip-fence

So in case it helps, you can add a

Tested-by: Rafael Antognolli <rafael.antogno...@intel.com>

PS: I can test it with this last series and everything more up to date,
if it will help to get this patch landed sooner.

Cheers,
Rafael

On Mon, Nov 14, 2016 at 08:57:03AM +, Chris Wilson wrote:
> Now that the user can opt-out of implicit fencing, we need to give them
> back control over the fencing. We employ sync_file to wrap our
> drm_i915_gem_request and provide an fd that userspace can merge with
> other sync_file fds and pass back to the kernel to wait upon before
> future execution.
> 
> Testcase: igt/gem_exec_fence
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/Kconfig   |  1 +
>  drivers/gpu/drm/i915/i915_drv.c|  3 +-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 54 
> +++---
>  include/uapi/drm/i915_drm.h| 35 ++-
>  4 files changed, 86 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index beed5c1d2cd7..bed81fe1d2a7 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -19,6 +19,7 @@ config DRM_I915
>   select INPUT if ACPI
>   select ACPI_VIDEO if ACPI
>   select ACPI_BUTTON if ACPI
> + select SYNC_FILE
>   help
> Choose this option if you have a system that has "Intel Graphics
> Media Accelerator" or "HD Graphics" integrated graphics,
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 657c465a8d50..6fe7f41a5b5b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -344,6 +344,7 @@ static int i915_getparam(struct drm_device *dev, void 
> *data,
>   case I915_PARAM_HAS_COHERENT_PHYS_GTT:
>   case I915_PARAM_HAS_EXEC_SOFTPIN:
>   case I915_PARAM_HAS_EXEC_ASYNC:
> + case I915_PARAM_HAS_EXEC_FENCE:
>   /* For the time being all of these are always true;
>* if some supported hardware does not have one of these
>* features this value needs to be provided from
> @@ -2533,7 +2534,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>   DRM_IOCTL_DEF_DRV(I915_HWS_ADDR, drm_noop, 
> DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>   DRM_IOCTL_DEF_DRV(I915_GEM_INIT, drm_noop, 
> DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>   DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER, i915_gem_execbuffer, DRM_AUTH),
> - DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER2, i915_gem_execbuffer2, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER2_WR, i915_gem_execbuffer2, 
> DRM_AUTH|DRM_RENDER_ALLOW),
>   DRM_IOCTL_DEF_DRV(I915_GEM_PIN, i915_gem_reject_pin_ioctl, 
> DRM_AUTH|DRM_ROOT_ONLY),
>   DRM_IOCTL_DEF_DRV(I915_GEM_UNPIN, i915_gem_reject_pin_ioctl, 
> DRM_AUTH|DRM_ROOT_ONLY),
>   DRM_IOCTL_DEF_DRV(I915_GEM_BUSY, i915_gem_busy_ioctl, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 781b5559f86e..facec610b55a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -28,6 +28,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -1597,6 +1598,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void 
> *data,
>   struct i915_execbuffer_params *params = _master;
>   const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
>   u32 dispatch_flags;
> + struct dma_fence *in_fence = NULL;
> + struct sync_file *out_fence = NULL;
> + int out_fence_fd = -1;
>   int ret;
>   bool need_relocs;
>  
> @@ -1640,6 +1644,23 @@ i915_gem_do_execbuffer(struct drm_device *dev, void 
> *data,
>   dispatch_flags |= I915_DISPATCH_RS;
>   }
>  
> + if (args->flags & I915_EXEC_FENCE_IN) {
> + in_fence = sync_file_get_fence(lower_32_

Re: [Intel-gfx] [PATCH libdrm 15/15] intel: Support passing of explicit fencing from execbuf

[Rafael Antognolli]

Hi Chris,

On Thu, Aug 25, 2016 at 10:08:33AM +0100, Chris Wilson wrote:
> Allow the caller to pass in an fd to an array of fences to control
> serialisation of the execbuf in the kernel and on the GPU, and in return
> allow creation of a fence fd for signaling the completion (and flushing)
> of the batch. When the returned fence is signaled, all writes to the
> buffers inside the batch will be complete and coherent from the cpu, or
> other consumers. The return fence is a sync_file object and can be
> passed to other users (such as atomic modesetting, or other drivers).
> 
> Signed-off-by: Chris Wilson 
> ---
>  intel/intel_bufmgr.h |  6 ++
>  intel/intel_bufmgr_gem.c | 38 +-
>  2 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 259543f..02ec757 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -2412,13 +2419,20 @@ do_exec2(drm_intel_bo *bo, int used, 
> drm_intel_context *ctx,
>   i915_execbuffer2_set_context_id(execbuf, 0);
>   else
>   i915_execbuffer2_set_context_id(execbuf, ctx->ctx_id);
> - execbuf.rsvd2 = 0;
> + if (in_fence != -1) {
> + execbuf.rsvd2 = in_fence;
> + flags |= I915_EXEC_FENCE_IN;

The flags are being set here, but not really used anywhere.
Maybe you meant something like:

execbuf.flags != I915_EXEC_FENCE_IN;

?

> + if (out_fence != NULL) {
> + *out_fence = -1;
> + flags |= I915_EXEC_FENCE_OUT;
> + }

Same as above.

Regards,
Rafael
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t] tests/sw_sync: Add subtest test_sync_expired_merge

[Rafael Antognolli]

This test creates an already expired fence, then creates a merged fence
out of that expired one (passed twice to the merge operation), and
finally closes the merged fence. It shows that if the refcounts are
wrong on the original expired fence, it might get freed while still in
use. Usually a kernel panick will follow.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 tests/sw_sync.c | 28 
 1 file changed, 28 insertions(+)

diff --git a/tests/sw_sync.c b/tests/sw_sync.c
index 4336659..31cde50 100644
--- a/tests/sw_sync.c
+++ b/tests/sw_sync.c
@@ -582,6 +582,31 @@ static void test_sync_multi_producer_single_consumer(void)
pthread_join(threads[i], NULL);
 }
 
+static void test_sync_expired_merge(void)
+{
+   int iterations = 1 << 20;
+   int timeline;
+   int i;
+   int fence_expired, fence_merged;
+
+   timeline = sw_sync_timeline_create();
+
+   sw_sync_timeline_inc(timeline, 100);
+   fence_expired = sw_sync_fence_create(timeline, 1);
+   fence_merged = sw_sync_merge(fence_expired, fence_expired);
+   sw_sync_fence_destroy(fence_merged);
+
+   for (i = 0; i < iterations; i++) {
+   int fence = sw_sync_merge(fence_expired, fence_expired);
+
+   igt_assert_f(sw_sync_wait(fence, -1) > 0,
+"Failure waiting on fence\n");
+   sw_sync_fence_destroy(fence);
+   }
+
+   sw_sync_fence_destroy(fence_expired);
+}
+
 static void test_sync_random_merge(void)
 {
int i, size, ret;
@@ -687,6 +712,9 @@ igt_main
igt_subtest("sync_multi_producer_single_consumer")
test_sync_multi_producer_single_consumer();
 
+   igt_subtest("sync_expired_merge")
+   test_sync_expired_merge();
+
igt_subtest("sync_random_merge")
test_sync_random_merge();
 }
-- 
2.7.4

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

[Intel-gfx] [PATCH] dma-buf/sync_file: Always increment refcount when merging fences.

[Rafael Antognolli]

The refcount of a fence should be increased whenever it is added to a merged
fence, since it will later be decreased when the merged fence is destroyed.
Failing to do so will cause the original fence to be freed if the merged fence
gets freed, but other places still referencing won't know about it.

This patch fixes a kernel panic that can be triggered by creating a fence that
is expired (or increasing the timeline until it expires), then creating a
merged fence out of it, and deleting the merged fence. This will make the
original expired fence's refcount go to zero.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---

Sample code to trigger the mentioned kernel panic (might need to be executed a
couple times before it actually breaks everything):

static void test_sync_expired_merge(void)
{
   int iterations = 1 << 20;
   int timeline;
   int i;
   int fence_expired, fence_merged;

   timeline = sw_sync_timeline_create();

   sw_sync_timeline_inc(timeline, 100);
   fence_expired = sw_sync_fence_create(timeline, 1);
   fence_merged = sw_sync_merge(fence_expired, fence_expired);
   sw_sync_fence_destroy(fence_merged);

   for (i = 0; i < iterations; i++) {
   int fence = sw_sync_merge(fence_expired, fence_expired);

   igt_assert_f(sw_sync_wait(fence, -1) > 0,
"Failure waiting on fence\n");
   sw_sync_fence_destroy(fence);
   }

   sw_sync_fence_destroy(fence_expired);
}

 drivers/dma-buf/sync_file.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 486d29c..6ce6b8f 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -178,11 +178,8 @@ static struct fence **get_fences(struct sync_file 
*sync_file, int *num_fences)
 static void add_fence(struct fence **fences, int *i, struct fence *fence)
 {
fences[*i] = fence;
-
-   if (!fence_is_signaled(fence)) {
-   fence_get(fence);
-   (*i)++;
-   }
+   fence_get(fence);
+   (*i)++;
 }
 
 /**
-- 
2.7.4

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

[Intel-gfx] [PATCH i-g-t v2] tests: Adding kms_dp_aux_dev test.

[Rafael Antognolli]

This new test makes some basic testing on the proposed drm_dp_aux_dev
interface. If the feature is enabled and the drm_dp_aux_dev class is
present, it will check for available DP aux channels and test them for:
- basic seek to 0 and read 1 byte
- seek to the last address and read, to confirm 0 is returned
- seek one more byte and confirm that EINVAL is returned
- try to read 64 bytes when at 8 bytes from the end of the
address space
- try to read 64 bytes at the address 0.

So far, no write checks are done.

v2:
 - compare DP version, max link rate and max lane count
 - check seek position after reading
 - merge all subtests into a single one.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 tests/Makefile.sources |   1 +
 tests/kms_dp_aux_dev.c | 190 +
 2 files changed, 191 insertions(+)
 create mode 100644 tests/kms_dp_aux_dev.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index f8b18b0..7920e23 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -79,6 +79,7 @@ TESTS_progs_M = \
kms_atomic \
kms_chv_cursor_fail \
kms_cursor_crc \
+   kms_dp_aux_dev \
kms_draw_crc \
kms_fbc_crc \
kms_fbcon_fbt \
diff --git a/tests/kms_dp_aux_dev.c b/tests/kms_dp_aux_dev.c
new file mode 100644
index 000..088d980
--- /dev/null
+++ b/tests/kms_dp_aux_dev.c
@@ -0,0 +1,190 @@
+/*
+ * Copyright © 2016 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.
+ */
+
+/** @file kms_dp_aux_dev.c
+ *
+ * This is a test of functionality of drm_dp_aux_dev.
+ */
+
+#include "igt.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define MAX_CONNECTORS 32
+#define DRM_DP_AUX_SYSFS_PATH "/sys/class/drm_dp_aux_dev"
+
+IGT_TEST_DESCRIPTION("Test of drm_aux_dev nodes.");
+
+static void basic_seek_and_read(int fd)
+{
+   off_t address;
+   uint8_t buf;
+   uint8_t bigbuf[64];
+   ssize_t res;
+
+   /* Basic seek and read */
+   address = lseek(fd, 0x0, SEEK_SET);
+   igt_assert_eq(address, 0x0);
+
+   res = read(fd, , sizeof(buf)); /* read DP version */
+   igt_assert_eq(res, sizeof(buf));
+
+   address = lseek(fd, 0, SEEK_CUR);
+   igt_assert_eq(address, 0x1);
+
+   igt_assert((buf == 0x10) || (buf == 0x11) || (buf == 0x12));
+
+   res = read(fd, , sizeof(buf)); /* read max link rate */
+   igt_assert_eq(res, sizeof(buf));
+
+   address = lseek(fd, 0, SEEK_CUR);
+   igt_assert_eq(address, 0x2);
+
+   igt_assert((buf == 0x06) || (buf == 0x0a) || (buf == 0x14));
+
+   res = read(fd, , sizeof(buf)); /* read max max lane count */
+   igt_assert_eq(res, sizeof(buf));
+
+   address = lseek(fd, 0, SEEK_CUR);
+   igt_assert_eq(address, 0x3);
+
+   buf &= 0xf;
+   igt_assert((buf == 0x1) || (buf == 0x2) || (buf == 0x4));
+
+   /* Test reads on the end of address space */
+   address = lseek(fd, 0, SEEK_END);
+   igt_assert_eq(address, 1 << 20);
+   res = read(fd, , sizeof(buf));
+   igt_assert_eq(res, 0);
+
+   address = lseek(fd, 0, SEEK_CUR);
+   igt_assert_eq(address, 1 << 20);
+
+   address = lseek(fd, 1, SEEK_CUR);
+   igt_assert_eq(address, -1);
+   igt_assert_eq(errno, EINVAL);
+
+   address = lseek(fd, -8, SEEK_END);
+   res = read(fd, bigbuf, sizeof(bigbuf));
+   igt_assert_eq(res, 8);
+
+   address = lseek(fd, 0, SEEK_CUR);
+   igt_assert_eq(address, 1 << 20);
+
+   /* Test reading more than 16 bytes at once */
+   lseek(fd, 0, SEEK_SET);
+   res = read(fd, bigbuf, sizeof(bigbuf));
+   igt_assert_eq(res, sizeof(bigbuf));
+
+   address = lseek(fd, 0, SEEK_CUR);
+   igt_assert_eq(addr

[Intel-gfx] [PATCH i-g-t] tests: Adding kms_dp_aux_dev test.

[Rafael Antognolli]

This new test makes some basic testing on the proposed drm_dp_aux_dev
interface. If the feature is enabled and the drm_dp_aux_dev class is
present, it will check for available DP aux channels and test them for:
- basic seek to 0 and read 1 byte
- seek to the last address and read, to confirm 0 is returned
- seek one more byte and confirm that EINVAL is returned
- try to read 64 bytes when at 8 bytes from the end of the
address space
- try to read 64 bytes at the address 0.

So far, no write checks are done.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 tests/Makefile.sources |   1 +
 tests/kms_dp_aux_dev.c | 177 +
 2 files changed, 178 insertions(+)
 create mode 100644 tests/kms_dp_aux_dev.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index f8b18b0..7920e23 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -79,6 +79,7 @@ TESTS_progs_M = \
kms_atomic \
kms_chv_cursor_fail \
kms_cursor_crc \
+   kms_dp_aux_dev \
kms_draw_crc \
kms_fbc_crc \
kms_fbcon_fbt \
diff --git a/tests/kms_dp_aux_dev.c b/tests/kms_dp_aux_dev.c
new file mode 100644
index 000..92e96dd
--- /dev/null
+++ b/tests/kms_dp_aux_dev.c
@@ -0,0 +1,177 @@
+/*
+ * Copyright © 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.
+ */
+
+/** @file kms_dp_aux_dev.c
+ *
+ * This is a test of functionality of drm_dp_aux_dev.
+ */
+
+#include "igt.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define MAX_CONNECTORS 32
+#define DRM_DP_AUX_SYSFS_PATH "/sys/class/drm_dp_aux_dev"
+
+IGT_TEST_DESCRIPTION("Test of drm_aux_dev nodes.");
+
+static void seek_too_far(int fd)
+{
+   off_t address;
+   uint8_t buf;
+   uint8_t bigbuf[64];
+   ssize_t res;
+
+   /* Test reads on the end of address space */
+   address = lseek(fd, 0, SEEK_END);
+   igt_assert_eq(address, 1 << 20);
+   res = read(fd, , sizeof(buf));
+   igt_assert_eq(res, 0);
+
+   address = lseek(fd, 1, SEEK_CUR);
+   igt_assert_eq(address, -1);
+   igt_assert_eq(errno, EINVAL);
+
+   address = lseek(fd, -8, SEEK_END);
+   res = read(fd, bigbuf, sizeof(bigbuf));
+   igt_assert_eq(res, 8);
+}
+
+static void read_large_chunk(int fd)
+{
+   uint8_t bigbuf[64];
+   ssize_t res;
+
+   /* Test reading more than 16 bytes at once */
+   lseek(fd, 0, SEEK_SET);
+   res = read(fd, bigbuf, sizeof(bigbuf));
+   igt_assert_eq(res, sizeof(bigbuf));
+}
+
+static void basic_seek_and_read(int fd)
+{
+   char buf;
+   ssize_t res;
+   off_t address;
+
+   address = lseek(fd, 0x0, SEEK_SET);
+   igt_assert_eq(address, 0x0);
+
+   res = read(fd, , sizeof(buf));
+   igt_assert_eq(res, sizeof(buf));
+}
+
+static int count = 0;
+static int fds[MAX_CONNECTORS];
+static bool checked_connections = false;
+static bool connected[MAX_CONNECTORS];
+
+static bool check_aux_connected(int fd)
+{
+   char buf;
+   ssize_t res;
+
+   res = read(fd, , sizeof(buf));
+   igt_assert(res >= 0 || errno == ETIMEDOUT);
+   if (res < 0 && errno == ETIMEDOUT)
+   return false;
+   else
+   return true;
+}
+
+static void for_each_dp_aux(void (*fn)(int))
+{
+   int i;
+   int total = 0;
+
+   for (i = 0; i < count; i++) {
+   igt_assert_fd(fds[i]);
+
+   /* The first time that this loop is executed, check if each aux
+* dp is connected and store that result for the next pass
+*/
+   if (!checked_connections)
+   connected[i] = check_aux_connected(fds[i]);
+
+   if (co

Re: [Intel-gfx] ✗ Fi.CI.BAT: warning for Add drm_dp_aux chardev support. (rev5)

[Rafael Antognolli]

On Fri, Feb 12, 2016 at 02:01:11PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 22, 2016 at 09:15:31AM -, Patchwork wrote:
> > == Summary ==
> > 
> > Built on 8fe9e785ae04fa7c37f7935cff12d62e38054b60 drm-intel-nightly: 
> > 2016y-01m-21d-11h-02m-42s UTC integration manifest
> > 
> > Test kms_flip:
> > Subgroup basic-flip-vs-dpms:
> > pass   -> DMESG-WARN (ilk-hp8440p)
> 
> The usual ilk underrun
> https://bugs.freedesktop.org/show_bug.cgi?id=93787
> 
> > Subgroup basic-flip-vs-modeset:
> > pass   -> DMESG-WARN (skl-i7k-2)
> 
> This is the known "DMC doesn't know what state it's in" problem.

Does it mean that I have to fix it on my patch, or is it something
expected from this test?

> > pass   -> DMESG-WARN (ilk-hp8440p)
> 
> Another ilk underrun.
> 
> > 
> > bdw-nuci7total:140  pass:131  dwarn:0   dfail:0   fail:0   skip:9  
> > bdw-ultratotal:143  pass:137  dwarn:0   dfail:0   fail:0   skip:6  
> > bsw-nuc-2total:143  pass:119  dwarn:0   dfail:0   fail:0   skip:24 
> > byt-nuc  total:143  pass:128  dwarn:0   dfail:0   fail:0   skip:15 
> > hsw-brixbox  total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7  
> > ilk-hp8440p  total:143  pass:102  dwarn:3   dfail:0   fail:0   skip:38 
> > ivb-t430stotal:143  pass:137  dwarn:0   dfail:0   fail:0   skip:6  
> > skl-i5k-2total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
> > skl-i7k-2total:143  pass:133  dwarn:2   dfail:0   fail:0   skip:8  
> > snb-dellxps  total:143  pass:129  dwarn:0   dfail:0   fail:0   skip:14 
> > snb-x220ttotal:143  pass:129  dwarn:0   dfail:0   fail:1   skip:13 
> > 
> > Results at /archive/results/CI_IGT_test/Patchwork_1244/
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v10 4/4] drm/radeon: Fix WARN_ON if DRM_DP_AUX_CHARDEV is enabled

[Rafael Antognolli]

From: Lukas Wunner <lu...@wunner.de>

Rafael Antognolli's new DRM_DP_AUX_CHARDEV feature causes a WARN_ON
if drm_dp_aux->dev == drm_connector->kdev and drm_dp_aux_unregister()
is called after drm_connector_unregister(). radeon is the only driver
affected by this besides i915. (amdgpu calls drm_dp_aux_unregister()
before drm_connector_unregister().)

Cc: Rafael Antognolli <rafael.antogno...@intel.com>
Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/radeon/radeon_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c 
b/drivers/gpu/drm/radeon/radeon_display.c
index b3bb923..a885dae 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1684,6 +1684,9 @@ void radeon_modeset_fini(struct radeon_device *rdev)
radeon_fbdev_fini(rdev);
kfree(rdev->mode_info.bios_hardcoded_edid);
 
+   /* free i2c buses */
+   radeon_i2c_fini(rdev);
+
if (rdev->mode_info.mode_config_initialized) {
radeon_afmt_fini(rdev);
drm_kms_helper_poll_fini(rdev->ddev);
@@ -1691,8 +1694,6 @@ void radeon_modeset_fini(struct radeon_device *rdev)
drm_mode_config_cleanup(rdev->ddev);
rdev->mode_info.mode_config_initialized = false;
}
-   /* free i2c buses */
-   radeon_i2c_fini(rdev);
 }
 
 static bool is_hdtv_mode(const struct drm_display_mode *mode)
-- 
2.4.3

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

[Intel-gfx] [PATCH v10 1/4] drm/kms_helper: Add a common place to call init and exit functions.

[Rafael Antognolli]

The module_init and module_exit functions will start here, and call the
subsequent init's and exit's.

v10:
 - Keep __init on drm_fb_helper init function.
 - Move MODULE_* macros to the common file.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/Makefile|  4 ++-
 drivers/gpu/drm/drm_crtc_helper.c   |  3 ---
 drivers/gpu/drm/drm_fb_helper.c |  9 +++
 drivers/gpu/drm/drm_kms_helper_common.c | 47 +
 include/drm/drm_fb_helper.h |  6 +
 5 files changed, 60 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index f858aa2..dfe513f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -24,7 +24,9 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-y += $(drm-m)
 
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
-   drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o
+   drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
+   drm_kms_helper_common.o
+
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
b/drivers/gpu/drm/drm_crtc_helper.c
index 5d4bc64..baac181 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -73,9 +73,6 @@
  * _crtc_helper_funcs, struct _encoder_helper_funcs and struct
  * _connector_helper_funcs.
  */
-MODULE_AUTHOR("David Airlie, Jesse Barnes");
-MODULE_DESCRIPTION("DRM KMS helper");
-MODULE_LICENSE("GPL and additional rights");
 
 /**
  * drm_helper_move_panel_connectors_to_head() - move panels to the front in the
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1e103c4..c27b964 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2175,9 +2175,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
  * but the module doesn't depend on any fb console symbols.  At least
  * attempt to load fbcon to avoid leaving the system without a usable console.
  */
-#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
-static int __init drm_fb_helper_modinit(void)
+int __init drm_fb_helper_modinit(void)
 {
+#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
const char *name = "fbcon";
struct module *fbcon;
 
@@ -2187,8 +2187,7 @@ static int __init drm_fb_helper_modinit(void)
 
if (!fbcon)
request_module_nowait(name);
+#endif
return 0;
 }
-
-module_init(drm_fb_helper_modinit);
-#endif
+EXPORT_SYMBOL(drm_fb_helper_modinit);
diff --git a/drivers/gpu/drm/drm_kms_helper_common.c 
b/drivers/gpu/drm/drm_kms_helper_common.c
new file mode 100644
index 000..d361005
--- /dev/null
+++ b/drivers/gpu/drm/drm_kms_helper_common.c
@@ -0,0 +1,47 @@
+/*
+ * Copyright © 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:
+ *Rafael Antognolli <rafael.antogno...@intel.com>
+ *
+ */
+
+#include 
+#include 
+
+MODULE_AUTHOR("David Airlie, Jesse Barnes");
+MODULE_DESCRIPTION("DRM KMS helper");
+MODULE_LICENSE("GPL and additional rights");
+
+static int __init drm_kms_helper_init(void)
+{
+   /* Call init functions from specific kms helpers here */
+   return drm_fb_helper_modinit();
+}
+
+static void __exit drm_kms_helper_exit(void)
+{
+   /* Call exit functions from specific kms helpers here */
+}
+
+module_init(drm_kms_helper_init);
+module_exit(drm_kms_helper_exit);
diff --git a/include/drm/drm_fb_he

[Intel-gfx] [PATCH v10 0/4] Add drm_dp_aux chardev support.

[Rafael Antognolli]

This series implement support to a drm_dp_aux chardev that allows reading and
writing an arbitrary amount of bytes to arbitrary dpcd register addresses using
regular read, write and lseek operations.

Lukas Wunner (1):
  drm/radeon: Fix WARN_ON if DRM_DP_AUX_CHARDEV is enabled

Rafael Antognolli (3):
  drm/kms_helper: Add a common place to call init and exit functions.
  drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
  drm/i915: Set aux.dev to the drm_connector device, instead of
drm_device.

 drivers/gpu/drm/Kconfig |   8 +
 drivers/gpu/drm/Makefile|   5 +-
 drivers/gpu/drm/drm_crtc_helper.c   |   3 -
 drivers/gpu/drm/drm_dp_aux_dev.c| 368 
 drivers/gpu/drm/drm_dp_helper.c |  16 +-
 drivers/gpu/drm/drm_fb_helper.c |   9 +-
 drivers/gpu/drm/drm_kms_helper_common.c |  60 ++
 drivers/gpu/drm/i915/intel_dp.c |  18 +-
 drivers/gpu/drm/radeon/radeon_display.c |   5 +-
 include/drm/drm_dp_aux_dev.h|  62 ++
 include/drm/drm_fb_helper.h |   6 +
 11 files changed, 532 insertions(+), 28 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

-- 
2.4.3

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

[Intel-gfx] [PATCH v10 2/4] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.

It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.

v2:
 - lseek is used to select the register to read/write
 - read/write are used instead of ioctl
 - no blocking_notifier is used, just a direct callback

v3:
 - use drm_dp_aux_dev prefix for public functions
 - chardev is named drm_dp_auxN
 - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
   time
 - remove notifier list from the implementation
 - option on menuconfig is now a boolean
 - add inline stub functions to avoid breakage when this option is disabled

v4:
 - fix build system changes - actually disable this module when not selected.

v5:
 - Use kref to avoid device closing while still in use
 - Don't use list, use an idr for storing aux_dev
 - Remove "connector" attribute
 - set aux.dev to the connector drm_connector device, instead of
   drm_device

v6:
 - Use atomic_t for usage count
 - Use a mutex instead of spinlock for idr lock
 - Destroy chardev immediately on unregister
 - other minor suggestions from Ville

v7:
 - style fixes
 - error handling fixes

v8:
 - more error handling fixes

v9:
 - remove module_init and module_exit, and add drm_dp_aux_dev_init/exit
 to drm_kms_helper_init/exit.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/Kconfig |   8 +
 drivers/gpu/drm/Makefile|   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c| 368 
 drivers/gpu/drm/drm_dp_helper.c |  16 +-
 drivers/gpu/drm/drm_kms_helper_common.c |  15 +-
 include/drm/drm_dp_aux_dev.h|  62 ++
 6 files changed, 468 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 59babd5..dff87ca 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
 
+config DRM_DP_AUX_CHARDEV
+   bool "DRM DP AUX Interface"
+   depends on DRM
+   help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
 config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index dfe513f..424fcb7 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -30,6 +30,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
drm_probe_helper.o \
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..f73b38b
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,368 @@
+/*
+ * Copyright © 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:
+ *Rafael Antognolli <rafael.antogno...@intel.com>
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#includ

[Intel-gfx] [PATCH v10 3/4] drm/i915: Set aux.dev to the drm_connector device, instead of drm_device.

[Rafael Antognolli]

So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.

This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.

v9:
 - As a side effect, drm_dp_aux_unregister() must be called before
 intel_connector_unregister(), as both the aux.dev and the i2c adapter
 dev are children of the drm_connector device now. Calling
 drm_dp_aux_unregister() before prevents them from being destroyed
 twice.

v10:
 - move aux_fini() to connector_unregister(), instead of moving
 drm_dp_aux_unregister() outside of connector_register().

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 18 ++
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e2bea710..da704c6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1188,7 +1188,6 @@ intel_dp_aux_fini(struct intel_dp *intel_dp)
 static int
 intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
 {
-   struct drm_device *dev = intel_dp_to_dev(intel_dp);
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
enum port port = intel_dig_port->port;
int ret;
@@ -1199,7 +1198,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
intel_connector *connector)
if (!intel_dp->aux.name)
return -ENOMEM;
 
-   intel_dp->aux.dev = dev->dev;
+   intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
 
DRM_DEBUG_KMS("registering %s bus for %s\n",
@@ -1214,16 +1213,6 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
intel_connector *connector)
return ret;
}
 
-   ret = sysfs_create_link(>base.kdev->kobj,
-   _dp->aux.ddc.dev.kobj,
-   intel_dp->aux.ddc.dev.kobj.name);
-   if (ret < 0) {
-   DRM_ERROR("sysfs_create_link() for %s failed (%d)\n",
- intel_dp->aux.name, ret);
-   intel_dp_aux_fini(intel_dp);
-   return ret;
-   }
-
return 0;
 }
 
@@ -1232,9 +1221,7 @@ intel_dp_connector_unregister(struct intel_connector 
*intel_connector)
 {
struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
 
-   if (!intel_connector->mst_port)
-   sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
+   intel_dp_aux_fini(intel_dp);
intel_connector_unregister(intel_connector);
 }
 
@@ -4868,7 +4855,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
struct intel_dp *intel_dp = _dig_port->dp;
 
-   intel_dp_aux_fini(intel_dp);
intel_dp_mst_encoder_cleanup(intel_dig_port);
if (is_edp(intel_dp)) {
cancel_delayed_work_sync(_dp->panel_vdd_work);
-- 
2.4.3

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

Re: [Intel-gfx] [PATCH v9 3/3] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.

[Rafael Antognolli]

Hi Lukas,

Sorry for the late answer, I went on vacation and then was busy with
something else. Anyway, I tried to address all comments (yours and
Daniel's) on a new version. See some comments below.

On Sun, Dec 06, 2015 at 05:08:49PM +0100, Lukas Wunner wrote:
> Hi Rafael,
> 
> On Thu, Dec 03, 2015 at 02:54:02PM -0800, Rafael Antognolli wrote:
> > So far, the i915 driver and some other drivers set it to the drm_device,
> > which doesn't allow one to know which DP a given aux channel is related
> > to. Changing this to be the drm_connector provides proper nesting, still
> > allowing one to get the drm_device from it. Some drivers already set it
> > to the drm_connector.
> > 
> > This also removes the need to add a sysfs link for the i2c device under
> > the connector, as it will already be there.
> > 
> > v2:
> 
> Two minor nits, I think this should be "v9" instead of "v2" because
> this was newly added since v8, and the subject should be prefixed
> "drm/i915" (or "drm/i915/dp" or whatever) instead of "drm/dp" because
> this only touches i915 and not drm core.
> 
> 
> >  - As a side effect, drm_dp_aux_unregister() must be called before
> >  intel_connector_unregister(), as both the aux.dev and the i2c adapter
> >  dev are children of the drm_connector device now. Calling
> >  drm_dp_aux_unregister() before prevents them from being destroyed
> >  twice.
> 
> I haven't verified what Ville wrote (that there are WARNs because of
> this ordering issue), but if this is true then we need to make sure
> all other drivers get the order right, otherwise they'd spew WARNs
> as well once this gets merged.
> 
> I've checked that for every driver and the only one affected is radeon.
> A fix is now in my GitHub repo, feel free to include the commit in your
> series if you want to. I haven't been able to actually test it though
> as I don't have a radeon card:
> https://github.com/l1k/linux/commit/485e197a7cac88e24348150526988149428feeaa
> 

Nice, I added it to the series, though I also don't have a radeon to
test it.

> > 
> > Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 22 --
> >  1 file changed, 4 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f2bfca0..515893c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp 
> > *intel_dp)
> >  static void
> >  intel_dp_aux_fini(struct intel_dp *intel_dp)
> >  {
> > -   drm_dp_aux_unregister(_dp->aux);
> > kfree(intel_dp->aux.name);
> >  }
> 
> Hm, instead of moving the single call of drm_dp_aux_unregister()
> to intel_dp_connector_unregister() I think it would be more clear
> to call intel_dp_aux_fini() from intel_dp_connector_unregister()
> and remove its invocation from intel_dp_encoder_destroy().
> 
> Ville introduced intel_dp_aux_fini() very recently with a121f4e5fae5,
> he should probably have a say on how to solve this.

I makes sense to me, so I did what you suggest.

Thanks for the review,
Rafael

> >  
> >  static int
> >  intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector 
> > *connector)
> >  {
> > -   struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > enum port port = intel_dig_port->port;
> > int ret;
> > @@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
> > intel_connector *connector)
> > if (!intel_dp->aux.name)
> > return -ENOMEM;
> >  
> > -   intel_dp->aux.dev = dev->dev;
> > +   intel_dp->aux.dev = connector->base.kdev;
> > intel_dp->aux.transfer = intel_dp_aux_transfer;
> >  
> > DRM_DEBUG_KMS("registering %s bus for %s\n",
> > @@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
> > intel_connector *connector)
> > return ret;
> > }
> >  
> > -   ret = sysfs_create_link(>base.kdev->kobj,
> > -   _dp->aux.ddc.dev.kobj,
> > -   intel_dp->aux.ddc.dev.kobj.name);
> > -   if (ret < 0) {
> > -   DRM_ERROR("sysfs_create_link() for %s failed (%d)\n",
> > - intel_dp->aux.name, ret);
> > -   intel_dp_aux_fini(intel_

[Intel-gfx] [PATCH v9 3/3] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.

[Rafael Antognolli]

So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.

This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.

v2:
 - As a side effect, drm_dp_aux_unregister() must be called before
 intel_connector_unregister(), as both the aux.dev and the i2c adapter
 dev are children of the drm_connector device now. Calling
 drm_dp_aux_unregister() before prevents them from being destroyed
 twice.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 22 --
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f2bfca0..515893c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp 
*intel_dp)
 static void
 intel_dp_aux_fini(struct intel_dp *intel_dp)
 {
-   drm_dp_aux_unregister(_dp->aux);
kfree(intel_dp->aux.name);
 }
 
 static int
 intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
 {
-   struct drm_device *dev = intel_dp_to_dev(intel_dp);
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
enum port port = intel_dig_port->port;
int ret;
@@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
intel_connector *connector)
if (!intel_dp->aux.name)
return -ENOMEM;
 
-   intel_dp->aux.dev = dev->dev;
+   intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
 
DRM_DEBUG_KMS("registering %s bus for %s\n",
@@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
intel_connector *connector)
return ret;
}
 
-   ret = sysfs_create_link(>base.kdev->kobj,
-   _dp->aux.ddc.dev.kobj,
-   intel_dp->aux.ddc.dev.kobj.name);
-   if (ret < 0) {
-   DRM_ERROR("sysfs_create_link() for %s failed (%d)\n",
- intel_dp->aux.name, ret);
-   intel_dp_aux_fini(intel_dp);
-   return ret;
-   }
-
return 0;
 }
 
 static void
 intel_dp_connector_unregister(struct intel_connector *intel_connector)
 {
-   struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
-   if (!intel_connector->mst_port)
-   sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
+   struct intel_dp *intel_dp =
+   enc_to_intel_dp(_connector->encoder->base);
+   drm_dp_aux_unregister(_dp->aux);
intel_connector_unregister(intel_connector);
 }
 
-- 
2.4.3

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

[Intel-gfx] [PATCH v9 0/3] Add drm_dp_aux chardev support.

[Rafael Antognolli]

This series implement support to a drm_dp_aux chardev that allows reading and
writing an arbitrary amount of bytes to arbitrary dpcd register addresses using
regular read, write and lseek operations.

Rafael Antognolli (3):
  drm/kms_helper: Add a common place to call init and exit functions.
  drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
  drm/dp: Set aux.dev to the drm_connector device, instead of
drm_device.

 drivers/gpu/drm/Kconfig |   8 +
 drivers/gpu/drm/Makefile|   5 +-
 drivers/gpu/drm/drm_dp_aux_dev.c| 368 
 drivers/gpu/drm/drm_dp_helper.c |  16 +-
 drivers/gpu/drm/drm_fb_helper.c |   9 +-
 drivers/gpu/drm/drm_kms_helper_common.c |  56 +
 drivers/gpu/drm/i915/intel_dp.c |  22 +-
 include/drm/drm_dp_aux_dev.h|  62 ++
 include/drm/drm_fb_helper.h |   6 +
 9 files changed, 527 insertions(+), 25 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

-- 
2.4.3

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

[Intel-gfx] [PATCH v9 2/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.

It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.

v2:
 - lseek is used to select the register to read/write
 - read/write are used instead of ioctl
 - no blocking_notifier is used, just a direct callback

v3:
 - use drm_dp_aux_dev prefix for public functions
 - chardev is named drm_dp_auxN
 - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
   time
 - remove notifier list from the implementation
 - option on menuconfig is now a boolean
 - add inline stub functions to avoid breakage when this option is disabled

v4:
 - fix build system changes - actually disable this module when not selected.

v5:
 - Use kref to avoid device closing while still in use
 - Don't use list, use an idr for storing aux_dev
 - Remove "connector" attribute
 - set aux.dev to the connector drm_connector device, instead of
   drm_device

v6:
 - Use atomic_t for usage count
 - Use a mutex instead of spinlock for idr lock
 - Destroy chardev immediately on unregister
 - other minor suggestions from Ville

v7:
 - style fixes
 - error handling fixes

v8:
 - more error handling fixes

v9:
 - remove module_init and module_exit, and add drm_dp_aux_dev_init/exit
 to drm_kms_helper_init/exit.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/Kconfig |   8 +
 drivers/gpu/drm/Makefile|   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c| 368 
 drivers/gpu/drm/drm_dp_helper.c |  16 +-
 drivers/gpu/drm/drm_kms_helper_common.c |  15 +-
 include/drm/drm_dp_aux_dev.h|  62 ++
 6 files changed, 468 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index c4bf9a1..daefcce 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
 
+config DRM_DP_AUX_CHARDEV
+   bool "DRM DP AUX Interface"
+   depends on DRM
+   help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
 config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 76399da..57e153d 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -30,6 +30,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
drm_probe_helper.o \
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..f73b38b
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,368 @@
+/*
+ * Copyright © 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:
+ *Rafael Antognolli <rafael.antogno...@intel.com>
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#includ

[Intel-gfx] [PATCH v9 1/3] drm/kms_helper: Add a common place to call init and exit functions.

[Rafael Antognolli]

The module_init and module_exit functions will start here, and call the
subsequent init's and exit's.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/Makefile|  4 ++-
 drivers/gpu/drm/drm_fb_helper.c |  9 +++
 drivers/gpu/drm/drm_kms_helper_common.c | 43 +
 include/drm/drm_fb_helper.h |  6 +
 4 files changed, 56 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1e9ff4c..76399da 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -24,7 +24,9 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-y += $(drm-m)
 
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
-   drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o
+   drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
+   drm_kms_helper_common.o
+
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 69cbab5..576f769 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2175,9 +2175,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
  * but the module doesn't depend on any fb console symbols.  At least
  * attempt to load fbcon to avoid leaving the system without a usable console.
  */
-#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
-static int __init drm_fb_helper_modinit(void)
+int drm_fb_helper_modinit(void)
 {
+#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
const char *name = "fbcon";
struct module *fbcon;
 
@@ -2187,8 +2187,7 @@ static int __init drm_fb_helper_modinit(void)
 
if (!fbcon)
request_module_nowait(name);
+#endif
return 0;
 }
-
-module_init(drm_fb_helper_modinit);
-#endif
+EXPORT_SYMBOL(drm_fb_helper_modinit);
diff --git a/drivers/gpu/drm/drm_kms_helper_common.c 
b/drivers/gpu/drm/drm_kms_helper_common.c
new file mode 100644
index 000..920b2fb
--- /dev/null
+++ b/drivers/gpu/drm/drm_kms_helper_common.c
@@ -0,0 +1,43 @@
+/*
+ * Copyright © 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:
+ *Rafael Antognolli <rafael.antogno...@intel.com>
+ *
+ */
+
+#include 
+#include 
+
+static int __init drm_kms_helper_init(void)
+{
+   /* Call init functions from specific kms helpers here */
+   return drm_fb_helper_modinit();
+}
+
+static void __exit drm_kms_helper_exit(void)
+{
+   /* Call exit functions from specific kms helpers here */
+}
+
+module_init(drm_kms_helper_init);
+module_exit(drm_kms_helper_exit);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 87b090c..4fed7a2 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -148,6 +148,7 @@ struct drm_fb_helper {
 };
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
+int drm_fb_helper_modinit(void);
 void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper 
*helper,
   const struct drm_fb_helper_funcs *funcs);
 int drm_fb_helper_init(struct drm_device *dev,
@@ -212,6 +213,11 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper 
*fb_helper, struct drm_
 int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
   struct drm_connector *connector);
 #else
+static inline int drm_fb_helper_modinit(void)
+{
+   return 0;
+}
+
 static inline void drm_fb_helper_prepare(struct drm_device *dev,
  

Re: [Intel-gfx] [PATCH v8 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.

[Rafael Antognolli]

On Tue, Nov 24, 2015 at 10:31:41PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 02, 2015 at 12:33:48PM -0800, Rafael Antognolli wrote:
> > So far, the i915 driver and some other drivers set it to the drm_device,
> > which doesn't allow one to know which DP a given aux channel is related
> > to. Changing this to be the drm_connector provides proper nesting, still
> > allowing one to get the drm_device from it. Some drivers already set it
> > to the drm_connector.
> > 
> > This also removes the need to add a sysfs link for the i2c device under
> > the connector, as it will already be there.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> 
> I gave aux_dev a bit of a testing here, and it appaers to work quite
> splendidly.
> 
> This patch however causes lots of WARN spew if I unload the driver
> while in middle of dumping the DPCD via the aux_dev. It appears we
> we clean up things the wrong order now.

It looks like device_destroy is being called twice on the drm_aux_dev
and i2c adapter devices.

If I understood correctly, this is happening because I changed the
parent device, from the drm_device to the drm_connector, but the
drm_connector is being destroyed before the drm_aux_dev and i2c adapter
devices, which already causes them to be destroyed too.

Changing the drm_dp_aux_unregister() from inside intel_dp_aux_fini() to
intel_dp_connector_unregister(), right before
intel_connector_unregister(), seems to fix everything, and it still
makes sense in my opinion, since the auxdev is going to be unregistered
before we destroy the drm_connector. Do you think that would be ok?

> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 19 ++-
> >  1 file changed, 2 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 8287df4..7aacc08 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
> > intel_connector *connector)
> > intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
> >  
> > intel_dp->aux.name = name;
> > -   intel_dp->aux.dev = dev->dev;
> > +   intel_dp->aux.dev = connector->base.kdev;
> > intel_dp->aux.transfer = intel_dp_aux_transfer;
> >  
> > DRM_DEBUG_KMS("registering %s bus for %s\n", name,
> >   connector->base.kdev->kobj.name);
> >  
> > ret = drm_dp_aux_register(_dp->aux);
> > -   if (ret < 0) {
> > +   if (ret < 0)
> > DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
> >   name, ret);
> > -   return;
> > -   }
> > -
> > -   ret = sysfs_create_link(>base.kdev->kobj,
> > -   _dp->aux.ddc.dev.kobj,
> > -   intel_dp->aux.ddc.dev.kobj.name);
> > -   if (ret < 0) {
> > -   DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, 
> > ret);
> > -   drm_dp_aux_unregister(_dp->aux);
> > -   }
> >  }
> >  
> >  static void
> >  intel_dp_connector_unregister(struct intel_connector *intel_connector)
> >  {
> > -   struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
> > -
> > -   if (!intel_connector->mst_port)
> > -   sysfs_remove_link(_connector->base.kdev->kobj,
> > - intel_dp->aux.ddc.dev.kobj.name);
> > intel_connector_unregister(intel_connector);
> >  }
> >  
> > -- 
> > 2.4.3
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v8 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.

[Rafael Antognolli]

So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.

This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 19 ++-
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8287df4..7aacc08 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
intel_connector *connector)
intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
 
intel_dp->aux.name = name;
-   intel_dp->aux.dev = dev->dev;
+   intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
 
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
  connector->base.kdev->kobj.name);
 
ret = drm_dp_aux_register(_dp->aux);
-   if (ret < 0) {
+   if (ret < 0)
DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
  name, ret);
-   return;
-   }
-
-   ret = sysfs_create_link(>base.kdev->kobj,
-   _dp->aux.ddc.dev.kobj,
-   intel_dp->aux.ddc.dev.kobj.name);
-   if (ret < 0) {
-   DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, 
ret);
-   drm_dp_aux_unregister(_dp->aux);
-   }
 }
 
 static void
 intel_dp_connector_unregister(struct intel_connector *intel_connector)
 {
-   struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
-   if (!intel_connector->mst_port)
-   sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
intel_connector_unregister(intel_connector);
 }
 
-- 
2.4.3

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

[Intel-gfx] [PATCH v8 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.

It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.

v2:
 - lseek is used to select the register to read/write
 - read/write are used instead of ioctl
 - no blocking_notifier is used, just a direct callback

v3:
 - use drm_dp_aux_dev prefix for public functions
 - chardev is named drm_dp_auxN
 - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
   time
 - remove notifier list from the implementation
 - option on menuconfig is now a boolean
 - add inline stub functions to avoid breakage when this option is disabled

v4:
 - fix build system changes - actually disable this module when not selected.

v5:
 - Use kref to avoid device closing while still in use
 - Don't use list, use an idr for storing aux_dev
 - Remove "connector" attribute
 - set aux.dev to the connector drm_connector device, instead of
   drm_device

v6:
 - Use atomic_t for usage count
 - Use a mutex instead of spinlock for idr lock
 - Destroy chardev immediately on unregister
 - other minor suggestions from Ville

v7:
 - style fixes
 - error handling fixes

v8:
 - more error handling fixes

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++
 drivers/gpu/drm/drm_dp_helper.c  |  16 +-
 include/drm/drm_dp_aux_dev.h |  50 ++
 5 files changed, 447 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index c4bf9a1..daefcce 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
 
+config DRM_DP_AUX_CHARDEV
+   bool "DRM DP AUX Interface"
+   depends on DRM
+   help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
 config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1e9ff4c..e48ec8f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
drm_probe_helper.o \
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..0269556
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,373 @@
+/*
+ * Copyright © 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:
+ *Rafael Antognolli <rafael.antogno...@intel.com>
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct drm_dp_aux_dev {
+   unsigned index;
+   struct drm_dp_aux *aux;
+   struct device *dev;
+   struct kref refcount;
+   atomic_t usecount;
+};
+
+#define

[Intel-gfx] [PATCH v8 0/2] Add drm_dp_aux chardev support.

[Rafael Antognolli]

This series implement support to a drm_dp_aux chardev that allows reading and
writing an arbitrary amount of bytes to arbitrary dpcd register addresses using
regular read, write and lseek operations.

Rafael Antognolli (2):
  drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
  drm/dp: Set aux.dev to the drm_connector device, instead of
drm_device.

 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++
 drivers/gpu/drm/drm_dp_helper.c  |  16 +-
 drivers/gpu/drm/i915/intel_dp.c  |  19 +-
 include/drm/drm_dp_aux_dev.h |  50 ++
 6 files changed, 449 insertions(+), 18 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

-- 
2.4.3

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

Re: [Intel-gfx] [PATCH v6 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

On Fri, Oct 30, 2015 at 12:04:17PM +0200, Ville Syrjälä wrote:
> On Thu, Oct 29, 2015 at 04:23:45PM -0700, Rafael Antognolli wrote:
> > This module is heavily based on i2c-dev. Once loaded, it provides one
> > dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
> > 
> > It's possible to know which connector owns this aux channel by looking
> > at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
> > the connector device pointer was correctly set in the aux helper struct.
> > 
> > Two main operations are provided on the registers read and write. The
> > address of the register to be read or written is given using lseek. The
> > seek position is updated upon read or write.
> 
> Note that we (i915 folks at least) generally like to have the changelog
> in the commit message itself. So maybe move it here for the final
> version.
> 
> Anyways, this is looking rather nice now. I spotted a few remaining style
> issues, and some error handling problems. Once those are dealt with
> I think we should be done.
> 
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> > ---
> >  drivers/gpu/drm/Kconfig  |   8 +
> >  drivers/gpu/drm/Makefile |   1 +
> >  drivers/gpu/drm/drm_dp_aux_dev.c | 381 
> > +++
> >  drivers/gpu/drm/drm_dp_helper.c  |   5 +
> >  include/drm/drm_dp_aux_dev.h |  50 +
> >  5 files changed, 445 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
> >  create mode 100644 include/drm/drm_dp_aux_dev.h
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index c4bf9a1..daefcce 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -25,6 +25,14 @@ config DRM_MIPI_DSI
> > bool
> > depends on DRM
> >  
> > +config DRM_DP_AUX_CHARDEV
> > +   bool "DRM DP AUX Interface"
> > +   depends on DRM
> > +   help
> > + Choose this option to enable a /dev/drm_dp_auxN node that allows to
> > + read and write values to arbitrary DPCD registers on the DP aux
> > + channel.
> > +
> >  config DRM_KMS_HELPER
> > tristate
> > depends on DRM
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 1e9ff4c..e48ec8f 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
> > drm_probe_helper.o \
> >  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> > +drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> >  
> >  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >  
> > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c 
> > b/drivers/gpu/drm/drm_dp_aux_dev.c
> > new file mode 100644
> > index 000..16dbc2e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> > @@ -0,0 +1,381 @@
> > +/*
> > + * Copyright © 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:
> > + *Rafael Antognolli <

[Intel-gfx] [PATCH v7 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.

It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.

v2:
 - lseek is used to select the register to read/write
 - read/write are used instead of ioctl
 - no blocking_notifier is used, just a direct callback

v3:
 - use drm_dp_aux_dev prefix for public functions
 - chardev is named drm_dp_auxN
 - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
   time
 - remove notifier list from the implementation
 - option on menuconfig is now a boolean
 - add inline stub functions to avoid breakage when this option is disabled

v4:
 - fix build system changes - actually disable this module when not selected.

v5:
 - Use kref to avoid device closing while still in use 
 - Don't use list, use an idr for storing aux_dev
 - Remove "connector" attribute
 - set aux.dev to the connector drm_connector device, instead of
   drm_device

v6:
 - Use atomic_t for usage count
 - Use a mutex instead of spinlock for idr lock
 - Destroy chardev immediately on unregister
 - other minor suggestions from Ville

v7:
 - style fixes
 - error handling fixes

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 374 +++
 drivers/gpu/drm/drm_dp_helper.c  |   5 +
 include/drm/drm_dp_aux_dev.h |  50 ++
 5 files changed, 438 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index c4bf9a1..daefcce 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
 
+config DRM_DP_AUX_CHARDEV
+   bool "DRM DP AUX Interface"
+   depends on DRM
+   help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
 config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1e9ff4c..e48ec8f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
drm_probe_helper.o \
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..1a3ca39
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,374 @@
+/*
+ * Copyright © 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:
+ *Rafael Antognolli <rafael.antogno...@intel.com>
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct drm_dp_aux_dev {
+   unsigned index;
+   struct drm_dp_aux *aux;
+   struct device *dev;
+   struct kref refcount;
+   atomic_t usecount;
+};
+
+#define DRM_AUX_MINORS 256
+#define AUX_MAX_OFFSET (1 <

[Intel-gfx] [PATCH v7 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.

[Rafael Antognolli]

So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.

This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 19 ++-
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8287df4..7aacc08 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
intel_connector *connector)
intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
 
intel_dp->aux.name = name;
-   intel_dp->aux.dev = dev->dev;
+   intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
 
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
  connector->base.kdev->kobj.name);
 
ret = drm_dp_aux_register(_dp->aux);
-   if (ret < 0) {
+   if (ret < 0)
DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
  name, ret);
-   return;
-   }
-
-   ret = sysfs_create_link(>base.kdev->kobj,
-   _dp->aux.ddc.dev.kobj,
-   intel_dp->aux.ddc.dev.kobj.name);
-   if (ret < 0) {
-   DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, 
ret);
-   drm_dp_aux_unregister(_dp->aux);
-   }
 }
 
 static void
 intel_dp_connector_unregister(struct intel_connector *intel_connector)
 {
-   struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
-   if (!intel_connector->mst_port)
-   sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
intel_connector_unregister(intel_connector);
 }
 
-- 
2.4.3

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

[Intel-gfx] [PATCH v7 0/2] Add drm_dp_aux chardev support.

[Rafael Antognolli]

This series implement support to a drm_dp_aux chardev that allows reading and
writing an arbitrary amount of bytes to arbitrary dpcd register addresses using
regular read, write and lseek operations.

Rafael Antognolli (2):
  drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
  drm/dp: Set aux.dev to the drm_connector device, instead of
drm_device.

 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 374 +++
 drivers/gpu/drm/drm_dp_helper.c  |   5 +
 drivers/gpu/drm/i915/intel_dp.c  |  19 +-
 include/drm/drm_dp_aux_dev.h |  50 ++
 6 files changed, 440 insertions(+), 17 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

-- 
2.4.3

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

[Intel-gfx] [PATCH v6 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.

It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 381 +++
 drivers/gpu/drm/drm_dp_helper.c  |   5 +
 include/drm/drm_dp_aux_dev.h |  50 +
 5 files changed, 445 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index c4bf9a1..daefcce 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
 
+config DRM_DP_AUX_CHARDEV
+   bool "DRM DP AUX Interface"
+   depends on DRM
+   help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
 config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1e9ff4c..e48ec8f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
drm_probe_helper.o \
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..16dbc2e
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,381 @@
+/*
+ * Copyright © 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:
+ *Rafael Antognolli <rafael.antogno...@intel.com>
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct drm_dp_aux_dev {
+   unsigned index;
+   struct drm_dp_aux *aux;
+   struct device *dev;
+   struct kref refcount;
+   atomic_t usecount;
+};
+
+#define DRM_AUX_MINORS 256
+#define AUX_MAX_OFFSET (1 << 20)
+static DEFINE_IDR(aux_idr);
+static DEFINE_MUTEX(aux_idr_mutex);
+static struct class *drm_dp_aux_dev_class;
+static int drm_dev_major = -1;
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
+{
+   struct drm_dp_aux_dev *aux_dev = NULL;
+
+   mutex_lock(_idr_mutex);
+   aux_dev = idr_find(_idr, index);
+   if (!kref_get_unless_zero(_dev->refcount))
+   aux_dev = NULL;
+   mutex_unlock(_idr_mutex);
+
+   return aux_dev;
+}
+
+static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
+{
+   struct drm_dp_aux_dev *aux_dev;
+   int index;
+
+
+   aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
+   if (!aux_dev)
+   return ERR_PTR(-ENOMEM);
+   aux_dev->aux = aux;
+   atomic_set(_dev->usecount, 1);
+   kref_init(_dev->refcount);
+
+   mutex_lock(_idr_mutex);
+   index = idr_alloc_cyclic(_idr, aux_dev, 0, DRM_AUX_MINORS,
+GFP_KERN

[Intel-gfx] [PATCH v6 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.

[Rafael Antognolli]

So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.

This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 19 ++-
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8287df4..7aacc08 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
intel_connector *connector)
intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
 
intel_dp->aux.name = name;
-   intel_dp->aux.dev = dev->dev;
+   intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
 
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
  connector->base.kdev->kobj.name);
 
ret = drm_dp_aux_register(_dp->aux);
-   if (ret < 0) {
+   if (ret < 0)
DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
  name, ret);
-   return;
-   }
-
-   ret = sysfs_create_link(>base.kdev->kobj,
-   _dp->aux.ddc.dev.kobj,
-   intel_dp->aux.ddc.dev.kobj.name);
-   if (ret < 0) {
-   DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, 
ret);
-   drm_dp_aux_unregister(_dp->aux);
-   }
 }
 
 static void
 intel_dp_connector_unregister(struct intel_connector *intel_connector)
 {
-   struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
-   if (!intel_connector->mst_port)
-   sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
intel_connector_unregister(intel_connector);
 }
 
-- 
2.4.3

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

[Intel-gfx] [PATCH v6 0/2] Add drm_dp_aux chardev support.

[Rafael Antognolli]

This series implement support to a drm_dp_aux chardev that allows reading and
writing an arbitrary amount of bytes to arbitrary dpcd register addresses using
regular read, write and lseek operations.

v2:
 - lseek is used to select the register to read/write
 - read/write are used instead of ioctl
 - no blocking_notifier is used, just a direct callback

v3:
 - use drm_dp_aux_dev prefix for public functions
 - chardev is named drm_dp_auxN
 - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
   time
 - remove notifier list from the implementation
 - option on menuconfig is now a boolean
 - add inline stub functions to avoid breakage when this option is disabled

v4:
 - fix build system changes - actually disable this module when not selected.

v5:
 - Use kref to avoid device closing while still in use 
 - Don't use list, use an idr for storing aux_dev
 - Remove "connector" attribute
 - set aux.dev to the connector drm_connector device, instead of
   drm_device

v6:
 - Use atomic_t for usage count
 - Use a mutex instead of spinlock for idr lock
 - Destroy chardev immediately on unregister
 - other minor suggestions from Ville

Rafael Antognolli (2):
  drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
  drm/dp: Set aux.dev to the drm_connector device, instead of
drm_device.

 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 381 +++
 drivers/gpu/drm/drm_dp_helper.c  |   5 +
 drivers/gpu/drm/i915/intel_dp.c  |  19 +-
 include/drm/drm_dp_aux_dev.h |  50 +
 6 files changed, 447 insertions(+), 17 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

-- 
2.4.3

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

Re: [Intel-gfx] [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Rafael Antognolli]

On Tue, Sep 29, 2015 at 06:25:44PM +0200, Daniel Vetter wrote:
> On Tue, Sep 29, 2015 at 05:27:33PM +0200, Lukas Wunner wrote:
> > Hi Daniel,
> > 
> > On Tue, Sep 29, 2015 at 05:04:03PM +0200, Daniel Vetter wrote:
> > > On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote:
> > > > On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote:
> > > > > This is useful to determine which connector owns this AUX channel.
> > > > 
> > > > WTF? I posted a patch in August which does exactly that:
> > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html
> > > > 
> > > > Can also be pulled in from this git repo:
> > > > https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6
> > > > 
> > > > My patch has the advantage that it updates all the drivers which use
> > > > drm_dp_aux to fill that attribute. Yours only updates i915.
> > > > 
> > > > Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux,
> > > > quote:
> > > > 
> > > > "That will also clear up the confusion with drm_dp_aux, adding a
> > > > drm_connector there feels wrong since not every dp_aux line has a
> > > > connector (e.g. for dp mst). If we can lift this relation out into 
> > > > drivers
> > > > (where this is known) that seems cleaner."
> > > > 
> > > > So now Intel itself does precisely what Daniel criticized? Confusing!
> > > > 
> > > > Source:
> > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html
> > > 
> > > Critism is still valid, and thinking about this again a cleaner solution
> > > would be to just have a correct parent/child relationship in the device
> > > hirarchy. I.e. add a struct device *parent to the aux channel structure
> > > which should point to the right connector.
> > 
> > We already have that:
> > 
> > struct drm_dp_aux {
> > const char *name;
> > struct i2c_adapter ddc;
> > struct device *dev; <---
> > struct mutex hw_mutex;
> > ssize_t (*transfer)(struct drm_dp_aux *aux,
> > struct drm_dp_aux_msg *msg);
> > unsigned i2c_nack_count, i2c_defer_count;
> > };
> > 
> > What Rafael is struggling with is that you cannot unambiguously
> > get from drm_dp_aux->dev to the drm_connector. (The drm_device
> > may have multiple drm_connectors with type
> > DRM_MODE_CONNECTOR_DisplayPort.)
> 
> What I meant to say is that we don't need that, if instead of filling in
> the overall dev in dp_aux->dev we fill in the connector sysfs device
> thing. The we have proper nesting, like with i2c buses. And then there's
> no need for a connector property in sysfs to show that link (which should
> be done with a proper sysfs link anyway).

OK, I sent a new version, which does not add a new *connector pointer,
and uses the dev pointer on the struct to store the drm_connector
device, instead of the drm_device device. Is that what you meant? In
any case, as I mention on the patch, it is already how some drivers do,
while others store the drm_device.

This leaves the aux device, for instance in my case, at:

/sys/class/drm/card0/card0-eDP-1/drm_dp_aux0

If this is what you wanted, I can send other patches to the proper
mailing lists, trying to update other drivers.

--
Rafael

> > 
> > > 
> > > Thanks for pointing out that I missed properly delayering this.
> > > -Daniel
> > > 
> > > > 
> > > > 
> > > > Best regards,
> > > > 
> > > > Lukas
> > > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 1 +
> > > > >  include/drm/drm_dp_helper.h | 1 +
> > > > >  2 files changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 77f7330..f90439d 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, 
> > > > > struct intel_connector *connector)
> > > > >  
> > > > >  

[Intel-gfx] [PATCH v5 0/2] Add drm_dp_aux chardev support.

[Rafael Antognolli]

This series implement support to a drm_dp_aux chardev that allows reading and
writing an arbitrary amount of bytes to arbitrary dpcd register addresses using
regular read, write and lseek operations.

v2:
 - lseek is used to select the register to read/write
 - read/write are used instead of ioctl
 - no blocking_notifier is used, just a direct callback

v3:
 - use drm_dp_aux_dev prefix for public functions
 - chardev is named drm_dp_auxN
 - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
   time
 - remove notifier list from the implementation
 - option on menuconfig is now a boolean
 - add inline stub functions to avoid breakage when this option is disabled

v4:
 - fix build system changes - actually disable this module when not selected.

v5:
 - Use kref to avoid device closing while still in use 
 - Don't use list, use an idr for storing aux_dev
 - Remove "connector" attribute
 - set aux.dev to the connector drm_connector device, instead of
   drm_device

Rafael Antognolli (2):
  drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
  drm/dp: Set aux.dev to the drm_connector device, instead of
drm_device.

 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++
 drivers/gpu/drm/drm_dp_helper.c  |   7 +
 drivers/gpu/drm/i915/intel_dp.c  |  14 +-
 include/drm/drm_dp_aux_dev.h |  50 ++
 6 files changed, 441 insertions(+), 12 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

-- 
2.4.3

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

[Intel-gfx] [PATCH v5 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.

[Rafael Antognolli]

So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.

This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 19 ++-
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 18bcfbe..0acdf0f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
intel_connector *connector)
intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
 
intel_dp->aux.name = name;
-   intel_dp->aux.dev = dev->dev;
+   intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
 
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
  connector->base.kdev->kobj.name);
 
ret = drm_dp_aux_register(_dp->aux);
-   if (ret < 0) {
+   if (ret < 0)
DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
  name, ret);
-   return;
-   }
-
-   ret = sysfs_create_link(>base.kdev->kobj,
-   _dp->aux.ddc.dev.kobj,
-   intel_dp->aux.ddc.dev.kobj.name);
-   if (ret < 0) {
-   DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, 
ret);
-   drm_dp_aux_unregister(_dp->aux);
-   }
 }
 
 static void
 intel_dp_connector_unregister(struct intel_connector *intel_connector)
 {
-   struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
-   if (!intel_connector->mst_port)
-   sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
intel_connector_unregister(intel_connector);
 }
 
-- 
2.4.3

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

[Intel-gfx] [PATCH v5 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.

It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++
 drivers/gpu/drm/drm_dp_helper.c  |   7 +
 include/drm/drm_dp_aux_dev.h |  50 ++
 5 files changed, 439 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df..64fa0f4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
 
+config DRM_DP_AUX_CHARDEV
+   bool "DRM DP AUX Interface"
+   depends on DRM
+   help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
 config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index e814517..e5bc0ca 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
drm_probe_helper.o \
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..a2861e2
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,373 @@
+/*
+ * Copyright © 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:
+ *Rafael Antognolli <rafael.antogno...@intel.com>
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct drm_dp_aux_dev {
+   unsigned index;
+   struct drm_dp_aux *aux;
+   struct device *dev;
+   struct kref refcount;
+   bool removed;
+   spinlock_t removed_lock;
+};
+
+#define DRM_AUX_MINORS 256
+#define AUX_MAX_OFFSET (1 << 20)
+static DEFINE_IDR(aux_idr);
+static DEFINE_SPINLOCK(aux_idr_lock);
+static struct class *drm_dp_aux_dev_class;
+static int drm_dev_major = -1;
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
+{
+   struct drm_dp_aux_dev *aux_dev = NULL;
+
+   spin_lock(_idr_lock);
+   aux_dev = idr_find(_idr, index);
+   if (!kref_get_unless_zero(_dev->refcount))
+   aux_dev = NULL;
+   spin_unlock(_idr_lock);
+
+   return aux_dev;
+}
+
+static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
+{
+   struct drm_dp_aux_dev *aux_dev;
+   int index;
+
+
+   aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
+   if (!aux_dev)
+   return ERR_PTR(-ENOMEM);
+   aux_dev->aux = aux;
+   aux_dev->removed = false;
+   spin_lock_init(_dev->removed_lock);
+   kref_init(_dev->refcount);
+
+   idr_preload(GFP_KERNEL);
+   spin_lock(_idr_lock);
+ 

Re: [Intel-gfx] [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Rafael Antognolli]

On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote:
> Hi Rafael,
> 
> On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote:
> > This is useful to determine which connector owns this AUX channel.
> 
> WTF? I posted a patch in August which does exactly that:
> http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html
> 
> Can also be pulled in from this git repo:
> https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6
> 
> My patch has the advantage that it updates all the drivers which use
> drm_dp_aux to fill that attribute. Yours only updates i915.
> 
> Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux,
> quote:
> 
> "That will also clear up the confusion with drm_dp_aux, adding a
> drm_connector there feels wrong since not every dp_aux line has a
> connector (e.g. for dp mst). If we can lift this relation out into drivers
> (where this is known) that seems cleaner."
> 
> So now Intel itself does precisely what Daniel criticized? Confusing!

I'm sorry, I don't follow this list as closely yet, so I didn't see that
patch there, otherwise I would have talked to Daniel about it in the
first place.

Thanks,
Rafael

> Source:
> http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html
> 
> 
> Best regards,
> 
> Lukas
> 
> 
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 1 +
> >  include/drm/drm_dp_helper.h | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 77f7330..f90439d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
> > intel_connector *connector)
> >  
> > intel_dp->aux.name = name;
> > intel_dp->aux.dev = dev->dev;
> > +   intel_dp->aux.connector = connector->base.kdev;
> > intel_dp->aux.transfer = intel_dp_aux_transfer;
> >  
> > DRM_DEBUG_KMS("registering %s bus for %s\n", name,
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 9ec4716..e009b5d 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -702,6 +702,7 @@ struct drm_dp_aux {
> > const char *name;
> > struct i2c_adapter ddc;
> > struct device *dev;
> > +   struct device *connector;
> > struct mutex hw_mutex;
> > ssize_t (*transfer)(struct drm_dp_aux *aux,
> > struct drm_dp_aux_msg *msg);
> > -- 
> > 2.4.3
> > 
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

Thanks Ville for the review, I'm addressing all the issues but have some
questions on the ones mentioned below:

On Tue, Sep 29, 2015 at 05:09:23PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 28, 2015 at 04:45:36PM -0700, Rafael Antognolli wrote:
> > This module is heavily based on i2c-dev. Once loaded, it provides one
> > dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
> > 
> > It's possible to know which connector owns this aux channel by looking
> > at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
> > the connector device pointer was correctly set in the aux helper struct.
> > 
> > Two main operations are provided on the registers read and write. The
> > address of the register to be read or written is given using lseek. The
> > seek position is updated upon read or write.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> > ---
> >  drivers/gpu/drm/Kconfig  |   8 +
> >  drivers/gpu/drm/Makefile |   1 +
> >  drivers/gpu/drm/drm_dp_aux_dev.c | 357 
> > +++
> >  drivers/gpu/drm/drm_dp_helper.c  |   5 +
> >  include/drm/drm_dp_aux_dev.h |  50 ++
> >  5 files changed, 421 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
> >  create mode 100644 include/drm/drm_dp_aux_dev.h
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 1a0a8df..64fa0f4 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -25,6 +25,14 @@ config DRM_MIPI_DSI
> > bool
> > depends on DRM
> >  
> > +config DRM_DP_AUX_CHARDEV
> > +   bool "DRM DP AUX Interface"
> > +   depends on DRM
> > +   help
> > + Choose this option to enable a /dev/drm_dp_auxN node that allows to
> > + read and write values to arbitrary DPCD registers on the DP aux
> > + channel.
> > +
> >  config DRM_KMS_HELPER
> > tristate
> > depends on DRM
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 45e7719..e5ae7f0 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -25,6 +25,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
> > drm_probe_helper.o \
> >  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> > +drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> >  
> >  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >  
> > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c 
> > b/drivers/gpu/drm/drm_dp_aux_dev.c
> > new file mode 100644
> > index 000..e337081
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> > @@ -0,0 +1,357 @@
> > +/*
> > + * Copyright © 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:
> > + *Rafael Antognolli <rafael.antogno...@intel.com>
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#inclu

[Intel-gfx] [PATCH v4 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.

It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++
 drivers/gpu/drm/drm_dp_helper.c  |   5 +
 include/drm/drm_dp_aux_dev.h |  50 ++
 5 files changed, 421 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df..64fa0f4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
 
+config DRM_DP_AUX_CHARDEV
+   bool "DRM DP AUX Interface"
+   depends on DRM
+   help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
 config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 45e7719..e5ae7f0 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -25,6 +25,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
drm_probe_helper.o \
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..e337081
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,357 @@
+/*
+ * Copyright © 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:
+ *Rafael Antognolli <rafael.antogno...@intel.com>
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct drm_dp_aux_dev {
+   struct list_head list;
+   unsigned index;
+   struct drm_dp_aux *aux;
+   struct device *dev;
+};
+
+#define DRM_AUX_MINORS 256
+static int aux_max_offset = 1 << 20;
+static int drm_dp_aux_dev_count = 0;
+static LIST_HEAD(drm_dp_aux_dev_list);
+static DEFINE_SPINLOCK(drm_dp_aux_dev_list_lock);
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
+{
+   struct drm_dp_aux_dev *aux_dev;
+
+   spin_lock(_dp_aux_dev_list_lock);
+   list_for_each_entry(aux_dev, _dp_aux_dev_list, list) {
+   if (aux_dev->index == index)
+   goto found;
+   }
+
+   aux_dev = NULL;
+found:
+   spin_unlock(_dp_aux_dev_list_lock);
+   return aux_dev;
+}
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_aux(struct drm_dp_aux *aux)
+{
+   struct drm_dp_aux_dev *aux_dev;
+
+   spin_lock(_dp_aux_dev_list_lock);
+   list_for_each_entry(aux_dev, _dp_aux_dev_list, list) {
+   if (aux_dev->aux == aux)
+   goto found;
+   }
+
+   aux_dev = NULL;
+found:
+   spin_unlock(_dp_aux_dev_list_lock);
+   return aux_dev;
+}
+
+static struct drm_dp_aux_dev *get_free_drm_dp_aux_dev(st

[Intel-gfx] [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Rafael Antognolli]

This is useful to determine which connector owns this AUX channel.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 1 +
 include/drm/drm_dp_helper.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 77f7330..f90439d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct 
intel_connector *connector)
 
intel_dp->aux.name = name;
intel_dp->aux.dev = dev->dev;
+   intel_dp->aux.connector = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
 
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 9ec4716..e009b5d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -702,6 +702,7 @@ struct drm_dp_aux {
const char *name;
struct i2c_adapter ddc;
struct device *dev;
+   struct device *connector;
struct mutex hw_mutex;
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
-- 
2.4.3

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

[Intel-gfx] [PATCH v4 0/2] Add drm_dp_aux chardev support.

[Rafael Antognolli]

This series implement support to a drm_dp_aux chardev that allows reading and
writing an arbitrary amount of bytes to arbitrary dpcd register addresses using
regular read, write and lseek operations.

v2:
 - lseek is used to select the register to read/write
 - read/write are used instead of ioctl
 - no blocking_notifier is used, just a direct callback

v3:
 - use drm_dp_aux_dev prefix for public functions
 - chardev is named drm_dp_auxN
 - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
 time
 - remove notifier list from the implementation
 - option on menuconfig is now a boolean
 - add inline stub functions to avoid breakage when this option is disabled

v4:
 - fix build system changes - actually disable this module when not selected.

Rafael Antognolli (2):
  drm/dp: Store the drm_connector device pointer on the helper.
  drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

 drivers/gpu/drm/Kconfig  |   8 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++
 drivers/gpu/drm/drm_dp_helper.c  |   5 +
 drivers/gpu/drm/i915/intel_dp.c  |   1 +
 include/drm/drm_dp_aux_dev.h |  50 ++
 include/drm/drm_dp_helper.h  |   1 +
 7 files changed, 423 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

-- 
2.4.3

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

[Intel-gfx] [PATCH i-g-t] tests: Adding kms_dp_aux_dev test.

[Rafael Antognolli]

This new test makes some basic testing on the proposed drm_dp_aux_dev
interface. If the feature is enabled and the drm_dp_aux_dev class is
present, it will check for available DP aux channels and test them for:
- basic seek to 0 and read 1 byte
- seek to the last address and read, to confirm 0 is returned
- seek one more byte and confirm that EINVAL is returned
- try to read 64 bytes when at 8 bytes from the end of the
address space
- try to read 64 bytes at the address 0.

So far, no write checks are done.

Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
---
 tests/.gitignore   |   1 +
 tests/Makefile.sources |   1 +
 tests/kms_dp_aux_dev.c | 134 +
 3 files changed, 136 insertions(+)
 create mode 100644 tests/kms_dp_aux_dev.c

diff --git a/tests/.gitignore b/tests/.gitignore
index dc8bb53..efdad75 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -127,6 +127,7 @@ kms_3d
 kms_addfb_basic
 kms_crtc_background_color
 kms_cursor_crc
+kms_dp_aux_dev
 kms_draw_crc
 kms_fbc_crc
 kms_fbcon_fbt
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 2e2e088..dc07737 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -64,6 +64,7 @@ TESTS_progs_M = \
gem_write_read_ring_switch \
kms_addfb_basic \
kms_cursor_crc \
+   kms_dp_aux_dev \
kms_draw_crc \
kms_fbc_crc \
kms_fbcon_fbt \
diff --git a/tests/kms_dp_aux_dev.c b/tests/kms_dp_aux_dev.c
new file mode 100644
index 000..aab8c95
--- /dev/null
+++ b/tests/kms_dp_aux_dev.c
@@ -0,0 +1,134 @@
+/*
+ * Copyright © 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.
+ */
+
+/** @file kms_dp_aux_dev.c
+ *
+ * This is a test of functionality of drm_dp_aux_dev.
+ */
+
+#include "igt.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define DRM_DP_AUX_SYSFS_PATH "/sys/class/drm_dp_aux_dev"
+
+IGT_TEST_DESCRIPTION("Test of drm_aux_dev nodes.");
+
+static void drm_dp_aux_test(const char *devname)
+{
+   char basedir[] = "/dev/";
+   char name[256 + sizeof(basedir)];
+   off_t address;
+   uint8_t buf;
+   uint8_t bigbuf[64];
+   int fd;
+
+   ssize_t res;
+
+   snprintf(name, sizeof(name), "%s%s", basedir, devname);
+   igt_info("Running test on %s\n", name);
+ 
+   /* Check if this DP aux channel is connected before continuing tests */
+   fd = open(name, O_RDONLY);
+   igt_assert_lte(0, fd);
+   address = lseek(fd, 0x0, SEEK_SET);
+   igt_assert_eq(address, 0x0);
+   res = read(fd, , sizeof(buf));
+   igt_skip_on(res < 0 && errno == ETIMEDOUT);
+ 
+
+   /* Channel is connected, start tests */
+   igt_assert_eq(res, sizeof(buf));
+
+   /* Test reads on the end of address space */
+   address = lseek(fd, 0, SEEK_END);
+   igt_assert_eq(address, 1 << 20);
+   res = read(fd, , sizeof(buf));
+   igt_assert_eq(res, 0);
+
+   address = lseek(fd, 1, SEEK_CUR);
+   igt_assert_eq(address, -1);
+   igt_assert_eq(errno, EINVAL);
+
+   address = lseek(fd, -8, SEEK_END);
+   res = read(fd, bigbuf, sizeof(bigbuf));
+   igt_assert_eq(res, 8);
+
+   /* Test reading more than 16 bytes at once */
+   address = lseek(fd, 0, SEEK_SET);
+   res = read(fd, bigbuf, sizeof(bigbuf));
+   igt_assert_eq(res, sizeof(bigbuf));
+}
+
+static void for_each_dp_aux(DIR *dir)
+{
+   int count = 0;
+   struct dirent *dirent;
+
+   if (!dir)
+   return;
+
+   rewinddir(dir);
+   while ((dirent = readdir(dir))) {
+   if (dirent->d_name[0] == '.')
+   continue;
+
+   c

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Dont -ETIMEDOUT on identical new and previous (count, crc).

[Rafael Antognolli]

On Wed, Jul 29, 2015 at 10:26:53AM +0200, Daniel Vetter wrote:
 On Tue, Jul 28, 2015 at 10:05:21PM +, Vivi, Rodrigo wrote:
  On Tue, 2015-07-28 at 13:25 -0700, Rafael Antognolli wrote:
   On Thu, Jul 23, 2015 at 04:35:50PM -0700, Rodrigo Vivi wrote:
By Vesa DP 1.2 spec TEST_CRC_COUNT is a 4 bit wrap counter which
increments each time the TEST_CRC_x_x are updated.

However if we are trying to verify the screen hasn't changed we get
same (count, crc) pair twice. Without this patch we would return
-ETIMEOUT in this case.

So, if in 6 vblanks the pair (count, crc) hasn't changed we
return it anyway instead of returning error and let test case decide
if it was right or not.

Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
   
   Looks good.
   
---
 drivers/gpu/drm/i915/intel_dp.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c 
b/drivers/gpu/drm/i915/intel_dp.c
index c7372a1..e99ec7a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4028,6 +4028,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, 
u8 *crc)
u8 buf;
int count, ret;
int attempts = 6;
+   bool old_equal_new;
 
ret = intel_dp_sink_crc_start(intel_dp);
if (ret)
@@ -4042,6 +4043,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, 
u8 *crc)
goto stop;
}
count = buf  DP_TEST_COUNT_MASK;
+
/*
 * Count might be reset during the loop. In this case
 * last known count needs to be reset as well.
@@ -4053,17 +4055,24 @@ int intel_dp_sink_crc(struct intel_dp 
*intel_dp, u8 *crc)
ret = -EIO;
goto stop;
}
-   } while (--attempts  (count == 0 || (count == 
intel_dp-sink_crc.last_count 
-  
!memcmp(intel_dp-sink_crc.last_crc, crc,
-  6 * 
sizeof(u8);
+
+   old_equal_new = (count == intel_dp-sink_crc.last_count 

+!memcmp(intel_dp-sink_crc.last_crc, 
crc,
+6 * sizeof(u8)));
+
+   } while (--attempts  (count == 0 || old_equal_new));
 
intel_dp-sink_crc.last_count = buf  DP_TEST_COUNT_MASK;
memcpy(intel_dp-sink_crc.last_crc, crc, 6 * sizeof(u8));
 
if (attempts == 0) {
-   DRM_DEBUG_KMS(Panel is unable to calculate CRC after 6 
vblanks\n);
-   ret = -ETIMEDOUT;
-   goto stop;
+   if (old_equal_new) {
+   DRM_DEBUG_KMS(Unreliable Sink CRC counter: 
Current returned CRC is identical to the previous one\n);
   
   Isn't this line a little too long?
  
  I agree, but I had no idea how to make it shorter. I believe this long
  debug message is the only case where we can go over 80 characters in
  i915. but if it isn't true and/or have a suggestion how to make it
  shorter please let me know that I can change.
 
 dmesg output is explicitly an exception since breaking lines makes it much
 harder to grep for a line you spot in dmesg. Ofc 500 lines would be a bit
 too much, we're breaking those. But this one here is totally fine.

Nice, I never thought about being able to grep, but makes total sense.

 Remember, checkpatch is just suggestions mostly, not law.

I wasn't aware of it, but good to know that it exists. I'll check it out.

Reviewed-by: Rafael Antognolli rafael.antogno...@intel.com

  
   
+   } else {
+   DRM_ERROR(Panel is unable to calculate any CRC 
after 6 vblanks\n);
+   ret = -ETIMEDOUT;
+   goto stop;
+   }
}
 
 stop:
-- 
2.1.0

___
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
 
 -- 
 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 4/7] drm/i915: Split sink_crc function in start, stop and read.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:47PM -0700, Rodrigo Vivi wrote:
 No functional change. Just a preparation patch to make clear
 what operation we are performing.
 
 Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com

Good. The place where you call hsw_disable_ips() changes, but as you
explained earlier, this is required.

Reviewed-by: Rafael Antognolli rafael.antogno...@intel.com

 ---
  drivers/gpu/drm/i915/intel_dp.c | 89 
 +++--
  1 file changed, 50 insertions(+), 39 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
 index 44f8a32..10cbc98 100644
 --- a/drivers/gpu/drm/i915/intel_dp.c
 +++ b/drivers/gpu/drm/i915/intel_dp.c
 @@ -3958,40 +3958,64 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
   return intel_dp-is_mst;
  }
  
 -int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 +static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
  {
 - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 - struct drm_device *dev = intel_dig_port-base.base.dev;
 - struct intel_crtc *intel_crtc =
 - to_intel_crtc(intel_dig_port-base.base.crtc);
 + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 + struct intel_crtc *intel_crtc = to_intel_crtc(dig_port-base.base.crtc);
   u8 buf;
 - int test_crc_count;
 - int attempts = 6;
 - int ret = 0;
 -
 - hsw_disable_ips(intel_crtc);
  
 - if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK_MISC, buf)  0) {
 - ret = -EIO;
 - goto out;
 + if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK, buf)  0) {
 + DRM_DEBUG_KMS(Sink CRC couldn't be stopped properly\n);
 + return;
   }
  
 - if (!(buf  DP_TEST_CRC_SUPPORTED)) {
 - ret = -ENOTTY;
 - goto out;
 - }
 + if (drm_dp_dpcd_writeb(intel_dp-aux, DP_TEST_SINK,
 +buf  ~DP_TEST_SINK_START)  0)
 + DRM_DEBUG_KMS(Sink CRC couldn't be stopped properly\n);
  
 - if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK, buf)  0) {
 - ret = -EIO;
 - goto out;
 - }
 + hsw_enable_ips(intel_crtc);
 +}
 +
 +static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 +{
 + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 + struct intel_crtc *intel_crtc = to_intel_crtc(dig_port-base.base.crtc);
 + u8 buf;
 +
 + if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK_MISC, buf)  0)
 + return -EIO;
 +
 + if (!(buf  DP_TEST_CRC_SUPPORTED))
 + return -ENOTTY;
 +
 + if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK, buf)  0)
 + return -EIO;
 +
 + hsw_disable_ips(intel_crtc);
  
   if (drm_dp_dpcd_writeb(intel_dp-aux, DP_TEST_SINK,
 - buf | DP_TEST_SINK_START)  0) {
 - ret = -EIO;
 - goto out;
 +buf | DP_TEST_SINK_START)  0) {
 + hsw_enable_ips(intel_crtc);
 + return -EIO;
   }
  
 + return 0;
 +}
 +
 +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 +{
 + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 + struct drm_device *dev = dig_port-base.base.dev;
 + struct intel_crtc *intel_crtc = to_intel_crtc(dig_port-base.base.crtc);
 + u8 buf;
 + int test_crc_count;
 + int attempts = 6;
 + int ret;
 +
 + ret = intel_dp_sink_crc_start(intel_dp);
 + if (ret)
 + return ret;
 +
   if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK_MISC, buf)  0) {
   ret = -EIO;
   goto stop;
 @@ -4014,23 +4038,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
   goto stop;
   }
  
 - if (drm_dp_dpcd_read(intel_dp-aux, DP_TEST_CRC_R_CR, crc, 6)  0) {
 + if (drm_dp_dpcd_read(intel_dp-aux, DP_TEST_CRC_R_CR, crc, 6)  0)
   ret = -EIO;
 - goto stop;
 - }
 -
  stop:
 - if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK, buf)  0) {
 - DRM_DEBUG_KMS(Sink CRC couldn't be stopped properly\n);
 - goto out;
 - }
 - if (drm_dp_dpcd_writeb(intel_dp-aux, DP_TEST_SINK,
 -buf  ~DP_TEST_SINK_START)  0) {
 - DRM_DEBUG_KMS(Sink CRC couldn't be stopped properly\n);
 - goto out;
 - }
 -out:
 - hsw_enable_ips(intel_crtc);
 + intel_dp_sink_crc_stop(intel_dp);
   return ret;
  }
  
 -- 
 2.1.0
 
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915: Force sink crc stop before start.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:48PM -0700, Rodrigo Vivi wrote:
 By Vesa DP spec, test counter at DP_TEST_SINK_MISC just reset to 0
 when unsetting DP_TEST_SINK_START, so let's force this stop here.
 
 But let's minimize the aux transactions and just do it when we know
 it hasn't been properly stoped.
 
 Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com

Reviewed-by: Rafael Antognolli rafael.antogno...@intel.com

 ---
  drivers/gpu/drm/i915/intel_dp.c  | 22 +++---
  drivers/gpu/drm/i915/intel_drv.h |  1 +
  2 files changed, 20 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
 index 10cbc98..3ba031d 100644
 --- a/drivers/gpu/drm/i915/intel_dp.c
 +++ b/drivers/gpu/drm/i915/intel_dp.c
 @@ -3958,22 +3958,30 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
   return intel_dp-is_mst;
  }
  
 -static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 +static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
  {
   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
   struct intel_crtc *intel_crtc = to_intel_crtc(dig_port-base.base.crtc);
   u8 buf;
 + int ret = 0;
  
   if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK, buf)  0) {
   DRM_DEBUG_KMS(Sink CRC couldn't be stopped properly\n);
 - return;
 + ret = -EIO;
 + goto out;
   }
  
   if (drm_dp_dpcd_writeb(intel_dp-aux, DP_TEST_SINK,
 -buf  ~DP_TEST_SINK_START)  0)
 +buf  ~DP_TEST_SINK_START)  0) {
   DRM_DEBUG_KMS(Sink CRC couldn't be stopped properly\n);
 + ret = -EIO;
 + goto out;
 + }
  
 + intel_dp-sink_crc_started = false;
 + out:
   hsw_enable_ips(intel_crtc);
 + return ret;
  }
  
  static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 @@ -3981,6 +3989,13 @@ static int intel_dp_sink_crc_start(struct intel_dp 
 *intel_dp)
   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
   struct intel_crtc *intel_crtc = to_intel_crtc(dig_port-base.base.crtc);
   u8 buf;
 + int ret;
 +
 + if (intel_dp-sink_crc_started) {
 + ret = intel_dp_sink_crc_stop(intel_dp);
 + if (ret)
 + return ret;
 + }
  
   if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK_MISC, buf)  0)
   return -EIO;
 @@ -3999,6 +4014,7 @@ static int intel_dp_sink_crc_start(struct intel_dp 
 *intel_dp)
   return -EIO;
   }
  
 + intel_dp-sink_crc_started = true;
   return 0;
  }
  
 diff --git a/drivers/gpu/drm/i915/intel_drv.h 
 b/drivers/gpu/drm/i915/intel_drv.h
 index 47cef0e..cc74400 100644
 --- a/drivers/gpu/drm/i915/intel_drv.h
 +++ b/drivers/gpu/drm/i915/intel_drv.h
 @@ -714,6 +714,7 @@ struct intel_dp {
   /* sink rates as reported by DP_SUPPORTED_LINK_RATES */
   uint8_t num_sink_rates;
   int sink_rates[DP_MAX_SUPPORTED_RATES];
 + bool sink_crc_started;
   struct drm_dp_aux aux;
   uint8_t train_set[4];
   int panel_power_up_delay;
 -- 
 2.1.0
 
 ___
 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 7/7] drm/i915: Dont -ETIMEDOUT on identical new and previous (count, crc).

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:50PM -0700, Rodrigo Vivi wrote:
 By Vesa DP 1.2 spec TEST_CRC_COUNT is a 4 bit wrap counter which
 increments each time the TEST_CRC_x_x are updated.
 
 However if we are trying to verify the screen hasn't changed we get
 same (count, crc) pair twice. Without this patch we would return
 -ETIMEOUT in this case.
 
 So, if in 6 vblanks the pair (count, crc) hasn't changed we
 return it anyway instead of returning error and let test case decide
 if it was right or not.
 
 Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com

Looks good.

 ---
  drivers/gpu/drm/i915/intel_dp.c | 21 +++--
  1 file changed, 15 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
 index c7372a1..e99ec7a 100644
 --- a/drivers/gpu/drm/i915/intel_dp.c
 +++ b/drivers/gpu/drm/i915/intel_dp.c
 @@ -4028,6 +4028,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
   u8 buf;
   int count, ret;
   int attempts = 6;
 + bool old_equal_new;
  
   ret = intel_dp_sink_crc_start(intel_dp);
   if (ret)
 @@ -4042,6 +4043,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
   goto stop;
   }
   count = buf  DP_TEST_COUNT_MASK;
 +
   /*
* Count might be reset during the loop. In this case
* last known count needs to be reset as well.
 @@ -4053,17 +4055,24 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
   ret = -EIO;
   goto stop;
   }
 - } while (--attempts  (count == 0 || (count == 
 intel_dp-sink_crc.last_count 
 -
 !memcmp(intel_dp-sink_crc.last_crc, crc,
 -6 * sizeof(u8);
 +
 + old_equal_new = (count == intel_dp-sink_crc.last_count 
 +  !memcmp(intel_dp-sink_crc.last_crc, crc,
 +  6 * sizeof(u8)));
 +
 + } while (--attempts  (count == 0 || old_equal_new));
  
   intel_dp-sink_crc.last_count = buf  DP_TEST_COUNT_MASK;
   memcpy(intel_dp-sink_crc.last_crc, crc, 6 * sizeof(u8));
  
   if (attempts == 0) {
 - DRM_DEBUG_KMS(Panel is unable to calculate CRC after 6 
 vblanks\n);
 - ret = -ETIMEDOUT;
 - goto stop;
 + if (old_equal_new) {
 + DRM_DEBUG_KMS(Unreliable Sink CRC counter: Current 
 returned CRC is identical to the previous one\n);

Isn't this line a little too long?

 + } else {
 + DRM_ERROR(Panel is unable to calculate any CRC after 6 
 vblanks\n);
 + ret = -ETIMEDOUT;
 + goto stop;
 + }
   }
  
  stop:
 -- 
 2.1.0
 
 ___
 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 6/7] drm/i915: Save latest known sink CRC to compensate delayed counter reset.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:49PM -0700, Rodrigo Vivi wrote:
 By Vesa DP 1.2 Spec TEST_CRC_COUNT should be
 reset to 0 when TEST_SINK bit 0 = 0.
 
 However for some strange reason when PSR is enabled in
 certain platforms this is not true. At least not immediatelly.
 
 So we face cases like this:
 
 first get_sink_crc operation:
count: 0, crc: 
count: 1, crc: c101c101c101
 returned expected crc: c101c101c101
 
 secont get_sink_crc operation:
count: 1, crc: c101c101c101
count: 0, crc: 
count: 1, crc: c101
 should return expected crc: c101
 
 But also the reset to 0 should be faster resulting into:
 
 get_sink_crc operation:
count: 1, crc: c101c101c101
count: 1, crc: c101
 should return expected crc: c101
 
 So in order to know that the second one is valid one
 we need to compare the pair (count, crc) with latest (count, crc).
 
 If the pair changed you have your valid CRC.
 
 Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com

We discussed this before, unfortunately I don't see any other way to do
this, since there is no way to know that the crc count really restarted,
became 0 and then 1 again. So I think this workaround is needed.

Reviewed-by: Rafael Antognolli rafael.antogno...@intel.com

 ---
  drivers/gpu/drm/i915/intel_dp.c  | 42 
 +---
  drivers/gpu/drm/i915/intel_drv.h |  8 +++-
  2 files changed, 33 insertions(+), 17 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
 index 3ba031d..c7372a1 100644
 --- a/drivers/gpu/drm/i915/intel_dp.c
 +++ b/drivers/gpu/drm/i915/intel_dp.c
 @@ -3978,7 +3978,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp 
 *intel_dp)
   goto out;
   }
  
 - intel_dp-sink_crc_started = false;
 + intel_dp-sink_crc.started = false;
   out:
   hsw_enable_ips(intel_crtc);
   return ret;
 @@ -3991,7 +3991,7 @@ static int intel_dp_sink_crc_start(struct intel_dp 
 *intel_dp)
   u8 buf;
   int ret;
  
 - if (intel_dp-sink_crc_started) {
 + if (intel_dp-sink_crc.started) {
   ret = intel_dp_sink_crc_stop(intel_dp);
   if (ret)
   return ret;
 @@ -4003,6 +4003,8 @@ static int intel_dp_sink_crc_start(struct intel_dp 
 *intel_dp)
   if (!(buf  DP_TEST_CRC_SUPPORTED))
   return -ENOTTY;
  
 + intel_dp-sink_crc.last_count = buf  DP_TEST_COUNT_MASK;
 +
   if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK, buf)  0)
   return -EIO;
  
 @@ -4014,7 +4016,7 @@ static int intel_dp_sink_crc_start(struct intel_dp 
 *intel_dp)
   return -EIO;
   }
  
 - intel_dp-sink_crc_started = true;
 + intel_dp-sink_crc.started = true;
   return 0;
  }
  
 @@ -4024,29 +4026,39 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
   struct drm_device *dev = dig_port-base.base.dev;
   struct intel_crtc *intel_crtc = to_intel_crtc(dig_port-base.base.crtc);
   u8 buf;
 - int test_crc_count;
 + int count, ret;
   int attempts = 6;
 - int ret;
  
   ret = intel_dp_sink_crc_start(intel_dp);
   if (ret)
   return ret;
  
 - if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK_MISC, buf)  0) {
 - ret = -EIO;
 - goto stop;
 - }
 -
 - test_crc_count = buf  DP_TEST_COUNT_MASK;
 -
   do {
 + intel_wait_for_vblank(dev, intel_crtc-pipe);
 +
   if (drm_dp_dpcd_readb(intel_dp-aux,
 DP_TEST_SINK_MISC, buf)  0) {
   ret = -EIO;
   goto stop;
   }
 - intel_wait_for_vblank(dev, intel_crtc-pipe);
 - } while (--attempts  (buf  DP_TEST_COUNT_MASK) == test_crc_count);
 + count = buf  DP_TEST_COUNT_MASK;
 + /*
 +  * Count might be reset during the loop. In this case
 +  * last known count needs to be reset as well.
 +  */
 + if (count == 0)
 + intel_dp-sink_crc.last_count = 0;
 +
 + if (drm_dp_dpcd_read(intel_dp-aux, DP_TEST_CRC_R_CR, crc, 6) 
  0) {
 + ret = -EIO;
 + goto stop;
 + }
 + } while (--attempts  (count == 0 || (count == 
 intel_dp-sink_crc.last_count 
 +
 !memcmp(intel_dp-sink_crc.last_crc, crc,
 +6 * sizeof(u8);
 +
 + intel_dp-sink_crc.last_count = buf  DP_TEST_COUNT_MASK;
 + memcpy(intel_dp-sink_crc.last_crc, crc, 6 * sizeof(u8));
  
   if (attempts == 0) {
   DRM_DEBUG_KMS(Panel is unable to calculate CRC after 6 
 vblanks\n);
 @@ -4054,8 +4066,6 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
   goto stop

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Try to stop sink crc calculation on error.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:45PM -0700, Rodrigo Vivi wrote:
 Right now if we face any kind of error sink crc calculation
 stays enabled.
 
 So, let's give a shot and try to stop it anyway if it got enabled.
 
 Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
 ---
  drivers/gpu/drm/i915/intel_dp.c | 9 +
  1 file changed, 5 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
 index f1b9f93..70a4a37 100644
 --- a/drivers/gpu/drm/i915/intel_dp.c
 +++ b/drivers/gpu/drm/i915/intel_dp.c
 @@ -3994,7 +3994,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
  
   if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK_MISC, buf)  0) {
   ret = -EIO;
 - goto out;
 + goto stop;
   }
  
   test_crc_count = buf  DP_TEST_COUNT_MASK;
 @@ -4003,7 +4003,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
   if (drm_dp_dpcd_readb(intel_dp-aux,
 DP_TEST_SINK_MISC, buf)  0) {
   ret = -EIO;
 - goto out;
 + goto stop;
   }
   intel_wait_for_vblank(dev, intel_crtc-pipe);
   } while (--attempts  (buf  DP_TEST_COUNT_MASK) == test_crc_count);
 @@ -4011,14 +4011,15 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
   if (attempts == 0) {
   DRM_DEBUG_KMS(Panel is unable to calculate CRC after 6 
 vblanks\n);
   ret = -ETIMEDOUT;
 - goto out;
 + goto stop;
   }
  
   if (drm_dp_dpcd_read(intel_dp-aux, DP_TEST_CRC_R_CR, crc, 6)  0) {
   ret = -EIO;
 - goto out;
 + goto stop;
   }
  
 +stop:
   if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK, buf)  0) {
   ret = -EIO;
   goto out;

Nice cleanup on exit.

Reviewed-by: Rafael Antognolli rafael.antogno...@intel.com


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

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Don't return error on sink crc stop.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:46PM -0700, Rodrigo Vivi wrote:
 If we got to the point where we are trying to stop sink CRC
 the main output of this function was already gotten properly,
 so don't return the error and let userspace use the crc data.
 
 Let's replace the errnos returns with some log messages.

So, up to this commit, there's no way to know that an error has ocurred
and the next CRC calculation can go wrong.

I know that in a follow up patch this is fixed by trying to stop the
calculation at the beginning too, but just pointing out that this one
specifically has this problem.

Not sure if this is a problem though, since the patches are submitted
together. If not, then

Reviewed-by: Rafael Antognolli rafael.antogno...@intel.com


 Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
 ---
  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
 index 70a4a37..44f8a32 100644
 --- a/drivers/gpu/drm/i915/intel_dp.c
 +++ b/drivers/gpu/drm/i915/intel_dp.c
 @@ -4021,12 +4021,12 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
 *crc)
  
  stop:
   if (drm_dp_dpcd_readb(intel_dp-aux, DP_TEST_SINK, buf)  0) {
 - ret = -EIO;
 + DRM_DEBUG_KMS(Sink CRC couldn't be stopped properly\n);
   goto out;
   }
   if (drm_dp_dpcd_writeb(intel_dp-aux, DP_TEST_SINK,
  buf  ~DP_TEST_SINK_START)  0) {
 - ret = -EIO;
 + DRM_DEBUG_KMS(Sink CRC couldn't be stopped properly\n);
   goto out;
   }
  out:
 -- 
 2.1.0
 
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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