Re: [Intel-gfx] [PATCH 14/22] drm/i915: Don't treat differently sized rotated views as equal

[Daniel Vetter]

On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote:
> 
> On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:
> >From: Ville Syrjälä 
> >
> >In case we have multiple different rotated views into the same object,
> >each one may need its own vma due to being of different sizes. So don't
> >treat all rotated views as equal.
> >
> >Signed-off-by: Ville Syrjälä 
> >---
> >  drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
> >b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >index caa182f..68de734 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
> >
> > if (a->type != b->type)
> > return false;
> >+if (a->type == I915_GGTT_VIEW_ROTATED)
> >+return !memcmp(>rotated, >rotated, sizeof(a->rotated));
> > if (a->type == I915_GGTT_VIEW_PARTIAL)
> > return !memcmp(>partial, >partial, sizeof(a->partial));
> > return true;
> 
> This is where anonymous union causes problems since you have to build a
> switch statement before memcmp while the original goal was for memcmp to
> elegantly avoid that.

Yup, mentioned the same on irc. I much prefer we do the memcmp on type !=
NORMAL, to avoid mistakes when adding more views in the future.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 12/22] drm/i915: Set i915_ggtt_view_normal type explicitly

[Daniel Vetter]

On Thu, Oct 15, 2015 at 12:15:21PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:
> >From: Ville Syrjälä 
> >
> >Just for clarity set the type for i915_ggtt_view_normal explicitly.
> >
> >While at it fix the indentation fail for i915_ggtt_view_rotated.
> >
> >Signed-off-by: Ville Syrjälä 
> >---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> >b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >index 620d57e..71acc71 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >@@ -95,9 +95,11 @@
> >  static int
> >  i915_get_ggtt_vma_pages(struct i915_vma *vma);
> >
> >-const struct i915_ggtt_view i915_ggtt_view_normal;
> >+const struct i915_ggtt_view i915_ggtt_view_normal = {
> >+.type = I915_GGTT_VIEW_NORMAL,
> >+};
> 
> AFAIR while this was developed Daniel was very keen on the normal view type
> being implicitly zero. Can't remember at this very moment if some of the
> current code actually depends on it though.

We kzalloc vmas, which means any view in there is normal by default. Still
very much keen on that ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 14/22] drm/i915: Don't treat differently sized rotated views as equal

[Daniel Vetter]

On Thu, Oct 15, 2015 at 03:06:11PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 15, 2015 at 02:02:24PM +0200, Daniel Vetter wrote:
> > On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:
> > > >From: Ville Syrjälä 
> > > >
> > > >In case we have multiple different rotated views into the same object,
> > > >each one may need its own vma due to being of different sizes. So don't
> > > >treat all rotated views as equal.
> > > >
> > > >Signed-off-by: Ville Syrjälä 
> > > >---
> > > >  drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
> > > >b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > > >index caa182f..68de734 100644
> > > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > > >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
> > > >
> > > > if (a->type != b->type)
> > > > return false;
> > > >+if (a->type == I915_GGTT_VIEW_ROTATED)
> > > >+return !memcmp(>rotated, >rotated, 
> > > >sizeof(a->rotated));
> > > > if (a->type == I915_GGTT_VIEW_PARTIAL)
> > > > return !memcmp(>partial, >partial, 
> > > > sizeof(a->partial));
> > > > return true;
> > > 
> > > This is where anonymous union causes problems since you have to build a
> > > switch statement before memcmp while the original goal was for memcmp to
> > > elegantly avoid that.
> > 
> > Yup, mentioned the same on irc. I much prefer we do the memcmp on type !=
> > NORMAL, to avoid mistakes when adding more views in the future.
> 
> I just dislike the .partial. extra stuff all over. But whatever, it's a
> minor detail in my book. We can go with the non-anon union if people
> prefer that.

Here it would be
if (a->type != NORMA))
return !memcmp(>params, >params);

Seems at least in this case cleaner. But yet everywhere else you'll get a
params., but that can be alleviated a bit with local variables.
-Daniel
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

[Intel-gfx] [PATCH 0/2] Introducing crtc_clock_get()

[Vandana Kannan]

The implementation of crtc_clock_get() will not work for BXT. The 2 patches in
this series, cleans up the code related to crtc_clock_get() and includes
implementation for BXT (DSI and otherwise).

A patch to correct crtc_mode_get() for BXT DSI will follow shortly.

Vandana Kannan (2):
  drm/i915: Create crtc_clock_get function pointers
  drm/i915: Add crtc_clock_get for hsw, skl, bxt

 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 57 +++-
 2 files changed, 51 insertions(+), 8 deletions(-)

-- 
1.9.1

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

[Intel-gfx] [PATCH 1/2] drm/i915: Create crtc_clock_get function pointers

[Vandana Kannan]

There are separate functions i9xx_crtc_clock_get(), vlv_crtc_clock_get(),
chv_crtc_clock_get(). instead of calling these using if-else, making
func pointers. This will also be useful going forward when the implementation
for BXT is done.

Signed-off-by: Vandana Kannan 
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 20 
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8afda45..773f507 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -607,6 +607,8 @@ struct intel_limit;
 struct dpll;
 
 struct drm_i915_display_funcs {
+   void (*crtc_clock_get)(struct intel_crtc *crtc,
+   struct intel_crtc_state *pipe_config);
int (*get_display_clock_speed)(struct drm_device *dev);
int (*get_fifo_size)(struct drm_device *dev, int plane);
/**
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index c28fb6a..d98385e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8130,12 +8130,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 DPLL_PORTB_READY_MASK);
}
 
-   if (IS_CHERRYVIEW(dev))
-   chv_crtc_clock_get(crtc, pipe_config);
-   else if (IS_VALLEYVIEW(dev))
-   vlv_crtc_clock_get(crtc, pipe_config);
-   else
-   i9xx_crtc_clock_get(crtc, pipe_config);
+   dev_priv->display.crtc_clock_get(crtc, pipe_config);
 
/*
 * Normally the dotclock is filled in by the encoder .get_config()
@@ -10579,9 +10574,10 @@ static void ironlake_pch_clock_get(struct intel_crtc 
*crtc,
   struct intel_crtc_state *pipe_config)
 {
struct drm_device *dev = crtc->base.dev;
+   struct drm_i915_private *dev_priv = dev->dev_private;
 
/* read out port_clock from the DPLL */
-   i9xx_crtc_clock_get(crtc, pipe_config);
+   dev_priv->display.crtc_clock_get(crtc, _config);
 
/*
 * This value does not include pixel_multiplier.
@@ -10625,7 +10621,7 @@ struct drm_display_mode *intel_crtc_mode_get(struct 
drm_device *dev,
pipe_config.dpll_hw_state.dpll = I915_READ(DPLL(pipe));
pipe_config.dpll_hw_state.fp0 = I915_READ(FP0(pipe));
pipe_config.dpll_hw_state.fp1 = I915_READ(FP1(pipe));
-   i9xx_crtc_clock_get(intel_crtc, _config);
+   dev_priv->display.crtc_clock_get(intel_crtc, _config);
 
mode->clock = pipe_config.port_clock / pipe_config.pixel_multiplier;
mode->hdisplay = (htot & 0x) + 1;
@@ -14413,6 +14409,7 @@ static void intel_init_display(struct drm_device *dev)
dev_priv->display.crtc_disable = haswell_crtc_disable;
dev_priv->display.update_primary_plane =
skylake_update_primary_plane;
+   dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get;
} else if (HAS_DDI(dev)) {
dev_priv->display.get_pipe_config = haswell_get_pipe_config;
dev_priv->display.get_initial_plane_config =
@@ -14423,6 +14420,7 @@ static void intel_init_display(struct drm_device *dev)
dev_priv->display.crtc_disable = haswell_crtc_disable;
dev_priv->display.update_primary_plane =
ironlake_update_primary_plane;
+   dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get;
} else if (HAS_PCH_SPLIT(dev)) {
dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
dev_priv->display.get_initial_plane_config =
@@ -14433,6 +14431,7 @@ static void intel_init_display(struct drm_device *dev)
dev_priv->display.crtc_disable = ironlake_crtc_disable;
dev_priv->display.update_primary_plane =
ironlake_update_primary_plane;
+   dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get;
} else if (IS_VALLEYVIEW(dev)) {
dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
dev_priv->display.get_initial_plane_config =
@@ -14442,6 +14441,10 @@ static void intel_init_display(struct drm_device *dev)
dev_priv->display.crtc_disable = i9xx_crtc_disable;
dev_priv->display.update_primary_plane =
i9xx_update_primary_plane;
+   if (IS_CHERRYVIEW(dev))
+   dev_priv->display.crtc_clock_get = chv_crtc_clock_get;
+   else
+   dev_priv->display.crtc_clock_get = vlv_crtc_clock_get;
} else {
dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
dev_priv->display.get_initial_plane_config =
@@ -14451,6 +14454,7 @@ static void 

[Intel-gfx] [PATCH 2/2] drm/i915: Add crtc_clock_get for hsw, skl, bxt

[Vandana Kannan]

Reusing the ddi_clock_get functions for hsw, skl, bxt and creating a
common crtc_clock_get function.
for BXT, there is a difference in clock between DSI and DDI. Taking care of
this as well.

Signed-off-by: Vandana Kannan 
---
 drivers/gpu/drm/i915/intel_display.c | 43 +---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index d98385e..c9fcadd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include "intel_drv.h"
+#include "intel_dsi.h"
 #include 
 #include "i915_drv.h"
 #include "i915_trace.h"
@@ -112,6 +113,15 @@ static void skl_init_scalers(struct drm_device *dev, 
struct intel_crtc *intel_cr
struct intel_crtc_state *crtc_state);
 static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
   int num_connectors);
+static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
+   enum port port,
+   struct intel_crtc_state *pipe_config);
+static void skylake_get_ddi_pll(struct drm_i915_private *dev_priv,
+   enum port port,
+   struct intel_crtc_state *pipe_config);
+static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
+   enum port port,
+   struct intel_crtc_state *pipe_config);
 static void skylake_pfit_enable(struct intel_crtc *crtc);
 static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
 static void ironlake_pfit_enable(struct intel_crtc *crtc);
@@ -8050,6 +8060,33 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
pipe_config->port_clock = chv_calc_dpll_params(refclk, );
 }
 
+static void haswell_crtc_clock_get(struct intel_crtc *crtc,
+   struct intel_crtc_state *pipe_config)
+{
+   struct drm_device *dev = crtc->base.dev;
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct intel_encoder *encoder = NULL;
+   enum port port;
+   bool is_dsi = intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI);
+
+   for_each_encoder_on_crtc(dev, >base, encoder) {
+   port = intel_ddi_get_encoder_port(encoder);
+   if (IS_BROXTON(dev) && is_dsi) {
+   pipe_config->port_clock = bxt_get_dsi_pclk(encoder,
+   pipe_config->pipe_bpp);
+   break;
+   }
+   if (IS_SKYLAKE(dev))
+   skylake_get_ddi_pll(dev_priv, port, pipe_config);
+   else if (IS_BROXTON(dev))
+   bxt_get_ddi_pll(dev_priv, port, pipe_config);
+   else
+   haswell_get_ddi_pll(dev_priv, port, pipe_config);
+
+   intel_ddi_clock_get(encoder, pipe_config);
+   }
+}
+
 static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 struct intel_crtc_state *pipe_config)
 {
@@ -10577,7 +10614,7 @@ static void ironlake_pch_clock_get(struct intel_crtc 
*crtc,
struct drm_i915_private *dev_priv = dev->dev_private;
 
/* read out port_clock from the DPLL */
-   dev_priv->display.crtc_clock_get(crtc, _config);
+   dev_priv->display.crtc_clock_get(crtc, pipe_config);
 
/*
 * This value does not include pixel_multiplier.
@@ -14409,7 +14446,7 @@ static void intel_init_display(struct drm_device *dev)
dev_priv->display.crtc_disable = haswell_crtc_disable;
dev_priv->display.update_primary_plane =
skylake_update_primary_plane;
-   dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get;
+   dev_priv->display.crtc_clock_get = haswell_crtc_clock_get;
} else if (HAS_DDI(dev)) {
dev_priv->display.get_pipe_config = haswell_get_pipe_config;
dev_priv->display.get_initial_plane_config =
@@ -14420,7 +14457,7 @@ static void intel_init_display(struct drm_device *dev)
dev_priv->display.crtc_disable = haswell_crtc_disable;
dev_priv->display.update_primary_plane =
ironlake_update_primary_plane;
-   dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get;
+   dev_priv->display.crtc_clock_get = haswell_crtc_clock_get;
} else if (HAS_PCH_SPLIT(dev)) {
dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
dev_priv->display.get_initial_plane_config =
-- 
1.9.1

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

Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()

[Daniel Vetter]

On Thu, Oct 15, 2015 at 12:36:43PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 15, 2015 at 10:08:53AM +0100, Chris Wilson wrote:
> > On Wed, Oct 14, 2015 at 07:29:03PM +0300, ville.syrj...@linux.intel.com 
> > wrote:
> > > From: Ville Syrjälä 
> > > 
> > > intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird
> > > rotation (to find the right GTT view for it), so no need to pass all
> > > kinds of plane stuff.
> > 
> > imho this is a mistep, I think using the plane-state to not only pass
> > the full description of the plane being bound (which may have additional
> > information like the need for fencing for fbc as well as alternative
> > views, i.e. it is a lot more versatile) but also allows us to track the
> > binding for the plane-state and tie the VMA to lifetime of the plane.
> > 
> > i.e. I think intel_pin_and_fence_fb_obj would be better described as
> > intel_plane_state_pin_vma (and correspondingly
> > intel_plane_state_unpin_vma).
> > 
> > Yes, intel_fbdev.c is a wart to any proposed interface.
> 
> The current code is just too ugly to live IMO (due to fbdev, yes), so
> I think we want this for now. We can always wrap it up in fancier
> clothing for users that actually have a plane state once someone comes
> up with some real code that needs it.

To handle this we could make intel_pin_and_fence_fb return the vma for
callers to store in the plane state eventually, with errors encoded using
ERR_PTR. That way we can keep intel_fbdev.c as is and still store the vma
in the plane state.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915/hsw: keep gamma and CSC enabled for primary plane disable

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 02:31:09PM +0200, Daniel Vetter wrote:
> 
> On Thu, Oct 15, 2015 at 12:51 AM, Kevin Strasser 
>  wrote:
> > On HSW the crc differs between black and disabled primary planes, causing an
> > assert to fail in the kms_universal_plane test. It seems that gamma 
> > correction
> > and color space conversion are causing the black primary plane case to 
> > result in
> > a brighter color than the disabled primary plane case.
> >
> > Keep gamma and CSC bits enabled for plane disable path on HSW.
> >
> > v2: Avoid use of RMW
> > Keep path unchanged for non-HSW users
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331
> > Testcase: igt/kms_universal_plane/universal-plane-pipe-A-functional
> > Signed-off-by: Kevin Strasser 
> 
> With big discussions please add everyone who participated (Ville, Chris,
> Jani, me) to the cc list of the sob section of the patch when doing a new
> revisions.
> 
> Bob did something eerily similarly a while ago to fix crc failures:
> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-August/074657.html
> http://lists.freedesktop.org/archives/intel-gfx/2015-August/074828.html
> 
> Unfortunately those patches did go nowhere :( Related?

Those seem to be for active plane cases. I'll go review them...

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 14/22] drm/i915: Don't treat differently sized rotated views as equal

[Tvrtko Ursulin]

On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:

From: Ville Syrjälä 

In case we have multiple different rotated views into the same object,
each one may need its own vma due to being of different sizes. So don't
treat all rotated views as equal.

Signed-off-by: Ville Syrjälä 
---
  drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index caa182f..68de734 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,

if (a->type != b->type)
return false;
+   if (a->type == I915_GGTT_VIEW_ROTATED)
+   return !memcmp(>rotated, >rotated, sizeof(a->rotated));
if (a->type == I915_GGTT_VIEW_PARTIAL)
return !memcmp(>partial, >partial, sizeof(a->partial));
return true;


This is where anonymous union causes problems since you have to build a 
switch statement before memcmp while the original goal was for memcmp to 
elegantly avoid that.


Regards,

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

Re: [Intel-gfx] [PATCH v2] drm/i915/hsw: keep gamma and CSC enabled for primary plane disable

[Daniel Vetter]

On Thu, Oct 15, 2015 at 12:51 AM, Kevin Strasser 
 wrote:
> On HSW the crc differs between black and disabled primary planes, causing an
> assert to fail in the kms_universal_plane test. It seems that gamma correction
> and color space conversion are causing the black primary plane case to result 
> in
> a brighter color than the disabled primary plane case.
>
> Keep gamma and CSC bits enabled for plane disable path on HSW.
>
> v2: Avoid use of RMW
> Keep path unchanged for non-HSW users
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331
> Testcase: igt/kms_universal_plane/universal-plane-pipe-A-functional
> Signed-off-by: Kevin Strasser 

With big discussions please add everyone who participated (Ville, Chris,
Jani, me) to the cc list of the sob section of the patch when doing a new
revisions.

Bob did something eerily similarly a while ago to fix crc failures:

http://lists.freedesktop.org/archives/intel-gfx/2015-August/074657.html
http://lists.freedesktop.org/archives/intel-gfx/2015-August/074828.html

Unfortunately those patches did go nowhere :( Related?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915/skl+: Enable pipe CSC on cursor planes. (v2)

[Ville Syrjälä]

On Mon, Aug 31, 2015 at 02:03:30PM -0700, Bob Paauwe wrote:
> Extend this to SKL and BXT as it's needed for these platforms as well.
> 
> v2: Change if condition to HAS_DDI() instead of listing each platform
> Signed-off-by: Bob Paauwe 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 88f9764..ba180f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10001,7 +10001,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, 
> u32 base)
>   }
>   cntl |= pipe << 28; /* Connect to correct pipe */
>  
> - if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> + if (HAS_DDI(dev))

Yeah, while cursors themselves don't change between non-DDI and DDI
platforms, the reason for this stuff is the fact that that we use
the pipe csc for HDMI color range mangling on DDI platforms, since
they no longer have any other pipe/port controls for it.

So makese sense to check for DDI here too I think:
Reviewed-by: Ville Syrjälä 

>   cntl |= CURSOR_PIPE_CSC_ENABLE;
>   }
>  
> -- 
> 2.1.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 12:10:32PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> >
> > intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird
> > rotation (to find the right GTT view for it), so no need to pass all
> > kinds of plane stuff.
> >
> > Signed-off-by: Ville Syrjälä 
> > ---
> >   drivers/gpu/drm/i915/intel_display.c | 39 
> > 
> >   drivers/gpu/drm/i915/intel_drv.h |  5 ++---
> >   drivers/gpu/drm/i915/intel_fbdev.c   |  2 +-
> >   3 files changed, 20 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 85e1473..80e9f2e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, 
> > unsigned int height,
> >   }
> >
> >   static int
> > -intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct 
> > drm_framebuffer *fb,
> > -   const struct drm_plane_state *plane_state)
> > +intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
> > +   const struct drm_framebuffer *fb,
> > +   unsigned int rotation)
> >   {
> > struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > struct intel_rotation_info *info = >rotation_info;
> > @@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, 
> > struct drm_framebuffer *fb,
> >
> > *view = i915_ggtt_view_normal;
> >
> > -   if (!plane_state)
> > -   return 0;
> > -
> > -   if (!intel_rotation_90_or_270(plane_state->rotation))
> > +   if (!intel_rotation_90_or_270(rotation))
> > return 0;
> >
> > *view = i915_ggtt_view_rotated;
> > @@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const struct 
> > drm_i915_private *dev_priv
> >   }
> >
> >   int
> > -intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> > -  struct drm_framebuffer *fb,
> > -  const struct drm_plane_state *plane_state,
> > +intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> > +  unsigned int rotation,
> >struct intel_engine_cs *pipelined,
> >struct drm_i915_gem_request **pipelined_request)
> >   {
> 
> It feels like you are losing the benefit of cleaning this up by having 
> to pass in rotation anyway. So I think it makes more sense to keep 
> passing in plane_state and only get rid of the plane. Or vice-versa, not 
> really sure what is conceptually better. Possibly plane and then access 
> the state from it.

The only thing we basically need is "which vma do we want". But just
passing rotation directly looks nicer I think. The benefit really is
eliminating the ugly 'if (!plane_state)' mess caused by intel_fbdev.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()

[Tvrtko Ursulin]

On 15/10/15 12:17, Ville Syrjälä wrote:

On Thu, Oct 15, 2015 at 12:10:32PM +0100, Tvrtko Ursulin wrote:


Hi,

On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:

From: Ville Syrjälä 

intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird
rotation (to find the right GTT view for it), so no need to pass all
kinds of plane stuff.

Signed-off-by: Ville Syrjälä 
---
   drivers/gpu/drm/i915/intel_display.c | 39 

   drivers/gpu/drm/i915/intel_drv.h |  5 ++---
   drivers/gpu/drm/i915/intel_fbdev.c   |  2 +-
   3 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 85e1473..80e9f2e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, unsigned 
int height,
   }

   static int
-intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer 
*fb,
-   const struct drm_plane_state *plane_state)
+intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
+   const struct drm_framebuffer *fb,
+   unsigned int rotation)
   {
struct drm_i915_private *dev_priv = to_i915(fb->dev);
struct intel_rotation_info *info = >rotation_info;
@@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, 
struct drm_framebuffer *fb,

*view = i915_ggtt_view_normal;

-   if (!plane_state)
-   return 0;
-
-   if (!intel_rotation_90_or_270(plane_state->rotation))
+   if (!intel_rotation_90_or_270(rotation))
return 0;

*view = i915_ggtt_view_rotated;
@@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const struct 
drm_i915_private *dev_priv
   }

   int
-intel_pin_and_fence_fb_obj(struct drm_plane *plane,
-  struct drm_framebuffer *fb,
-  const struct drm_plane_state *plane_state,
+intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
+  unsigned int rotation,
   struct intel_engine_cs *pipelined,
   struct drm_i915_gem_request **pipelined_request)
   {


It feels like you are losing the benefit of cleaning this up by having
to pass in rotation anyway. So I think it makes more sense to keep
passing in plane_state and only get rid of the plane. Or vice-versa, not
really sure what is conceptually better. Possibly plane and then access
the state from it.


The only thing we basically need is "which vma do we want". But just
passing rotation directly looks nicer I think. The benefit really is
eliminating the ugly 'if (!plane_state)' mess caused by intel_fbdev.


We'll have to disagree there then because I find it really inelegant to 
express the knowledge of what exact information is needed when preparing 
the frame buffer for display into the function parameters.


It is conceptually much more elegant to say "this fb for this plane - do 
what is right".


Regards,

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

[Intel-gfx] [drm-intel:topic/drm-misc 38/38] drivers/gpu/drm/vgem/vgem_drv.c:249:9: error: implicit declaration of function 'drm_vma_offset_exact_lookup'

[kbuild test robot]

tree:   git://anongit.freedesktop.org/drm-intel topic/drm-misc
head:   0f3609e5b5c53ed52b8e82993cbda72806ef9c69
commit: 0f3609e5b5c53ed52b8e82993cbda72806ef9c69 [38/38] drm/gem: Use 
kref_get_unless_zero for the weak mmap references
config: i386-randconfig-s0-201541 (attached as .config)
reproduce:
git checkout 0f3609e5b5c53ed52b8e82993cbda72806ef9c69
# save the attached .config to linux build tree
make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   drivers/gpu/drm/vgem/vgem_drv.c: In function 'vgem_drm_gem_mmap':
>> drivers/gpu/drm/vgem/vgem_drv.c:249:9: error: implicit declaration of 
>> function 'drm_vma_offset_exact_lookup' 
>> [-Werror=implicit-function-declaration]
 node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
^
>> drivers/gpu/drm/vgem/vgem_drv.c:249:7: warning: assignment makes pointer 
>> from integer without a cast [-Wint-conversion]
 node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
  ^
   cc1: some warnings being treated as errors

vim +/drm_vma_offset_exact_lookup +249 drivers/gpu/drm/vgem/vgem_drv.c

502e95c6 Zach Reizner 2015-03-04  243   struct drm_gem_object *obj;
502e95c6 Zach Reizner 2015-03-04  244   struct drm_vgem_gem_object *vgem_obj;
502e95c6 Zach Reizner 2015-03-04  245   int ret = 0;
502e95c6 Zach Reizner 2015-03-04  246  
502e95c6 Zach Reizner 2015-03-04  247   mutex_lock(>struct_mutex);
502e95c6 Zach Reizner 2015-03-04  248  
502e95c6 Zach Reizner 2015-03-04 @249   node = 
drm_vma_offset_exact_lookup(dev->vma_offset_manager,
502e95c6 Zach Reizner 2015-03-04  250  
vma->vm_pgoff,
502e95c6 Zach Reizner 2015-03-04  251  
vma_pages(vma));
502e95c6 Zach Reizner 2015-03-04  252   if (!node) {

:: The code at line 249 was first introduced by commit
:: 502e95c6678505474f1056480310cd9382bacbac drm/vgem: implement virtual GEM

:: TO: Zach Reizner 
:: CC: Dave Airlie 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: restore ggtt double-bind avoidance

[Daniel Vetter]

This was accidentally lost in

commit 75d04a3773ecee617847de963ae4195d6aa74c28
Author: Mika Kuoppala 
Date:   Tue Apr 28 17:56:17 2015 +0300

drm/i915/gtt: Allocate va range only if vma is not bound

While at it implement an improved version suggested by Chris which
avoids the double-bind irrespective of what type of bind is done
first.

Note that this exact bug was already addressed in

commit d0e30adc42d979e4adc36b6c112b57337423b70c
Author: Chris Wilson 
Date:   Wed Jul 29 20:02:48 2015 +0100

drm/i915: Mark PIN_USER binding as GLOBAL_BIND without the aliasing ppgtt

but the problem is still that originally in

commit 0875546c5318c85c13d07014af5350e9000bc9e9
Author: Daniel Vetter 
Date:   Mon Apr 20 09:04:05 2015 -0700

drm/i915: Fix up the vma aliasing ppgtt binding

if forgotten to take into account there case where we have a
GLOBAL_BIND before a LOCAL_BIND. This patch here fixes that.

v2: Pimp commit message and revert the partial fix.

v3: Split into two functions to specialize on aliasing_ppgtt y/n.

v4: WARN_ON for paranoia in the init sequence, since the ggtt probe
and aliasing ppgtt setup are far apart.

Cc: Chris Wilson 
Cc: Michel Thierry 
Cc: Mika Kuoppala 
Link: 
http://mid.gmane.org/1444894651-5332-1-git-send-email-daniel.vet...@ffwll.ch
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 49 -
 1 file changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ff1004876f56..419687135f41 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2502,6 +2502,39 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 enum i915_cache_level cache_level,
 u32 flags)
 {
+   struct drm_i915_gem_object *obj = vma->obj;
+   struct sg_table *pages = obj->pages;
+   u32 pte_flags = 0;
+   int ret;
+
+   ret = i915_get_ggtt_vma_pages(vma);
+   if (ret)
+   return ret;
+   pages = vma->ggtt_view.pages;
+
+   /* Currently applicable only to VLV */
+   if (obj->gt_ro)
+   pte_flags |= PTE_READ_ONLY;
+
+
+   vma->vm->insert_entries(vma->vm, pages,
+   vma->node.start,
+   cache_level, pte_flags);
+
+   /*
+* Without aliasing PPGTT there's no difference between
+* GLOBAL/LOCAL_BIND, it's all the same ptes. Hence unconditionally
+* upgrade to both bound if we bind either to avoid double-binding.
+*/
+   vma->bound |= GLOBAL_BIND | LOCAL_BIND;
+
+   return 0;
+}
+
+static int aliasing_gtt_bind_vma(struct i915_vma *vma,
+enum i915_cache_level cache_level,
+u32 flags)
+{
struct drm_device *dev = vma->vm->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_gem_object *obj = vma->obj;
@@ -2519,23 +2552,13 @@ static int ggtt_bind_vma(struct i915_vma *vma,
pte_flags |= PTE_READ_ONLY;
 
 
-   if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
+   if (flags & GLOBAL_BIND) {
vma->vm->insert_entries(vma->vm, pages,
vma->node.start,
cache_level, pte_flags);
-
-   /* Note the inconsistency here is due to absence of the
-* aliasing ppgtt on gen4 and earlier. Though we always
-* request PIN_USER for execbuffer (translated to LOCAL_BIND),
-* without the appgtt, we cannot honour that request and so
-* must substitute it with a global binding. Since we do this
-* behind the upper layers back, we need to explicitly set
-* the bound flag ourselves.
-*/
-   vma->bound |= GLOBAL_BIND;
}
 
-   if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
+   if (flags & LOCAL_BIND) {
struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
appgtt->base.insert_entries(>base, pages,
vma->node.start,
@@ -2699,6 +2722,8 @@ static int i915_gem_setup_global_gtt(struct drm_device 
*dev,
true);
 
dev_priv->mm.aliasing_ppgtt = ppgtt;
+   WARN_ON(dev_priv->gtt.base.bind_vma != ggtt_bind_vma);
+   dev_priv->gtt.base.bind_vma = aliasing_gtt_bind_vma;
}
 
return 0;
-- 
2.5.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

[Intel-gfx] [drm-intel:topic/drm-misc 35/38] drivers/gpu/drm/drm_gem_cma_helper.c:484:21: warning: unused variable 'dev'

[kbuild test robot]

tree:   git://anongit.freedesktop.org/drm-intel topic/drm-misc
head:   0f3609e5b5c53ed52b8e82993cbda72806ef9c69
commit: 9da239e3b9538cd5dd883d8258a7c3e2e0499e13 [35/38] drm/gem: Drop 
struct_mutex requirement from drm_gem_mmap_obj
config: arm-at91_dt_defconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 9da239e3b9538cd5dd883d8258a7c3e2e0499e13
# save the attached .config to linux build tree
make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_gem_cma_helper.c: In function 'drm_gem_cma_prime_mmap':
>> drivers/gpu/drm/drm_gem_cma_helper.c:484:21: warning: unused variable 'dev' 
>> [-Wunused-variable]
 struct drm_device *dev = obj->dev;
^

vim +/dev +484 drivers/gpu/drm/drm_gem_cma_helper.c

d7883f87 Thierry Reding 2014-11-03  468  /**
d7883f87 Thierry Reding 2014-11-03  469   * drm_gem_cma_prime_mmap - memory-map 
an exported CMA GEM object
d7883f87 Thierry Reding 2014-11-03  470   * @obj: GEM object
d7883f87 Thierry Reding 2014-11-03  471   * @vma: VMA for the area to be mapped
d7883f87 Thierry Reding 2014-11-03  472   *
d7883f87 Thierry Reding 2014-11-03  473   * This function maps a buffer 
imported via DRM PRIME into a userspace
d7883f87 Thierry Reding 2014-11-03  474   * process's address space. Drivers 
that use the CMA helpers should set this
d7883f87 Thierry Reding 2014-11-03  475   * as their DRM driver's 
->gem_prime_mmap() callback.
d7883f87 Thierry Reding 2014-11-03  476   *
d7883f87 Thierry Reding 2014-11-03  477   * Returns:
d7883f87 Thierry Reding 2014-11-03  478   * 0 on success or a negative error 
code on failure.
d7883f87 Thierry Reding 2014-11-03  479   */
78467dc5 Joonyoung Shim 2013-06-28  480  int drm_gem_cma_prime_mmap(struct 
drm_gem_object *obj,
78467dc5 Joonyoung Shim 2013-06-28  481struct 
vm_area_struct *vma)
78467dc5 Joonyoung Shim 2013-06-28  482  {
78467dc5 Joonyoung Shim 2013-06-28  483 struct drm_gem_cma_object 
*cma_obj;
78467dc5 Joonyoung Shim 2013-06-28 @484 struct drm_device *dev = 
obj->dev;
78467dc5 Joonyoung Shim 2013-06-28  485 int ret;
78467dc5 Joonyoung Shim 2013-06-28  486  
78467dc5 Joonyoung Shim 2013-06-28  487 ret = drm_gem_mmap_obj(obj, 
obj->size, vma);
78467dc5 Joonyoung Shim 2013-06-28  488 if (ret < 0)
78467dc5 Joonyoung Shim 2013-06-28  489 return ret;
78467dc5 Joonyoung Shim 2013-06-28  490  
78467dc5 Joonyoung Shim 2013-06-28  491 cma_obj = 
to_drm_gem_cma_obj(obj);
78467dc5 Joonyoung Shim 2013-06-28  492 return 
drm_gem_cma_mmap_obj(cma_obj, vma);

:: The code at line 484 was first introduced by commit
:: 78467dc5f70fb9bee4a32c0c3714c99b0b5465c7 drm/cma: add low-level hook 
functions to use prime helpers

:: TO: Joonyoung Shim 
:: CC: Dave Airlie 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/skl: Enable pipe gamma for sprite planes.

[Ville Syrjälä]

On Thu, Aug 27, 2015 at 01:46:30PM -0700, Bob Paauwe wrote:
> Since SKL has universal planes, we should configure the sprite planes
> and the primary plane the same.  For the primary plane we do enable
> the pipe gamma on the plane so do the same for the non-primary planes.
> 
> Without this, the pipe CRC values will be different for something
> displayed on the primary plane and something displayed on a sprite
> plane when the ARGB format is used.
> 
> Signed-off-by: Bob Paauwe 

Reviewed-by: Ville Syrjälä 

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 9d8af2f..048d397 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -189,6 +189,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct 
> drm_crtc *crtc,
>   int scaler_id;
>  
>   plane_ctl = PLANE_CTL_ENABLE |
> + PLANE_CTL_PIPE_GAMMA_ENABLE |
>   PLANE_CTL_PIPE_CSC_ENABLE;
>  
>   plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
> -- 
> 2.1.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 14/22] drm/i915: Don't treat differently sized rotated views as equal

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 02:02:24PM +0200, Daniel Vetter wrote:
> On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:
> > >From: Ville Syrjälä 
> > >
> > >In case we have multiple different rotated views into the same object,
> > >each one may need its own vma due to being of different sizes. So don't
> > >treat all rotated views as equal.
> > >
> > >Signed-off-by: Ville Syrjälä 
> > >---
> > >  drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
> > >b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > >index caa182f..68de734 100644
> > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
> > >
> > >   if (a->type != b->type)
> > >   return false;
> > >+  if (a->type == I915_GGTT_VIEW_ROTATED)
> > >+  return !memcmp(>rotated, >rotated, sizeof(a->rotated));
> > >   if (a->type == I915_GGTT_VIEW_PARTIAL)
> > >   return !memcmp(>partial, >partial, sizeof(a->partial));
> > >   return true;
> > 
> > This is where anonymous union causes problems since you have to build a
> > switch statement before memcmp while the original goal was for memcmp to
> > elegantly avoid that.
> 
> Yup, mentioned the same on irc. I much prefer we do the memcmp on type !=
> NORMAL, to avoid mistakes when adding more views in the future.

I just dislike the .partial. extra stuff all over. But whatever, it's a
minor detail in my book. We can go with the non-anon union if people
prefer that.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: restore ggtt double-bind avoidance

[Daniel Vetter]

On Thu, Oct 15, 2015 at 09:48:26AM +0100, Chris Wilson wrote:
> On Thu, Oct 15, 2015 at 09:37:31AM +0200, Daniel Vetter wrote:
> > This was accidentally lost in
> > 
> > commit 75d04a3773ecee617847de963ae4195d6aa74c28
> > Author: Mika Kuoppala 
> > Date:   Tue Apr 28 17:56:17 2015 +0300
> > 
> > drm/i915/gtt: Allocate va range only if vma is not bound
> > 
> > While at it implement an improved version suggested by Chris which
> > avoids the double-bind irrespective of what type of bind is done
> > first.
> > 
> > Cc: Chris Wilson 
> > Cc: Michel Thierry 
> > Cc: Mika Kuoppala 
> > Signed-off-by: Daniel Vetter 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index a3910fb656e7..e90c062e1122 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2542,6 +2542,9 @@ static int ggtt_bind_vma(struct i915_vma *vma,
> > cache_level, pte_flags);
> > }
> >  
> > +   if (!dev_priv->mm.aliasing_ppgtt)
> > +   vma->bound |= GLOBAL_BIND | LOCAL_BIND;
> > +
> > return 0;
> >  }
> 
> Didn't we already add:
> 
> if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
> vma->vm->insert_entries(vma->vm,
> vma->ggtt_view.pages,
> vma->node.start,
> cache_level, pte_flags);
> 
> /* Note the inconsistency here is due to absence of the
>  * aliasing ppgtt on gen4 and earlier. Though we always
>  * request PIN_USER for execbuffer (translated to LOCAL_BIND),
>  * without the appgtt, we cannot honour that request and so
>  * must substitute it with a global binding. Since we do this
>  * behind the upper layers back, we need to explicitly set
>  * the bound flag ourselves.
>  */
> vma->bound |= GLOBAL_BIND;
> 
> }
> 
> to ggtt_bind_vma() ?

Yeah, my patch superseeds this one (it's an old one, cherrished for a long
time) - LOCAL_BIND also implies GLOBAL_BIND if we lack a ppgtt. This is
something I've missed in

commit 0875546c5318c85c13d07014af5350e9000bc9e9
Author: Daniel Vetter 
Date:   Mon Apr 20 09:04:05 2015 -0700

drm/i915: Fix up the vma aliasing ppgtt binding

I'll frob the patch a bit and ping you on irc, but first lunch.
-Daniel

> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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

[Intel-gfx] [PATCH] drm/i915: Split out aliasing-ppgtt from ggtt_bind_vma()

[Chris Wilson]

The presence of the aliasing-ppgtt (or rather its absence) causes some
confusion on old generations where we still pretend that there is a
difference between PIN_USER and PIN_GLOBAL even though we only have the
global GTT. The confusion has resulted in a bug where we do not mark the
vma as suitably bound for both types of access and so end up processing
the binding twice.

The double bind was accidentally introduced in

commit 75d04a3773ecee617847de963ae4195d6aa74c28
Author: Mika Kuoppala 
Date:   Tue Apr 28 17:56:17 2015 +0300

drm/i915/gtt: Allocate va range only if vma is not bound

While at it implement an improved version suggested by Chris which
avoids the double-bind irrespective of what type of bind is done
first.

Note that this exact bug was already addressed in

commit d0e30adc42d979e4adc36b6c112b57337423b70c
Author: Chris Wilson 
Date:   Wed Jul 29 20:02:48 2015 +0100

drm/i915: Mark PIN_USER binding as GLOBAL_BIND without the aliasing 
ppgtt

but the problem is still that originally in

commit 0875546c5318c85c13d07014af5350e9000bc9e9
Author: Daniel Vetter 
Date:   Mon Apr 20 09:04:05 2015 -0700

drm/i915: Fix up the vma aliasing ppgtt binding

if forgotten to take into account there case where we have a
GLOBAL_BIND before a LOCAL_BIND.

Here we separate out the binding into the global GTT for machines where
we have the aliasing-ppgtt and where we do not, so that we can perform
the fixup without further confusion.

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 62 +++--
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0796aa06e9f5..81f4628ae0e6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2493,7 +2493,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 enum i915_cache_level cache_level,
 u32 flags)
 {
-   struct drm_i915_private *dev_priv = to_i915(vma->vm->dev);
u32 pte_flags;
int ret;
 
@@ -2506,26 +2505,49 @@ static int ggtt_bind_vma(struct i915_vma *vma,
if (vma->obj->gt_ro)
pte_flags |= PTE_READ_ONLY;
 
-   if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
+   vma->vm->insert_entries(vma->vm,
+   vma->ggtt_view.pages,
+   vma->node.start,
+   cache_level, pte_flags);
+
+   /* Note the inconsistency here is due to absence of the
+* aliasing ppgtt on gen5 and earlier. Though we always
+* request PIN_USER for execbuffer (translated to LOCAL_BIND),
+* without the appgtt, we cannot honour that request and so
+* must substitute it with a global binding. Since we do this
+* behind the upper layers back, we need to explicitly set
+* the bound flag ourselves.
+*/
+   vma->bound |= GLOBAL_BIND | LOCAL_BIND;
+   return 0;
+}
+
+static int agtt_bind_vma(struct i915_vma *vma,
+enum i915_cache_level cache_level,
+u32 flags)
+{
+   u32 pte_flags;
+   int ret;
+
+   ret = i915_get_ggtt_vma_pages(vma);
+   if (ret)
+   return ret;
+
+   /* Currently applicable only to VLV */
+   pte_flags = 0;
+   if (vma->obj->gt_ro)
+   pte_flags |= PTE_READ_ONLY;
+
+   if (flags & GLOBAL_BIND) {
vma->vm->insert_entries(vma->vm,
vma->ggtt_view.pages,
vma->node.start,
cache_level, pte_flags);
-
-   /* Note the inconsistency here is due to absence of the
-* aliasing ppgtt on gen4 and earlier. Though we always
-* request PIN_USER for execbuffer (translated to LOCAL_BIND),
-* without the appgtt, we cannot honour that request and so
-* must substitute it with a global binding. Since we do this
-* behind the upper layers back, we need to explicitly set
-* the bound flag ourselves.
-*/
-   vma->bound |= GLOBAL_BIND;
-
}
 
-   if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
-   struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
+   if (flags & LOCAL_BIND) {
+   struct i915_hw_ppgtt *appgtt =
+   to_i915(vma->vm->dev)->mm.aliasing_ppgtt;
appgtt->base.insert_entries(>base,
vma->ggtt_view.pages,
vma->node.start,
@@ -2959,7 +2981,10 @@ static 

Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 12:30:44PM +0100, Tvrtko Ursulin wrote:
> 
> On 15/10/15 12:17, Ville Syrjälä wrote:
> > On Thu, Oct 15, 2015 at 12:10:32PM +0100, Tvrtko Ursulin wrote:
> >>
> >> Hi,
> >>
> >> On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:
> >>> From: Ville Syrjälä 
> >>>
> >>> intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird
> >>> rotation (to find the right GTT view for it), so no need to pass all
> >>> kinds of plane stuff.
> >>>
> >>> Signed-off-by: Ville Syrjälä 
> >>> ---
> >>>drivers/gpu/drm/i915/intel_display.c | 39 
> >>> 
> >>>drivers/gpu/drm/i915/intel_drv.h |  5 ++---
> >>>drivers/gpu/drm/i915/intel_fbdev.c   |  2 +-
> >>>3 files changed, 20 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> >>> b/drivers/gpu/drm/i915/intel_display.c
> >>> index 85e1473..80e9f2e 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, 
> >>> unsigned int height,
> >>>}
> >>>
> >>>static int
> >>> -intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct 
> >>> drm_framebuffer *fb,
> >>> - const struct drm_plane_state *plane_state)
> >>> +intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
> >>> + const struct drm_framebuffer *fb,
> >>> + unsigned int rotation)
> >>>{
> >>>   struct drm_i915_private *dev_priv = to_i915(fb->dev);
> >>>   struct intel_rotation_info *info = >rotation_info;
> >>> @@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view 
> >>> *view, struct drm_framebuffer *fb,
> >>>
> >>>   *view = i915_ggtt_view_normal;
> >>>
> >>> - if (!plane_state)
> >>> - return 0;
> >>> -
> >>> - if (!intel_rotation_90_or_270(plane_state->rotation))
> >>> + if (!intel_rotation_90_or_270(rotation))
> >>>   return 0;
> >>>
> >>>   *view = i915_ggtt_view_rotated;
> >>> @@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const 
> >>> struct drm_i915_private *dev_priv
> >>>}
> >>>
> >>>int
> >>> -intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> >>> -struct drm_framebuffer *fb,
> >>> -const struct drm_plane_state *plane_state,
> >>> +intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> >>> +unsigned int rotation,
> >>>  struct intel_engine_cs *pipelined,
> >>>  struct drm_i915_gem_request 
> >>> **pipelined_request)
> >>>{
> >>
> >> It feels like you are losing the benefit of cleaning this up by having
> >> to pass in rotation anyway. So I think it makes more sense to keep
> >> passing in plane_state and only get rid of the plane. Or vice-versa, not
> >> really sure what is conceptually better. Possibly plane and then access
> >> the state from it.
> >
> > The only thing we basically need is "which vma do we want". But just
> > passing rotation directly looks nicer I think. The benefit really is
> > eliminating the ugly 'if (!plane_state)' mess caused by intel_fbdev.
> 
> We'll have to disagree there then because I find it really inelegant to 
> express the knowledge of what exact information is needed when preparing 
> the frame buffer for display into the function parameters.
> 
> It is conceptually much more elegant to say "this fb for this plane - do 
> what is right".

Only if the function is actually used for that. With fbdev it's not.
Chris did sell the "vma in plane state" idea to me last night on irc, so
when we get that we probably want a function that accepts the plane
state. But that can basically just wrap the current thing, which means
fbdev can keep using the lower level function that doesn't need the
plane state.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: restore ggtt double-bind avoidance

[Chris Wilson]

On Thu, Oct 15, 2015 at 02:23:01PM +0200, Daniel Vetter wrote:
> This was accidentally lost in
> 
> commit 75d04a3773ecee617847de963ae4195d6aa74c28
> Author: Mika Kuoppala 
> Date:   Tue Apr 28 17:56:17 2015 +0300
> 
> drm/i915/gtt: Allocate va range only if vma is not bound
> 
> While at it implement an improved version suggested by Chris which
> avoids the double-bind irrespective of what type of bind is done
> first.
> 
> Note that this exact bug was already addressed in
> 
> commit d0e30adc42d979e4adc36b6c112b57337423b70c
> Author: Chris Wilson 
> Date:   Wed Jul 29 20:02:48 2015 +0100
> 
> drm/i915: Mark PIN_USER binding as GLOBAL_BIND without the aliasing ppgtt
> 
> but the problem is still that originally in
> 
> commit 0875546c5318c85c13d07014af5350e9000bc9e9
> Author: Daniel Vetter 
> Date:   Mon Apr 20 09:04:05 2015 -0700
> 
> drm/i915: Fix up the vma aliasing ppgtt binding
> 
> if forgotten to take into account there case where we have a
> GLOBAL_BIND before a LOCAL_BIND. This patch here fixes that.
> 
> v2: Pimp commit message and revert the partial fix.
> 
> v3: Split into two functions to specialize on aliasing_ppgtt y/n.
> 
> v4: WARN_ON for paranoia in the init sequence, since the ggtt probe
> and aliasing ppgtt setup are far apart.
> 
> Cc: Chris Wilson 
> Cc: Michel Thierry 
> Cc: Mika Kuoppala 
> Link: 
> http://mid.gmane.org/1444894651-5332-1-git-send-email-daniel.vet...@ffwll.ch
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 49 
> -
>  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ff1004876f56..419687135f41 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2502,6 +2502,39 @@ static int ggtt_bind_vma(struct i915_vma *vma,
>enum i915_cache_level cache_level,
>u32 flags)
>  {
> + struct drm_i915_gem_object *obj = vma->obj;
> + struct sg_table *pages = obj->pages;

Time to drop this as it is now correctly set later from the ggtt view.
As we then only use obj in one place, lets use it directly. (Having
read-only and read-write vma is a posibility to consider, too bad the
read-only flag didn't come into greater use by hw.)

> + u32 pte_flags = 0;
> + int ret;
> +
> + ret = i915_get_ggtt_vma_pages(vma);
> + if (ret)
> + return ret;
> + pages = vma->ggtt_view.pages;
> +
> + /* Currently applicable only to VLV */
> + if (obj->gt_ro)
> + pte_flags |= PTE_READ_ONLY;
> +
> +

Spare line we can trade in.

> + vma->vm->insert_entries(vma->vm, pages,

And since we only use pages here we can just use vma->gtt_view.pages
instead.

> + vma->node.start,
> + cache_level, pte_flags);
> +
> + /*
> +  * Without aliasing PPGTT there's no difference between
> +  * GLOBAL/LOCAL_BIND, it's all the same ptes. Hence unconditionally
> +  * upgrade to both bound if we bind either to avoid double-binding.
> +  */
> + vma->bound |= GLOBAL_BIND | LOCAL_BIND;
> +
> + return 0;
> +}
> +
> +static int aliasing_gtt_bind_vma(struct i915_vma *vma,
> +  enum i915_cache_level cache_level,
> +  u32 flags)
> +{
>   struct drm_device *dev = vma->vm->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct drm_i915_gem_object *obj = vma->obj;

Could make similar contractions here but meh.

Stylistic nitpicks aside,
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()

[Tvrtko Ursulin]

Hi,

On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:

From: Ville Syrjälä 

intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird
rotation (to find the right GTT view for it), so no need to pass all
kinds of plane stuff.

Signed-off-by: Ville Syrjälä 
---
  drivers/gpu/drm/i915/intel_display.c | 39 
  drivers/gpu/drm/i915/intel_drv.h |  5 ++---
  drivers/gpu/drm/i915/intel_fbdev.c   |  2 +-
  3 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 85e1473..80e9f2e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, unsigned 
int height,
  }

  static int
-intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer 
*fb,
-   const struct drm_plane_state *plane_state)
+intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
+   const struct drm_framebuffer *fb,
+   unsigned int rotation)
  {
struct drm_i915_private *dev_priv = to_i915(fb->dev);
struct intel_rotation_info *info = >rotation_info;
@@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, 
struct drm_framebuffer *fb,

*view = i915_ggtt_view_normal;

-   if (!plane_state)
-   return 0;
-
-   if (!intel_rotation_90_or_270(plane_state->rotation))
+   if (!intel_rotation_90_or_270(rotation))
return 0;

*view = i915_ggtt_view_rotated;
@@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const struct 
drm_i915_private *dev_priv
  }

  int
-intel_pin_and_fence_fb_obj(struct drm_plane *plane,
-  struct drm_framebuffer *fb,
-  const struct drm_plane_state *plane_state,
+intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
+  unsigned int rotation,
   struct intel_engine_cs *pipelined,
   struct drm_i915_gem_request **pipelined_request)
  {


It feels like you are losing the benefit of cleaning this up by having 
to pass in rotation anyway. So I think it makes more sense to keep 
passing in plane_state and only get rid of the plane. Or vice-versa, not 
really sure what is conceptually better. Possibly plane and then access 
the state from it.


Regards,

Tvrtko

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

Re: [Intel-gfx] [PATCH 12/22] drm/i915: Set i915_ggtt_view_normal type explicitly

[Tvrtko Ursulin]

Hi,

On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote:

From: Ville Syrjälä 

Just for clarity set the type for i915_ggtt_view_normal explicitly.

While at it fix the indentation fail for i915_ggtt_view_rotated.

Signed-off-by: Ville Syrjälä 
---
  drivers/gpu/drm/i915/i915_gem_gtt.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 620d57e..71acc71 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -95,9 +95,11 @@
  static int
  i915_get_ggtt_vma_pages(struct i915_vma *vma);

-const struct i915_ggtt_view i915_ggtt_view_normal;
+const struct i915_ggtt_view i915_ggtt_view_normal = {
+   .type = I915_GGTT_VIEW_NORMAL,
+};


AFAIR while this was developed Daniel was very keen on the normal view 
type being implicitly zero. Can't remember at this very moment if some 
of the current code actually depends on it though.


Regards,

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

[Intel-gfx] [PATCH i-g-t] tests: Add gem_exec_nop_concurrent test

[Derek Morton]

This test is based on gem_exec_nop but submits nop batch buffers concurrently
from different threads to check for ring hangs and other issues during
concurrent submissions.

Signed-off-by: Derek Morton 
---
 tests/.gitignore|   1 +
 tests/Makefile.sources  |   1 +
 tests/gem_exec_nop_concurrent.c | 172 
 3 files changed, 174 insertions(+)
 create mode 100644 tests/gem_exec_nop_concurrent.c

diff --git a/tests/.gitignore b/tests/.gitignore
index dc8bb53..0ad36f3 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -46,6 +46,7 @@ gem_exec_blt
 gem_exec_faulting_reloc
 gem_exec_lut_handle
 gem_exec_nop
+gem_exec_nop_concurrent
 gem_exec_params
 gem_exec_parse
 gem_fd_exhaustion
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 2e2e088..aece831 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -27,6 +27,7 @@ TESTS_progs_M = \
gem_exec_bad_domains \
gem_exec_faulting_reloc \
gem_exec_nop \
+   gem_exec_nop_concurrent \
gem_exec_params \
gem_exec_parse \
gem_fenced_exec_thrash \
diff --git a/tests/gem_exec_nop_concurrent.c b/tests/gem_exec_nop_concurrent.c
new file mode 100644
index 000..578f651
--- /dev/null
+++ b/tests/gem_exec_nop_concurrent.c
@@ -0,0 +1,172 @@
+/*
+ * 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:
+ *Derek Morton 
+ *
+ * This test is based on gem_exec_nop but submits nop batch buffers 
concurrently
+ * from different threads to check for ring hangs and other issues during
+ * concurrent submissions.
+ *
+ */
+
+#include "igt.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "drm.h"
+
+#define LOCAL_I915_EXEC_NO_RELOC (1<<11)
+#define LOCAL_I915_EXEC_HANDLE_LUT (1<<12)
+
+#define LOCAL_I915_EXEC_VEBOX (4<<0)
+
+IGT_TEST_DESCRIPTION(
+"This Test will submit nop batch buffers concurrently to the same ring "
+ "and different rings in an attempt to trigger ring hangs.");
+
+const uint32_t batch[2] = {MI_BATCH_BUFFER_END};
+
+struct ring
+{
+   unsigned ring_id;
+   const char *ring_name;
+   bool direction;
+};
+
+static void loop(int fd, uint32_t handle, int child_nbr, struct ring* ring, 
bool up)
+{
+   struct drm_i915_gem_execbuffer2 execbuf;
+   struct drm_i915_gem_exec_object2 gem_exec[1];
+   int count;
+   int max_count = SLOW_QUICK(15, 4);
+
+   gem_require_ring(fd, ring->ring_id);
+
+   memset(_exec, 0, sizeof(gem_exec));
+   gem_exec[0].handle = handle;
+
+   memset(, 0, sizeof(execbuf));
+   execbuf.buffers_ptr = (uintptr_t)gem_exec;
+   execbuf.buffer_count = 1;
+   execbuf.flags = ring->ring_id;
+   execbuf.flags |= LOCAL_I915_EXEC_HANDLE_LUT;
+   execbuf.flags |= LOCAL_I915_EXEC_NO_RELOC;
+   if (drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, )) {
+   execbuf.flags = ring->ring_id;
+   do_ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, );
+   }
+   gem_sync(fd, handle);
+
+   for (count = 0; count <= max_count; count++) {
+   const int reps = 7;
+   int n, nbr_loops;
+
+   if (up)
+   nbr_loops = 1 << count;
+   else
+   nbr_loops = 1 << (max_count - count);
+
+   igt_info("Thread %d: starting submitting batches of %d batch 
buffers (ring=%s)\n",
+child_nbr, nbr_loops, ring->ring_name);
+   fflush(stdout);
+
+   for (n = 0; n < reps; n++) {
+   int loops = nbr_loops;
+
+   while (loops--)
+   do_ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, 
);
+   

[Intel-gfx] [PATCH] drm/i915: restore ggtt double-bind avoidance

[Daniel Vetter]

This was accidentally lost in

commit 75d04a3773ecee617847de963ae4195d6aa74c28
Author: Mika Kuoppala 
Date:   Tue Apr 28 17:56:17 2015 +0300

drm/i915/gtt: Allocate va range only if vma is not bound

While at it implement an improved version suggested by Chris which
avoids the double-bind irrespective of what type of bind is done
first.

Cc: Chris Wilson 
Cc: Michel Thierry 
Cc: Mika Kuoppala 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a3910fb656e7..e90c062e1122 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2542,6 +2542,9 @@ static int ggtt_bind_vma(struct i915_vma *vma,
cache_level, pte_flags);
}
 
+   if (!dev_priv->mm.aliasing_ppgtt)
+   vma->bound |= GLOBAL_BIND | LOCAL_BIND;
+
return 0;
 }
 
-- 
2.5.1

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

Re: [Intel-gfx] [RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL if it exists already

[R, Durgadoss]

>-Original Message-
>From: Jani Nikula [mailto:jani.nik...@linux.intel.com]
>Sent: Wednesday, October 14, 2015 6:53 PM
>To: R, Durgadoss; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL 
>if it exists already
>
>On Wed, 14 Oct 2015, Durgadoss R  wrote:
>> Do not call intel_get_shared_dpll() if there exists a
>> valid shared DPLL already.
>>
>> Signed-off-by: Durgadoss R 
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 70 
>> 
>>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>>  drivers/gpu/drm/i915/intel_drv.h |  2 +-
>>  3 files changed, 42 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
>> b/drivers/gpu/drm/i915/intel_ddi.c
>> index 9098c12..8e4ea36 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1258,7 +1258,8 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */,
>>  static bool
>>  hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>> struct intel_crtc_state *crtc_state,
>> -   struct intel_encoder *intel_encoder)
>> +   struct intel_encoder *intel_encoder,
>> +   bool find_dpll)
>>  {
>>  int clock = crtc_state->port_clock;
>>
>> @@ -1278,14 +1279,16 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>>
>>  crtc_state->dpll_hw_state.wrpll = val;
>>
>> -pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> -if (pll == NULL) {
>> -DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> - pipe_name(intel_crtc->pipe));
>> -return false;
>> -}
>> +if (find_dpll) {
>> +pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> +if (pll == NULL) {
>> +DRM_DEBUG_DRIVER("failed to find PLL for pipe 
>> %c\n",
>> + pipe_name(intel_crtc->pipe));
>> +return false;
>> +}
>
>You have to do a *lot* of passing around parameters here. I wonder (and
>really, I don't know) if we could make intel_get_shared_dpll() grow
>smarts about reusing the pll with intel_crtc_to_shared_dpll() when it's
>there already.

Sure. I will try it out and do this change if it works without issues.

Thanks,
Durga

>
>BR,
>Jani.
>
>
>>
>> -crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
>> +crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
>> +}
>>  }
>>
>>  return true;
>> @@ -1540,7 +1543,8 @@ skip_remaining_dividers:
>>  static bool
>>  skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>> struct intel_crtc_state *crtc_state,
>> -   struct intel_encoder *intel_encoder)
>> +   struct intel_encoder *intel_encoder,
>> +   bool find_dpll)
>>  {
>>  struct intel_shared_dpll *pll;
>>  uint32_t ctrl1, cfgcr1, cfgcr2;
>> @@ -1594,15 +1598,17 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  crtc_state->dpll_hw_state.cfgcr1 = cfgcr1;
>>  crtc_state->dpll_hw_state.cfgcr2 = cfgcr2;
>>
>> -pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> -if (pll == NULL) {
>> -DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> - pipe_name(intel_crtc->pipe));
>> -return false;
>> -}
>> +if (find_dpll) {
>> +pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> +if (pll == NULL) {
>> +DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> + pipe_name(intel_crtc->pipe));
>> +return false;
>> +}
>>
>> -/* shared DPLL id 0 is DPLL 1 */
>> -crtc_state->ddi_pll_sel = pll->id + 1;
>> +/* shared DPLL id 0 is DPLL 1 */
>> +crtc_state->ddi_pll_sel = pll->id + 1;
>> +}
>>
>>  return true;
>>  }
>> @@ -1632,7 +1638,8 @@ static const struct bxt_clk_div bxt_dp_clk_val[] = {
>>  static bool
>>  bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>> struct intel_crtc_state *crtc_state,
>> -   struct intel_encoder *intel_encoder)
>> +   struct intel_encoder *intel_encoder,
>> +   bool find_pll)
>>  {
>>  struct intel_shared_dpll *pll;
>>  struct bxt_clk_div clk_div = {0};
>> @@ -1741,15 +1748,17 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  crtc_state->dpll_hw_state.pcsdw12 =
>>  LANESTAGGER_STRAP_OVRD | lanestagger;
>>
>> -pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> -if (pll == NULL) {
>> -DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> -pipe_name(intel_crtc->pipe));
>> -return 

Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()

[Chris Wilson]

On Wed, Oct 14, 2015 at 07:29:03PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird
> rotation (to find the right GTT view for it), so no need to pass all
> kinds of plane stuff.

imho this is a mistep, I think using the plane-state to not only pass
the full description of the plane being bound (which may have additional
information like the need for fencing for fbc as well as alternative
views, i.e. it is a lot more versatile) but also allows us to track the
binding for the plane-state and tie the VMA to lifetime of the plane.

i.e. I think intel_pin_and_fence_fb_obj would be better described as
intel_plane_state_pin_vma (and correspondingly
intel_plane_state_unpin_vma).

Yes, intel_fbdev.c is a wart to any proposed interface.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: respect previous reg values on primary plane disable

[Ville Syrjälä]

On Wed, Oct 14, 2015 at 01:33:57PM -0700, Kevin Strasser wrote:
> On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote:
> > On Wed, Oct 14, 2015 at 11:59:59AM -0700, Kevin Strasser wrote:
> [...]
> > > Just to level set, these cases will produce different CRCs on HSW:
> > >  1. Primary plane disabled, gamma correction disabled
> > >  2. Primary plane disabled, gamma correction enabled
> > > 
> > > Case 2 is visibly brighter than case 1 and looks more like the enabled 
> > > black
> > > primary plane case.
> > 
> > Ugh. That's weird. I thought data not going through any plane would
> > bypass the gamma too.
> > 
> > > The purpose of this patch is to get the behavior of a
> > > disabled primary plane to match that of an enabled black plane, just as 
> > > it does
> > > on non-HSW platforms.
> > 
> > Does it? I just tried it on IVB, and behaves just like you said. So not
> > sure how far back this goes.
> 
> Ah, so this test case is failing on IVB too?

I just poked at the registers. I don't think we have specific test cases
for this, so any currently failing test case is just bad luck.

It would be good to write a small tool that just frobs the registers in
specific ways, and tells us if the test machine suffers from this issue.
Would be easy to run on any machine then.

What I did manually was:
intel_reg write 0x4a000 0x40
intel_reg write 0x70180 0x0
 = intel_reg read 0x7019c
intel_reg write 0x7019c 

and as a second test I tried:
intel_reg write 0x70180  = intel_reg read 0x7019c
intel_reg write 0x7019c 

And the result was black in the first test, dark red in
the second.

On SKL I additionally tried to resize the plane to be smaller, and then
tried the same thing. But as stated the palette seems to misbehave
somehow, so there was no change in the output from changing entry 0 even
though most of the screen was supposed to be black.

> Are there any reporting tools we
> can look at to find out what tests are passing for each platform?
> 
> > And now I'm really wondering about platforms where the primary
> > plane need not be fullscreen (gen2/3 and chv).
> > 
> > I tried this on SKL too, but that confused me even more. The data not
> > going through any plane seems to be gamma corrected regardless of any
> > plane control bits, so that's good. However the legacy palette seems
> > all fubar. Black input apparently doesn't map to palette entry 0.
> > I wonder if you're seeing this on HSW too, or is your palette entry 0
> > supposed to be non-black?
> 
> I also tried on BDW and it is passing the test with and without my patch
> applied.
> 
> I'm not really sure what 'palette entry 0' you are looking for. Could you 
> point
> me to where in the code I can find out?

Register 0x4a000, 0x4a800, 0x4b000, depending on which pipe you're
using.

> 
> > Looks like quite a bit more testing is needed to get to the bottom of
> > this.
> 
> Agreed, this does seem to extend beyond HSW. For now do you think my patch is
> the right approach at least for HSW alone?

Yeah, looks that way. But we do need to figure out which bits behave
this way. Ie. is it just gamma, or pipe csc too, or perhaps even some
other bits?

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 10:08:53AM +0100, Chris Wilson wrote:
> On Wed, Oct 14, 2015 at 07:29:03PM +0300, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird
> > rotation (to find the right GTT view for it), so no need to pass all
> > kinds of plane stuff.
> 
> imho this is a mistep, I think using the plane-state to not only pass
> the full description of the plane being bound (which may have additional
> information like the need for fencing for fbc as well as alternative
> views, i.e. it is a lot more versatile) but also allows us to track the
> binding for the plane-state and tie the VMA to lifetime of the plane.
> 
> i.e. I think intel_pin_and_fence_fb_obj would be better described as
> intel_plane_state_pin_vma (and correspondingly
> intel_plane_state_unpin_vma).
> 
> Yes, intel_fbdev.c is a wart to any proposed interface.

The current code is just too ugly to live IMO (due to fbdev, yes), so
I think we want this for now. We can always wrap it up in fancier
clothing for users that actually have a plane state once someone comes
up with some real code that needs it.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: restore ggtt double-bind avoidance

[Chris Wilson]

On Thu, Oct 15, 2015 at 09:37:31AM +0200, Daniel Vetter wrote:
> This was accidentally lost in
> 
> commit 75d04a3773ecee617847de963ae4195d6aa74c28
> Author: Mika Kuoppala 
> Date:   Tue Apr 28 17:56:17 2015 +0300
> 
> drm/i915/gtt: Allocate va range only if vma is not bound
> 
> While at it implement an improved version suggested by Chris which
> avoids the double-bind irrespective of what type of bind is done
> first.
> 
> Cc: Chris Wilson 
> Cc: Michel Thierry 
> Cc: Mika Kuoppala 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a3910fb656e7..e90c062e1122 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2542,6 +2542,9 @@ static int ggtt_bind_vma(struct i915_vma *vma,
>   cache_level, pte_flags);
>   }
>  
> + if (!dev_priv->mm.aliasing_ppgtt)
> + vma->bound |= GLOBAL_BIND | LOCAL_BIND;
> +
>   return 0;
>  }

Didn't we already add:

if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
vma->vm->insert_entries(vma->vm,
vma->ggtt_view.pages,
vma->node.start,
cache_level, pte_flags);

/* Note the inconsistency here is due to absence of the
 * aliasing ppgtt on gen4 and earlier. Though we always
 * request PIN_USER for execbuffer (translated to LOCAL_BIND),
 * without the appgtt, we cannot honour that request and so
 * must substitute it with a global binding. Since we do this
 * behind the upper layers back, we need to explicitly set
 * the bound flag ourselves.
 */
vma->bound |= GLOBAL_BIND;

}

to ggtt_bind_vma() ?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] HD 4000 ubuntu 14.04 driver

[Samy CHBINOU]

Hello,
I am new to this list. I work on an OpenCL project. I try to find 
support to setup the environment: to install the ubuntu driver for my 
i5-3230M HD 4000 to run OpenCL kernels on it
I've already downloaded the Intel openCL SDK for linux but I dont know 
how to use it as the HD4000 is not recognized (clinfo shows me only the 
CL_DEVICE_TYPE_CPU and not the CL_DEVICE_TYPE_GPU...)

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

[Intel-gfx] [PATCH] drm/i915: skl_update_scaler() wants a rotation bitmask instead of bit number

[ville . syrjala]

From: Ville Syrjälä 

Pass BIT(DRM_ROTATE_0) instead of DRM_ROTATE_0 to skl_update_scaler().
The former is a mask, the latter just the bit number.

Fortunately the only thing skl_update_scaler() does with the rotation
is check if it's 90/270 degrees or not, and so in this case it would
still do the right thing.

Cc: Chandra Konduru 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 7498c9d..02316d0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4673,7 +4673,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
  intel_crtc->base.base.id, intel_crtc->pipe, 
SKL_CRTC_INDEX);
 
return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
-   >scaler_state.scaler_id, DRM_ROTATE_0,
+   >scaler_state.scaler_id, BIT(DRM_ROTATE_0),
state->pipe_src_w, state->pipe_src_h,
adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay);
 }
-- 
2.4.9

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

Re: [Intel-gfx] [PATCH i-g-t v2] tests/core_prop_blob: Fix core_prop_blob for android

[Thomas Wood]

On 14 October 2015 at 17:23, Derek Morton  wrote:
> core_prop_blob was using ioctls not in the android kernel. Added a
> igt_require_propblob() function and local defines/structures so the
> test will compile and skip on kernels where the feature is unsupported.
>
> v2: moved igt_require_propblob() to core_prop_blob.c (Daniel Vetter)
> Moved gem_blt.c to a seperate patch (Thomas Wood)
>
> Signed-off-by: Derek Morton 
> ---
>  tests/core_prop_blob.c | 71 
> --
>  1 file changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/tests/core_prop_blob.c b/tests/core_prop_blob.c
> index d704158..4dc6d7d 100644
> --- a/tests/core_prop_blob.c
> +++ b/tests/core_prop_blob.c
> @@ -25,18 +25,35 @@
>   *   Daniel Stone 
>   */
>
> +#include "igt.h"
>  #include 
>  #include 
>  #include 
>  #include 
>
> -#include "drmtest.h"
> -#include "igt_debugfs.h"
> -#include "igt_kms.h"
> -#include "igt_aux.h"
> -
>  IGT_TEST_DESCRIPTION("Tests behaviour of mass-data 'blob' properties.");
>
> +struct local_drm_mode_get_blob {
> +   uint32_t blob_id;
> +   uint32_t length;
> +   uint64_t data;
> +};
> +struct local_drm_mode_create_blob {
> +   uint64_t data;
> +   uint32_t length;
> +   uint32_t blob_id;
> +};
> +struct local_drm_mode_destroy_blob {
> +   uint32_t blob_id;
> +};
> +
> +#define LOCAL_DRM_IOCTL_MODE_GETPROPBLOB   DRM_IOWR(0xAC, \
> +   struct 
> local_drm_mode_get_blob)
> +#define LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOBDRM_IOWR(0xBD, \
> +   struct 
> local_drm_mode_create_blob)
> +#define LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB   DRM_IOWR(0xBE, \
> +   struct 
> local_drm_mode_destroy_blob)
> +
>  static const struct drm_mode_modeinfo test_mode_valid = {
> .clock = 1234,
> .hdisplay = 640,
> @@ -61,22 +78,35 @@ static const struct drm_mode_modeinfo test_mode_valid = {
> return errno; \
>  }
>
> +static void igt_require_propblob(int fd)
> +{
> +   struct local_drm_mode_create_blob c;
> +   struct local_drm_mode_destroy_blob d;
> +   uint32_t blob_data;
> +   c.data = _data;

Thanks for the patch. I fixed the compiler warning here (integer from
pointer) and pushed along with the gem_blt.c patch.


> +   c.length = sizeof(blob_data);
> +
> +   igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, ) == 
> 0);
> +   d.blob_id = c.blob_id;
> +   igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB, ) == 
> 0);
> +}
> +
>  static int
>  validate_prop(int fd, uint32_t prop_id)
>  {
> -   struct drm_mode_get_blob get;
> +   struct local_drm_mode_get_blob get;
> struct drm_mode_modeinfo ret_mode;
>
> get.blob_id = prop_id;
> get.length = 0;
> get.data = (uintptr_t) 0;
> -   ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_GETPROPBLOB, );
> +   ioctl_or_ret_errno(fd, LOCAL_DRM_IOCTL_MODE_GETPROPBLOB, );
>
> if (get.length != sizeof(test_mode_valid))
> return ENOMEM;
>
> get.data = (uintptr_t) _mode;
> -   ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_GETPROPBLOB, );
> +   ioctl_or_ret_errno(fd, LOCAL_DRM_IOCTL_MODE_GETPROPBLOB, );
>
> if (memcmp(_mode, _mode_valid, sizeof(test_mode_valid)) != 0)
> return EINVAL;
> @@ -87,12 +117,12 @@ validate_prop(int fd, uint32_t prop_id)
>  static uint32_t
>  create_prop(int fd)
>  {
> -   struct drm_mode_create_blob create;
> +   struct local_drm_mode_create_blob create;
>
> create.length = sizeof(test_mode_valid);
> create.data = (uintptr_t) _mode_valid;
>
> -   do_ioctl(fd, DRM_IOCTL_MODE_CREATEPROPBLOB, );
> +   do_ioctl(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, );
> igt_assert_neq_u32(create.blob_id, 0);
>
> return create.blob_id;
> @@ -101,10 +131,10 @@ create_prop(int fd)
>  static int
>  destroy_prop(int fd, uint32_t prop_id)
>  {
> -   struct drm_mode_destroy_blob destroy;
> +   struct local_drm_mode_destroy_blob destroy;
>
> destroy.blob_id = prop_id;
> -   ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_DESTROYPROPBLOB, );
> +   ioctl_or_ret_errno(fd, LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB, 
> );
>
> return 0;
>  }
> @@ -112,8 +142,8 @@ destroy_prop(int fd, uint32_t prop_id)
>  static void
>  test_validate(int fd)
>  {
> -   struct drm_mode_create_blob create;
> -   struct drm_mode_get_blob get;
> +   struct local_drm_mode_create_blob create;
> +   struct local_drm_mode_get_blob get;
> char too_small[32];
> uint32_t prop_id;
>
> @@ -122,24 +152,24 @@ test_validate(int fd)
> /* Outlandish size. */
> create.length = 0x1;
> create.data = (uintptr_t) _small;
> -   do_ioctl_err(fd, 

Re: [Intel-gfx] [PATCH 2/2] drm/i915/skl+: Enable pipe CSC on cursor planes. (v2)

[Daniel Vetter]

On Thu, Oct 15, 2015 at 03:49:44PM +0300, Ville Syrjälä wrote:
> On Mon, Aug 31, 2015 at 02:03:30PM -0700, Bob Paauwe wrote:
> > Extend this to SKL and BXT as it's needed for these platforms as well.
> > 
> > v2: Change if condition to HAS_DDI() instead of listing each platform
> > Signed-off-by: Bob Paauwe 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 88f9764..ba180f6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10001,7 +10001,7 @@ static void i9xx_update_cursor(struct drm_crtc 
> > *crtc, u32 base)
> > }
> > cntl |= pipe << 28; /* Connect to correct pipe */
> >  
> > -   if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > +   if (HAS_DDI(dev))
> 
> Yeah, while cursors themselves don't change between non-DDI and DDI
> platforms, the reason for this stuff is the fact that that we use
> the pipe csc for HDMI color range mangling on DDI platforms, since
> they no longer have any other pipe/port controls for it.
> 
> So makese sense to check for DDI here too I think:
> Reviewed-by: Ville Syrjälä 

Both patches applied, thanks.
-Daniel

> 
> > cntl |= CURSOR_PIPE_CSC_ENABLE;
> > }
> >  
> > -- 
> > 2.1.0
> > 
> > ___
> > 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

-- 
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 1/2] drm/i915: Create crtc_clock_get function pointers

[Daniel Vetter]

On Thu, Oct 15, 2015 at 06:52:48PM +0530, Vandana Kannan wrote:
> There are separate functions i9xx_crtc_clock_get(), vlv_crtc_clock_get(),
> chv_crtc_clock_get(). instead of calling these using if-else, making
> func pointers. This will also be useful going forward when the implementation
> for BXT is done.
> 
> Signed-off-by: Vandana Kannan 

I general I much prefer direct callers if there's only one caller ever.
The per-platform modeset code is already really hard to follow, no need to
insert even more indirection imo.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 20 
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8afda45..773f507 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -607,6 +607,8 @@ struct intel_limit;
>  struct dpll;
>  
>  struct drm_i915_display_funcs {
> + void (*crtc_clock_get)(struct intel_crtc *crtc,
> + struct intel_crtc_state *pipe_config);
>   int (*get_display_clock_speed)(struct drm_device *dev);
>   int (*get_fifo_size)(struct drm_device *dev, int plane);
>   /**
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index c28fb6a..d98385e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8130,12 +8130,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc 
> *crtc,
>DPLL_PORTB_READY_MASK);
>   }
>  
> - if (IS_CHERRYVIEW(dev))
> - chv_crtc_clock_get(crtc, pipe_config);
> - else if (IS_VALLEYVIEW(dev))
> - vlv_crtc_clock_get(crtc, pipe_config);
> - else
> - i9xx_crtc_clock_get(crtc, pipe_config);
> + dev_priv->display.crtc_clock_get(crtc, pipe_config);
>  
>   /*
>* Normally the dotclock is filled in by the encoder .get_config()
> @@ -10579,9 +10574,10 @@ static void ironlake_pch_clock_get(struct intel_crtc 
> *crtc,
>  struct intel_crtc_state *pipe_config)
>  {
>   struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
>  
>   /* read out port_clock from the DPLL */
> - i9xx_crtc_clock_get(crtc, pipe_config);
> + dev_priv->display.crtc_clock_get(crtc, _config);
>  
>   /*
>* This value does not include pixel_multiplier.
> @@ -10625,7 +10621,7 @@ struct drm_display_mode *intel_crtc_mode_get(struct 
> drm_device *dev,
>   pipe_config.dpll_hw_state.dpll = I915_READ(DPLL(pipe));
>   pipe_config.dpll_hw_state.fp0 = I915_READ(FP0(pipe));
>   pipe_config.dpll_hw_state.fp1 = I915_READ(FP1(pipe));
> - i9xx_crtc_clock_get(intel_crtc, _config);
> + dev_priv->display.crtc_clock_get(intel_crtc, _config);
>  
>   mode->clock = pipe_config.port_clock / pipe_config.pixel_multiplier;
>   mode->hdisplay = (htot & 0x) + 1;
> @@ -14413,6 +14409,7 @@ static void intel_init_display(struct drm_device *dev)
>   dev_priv->display.crtc_disable = haswell_crtc_disable;
>   dev_priv->display.update_primary_plane =
>   skylake_update_primary_plane;
> + dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get;
>   } else if (HAS_DDI(dev)) {
>   dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>   dev_priv->display.get_initial_plane_config =
> @@ -14423,6 +14420,7 @@ static void intel_init_display(struct drm_device *dev)
>   dev_priv->display.crtc_disable = haswell_crtc_disable;
>   dev_priv->display.update_primary_plane =
>   ironlake_update_primary_plane;
> + dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get;
>   } else if (HAS_PCH_SPLIT(dev)) {
>   dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
>   dev_priv->display.get_initial_plane_config =
> @@ -14433,6 +14431,7 @@ static void intel_init_display(struct drm_device *dev)
>   dev_priv->display.crtc_disable = ironlake_crtc_disable;
>   dev_priv->display.update_primary_plane =
>   ironlake_update_primary_plane;
> + dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get;
>   } else if (IS_VALLEYVIEW(dev)) {
>   dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>   dev_priv->display.get_initial_plane_config =
> @@ -14442,6 +14441,10 @@ static void intel_init_display(struct drm_device 
> *dev)
>   dev_priv->display.crtc_disable = i9xx_crtc_disable;
>   dev_priv->display.update_primary_plane =
>   i9xx_update_primary_plane;
> + if (IS_CHERRYVIEW(dev))
> + 

[Intel-gfx] [drm-intel:topic/drm-misc 35/37] drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c:143:21: error: unused variable 'dev'

[kbuild test robot]

tree:   git://anongit.freedesktop.org/drm-intel topic/drm-misc
head:   1032f72564813b1dd00184897d8291ad71676a56
commit: 9da239e3b9538cd5dd883d8258a7c3e2e0499e13 [35/37] drm/gem: Drop 
struct_mutex requirement from drm_gem_mmap_obj
config: arm-allmodconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 9da239e3b9538cd5dd883d8258a7c3e2e0499e13
# save the attached .config to linux build tree
make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

   drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c: In function 
'omap_gem_dmabuf_mmap':
>> drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c:143:21: error: unused variable 
>> 'dev' [-Werror=unused-variable]
 struct drm_device *dev = obj->dev;
^
   cc1: all warnings being treated as errors
--
   drivers/gpu/drm/rockchip/rockchip_drm_gem.c: In function 
'rockchip_gem_mmap_buf':
>> drivers/gpu/drm/rockchip/rockchip_drm_gem.c:82:21: warning: unused variable 
>> 'drm' [-Wunused-variable]
 struct drm_device *drm = obj->dev;
^

vim +/dev +143 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c

6ad11bc3 drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-04-10  137 
 }
6ad11bc3 drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-04-10  138 
 
8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17  139 
 static int omap_gem_dmabuf_mmap(struct dma_buf *buffer,
8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17  140 
struct vm_area_struct *vma)
8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17  141 
 {
8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17  142 
struct drm_gem_object *obj = buffer->priv;
4368dd84 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c YoungJun Cho 2013-06-27 @143 
struct drm_device *dev = obj->dev;
8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17  144 
int ret = 0;
8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17  145 
 
8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17  146 
if (WARN_ON(!obj->filp))

:: The code at line 143 was first introduced by commit
:: 4368dd846d067ed7d11281050e7676ae614d6053 drm/gem: add mutex lock when 
using drm_gem_mmap_obj

:: TO: YoungJun Cho 
:: CC: Dave Airlie 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Kill the leftover RMW from ivb_sprite_disable()

[ville . syrjala]

From: Ville Syrjälä 

We still had one lingering RMW in ivb_sprite_disable(), all the other
RMWs were killed off from the sprite code some time ago. Kill the
straggler too.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 7f0911e..a84cbf42 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -610,7 +610,7 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc 
*crtc)
struct intel_plane *intel_plane = to_intel_plane(plane);
int pipe = intel_plane->pipe;
 
-   I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
+   I915_WRITE(SPRCTL(pipe), 0);
/* Can't leave the scaler enabled... */
if (intel_plane->can_scale)
I915_WRITE(SPRSCALE(pipe), 0);
-- 
2.4.9

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

Re: [Intel-gfx] [PATCH] drm/i915: respect previous reg values on primary plane disable

[Kevin Strasser]

On Thu, Oct 15, 2015 at 11:20:59AM +0300, Ville Syrjälä wrote:
> On Wed, Oct 14, 2015 at 01:33:57PM -0700, Kevin Strasser wrote:
> > On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote:
> > > Does it? I just tried it on IVB, and behaves just like you said. So not
> > > sure how far back this goes.
> > 
> > Ah, so this test case is failing on IVB too?
> 
> I just poked at the registers. I don't think we have specific test cases
> for this, so any currently failing test case is just bad luck.
> 
> It would be good to write a small tool that just frobs the registers in
> specific ways, and tells us if the test machine suffers from this issue.
> Would be easy to run on any machine then.

Agreed, it is somewhat painful getting the whole test suite built and running in
some environments.

> What I did manually was:
> intel_reg write 0x4a000 0x40
> intel_reg write 0x70180 0x0
>  = intel_reg read 0x7019c
> intel_reg write 0x7019c 
> 
> and as a second test I tried:
> intel_reg write 0x70180   = intel_reg read 0x7019c
> intel_reg write 0x7019c 
> 
> And the result was black in the first test, dark red in
> the second.
> 
> On SKL I additionally tried to resize the plane to be smaller, and then
> tried the same thing. But as stated the palette seems to misbehave
> somehow, so there was no change in the output from changing entry 0 even
> though most of the screen was supposed to be black.
> 
> > Are there any reporting tools we
> > can look at to find out what tests are passing for each platform?
> > 
> > > And now I'm really wondering about platforms where the primary
> > > plane need not be fullscreen (gen2/3 and chv).
> > > 
> > > I tried this on SKL too, but that confused me even more. The data not
> > > going through any plane seems to be gamma corrected regardless of any
> > > plane control bits, so that's good. However the legacy palette seems
> > > all fubar. Black input apparently doesn't map to palette entry 0.
> > > I wonder if you're seeing this on HSW too, or is your palette entry 0
> > > supposed to be non-black?
> > 
> > I also tried on BDW and it is passing the test with and without my patch
> > applied.
> > 
> > I'm not really sure what 'palette entry 0' you are looking for. Could you 
> > point
> > me to where in the code I can find out?
> 
> Register 0x4a000, 0x4a800, 0x4b000, depending on which pipe you're
> using.

I'm using pipe A, here is the output of 'intel_reg read 0x4a000':
(0x0004a000): 0x

I suppose this means palette entry 0 is black for me.

> > > Looks like quite a bit more testing is needed to get to the bottom of
> > > this.
> > 
> > Agreed, this does seem to extend beyond HSW. For now do you think my patch 
> > is
> > the right approach at least for HSW alone?
> 
> Yeah, looks that way. But we do need to figure out which bits behave
> this way. Ie. is it just gamma, or pipe csc too, or perhaps even some
> other bits?

I did some testing and it seems that gamma and pipe csc are both needed to pass
the test on HSW. v2 of the patch sets them explicitly.

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

[Intel-gfx] [PATCH igt] kms_cursor_crc: Add test for unthrottled cursor movement

[Matt Roper]

We've had  bugs in the past that caused cursor updates to be synced to
vblank, resulting in sluggish cursor movement.  Add a test to try to
make sure we don't regress and reintroduce these bugs.

Cc: kalyan.kondapa...@intel.com
Signed-off-by: Matt Roper 
---
 tests/kms_cursor_crc.c | 42 ++
 1 file changed, 42 insertions(+)

diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
index 3495b26..d1de450 100644
--- a/tests/kms_cursor_crc.c
+++ b/tests/kms_cursor_crc.c
@@ -55,6 +55,7 @@ typedef struct {
igt_crc_t ref_crc;
int left, right, top, bottom;
int screenw, screenh;
+   int refresh;
int curw, curh; /* cursor size */
int cursor_max_w, cursor_max_h;
igt_pipe_crc_t *pipe_crc;
@@ -327,6 +328,7 @@ static bool prepare_crtc(data_t *data, igt_output_t *output,
data->screenh = mode->vdisplay;
data->curw = cursor_w;
data->curh = cursor_h;
+   data->refresh = mode->vrefresh;
 
/* make sure cursor is disabled */
cursor_disable(data);
@@ -479,6 +481,42 @@ static void test_cursor_size(data_t *data)
}
 }
 
+static void test_rapid_movement(data_t *data)
+{
+   struct timeval start, end, delta;
+   int x = 0, y = 0;
+   long usec;
+   int crtc_id = data->output->config.crtc->crtc_id;
+
+   igt_assert_eq(drmModeSetCursor(data->drm_fd, crtc_id,
+  data->fb.gem_handle, data->curw, data->curh), 0);
+
+   gettimeofday(, NULL);
+   for ( ; x < 100; x++)
+   igt_assert_eq(drmModeMoveCursor(data->drm_fd, crtc_id, x, y), 
0);
+   for ( ; y < 100; y++)
+   igt_assert_eq(drmModeMoveCursor(data->drm_fd, crtc_id, x, y), 
0);
+   for ( ; x > 0; x--)
+   igt_assert_eq(drmModeMoveCursor(data->drm_fd, crtc_id, x, y), 
0);
+   for ( ; y > 0; y--)
+   igt_assert_eq(drmModeMoveCursor(data->drm_fd, crtc_id, x, y), 
0);
+   gettimeofday(, NULL);
+
+   /*
+* We've done 400 cursor updates now.  If we're being throttled to
+* vblank, then that would take roughly 400/refresh seconds.  If the
+* elapsed time is greater than 90% of that value, we'll consider it
+* a failure (since cursor updates shouldn't be throttled).
+*/
+   timersub(, , );
+   usec = delta.tv_usec + 100 * delta.tv_sec;
+   igt_assert_lt(usec, 0.9 * 400 * 100 / data->refresh);
+
+   igt_assert_eq(drmModeSetCursor(data->drm_fd, crtc_id,
+  0, data->curw, data->curh), 0);
+
+}
+
 static void run_test_generic(data_t *data)
 {
int cursor_size;
@@ -510,6 +548,10 @@ static void run_test_generic(data_t *data)
data->flags = 0;
}
 
+   igt_subtest_f("cursor-%dx%d-rapid-movement", w, h) {
+   run_test(data, test_rapid_movement, w, h);
+   }
+
igt_fixture
igt_remove_fb(data->drm_fd, >fb);
 
-- 
2.1.4

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

Re: [Intel-gfx] [PATCH] igt/kms_addfb_basic: New subtest to check for fb modifier and tiling mode mismatch

[Vivek Kasireddy]

Hi Tvrtko,

On Fri, 9 Oct 2015 09:34:25 +0100
Tvrtko Ursulin  wrote:

> 
> 
> On 08/10/15 09:55, Tvrtko Ursulin wrote:
> > On 07/10/15 22:07, Vivek Kasireddy wrote:
> >>
> >> Hi Tvrtko,
> >>
> >> On Wed, 7 Oct 2015 15:07:30 +0100
> >> Tvrtko Ursulin  wrote:
> >>
> >>>
> >>> Hi,
> >>>
> >>> On 07/10/15 03:35, Vivek Kasireddy wrote:
>  This new subtest will validate a Y-tiled object's tiling mode
>  against its associated fb modifier.
> 
>  Cc: Tvrtko Ursulin 
>  Signed-off-by: Vivek Kasireddy 
>  ---
> tests/kms_addfb_basic.c | 9 +
> 1 file changed, 9 insertions(+)
> 
>  diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
>  index d466e4d..7ca1add 100644
>  --- a/tests/kms_addfb_basic.c
>  +++ b/tests/kms_addfb_basic.c
>  @@ -373,6 +373,15 @@ static void addfb25_ytile(int fd, int gen)
> f.handles[0] = gem_bo;
> }
> 
>  +igt_subtest("addfb25-Y-tiled-X-modifier-mismatch") {
>  +igt_require(gen >= 9);
>  +igt_require_fb_modifiers(fd);
>  +gem_set_tiling(fd, gem_bo, I915_TILING_Y, 1024*4);
>  +
>  +f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED;
>  +igt_assert(drmIoctl(fd,
>  LOCAL_DRM_IOCTL_MODE_ADDFB2, ) < 0 && errno == EINVAL);
>  +}
>  +
> igt_subtest("addfb25-Y-tiled") {
> igt_require_fb_modifiers(fd);
> 
> >>>
> >>> Wasn't the original WARN triggered by Y tiled object and Y fb
> >>> modifier?
> >>
> >> Creating a new fb using a Y-tiled object and Y/X fb modifier will
> >> not trigger the original WARN. It'll be triggered only if the fb
> >> is going to be pinned -- and flipped. I am not sure how to get
> >> that WARN to be triggered with the existing suite of igt tests.
> >
> > Ah yes, you would need to attempt display it, not even necessarily
> > flip it. I am sure there are tests which do that. :) I know from
> > recent activity kms_rotation_crc for example creates a Y tiled FB
> > and displays it. So maybe borrow some code to start with from there.
> 
> Even better, kms_flip_tiling does the majority of what is needed here 
> already. Just drop in a subtest which will do set_tiling and that
> should be good.

I have realized that I cannot get the the map_and_fenceable WARN to be
triggered with any of the IGT tests. This is because the WARN is
triggered only when pinning/fencing a Y-tiled fb that has a rotated
view (90/270 degree rotation) that none of the IGT tests can do. I
looked at and wrote a subtest in kms_rotation_crc but the WARN was not
triggered because IGT does not support atomic flip/commit yet.
Currently, since it does a SetPlane first, the object has a normal view
and its map_and-fenceable bit is set, however, when the rotation
property is applied, the object though has a rotated view, its
map_and-fenceable bit never gets updated and stays 1 and hence the
warning doesn't get triggered. I am copying the subtest code below for
reference.

From 5d7f6920663b2fd264aca6085170811276948a5b Mon Sep 17 00:00:00 2001
From: Vivek Kasireddy 
Date: Thu, 15 Oct 2015 17:48:50 -0700
Subject: [PATCH] igt/kms_rotation_crc: Add a new subtest to flip Y-tiled
 rotated fb

---
 tests/kms_rotation_crc.c | 70 
 1 file changed, 70 insertions(+)

diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index cc9847e..f10a770 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -264,6 +264,70 @@ static void test_plane_rotation(data_t *data, enum 
igt_plane plane_type)
igt_require_f(valid_tests, "no valid crtc/connector combinations 
found\n");
 }
 
+static void test_plane_rotation_ytiled(data_t *data, enum igt_plane plane_type)
+{
+   igt_display_t *display = >display;
+   igt_output_t *output;
+   enum igt_commit_style commit = COMMIT_LEGACY;
+   drmModeModeInfo *mode;
+   unsigned int w, h;
+   igt_plane_t *plane;
+   int ret;
+   uint64_t tiling = LOCAL_I915_FORMAT_MOD_Y_TILED;
+   uint32_t format = DRM_FORMAT_XRGB;
+   int bpp = igt_drm_format_to_bpp(format);
+
+   if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
+   igt_require(data->display.has_universal_planes);
+   commit = COMMIT_UNIVERSAL;
+   }
+
+   for_each_connected_output(display, output) {
+   mode = igt_output_get_mode(output);
+
+   w = mode->hdisplay;
+   h = mode->vdisplay;
+
+   for (data->fb.stride = 512; data->fb.stride < (w * bpp / 8); 
data->fb.stride *= 2);
+
+   for (data->fb.size = 1024*1024; data->fb.size < data->fb.stride 
* h; data->fb.size *= 2);
+
+   data->fb.gem_handle = gem_create(data->gfx_fd, 

Re: [Intel-gfx] [PATCH] drm/i915: respect previous reg values on primary plane disable

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 04:00:48PM -0700, Kevin Strasser wrote:
> On Thu, Oct 15, 2015 at 11:20:59AM +0300, Ville Syrjälä wrote:
> > On Wed, Oct 14, 2015 at 01:33:57PM -0700, Kevin Strasser wrote:
> > > On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote:
> > > > Does it? I just tried it on IVB, and behaves just like you said. So not
> > > > sure how far back this goes.
> > > 
> > > Ah, so this test case is failing on IVB too?
> > 
> > I just poked at the registers. I don't think we have specific test cases
> > for this, so any currently failing test case is just bad luck.
> > 
> > It would be good to write a small tool that just frobs the registers in
> > specific ways, and tells us if the test machine suffers from this issue.
> > Would be easy to run on any machine then.
> 
> Agreed, it is somewhat painful getting the whole test suite built and running 
> in
> some environments.
> 
> > What I did manually was:
> > intel_reg write 0x4a000 0x40
> > intel_reg write 0x70180 0x0
> >  = intel_reg read 0x7019c
> > intel_reg write 0x7019c 
> > 
> > and as a second test I tried:
> > intel_reg write 0x70180  >  = intel_reg read 0x7019c
> > intel_reg write 0x7019c 
> > 
> > And the result was black in the first test, dark red in
> > the second.
> > 
> > On SKL I additionally tried to resize the plane to be smaller, and then
> > tried the same thing. But as stated the palette seems to misbehave
> > somehow, so there was no change in the output from changing entry 0 even
> > though most of the screen was supposed to be black.
> > 
> > > Are there any reporting tools we
> > > can look at to find out what tests are passing for each platform?
> > > 
> > > > And now I'm really wondering about platforms where the primary
> > > > plane need not be fullscreen (gen2/3 and chv).
> > > > 
> > > > I tried this on SKL too, but that confused me even more. The data not
> > > > going through any plane seems to be gamma corrected regardless of any
> > > > plane control bits, so that's good. However the legacy palette seems
> > > > all fubar. Black input apparently doesn't map to palette entry 0.
> > > > I wonder if you're seeing this on HSW too, or is your palette entry 0
> > > > supposed to be non-black?
> > > 
> > > I also tried on BDW and it is passing the test with and without my patch
> > > applied.
> > > 
> > > I'm not really sure what 'palette entry 0' you are looking for. Could you 
> > > point
> > > me to where in the code I can find out?
> > 
> > Register 0x4a000, 0x4a800, 0x4b000, depending on which pipe you're
> > using.
> 
> I'm using pipe A, here is the output of 'intel_reg read 0x4a000':
> (0x0004a000): 0x
> 
> I suppose this means palette entry 0 is black for me.

It means it's supposed to be black. But if you saw a difference between
black when it went through the gamma and when it didn't, then I guess
black is not hitting entry 0 on hsw either. So something seems fishy.

> 
> > > > Looks like quite a bit more testing is needed to get to the bottom of
> > > > this.
> > > 
> > > Agreed, this does seem to extend beyond HSW. For now do you think my 
> > > patch is
> > > the right approach at least for HSW alone?
> > 
> > Yeah, looks that way. But we do need to figure out which bits behave
> > this way. Ie. is it just gamma, or pipe csc too, or perhaps even some
> > other bits?
> 
> I did some testing and it seems that gamma and pipe csc are both needed to 
> pass
> the test on HSW. v2 of the patch sets them explicitly.
> 
> Thanks,
> Kevin

-- 
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] drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4)

[Matt Roper]

Just pull the info out of the state structures rather than staging
it in an additional set of structures.  To make this more
straightforward, we change the signature of several internal WM
functions to take the crtc state as a parameter.

v2:
 - Don't forget to skip cursor planes on a loop in the DDB allocation
   function to match original behavior.  (Ander)
 - Change a use of intel_crtc->active to cstate->active.  They should
   be identical, but it's better to be consistent.  (Ander)
 - Rework more function signatures to pass states rather than crtc for
   consistency. (Ander)

v3:
  - Add missing "+ 1" to skl_wm_plane_id()'s 'overlay' case. (Maarten)
  - Packed formats should pass '0' to drm_format_plane_cpp(), not 1.
(Maarten)
  - Drop unwanted WARN_ON() for disabled planes when calculating data
rate for SKL.  (Maarten)

v4:
 - Don't include cursor plane in total relative data rate calculation;
   we've already handled the cursor allocation earlier.
 - Fix 'bytes_per_pixel' calculation braindamage.  Somehow I hardcoded
   the NV12 format as a parameter rather than the actual
   fb->pixel_format, and even then still managed to get the format plane
   wrong.  (Ville)
 - Use plane->state->fb rather than plane->fb in
   skl_allocate_pipe_ddb(); the plane->fb pointer isn't updated until
   after we've done our watermark recalculation, so it has stale
   values.  (Bob Paauwe)

Signed-off-by: Matt Roper 
Reviewed-by(v3): Maarten Lankhorst 
Cc: Paauwe, Bob J 
Cc: Ville Syrjälä 
Cc: Paulo Zanoni 
References: 
http://lists.freedesktop.org/archives/intel-gfx/2015-September/077060.html
References: 
http://lists.freedesktop.org/archives/intel-gfx/2015-October/077721.html
---
Paulo, would you mind trying this patch out when you have some free time and
see whether you still experience watermark problems?  This is the patch that
you bisected your problems to and there were at least three bugs in the version
of this patch that was merged before; I'm hoping that with these three bugfixes
your problems will be gone.

 drivers/gpu/drm/i915/intel_pm.c | 327 +++-
 1 file changed, 152 insertions(+), 175 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9dda3ea..df22b9c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1708,13 +1708,6 @@ static uint32_t ilk_wm_fbc(uint32_t pri_val, uint32_t 
horiz_pixels,
return DIV_ROUND_UP(pri_val * 64, horiz_pixels * bytes_per_pixel) + 2;
 }
 
-struct skl_pipe_wm_parameters {
-   bool active;
-   uint32_t pipe_htotal;
-   uint32_t pixel_rate; /* in KHz */
-   struct intel_plane_wm_parameters plane[I915_MAX_PLANES];
-};
-
 struct ilk_wm_maximums {
uint16_t pri;
uint16_t spr;
@@ -2755,18 +2748,40 @@ static bool ilk_disable_lp_wm(struct drm_device *dev)
 #define SKL_DDB_SIZE   896 /* in blocks */
 #define BXT_DDB_SIZE   512
 
+/*
+ * Return the index of a plane in the SKL DDB and wm result arrays.  Primary
+ * plane is always in slot 0, cursor is always in slot I915_MAX_PLANES-1, and
+ * other universal planes are in indices 1..n.  Note that this may leave unused
+ * indices between the top "sprite" plane and the cursor.
+ */
+static int
+skl_wm_plane_id(const struct intel_plane *plane)
+{
+   switch (plane->base.type) {
+   case DRM_PLANE_TYPE_PRIMARY:
+   return 0;
+   case DRM_PLANE_TYPE_CURSOR:
+   return PLANE_CURSOR;
+   case DRM_PLANE_TYPE_OVERLAY:
+   return plane->plane + 1;
+   default:
+   MISSING_CASE(plane->base.type);
+   return plane->plane;
+   }
+}
+
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
-  struct drm_crtc *for_crtc,
+  const struct intel_crtc_state *cstate,
   const struct intel_wm_config *config,
-  const struct skl_pipe_wm_parameters *params,
   struct skl_ddb_entry *alloc /* out */)
 {
+   struct drm_crtc *for_crtc = cstate->base.crtc;
struct drm_crtc *crtc;
unsigned int pipe_size, ddb_size;
int nth_active_pipe;
 
-   if (!params->active) {
+   if (!cstate->base.active) {
alloc->start = 0;
alloc->end = 0;
return;
@@ -2832,19 +2847,29 @@ void skl_ddb_get_hw_state(struct drm_i915_private 
*dev_priv,
 }
 
 static unsigned int
-skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p, int y)
+skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
+const struct drm_plane_state *pstate,
+int y)
 {
+   struct 

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

[Jani Nikula]

Hi Dave -

i915 fixes for v4.3.

The revert dance could use some explanation: we had stuff fixed in
-next, and initially backported one commit to v4.3. Now, turns out we
need more fixes, and we could cherry-pick them all without conflicts if
we reverted the backported one first. So did that to not have to edit
and backport them all.

BR,
Jani.

The following changes since commit 25cb62b76430a91cc6195f902e61c2cb84ade622:

  Linux 4.3-rc5 (2015-10-11 11:09:45 -0700)

are available in the git repository at:

  git://anongit.freedesktop.org/drm-intel tags/drm-intel-fixes-2015-10-16

for you to fetch changes up to 18e9345b0db9fe7bd18c3c43967789fe0a2fdb52:

  drm/i915: Add primary plane to mask if it's visible (2015-10-14 13:47:44 
+0300)


Chris Wilson (2):
  drm/i915: Flush pipecontrol post-sync writes
  drm/i915: Deny wrapping an userptr into a framebuffer

Daniel Vetter (1):
  drm/i915: Fix kerneldoc for i915_gem_shrink_all

Jani Nikula (1):
  Revert "drm/i915: Add primary plane to mask if it's visible"

Maarten Lankhorst (1):
  drm/i915: Add primary plane to mask if it's visible

Ville Syrjälä (4):
  drm/i915: Restore lost DPLL register write on gen2-4
  drm/i915: Enable DPLL VGA mode before P1/P2 divider write
  drm/i915: Assign hwmode after encoder state readout
  drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc()

 drivers/gpu/drm/i915/i915_gem_shrinker.c |   2 +-
 drivers/gpu/drm/i915/i915_gem_userptr.c  |   5 +-
 drivers/gpu/drm/i915/intel_display.c | 120 +--
 drivers/gpu/drm/i915/intel_lrc.c |   1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c  |   2 +
 5 files changed, 75 insertions(+), 55 deletions(-)

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

Re: [Intel-gfx] [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Lukas Wunner]

Hi Ville,

On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote:
> On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> > From: Tvrtko Ursulin 
> > 
> > We had two failure modes here:
> > 
> > 1.
> > Deadlock in intelfb_alloc failure path where it calls
> > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> > (caller of intelfb_alloc) was already holding it.
> > 
> > 2.
> > Deadlock in intelfb_create failure path where it calls
> > drm_framebuffer_unreference, which grabs the struct mutex and
> > intelfb_create was already holding it.
> > 
> > v2:
> >* Reformat commit msg to 72 chars. (Lukas Wunner)
> >* Add third failure mode. (Lukas Wunner)
> > 
> > v3:
> >* On fb alloc failure, unref gem object where it gets refed,
> >  fix double unref in separate commit. (Ville Syrjälä)
> > 
> > v4:
> >* Lock struct mutex on unref. (Chris Wilson)
> > 
> > v5:
> >* Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC,
> >  rephrase commit message. (Jani Nicula)
> > 
> > Tested-by: Pierre Moreau 
> > [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> > Tested-by: Paul Hordiienko 
> > [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> > Tested-by: William Brown 
> > [MBP  8,2 2011  intel SNB + amd turks pre-retina]
> > Tested-by: Lukas Wunner 
> > [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> > Tested-by: Bruno Bierbaumer 
> > [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> > 
> > Signed-off-by: Tvrtko Ursulin 
> > Fixes: 60a5ca015ffd ("drm/i915: Add locking around
> > framebuffer_references--")
> > Reported-by: Lukas Wunner 
> > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]
> > Signed-off-by: Lukas Wunner 
> > Cc: Chris Wilson 
> > Cc: Ville Syrjälä 
> > Cc: Jani Nikula 
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 20 
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> > b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 96476d7..eee3306 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> >  {
> > struct intel_fbdev *ifbdev =
> > container_of(helper, struct intel_fbdev, helper);
> > -   struct drm_framebuffer *fb;
> > +   struct drm_framebuffer *fb = NULL;
> > struct drm_device *dev = helper->dev;
> > struct drm_mode_fb_cmd2 mode_cmd = {};
> > struct drm_i915_gem_object *obj;
> > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> >   sizes->surface_depth);
> >  
> > +   mutex_lock(>struct_mutex);
> > +
> > size = mode_cmd.pitches[0] * mode_cmd.height;
> > size = PAGE_ALIGN(size);
> > obj = i915_gem_object_create_stolen(dev, size);
> > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
> > if (ret) {
> > DRM_ERROR("failed to pin obj: %d\n", ret);
> > -   goto out_fb;
> > +   goto out_unref;
> > }
> >  
> > +   mutex_unlock(>struct_mutex);
> > +
> > ifbdev->fb = to_intel_framebuffer(fb);
> >  
> > return 0;
> >  
> > -out_fb:
> > -   drm_framebuffer_remove(fb);
> >  out_unref:
> > drm_gem_object_unreference(>base);
> 
> If fb init succeeded it took over the ref, no? So drm_framebuffer_remove()
> will now attempt to unref one too many times.
> 
> This taking over refs stuff is confusing. Maybe it would be better if
> everyone just took an extra ref when they stash the obj pointer
> somewhere, and everyone would then always release whatever ref they own
> and no longer need.
> 
> >  out:
> > +   mutex_unlock(>struct_mutex);
> > +   if (fb)
> > +   drm_framebuffer_remove(fb);
> > return ret;
> >  }
> >  

Hm, why do you think we unref one too many times?

A bit further up in this function we call __intel_framebuffer_create()
which sets the refcount to 1. (It calls intel_framebuffer_init(), which
calls drm_framebuffer_init(), which calls kref_init(>refcount).)

So if intel_pin_and_fence_fb_obj() fails, we do need to unreference and
tear down the fb. Thus, drm_framebuffer_remove() seems right here to me.

However, because of your objection I've noticed now that "if (fb)" seems
to be wrong, I think this should be "if (!IS_ERR_OR_NULL(fb))".

Because if __intel_framebuffer_create() failed, fb will be a PTR_ERR(),
so not null, and we'd call drm_framebuffer_remove() on this. Is 

Re: [Intel-gfx] [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 07:14:35PM +0200, Lukas Wunner wrote:
> Hi Ville,
> 
> On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote:
> > On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> > > From: Tvrtko Ursulin 
> > > 
> > > We had two failure modes here:
> > > 
> > > 1.
> > > Deadlock in intelfb_alloc failure path where it calls
> > > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> > > (caller of intelfb_alloc) was already holding it.
> > > 
> > > 2.
> > > Deadlock in intelfb_create failure path where it calls
> > > drm_framebuffer_unreference, which grabs the struct mutex and
> > > intelfb_create was already holding it.
> > > 
> > > v2:
> > >* Reformat commit msg to 72 chars. (Lukas Wunner)
> > >* Add third failure mode. (Lukas Wunner)
> > > 
> > > v3:
> > >* On fb alloc failure, unref gem object where it gets refed,
> > >  fix double unref in separate commit. (Ville Syrjälä)
> > > 
> > > v4:
> > >* Lock struct mutex on unref. (Chris Wilson)
> > > 
> > > v5:
> > >* Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC,
> > >  rephrase commit message. (Jani Nicula)
> > > 
> > > Tested-by: Pierre Moreau 
> > > [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> > > Tested-by: Paul Hordiienko 
> > > [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> > > Tested-by: William Brown 
> > > [MBP  8,2 2011  intel SNB + amd turks pre-retina]
> > > Tested-by: Lukas Wunner 
> > > [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> > > Tested-by: Bruno Bierbaumer 
> > > [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> > > 
> > > Signed-off-by: Tvrtko Ursulin 
> > > Fixes: 60a5ca015ffd ("drm/i915: Add locking around
> > > framebuffer_references--")
> > > Reported-by: Lukas Wunner 
> > > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]
> > > Signed-off-by: Lukas Wunner 
> > > Cc: Chris Wilson 
> > > Cc: Ville Syrjälä 
> > > Cc: Jani Nikula 
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 20 
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> > > b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index 96476d7..eee3306 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >  {
> > >   struct intel_fbdev *ifbdev =
> > >   container_of(helper, struct intel_fbdev, helper);
> > > - struct drm_framebuffer *fb;
> > > + struct drm_framebuffer *fb = NULL;
> > >   struct drm_device *dev = helper->dev;
> > >   struct drm_mode_fb_cmd2 mode_cmd = {};
> > >   struct drm_i915_gem_object *obj;
> > > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >   mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > > sizes->surface_depth);
> > >  
> > > + mutex_lock(>struct_mutex);
> > > +
> > >   size = mode_cmd.pitches[0] * mode_cmd.height;
> > >   size = PAGE_ALIGN(size);
> > >   obj = i915_gem_object_create_stolen(dev, size);
> > > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper 
> > > *helper,
> > >   ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
> > >   if (ret) {
> > >   DRM_ERROR("failed to pin obj: %d\n", ret);
> > > - goto out_fb;
> > > + goto out_unref;
> > >   }
> > >  
> > > + mutex_unlock(>struct_mutex);
> > > +
> > >   ifbdev->fb = to_intel_framebuffer(fb);
> > >  
> > >   return 0;
> > >  
> > > -out_fb:
> > > - drm_framebuffer_remove(fb);
> > >  out_unref:
> > >   drm_gem_object_unreference(>base);
> > 
> > If fb init succeeded it took over the ref, no? So drm_framebuffer_remove()
> > will now attempt to unref one too many times.
> > 
> > This taking over refs stuff is confusing. Maybe it would be better if
> > everyone just took an extra ref when they stash the obj pointer
> > somewhere, and everyone would then always release whatever ref they own
> > and no longer need.
> > 
> > >  out:
> > > + mutex_unlock(>struct_mutex);
> > > + if (fb)
> > > + drm_framebuffer_remove(fb);
> > >   return ret;
> > >  }
> > >  
> 
> Hm, why do you think we unref one too many times?

Because the fb now owns the reference, so it gets unreffed by the fb
.destroy() hook... I think.

> 
> A bit further up in this function we call __intel_framebuffer_create()
> which sets the refcount to 1. (It calls intel_framebuffer_init(), which
> calls drm_framebuffer_init(), which calls kref_init(>refcount).)
> 
> So if intel_pin_and_fence_fb_obj() fails, we do need to 

Re: [Intel-gfx] [PATCH] drm/i915: Kill the leftover RMW from ivb_sprite_disable()

[Daniel Vetter]

On Thu, Oct 15, 2015 at 05:04:04PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> We still had one lingering RMW in ivb_sprite_disable(), all the other
> RMWs were killed off from the sprite code some time ago. Kill the
> straggler too.
> 
> Signed-off-by: Ville Syrjälä 

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 7f0911e..a84cbf42 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -610,7 +610,7 @@ ivb_disable_plane(struct drm_plane *plane, struct 
> drm_crtc *crtc)
>   struct intel_plane *intel_plane = to_intel_plane(plane);
>   int pipe = intel_plane->pipe;
>  
> - I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
> + I915_WRITE(SPRCTL(pipe), 0);
>   /* Can't leave the scaler enabled... */
>   if (intel_plane->can_scale)
>   I915_WRITE(SPRSCALE(pipe), 0);
> -- 
> 2.4.9
> 
> ___
> 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 v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Daniel Vetter]

On Thu, Oct 15, 2015 at 07:14:35PM +0200, Lukas Wunner wrote:
> Hi Ville,
> 
> On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote:
> > On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> > > From: Tvrtko Ursulin 
> > > 
> > > We had two failure modes here:
> > > 
> > > 1.
> > > Deadlock in intelfb_alloc failure path where it calls
> > > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> > > (caller of intelfb_alloc) was already holding it.
> > > 
> > > 2.
> > > Deadlock in intelfb_create failure path where it calls
> > > drm_framebuffer_unreference, which grabs the struct mutex and
> > > intelfb_create was already holding it.
> > > 
> > > v2:
> > >* Reformat commit msg to 72 chars. (Lukas Wunner)
> > >* Add third failure mode. (Lukas Wunner)
> > > 
> > > v3:
> > >* On fb alloc failure, unref gem object where it gets refed,
> > >  fix double unref in separate commit. (Ville Syrjälä)
> > > 
> > > v4:
> > >* Lock struct mutex on unref. (Chris Wilson)
> > > 
> > > v5:
> > >* Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC,
> > >  rephrase commit message. (Jani Nicula)
> > > 
> > > Tested-by: Pierre Moreau 
> > > [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> > > Tested-by: Paul Hordiienko 
> > > [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> > > Tested-by: William Brown 
> > > [MBP  8,2 2011  intel SNB + amd turks pre-retina]
> > > Tested-by: Lukas Wunner 
> > > [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> > > Tested-by: Bruno Bierbaumer 
> > > [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> > > 
> > > Signed-off-by: Tvrtko Ursulin 
> > > Fixes: 60a5ca015ffd ("drm/i915: Add locking around
> > > framebuffer_references--")
> > > Reported-by: Lukas Wunner 
> > > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]
> > > Signed-off-by: Lukas Wunner 
> > > Cc: Chris Wilson 
> > > Cc: Ville Syrjälä 
> > > Cc: Jani Nikula 
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 20 
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> > > b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index 96476d7..eee3306 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >  {
> > >   struct intel_fbdev *ifbdev =
> > >   container_of(helper, struct intel_fbdev, helper);
> > > - struct drm_framebuffer *fb;
> > > + struct drm_framebuffer *fb = NULL;
> > >   struct drm_device *dev = helper->dev;
> > >   struct drm_mode_fb_cmd2 mode_cmd = {};
> > >   struct drm_i915_gem_object *obj;
> > > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >   mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > > sizes->surface_depth);
> > >  
> > > + mutex_lock(>struct_mutex);
> > > +
> > >   size = mode_cmd.pitches[0] * mode_cmd.height;
> > >   size = PAGE_ALIGN(size);
> > >   obj = i915_gem_object_create_stolen(dev, size);
> > > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper 
> > > *helper,
> > >   ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
> > >   if (ret) {
> > >   DRM_ERROR("failed to pin obj: %d\n", ret);
> > > - goto out_fb;
> > > + goto out_unref;
> > >   }
> > >  
> > > + mutex_unlock(>struct_mutex);
> > > +
> > >   ifbdev->fb = to_intel_framebuffer(fb);
> > >  
> > >   return 0;
> > >  
> > > -out_fb:
> > > - drm_framebuffer_remove(fb);
> > >  out_unref:
> > >   drm_gem_object_unreference(>base);
> > 
> > If fb init succeeded it took over the ref, no? So drm_framebuffer_remove()
> > will now attempt to unref one too many times.
> > 
> > This taking over refs stuff is confusing. Maybe it would be better if
> > everyone just took an extra ref when they stash the obj pointer
> > somewhere, and everyone would then always release whatever ref they own
> > and no longer need.
> > 
> > >  out:
> > > + mutex_unlock(>struct_mutex);
> > > + if (fb)
> > > + drm_framebuffer_remove(fb);
> > >   return ret;
> > >  }
> > >  
> 
> Hm, why do you think we unref one too many times?
> 
> A bit further up in this function we call __intel_framebuffer_create()
> which sets the refcount to 1. (It calls intel_framebuffer_init(), which
> calls drm_framebuffer_init(), which calls kref_init(>refcount).)
> 
> So if intel_pin_and_fence_fb_obj() fails, we do need to unreference and
> tear down the fb. Thus, drm_framebuffer_remove() seems right here to me.
> 
> However, 

[Intel-gfx] [PATCH v2 21/22] drm/i915: Rewrite fb rotation GTT handling

[ville . syrjala]

From: Ville Syrjälä 

Redo the fb rotation handling in order to:
- eliminate the NV12 special casing
- handle fb->offsets[] properly
- make the rotation handling reasier for the plane code

To achieve these goals we reduce intel_rotation_info to only contain
(for each plane) the rotated view width,height,stride in tile units,
and the page offset into the object where the plane starts. Each plane
is handled exactly the same way, no special casing for NV12 or other
formats. We then store the computed rotation_info under
intel_framebuffer so that we don't have to recompute it again.

To handle fb->offsets[] we treat them as a linear offsets and convert
them to x/y offsets from the start of the relevant GTT mapping (either
normal or rotated). We store the x/y offsets under intel_framebuffer,
and for some extra convenience we also store the rotated pitch (ie.
tile aligned plane height). So for each plane we have the normal
x/y offsets, rotated x/y offsets, and the rotated pitch. The normal
pitch is available already in fb->pitches[].

While we're gathering up all that extra information, we can also easily
compute the storage requirements for the framebuffer, so that we can
check that the object is big enough to hold it.

When it comes time to deal with the plane source coordinates, we first
rotate the clipped src coordinates to match the relevant GTT view
orientation, then add to them the fb x/y offsets. Next we compute
the aligned surface page offset, and as a result we're left with some
residual x/y offsets. Finally, if required by the hardware, we convert
the remaining x/y offsets into a linear offset.

For gen2/3 we simply skip computing the final page offset, and just
convert the src+fb x/y offsets directly into a linear offset since
that's what the hardware wants.

After this all platforms, incluing SKL+, compute these things in exactly
the same way (excluding alignemnt differences).

v2: Use BIT(DRM_ROTATE_270) instead of ROTATE_270 when rotating
plane src coordinates
Drop some spurious changes that got left behind during development

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_gem_gtt.c  |  66 ++
 drivers/gpu/drm/i915/i915_gem_gtt.h  |  14 +-
 drivers/gpu/drm/i915/intel_display.c | 448 ---
 drivers/gpu/drm/i915/intel_drv.h |  32 ++-
 drivers/gpu/drm/i915/intel_sprite.c  | 102 
 5 files changed, 402 insertions(+), 260 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index bb95b86..98aee6c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3255,11 +3255,6 @@ rotate_pages(const dma_addr_t *in, unsigned int offset,
unsigned int column, row;
unsigned int src_idx;
 
-   if (!sg) {
-   st->nents = 0;
-   sg = st->sgl;
-   }
-
for (column = 0; column < width; column++) {
src_idx = stride * (height - 1) + column;
for (row = 0; row < height; row++) {
@@ -3280,16 +3275,14 @@ rotate_pages(const dma_addr_t *in, unsigned int offset,
 }
 
 static struct sg_table *
-intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info,
+intel_rotate_fb_obj_pages(const struct intel_rotation_info *info,
  struct drm_i915_gem_object *obj)
 {
-   unsigned int size_pages = rot_info->size >> PAGE_SHIFT;
-   unsigned int size_pages_uv;
+   unsigned int size = intel_rotation_info_size(info);
struct sg_page_iter sg_iter;
unsigned long i;
dma_addr_t *page_addr_list;
struct sg_table *st;
-   unsigned int uv_start_page;
struct scatterlist *sg;
int ret = -ENOMEM;
 
@@ -3299,57 +3292,32 @@ intel_rotate_fb_obj_pages(struct intel_rotation_info 
*rot_info,
if (!page_addr_list)
return ERR_PTR(ret);
 
-   /* Account for UV plane with NV12. */
-   if (rot_info->pixel_format == DRM_FORMAT_NV12)
-   size_pages_uv = rot_info->size_uv >> PAGE_SHIFT;
-   else
-   size_pages_uv = 0;
-
/* Allocate target SG list. */
st = kmalloc(sizeof(*st), GFP_KERNEL);
if (!st)
goto err_st_alloc;
 
-   ret = sg_alloc_table(st, size_pages + size_pages_uv, GFP_KERNEL);
+   ret = sg_alloc_table(st, size, GFP_KERNEL);
if (ret)
goto err_sg_alloc;
 
/* Populate source page list from the object. */
i = 0;
for_each_sg_page(obj->pages->sgl, _iter, obj->pages->nents, 0) {
-   page_addr_list[i] = sg_page_iter_dma_address(_iter);
-   i++;
+   page_addr_list[i++] = sg_page_iter_dma_address(_iter);
}
 
-   /* Rotate the pages. */
-   sg = rotate_pages(page_addr_list, 0,
-rot_info->width_pages, rot_info->height_pages,
-