Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Tue, Mar 10, 2015 at 10:10:40AM -0700, Matt Roper wrote:
On Tue, Mar 10, 2015 at 01:15:25PM +0200, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
In preparation to movable/resizable primary planes pass the clipped
plane size to .update_primary_plane().
Personally I feel like it would make more sense to just completely kill
off .update_primary_plane() now rather than trying to evolve it. We
already have an intel_plane-update_plane() function pointer which is
never set or called for non-sprites at the moment. We could unify the
handling of low-level plane programming by just using that function
pointer for primary planes as well.
I want to kill it off as well, but that means either killing off
set_base_atomic() or making it use the plane commit hook. I suppose we
could hand craft a suitable plane state for it and just commit that
without any checks or anything?
I wouldn't mind also seeing the name of that low-level entrypoint
renamed to something like 'update_hw_plane' to avoid confusion with the
drm_plane entrypoint...
Matt
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 63
2 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index b16c0a7..e99eef0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -578,7 +578,8 @@ struct drm_i915_display_funcs {
uint32_t flags);
void (*update_primary_plane)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
-int x, int y);
+int x, int y,
+int crtc_w, int crtc_h);
void (*hpd_irq_setup)(struct drm_device *dev);
/* clock updates for mode set */
/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index fdc83f1..1a789f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2482,7 +2482,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2517,20 +2518,16 @@ static void i9xx_update_primary_plane(struct
drm_crtc *crtc,
if (INTEL_INFO(dev)-gen 4) {
if (intel_crtc-pipe == PIPE_B)
dspcntr |= DISPPLANE_SEL_PIPE_B;
-
- /* pipesrc and dspsize control the size that is scaled from,
-* which should always be the user's requested size.
-*/
- I915_WRITE(DSPSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) 16) | (crtc_w - 1));
I915_WRITE(DSPPOS(plane), 0);
} else if (IS_CHERRYVIEW(dev) plane == PLANE_B) {
- I915_WRITE(PRIMSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) 16) | (crtc_w -
1));
I915_WRITE(PRIMPOS(plane), 0);
I915_WRITE(PRIMCNSTALPHA(plane), 0);
+ } else {
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
}
switch (fb-pixel_format) {
@@ -2586,14 +2583,14 @@ static void i9xx_update_primary_plane(struct
drm_crtc *crtc,
if (crtc-primary-state-rotation == BIT(DRM_ROTATE_180)) {
dspcntr |= DISPPLANE_ROTATE_180;
- x += (intel_crtc-config-pipe_src_w - 1);
- y += (intel_crtc-config-pipe_src_h - 1);
+ x += (crtc_w - 1);
+ y += (crtc_h - 1);
/* Finding the last pixel of the last line of the display
data and adding to linear_offset*/
linear_offset +=
- (intel_crtc-config-pipe_src_h - 1) * fb-pitches[0] +
- (intel_crtc-config-pipe_src_w - 1) * pixel_size;
+ (crtc_h - 1) * fb-pitches[0] +
+ (crtc_w - 1) * pixel_size;
}
I915_WRITE(reg, dspcntr);
@@ -2611,7 +2608,8
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Tue, Mar 10, 2015 at 01:15:25PM +0200, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
In preparation to movable/resizable primary planes pass the clipped
plane size to .update_primary_plane().
Personally I feel like it would make more sense to just completely kill
off .update_primary_plane() now rather than trying to evolve it. We
already have an intel_plane-update_plane() function pointer which is
never set or called for non-sprites at the moment. We could unify the
handling of low-level plane programming by just using that function
pointer for primary planes as well.
I wouldn't mind also seeing the name of that low-level entrypoint
renamed to something like 'update_hw_plane' to avoid confusion with the
drm_plane entrypoint...
Matt
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 63
2 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b16c0a7..e99eef0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -578,7 +578,8 @@ struct drm_i915_display_funcs {
uint32_t flags);
void (*update_primary_plane)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y);
+ int x, int y,
+ int crtc_w, int crtc_h);
void (*hpd_irq_setup)(struct drm_device *dev);
/* clock updates for mode set */
/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index fdc83f1..1a789f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2482,7 +2482,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2517,20 +2518,16 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (INTEL_INFO(dev)-gen 4) {
if (intel_crtc-pipe == PIPE_B)
dspcntr |= DISPPLANE_SEL_PIPE_B;
-
- /* pipesrc and dspsize control the size that is scaled from,
- * which should always be the user's requested size.
- */
- I915_WRITE(DSPSIZE(plane),
-((intel_crtc-config-pipe_src_h - 1) 16) |
-(intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) 16) | (crtc_w - 1));
I915_WRITE(DSPPOS(plane), 0);
} else if (IS_CHERRYVIEW(dev) plane == PLANE_B) {
- I915_WRITE(PRIMSIZE(plane),
-((intel_crtc-config-pipe_src_h - 1) 16) |
-(intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) 16) | (crtc_w -
1));
I915_WRITE(PRIMPOS(plane), 0);
I915_WRITE(PRIMCNSTALPHA(plane), 0);
+ } else {
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
}
switch (fb-pixel_format) {
@@ -2586,14 +2583,14 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (crtc-primary-state-rotation == BIT(DRM_ROTATE_180)) {
dspcntr |= DISPPLANE_ROTATE_180;
- x += (intel_crtc-config-pipe_src_w - 1);
- y += (intel_crtc-config-pipe_src_h - 1);
+ x += (crtc_w - 1);
+ y += (crtc_h - 1);
/* Finding the last pixel of the last line of the display
data and adding to linear_offset*/
linear_offset +=
- (intel_crtc-config-pipe_src_h - 1) * fb-pitches[0] +
- (intel_crtc-config-pipe_src_w - 1) * pixel_size;
+ (crtc_h - 1) * fb-pitches[0] +
+ (crtc_w - 1) * pixel_size;
}
I915_WRITE(reg, dspcntr);
@@ -2611,7 +2608,8 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
static void ironlake_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Wed, Mar 11, 2015 at 10:39:31AM +0530, sonika wrote:
On Tuesday 10 March 2015 04:45 PM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
In preparation to movable/resizable primary planes pass the clipped
plane size to .update_primary_plane().
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 63
2 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index b16c0a7..e99eef0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -578,7 +578,8 @@ struct drm_i915_display_funcs {
uint32_t flags);
void (*update_primary_plane)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
-int x, int y);
+int x, int y,
+int crtc_w, int crtc_h);
void (*hpd_irq_setup)(struct drm_device *dev);
/* clock updates for mode set */
/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index fdc83f1..1a789f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2482,7 +2482,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2517,20 +2518,16 @@ static void i9xx_update_primary_plane(struct
drm_crtc *crtc,
if (INTEL_INFO(dev)-gen 4) {
if (intel_crtc-pipe == PIPE_B)
dspcntr |= DISPPLANE_SEL_PIPE_B;
-
- /* pipesrc and dspsize control the size that is scaled from,
-* which should always be the user's requested size.
-*/
- I915_WRITE(DSPSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) 16) | (crtc_w - 1));
I915_WRITE(DSPPOS(plane), 0);
} else if (IS_CHERRYVIEW(dev) plane == PLANE_B) {
- I915_WRITE(PRIMSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) 16) | (crtc_w -
1));
I915_WRITE(PRIMPOS(plane), 0);
I915_WRITE(PRIMCNSTALPHA(plane), 0);
+ } else {
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
}
switch (fb-pixel_format) {
@@ -2586,14 +2583,14 @@ static void i9xx_update_primary_plane(struct
drm_crtc *crtc,
if (crtc-primary-state-rotation == BIT(DRM_ROTATE_180)) {
dspcntr |= DISPPLANE_ROTATE_180;
- x += (intel_crtc-config-pipe_src_w - 1);
- y += (intel_crtc-config-pipe_src_h - 1);
+ x += (crtc_w - 1);
+ y += (crtc_h - 1);
/* Finding the last pixel of the last line of the display
data and adding to linear_offset*/
linear_offset +=
- (intel_crtc-config-pipe_src_h - 1) * fb-pitches[0] +
- (intel_crtc-config-pipe_src_w - 1) * pixel_size;
+ (crtc_h - 1) * fb-pitches[0] +
+ (crtc_w - 1) * pixel_size;
}
I915_WRITE(reg, dspcntr);
@@ -2611,7 +2608,8 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
static void ironlake_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2643,6 +2641,10 @@ static void ironlake_update_primary_plane(struct
drm_crtc *crtc,
if (IS_HASWELL(dev) || IS_BROADWELL(dev))
dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Wednesday 11 March 2015 02:57 PM, Ville Syrjälä wrote:
On Wed, Mar 11, 2015 at 10:39:31AM +0530, sonika wrote:
On Tuesday 10 March 2015 04:45 PM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
In preparation to movable/resizable primary planes pass the clipped
plane size to .update_primary_plane().
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 63
2 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b16c0a7..e99eef0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -578,7 +578,8 @@ struct drm_i915_display_funcs {
uint32_t flags);
void (*update_primary_plane)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
-int x, int y);
+int x, int y,
+int crtc_w, int crtc_h);
void (*hpd_irq_setup)(struct drm_device *dev);
/* clock updates for mode set */
/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index fdc83f1..1a789f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2482,7 +2482,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2517,20 +2518,16 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (INTEL_INFO(dev)-gen 4) {
if (intel_crtc-pipe == PIPE_B)
dspcntr |= DISPPLANE_SEL_PIPE_B;
-
- /* pipesrc and dspsize control the size that is scaled from,
-* which should always be the user's requested size.
-*/
- I915_WRITE(DSPSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) 16) | (crtc_w - 1));
I915_WRITE(DSPPOS(plane), 0);
} else if (IS_CHERRYVIEW(dev) plane == PLANE_B) {
- I915_WRITE(PRIMSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) 16) | (crtc_w -
1));
I915_WRITE(PRIMPOS(plane), 0);
I915_WRITE(PRIMCNSTALPHA(plane), 0);
+ } else {
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
}
switch (fb-pixel_format) {
@@ -2586,14 +2583,14 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (crtc-primary-state-rotation == BIT(DRM_ROTATE_180)) {
dspcntr |= DISPPLANE_ROTATE_180;
- x += (intel_crtc-config-pipe_src_w - 1);
- y += (intel_crtc-config-pipe_src_h - 1);
+ x += (crtc_w - 1);
+ y += (crtc_h - 1);
/* Finding the last pixel of the last line of the display
data and adding to linear_offset*/
linear_offset +=
- (intel_crtc-config-pipe_src_h - 1) * fb-pitches[0] +
- (intel_crtc-config-pipe_src_w - 1) * pixel_size;
+ (crtc_h - 1) * fb-pitches[0] +
+ (crtc_w - 1) * pixel_size;
}
I915_WRITE(reg, dspcntr);
@@ -2611,7 +2608,8 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
static void ironlake_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2643,6 +2641,10 @@ static void ironlake_update_primary_plane(struct
drm_crtc *crtc,
if (IS_HASWELL(dev) || IS_BROADWELL(dev))
dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
+ WARN_ONCE(crtc_w !=
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Tue, Mar 10, 2015 at 01:57:09PM -0700, Matt Roper wrote: On Tue, Mar 10, 2015 at 07:59:15PM +0200, Ville Syrjälä wrote: On Tue, Mar 10, 2015 at 10:10:40AM -0700, Matt Roper wrote: On Tue, Mar 10, 2015 at 01:15:25PM +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com In preparation to movable/resizable primary planes pass the clipped plane size to .update_primary_plane(). Personally I feel like it would make more sense to just completely kill off .update_primary_plane() now rather than trying to evolve it. We already have an intel_plane-update_plane() function pointer which is never set or called for non-sprites at the moment. We could unify the handling of low-level plane programming by just using that function pointer for primary planes as well. I want to kill it off as well, but that means either killing off set_base_atomic() or making it use the plane commit hook. I suppose we could hand craft a suitable plane state for it and just commit that without any checks or anything? I'm not sure I follow your concern about set_base_atomic(). After your patch series, it'll be calling dev_priv-display.update_primary_plane(crtc, fb, x, y, 0, 0, intel_crtc-config-pipe_src_w, intel_crtc-config-pipe_src_h); which basically directly programs the hardware for the primary plane and doesn't do anything state-related. It seems to me that just making that low-level call be: intel_plane = to_intel_plane(crtc-primary); intel_state = to_intel_plane_state(crtc-primary-state); intel_plane-update_plane(crtc-primary, crtc, fb, intel_fb_obj(fb), 0, 0, intel_crtc-config-pipe_src_w, intel_crtc-config-pipe_src_h, x, y, drm_rect_width(intel_state-src), drm_rect_height(intel_state-src)); We can't really use the current plane state here. It might not match what we want. Although as I indicated with one FIXME in these patches, set_base_atomic() will do something bad anyway if the fbdev framebuffer is too small for the current pipe source size. I suppose we could try to enable the panel fitter to avoid that, but then we run into problems with managing the panel fitter which is a scarce resource (and there is only one on gmch platforms and potentially with extra restrictions on which pipes can use it). Maybe we should just sanity check that the fbdev framebuffer is suitably large for the current mode/pfit settings, and do nothing if it's not? Or we could try to use a plane that can be resized (or potentially even scaled) to preset the fbdev framebuffer. The other option would be to throw out set_base_atomic() entirely. I have no idea if it works at all currently. And, as you've mentioned .update_plane(), I'm actually thinking we'll be wanting to pass just a plane state there, and throw out most of the function arguments as all that data should be in the plane state already. And then we'd probably want to hand craft the plane state we pass into .update_plane() for set_base_atomic() (assuming we'll keep it at all) so that we don't leak fb references and whatnot. But all of that is material for another set of patches. shouldn't make a difference; the implementation of intel_plane-update_plane() would still be the same low-level register programming without any state manipulation, the only difference being that we get an extra pair of parameters containing source rect width/height. This would also allow us to point both primary and sprite planes at the same function on SKL, which has two nearly-identical functions at the moment for the lowest-level plane hardware programming. As I've noted in my reply to Sonika, the primary_enabled flag is the main complication here. We really need to sort that out before can fully unify this stuff. Matt I wouldn't mind also seeing the name of that low-level entrypoint renamed to something like 'update_hw_plane' to avoid confusion with the drm_plane entrypoint... Matt Cc: Sonika Jindal sonika.jin...@intel.com Signed-off-by: Ville
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Tue, Mar 10, 2015 at 07:59:15PM +0200, Ville Syrjälä wrote: On Tue, Mar 10, 2015 at 10:10:40AM -0700, Matt Roper wrote: On Tue, Mar 10, 2015 at 01:15:25PM +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com In preparation to movable/resizable primary planes pass the clipped plane size to .update_primary_plane(). Personally I feel like it would make more sense to just completely kill off .update_primary_plane() now rather than trying to evolve it. We already have an intel_plane-update_plane() function pointer which is never set or called for non-sprites at the moment. We could unify the handling of low-level plane programming by just using that function pointer for primary planes as well. I want to kill it off as well, but that means either killing off set_base_atomic() or making it use the plane commit hook. I suppose we could hand craft a suitable plane state for it and just commit that without any checks or anything? set_base_atomic is bonghits imo, I think we should just replace it with the set_base helper for the transitional helpers. set_base_atomic can't grab locks and assumes that the buffer is pinned already. Hm, so maybe a special version of the plane helper which forgoes the prepare/celanup_fb? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
From: Ville Syrjälä ville.syrj...@linux.intel.com
In preparation to movable/resizable primary planes pass the clipped
plane size to .update_primary_plane().
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 63
2 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b16c0a7..e99eef0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -578,7 +578,8 @@ struct drm_i915_display_funcs {
uint32_t flags);
void (*update_primary_plane)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
-int x, int y);
+int x, int y,
+int crtc_w, int crtc_h);
void (*hpd_irq_setup)(struct drm_device *dev);
/* clock updates for mode set */
/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index fdc83f1..1a789f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2482,7 +2482,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2517,20 +2518,16 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (INTEL_INFO(dev)-gen 4) {
if (intel_crtc-pipe == PIPE_B)
dspcntr |= DISPPLANE_SEL_PIPE_B;
-
- /* pipesrc and dspsize control the size that is scaled from,
-* which should always be the user's requested size.
-*/
- I915_WRITE(DSPSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) 16) | (crtc_w - 1));
I915_WRITE(DSPPOS(plane), 0);
} else if (IS_CHERRYVIEW(dev) plane == PLANE_B) {
- I915_WRITE(PRIMSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) 16) | (crtc_w -
1));
I915_WRITE(PRIMPOS(plane), 0);
I915_WRITE(PRIMCNSTALPHA(plane), 0);
+ } else {
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
}
switch (fb-pixel_format) {
@@ -2586,14 +2583,14 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (crtc-primary-state-rotation == BIT(DRM_ROTATE_180)) {
dspcntr |= DISPPLANE_ROTATE_180;
- x += (intel_crtc-config-pipe_src_w - 1);
- y += (intel_crtc-config-pipe_src_h - 1);
+ x += (crtc_w - 1);
+ y += (crtc_h - 1);
/* Finding the last pixel of the last line of the display
data and adding to linear_offset*/
linear_offset +=
- (intel_crtc-config-pipe_src_h - 1) * fb-pitches[0] +
- (intel_crtc-config-pipe_src_w - 1) * pixel_size;
+ (crtc_h - 1) * fb-pitches[0] +
+ (crtc_w - 1) * pixel_size;
}
I915_WRITE(reg, dspcntr);
@@ -2611,7 +2608,8 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
static void ironlake_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2643,6 +2641,10 @@ static void ironlake_update_primary_plane(struct
drm_crtc *crtc,
if (IS_HASWELL(dev) || IS_BROADWELL(dev))
dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
+
switch (fb-pixel_format) {
case
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Tuesday 10 March 2015 04:45 PM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
In preparation to movable/resizable primary planes pass the clipped
plane size to .update_primary_plane().
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 63
2 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b16c0a7..e99eef0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -578,7 +578,8 @@ struct drm_i915_display_funcs {
uint32_t flags);
void (*update_primary_plane)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
-int x, int y);
+int x, int y,
+int crtc_w, int crtc_h);
void (*hpd_irq_setup)(struct drm_device *dev);
/* clock updates for mode set */
/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index fdc83f1..1a789f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2482,7 +2482,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2517,20 +2518,16 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (INTEL_INFO(dev)-gen 4) {
if (intel_crtc-pipe == PIPE_B)
dspcntr |= DISPPLANE_SEL_PIPE_B;
-
- /* pipesrc and dspsize control the size that is scaled from,
-* which should always be the user's requested size.
-*/
- I915_WRITE(DSPSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) 16) | (crtc_w - 1));
I915_WRITE(DSPPOS(plane), 0);
} else if (IS_CHERRYVIEW(dev) plane == PLANE_B) {
- I915_WRITE(PRIMSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) 16) | (crtc_w -
1));
I915_WRITE(PRIMPOS(plane), 0);
I915_WRITE(PRIMCNSTALPHA(plane), 0);
+ } else {
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
}
switch (fb-pixel_format) {
@@ -2586,14 +2583,14 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (crtc-primary-state-rotation == BIT(DRM_ROTATE_180)) {
dspcntr |= DISPPLANE_ROTATE_180;
- x += (intel_crtc-config-pipe_src_w - 1);
- y += (intel_crtc-config-pipe_src_h - 1);
+ x += (crtc_w - 1);
+ y += (crtc_h - 1);
/* Finding the last pixel of the last line of the display
data and adding to linear_offset*/
linear_offset +=
- (intel_crtc-config-pipe_src_h - 1) * fb-pitches[0] +
- (intel_crtc-config-pipe_src_w - 1) * pixel_size;
+ (crtc_h - 1) * fb-pitches[0] +
+ (crtc_w - 1) * pixel_size;
}
I915_WRITE(reg, dspcntr);
@@ -2611,7 +2608,8 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
static void ironlake_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2643,6 +2641,10 @@ static void ironlake_update_primary_plane(struct
drm_crtc *crtc,
if (IS_HASWELL(dev) || IS_BROADWELL(dev))
dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
+
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Tue, Mar 10, 2015 at 07:59:15PM +0200, Ville Syrjälä wrote:
On Tue, Mar 10, 2015 at 10:10:40AM -0700, Matt Roper wrote:
On Tue, Mar 10, 2015 at 01:15:25PM +0200, ville.syrj...@linux.intel.com
wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
In preparation to movable/resizable primary planes pass the clipped
plane size to .update_primary_plane().
Personally I feel like it would make more sense to just completely kill
off .update_primary_plane() now rather than trying to evolve it. We
already have an intel_plane-update_plane() function pointer which is
never set or called for non-sprites at the moment. We could unify the
handling of low-level plane programming by just using that function
pointer for primary planes as well.
I want to kill it off as well, but that means either killing off
set_base_atomic() or making it use the plane commit hook. I suppose we
could hand craft a suitable plane state for it and just commit that
without any checks or anything?
I'm not sure I follow your concern about set_base_atomic(). After your
patch series, it'll be calling
dev_priv-display.update_primary_plane(crtc, fb, x, y,
0, 0,
intel_crtc-config-pipe_src_w,
intel_crtc-config-pipe_src_h);
which basically directly programs the hardware for the primary plane and
doesn't do anything state-related.
It seems to me that just making that low-level call be:
intel_plane = to_intel_plane(crtc-primary);
intel_state = to_intel_plane_state(crtc-primary-state);
intel_plane-update_plane(crtc-primary, crtc, fb, intel_fb_obj(fb),
0, 0,
intel_crtc-config-pipe_src_w,
intel_crtc-config-pipe_src_h,
x, y,
drm_rect_width(intel_state-src),
drm_rect_height(intel_state-src));
shouldn't make a difference; the implementation of
intel_plane-update_plane() would still be the same low-level register
programming without any state manipulation, the only difference being
that we get an extra pair of parameters containing source rect
width/height.
This would also allow us to point both primary and sprite planes at the
same function on SKL, which has two nearly-identical functions at the
moment for the lowest-level plane hardware programming.
Matt
I wouldn't mind also seeing the name of that low-level entrypoint
renamed to something like 'update_hw_plane' to avoid confusion with the
drm_plane entrypoint...
Matt
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 63
2 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index b16c0a7..e99eef0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -578,7 +578,8 @@ struct drm_i915_display_funcs {
uint32_t flags);
void (*update_primary_plane)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y);
+ int x, int y,
+ int crtc_w, int crtc_h);
void (*hpd_irq_setup)(struct drm_device *dev);
/* clock updates for mode set */
/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index fdc83f1..1a789f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2482,7 +2482,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2517,20 +2518,16 @@ static
