[Intel-gfx] [PATCH v2 09/16] drm/i915: Actually perform the watermark update in two phases
From: Ville Syrjälä
Switch the code over to using the two phase watermark update. The steps
generally follow this pattern:
1. Calculate new plane parameters for changed planes
2. Calculate new target and intermediate watermarks
3. Check that both the target and intermediate watermarks are valid
4. Program the hardware with the intermediate watermarks
5. Program the plane registers
6. Arm the vblank watermark update machinery for the next vblank
7. Program the hardware with the target watermarks (after vblank)
v2: Rebased, pass intel_crtc around, s/intel_crtc/crtc/
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/i915_drv.h | 14 ++-
drivers/gpu/drm/i915/intel_display.c | 58 -
drivers/gpu/drm/i915/intel_drv.h | 35 --
drivers/gpu/drm/i915/intel_pm.c | 229 +++
drivers/gpu/drm/i915/intel_sprite.c | 119 --
5 files changed, 347 insertions(+), 108 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d4f8ae8..5b1404e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -405,9 +405,12 @@ struct intel_connector;
struct intel_crtc_config;
struct intel_plane_config;
struct intel_crtc;
+struct intel_plane;
struct intel_limit;
struct dpll;
struct intel_crtc_wm_config;
+struct intel_plane_wm_parameters;
+struct intel_crtc_wm_config;
struct drm_i915_display_funcs {
bool (*fbc_enabled)(struct drm_device *dev);
@@ -434,10 +437,13 @@ struct drm_i915_display_funcs {
struct dpll *match_clock,
struct dpll *best_clock);
void (*update_wm)(struct drm_crtc *crtc);
- void (*update_sprite_wm)(struct drm_plane *plane,
-struct drm_crtc *crtc,
-uint32_t sprite_width, int pixel_size,
-bool enable, bool scaled);
+ int (*update_primary_wm)(struct intel_crtc *crtc,
+struct intel_crtc_wm_config *config);
+ int (*update_cursor_wm)(struct intel_crtc *crtc,
+ struct intel_crtc_wm_config *config);
+ int (*update_sprite_wm)(struct intel_plane *plane,
+ struct intel_crtc *crtc,
+ struct intel_crtc_wm_config *config);
void (*program_wm_pre)(struct intel_crtc *crtc,
const struct intel_crtc_wm_config *config);
void (*program_wm_post)(struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 408b238..5bf1633 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2078,6 +2078,19 @@ void intel_flush_primary_plane(struct drm_i915_private
*dev_priv,
POSTING_READ(reg);
}
+static void update_pri_params(struct intel_crtc *crtc,
+ struct intel_plane_wm_parameters *params,
+ bool primary_enabled)
+{
+ if (!crtc->active || !primary_enabled)
+ return;
+
+ params->horiz_pixels = crtc->config.pipe_src_w;
+ params->bytes_per_pixel =
+ drm_format_plane_cpp(crtc->base.primary->fb->pixel_format, 0);
+ params->enabled = true;
+}
+
/**
* intel_enable_primary_hw_plane - enable the primary plane on a given pipe
* @dev_priv: i915 private structure
@@ -2091,6 +2104,8 @@ static void intel_enable_primary_hw_plane(struct
drm_i915_private *dev_priv,
{
struct intel_crtc *intel_crtc =
to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+ struct intel_crtc_wm_config config = {};
+ int ret;
int reg;
u32 val;
@@ -2100,14 +2115,24 @@ static void intel_enable_primary_hw_plane(struct
drm_i915_private *dev_priv,
if (intel_crtc->primary_enabled)
return;
+ update_pri_params(intel_crtc, &config.pri, true);
+
+ ret = intel_update_primary_watermarks(intel_crtc, &config);
+ WARN(ret, "primary watermarks invalid\n");
+
intel_crtc->primary_enabled = true;
reg = DSPCNTR(plane);
val = I915_READ(reg);
WARN_ON(val & DISPLAY_PLANE_ENABLE);
+ intel_crtc->pri_wm = config.pri;
+ intel_program_watermarks_pre(intel_crtc, &config);
+
I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
intel_flush_primary_plane(dev_priv, plane);
+
+ intel_program_watermarks_post(intel_crtc, &config);
}
/**
@@ -2123,20 +2148,32 @@ static void intel_disable_primary_hw_plane(struct
drm_i915_private *dev_priv,
{
struct intel_crtc *intel_crtc =
to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+ struct intel_crtc_wm_config config = {};
+ int ret;
int reg;
u32 val;
if (!intel_crtc->primary_enabled)
return;
+ update_pri_
Re: [Intel-gfx] [PATCH v2 09/16] drm/i915: Actually perform the watermark update in two phases
2014-05-22 11:48 GMT-03:00 :
> From: Ville Syrjälä
>
> Switch the code over to using the two phase watermark update. The steps
> generally follow this pattern:
>
> 1. Calculate new plane parameters for changed planes
> 2. Calculate new target and intermediate watermarks
> 3. Check that both the target and intermediate watermarks are valid
> 4. Program the hardware with the intermediate watermarks
> 5. Program the plane registers
> 6. Arm the vblank watermark update machinery for the next vblank
> 7. Program the hardware with the target watermarks (after vblank)
>
> v2: Rebased, pass intel_crtc around, s/intel_crtc/crtc/
>
> Signed-off-by: Ville Syrjälä
This patch is huge. If we bisect any regression to it, we will be
completely lost and hopeless. Also, my tiny little brain doesn't have
enough power to do a proper review of this patch with so many changes
happening all at the same place. I tried, but I gave up in the middle.
Please try to convert this patch into many smaller patches. I don't
know if the following idea is actually possible, but it is what I
could extract from what I read of your patch:
- I'd start with the way you update watermarks parameters. Start by
patching ilk_compute_wm_parameters() and making it directly call your
new update_xxx_params() functions. You can even do separate patches
for _pri, _cur and _spr patches since it seems the _spr code is
different for most of your patch due to the old
intel_update_sprite_watermarks() function.
- Then you change the way you use to set your parameters (there's a
comment below mentioning the specific line which I'm talking about
here)
- Then you can patch intel_display.c so you will only update the WM
params when you actually change the HW (the _hw_plane and
update_cursor functions). Again, you can even split _pri, _cur and
_spr on separate patches.
- Then you can introduce the update_cursor_wm() and
update_primary_wm() functions, but still make them call the old-style
intel_update_watermarks() or something similar.
- You can also add the functions to deal with intermediate watermarks,
but without using them. Or you could, for example, make the
intermediate watermarks just be the same as the "old" ones, so the
only real update will be the final one (which, I believe, will mean
the code still behaves as it does today, no regressions).
- Then you can add the final piece of the code that uses all the new
infrastructure to actually generate and use the intermediate
watermarks. And this would be a much smaller patch.
There are still some comments below:
> ---
> drivers/gpu/drm/i915/i915_drv.h | 14 ++-
> drivers/gpu/drm/i915/intel_display.c | 58 -
> drivers/gpu/drm/i915/intel_drv.h | 35 --
> drivers/gpu/drm/i915/intel_pm.c | 229
> +++
> drivers/gpu/drm/i915/intel_sprite.c | 119 --
> 5 files changed, 347 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d4f8ae8..5b1404e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -405,9 +405,12 @@ struct intel_connector;
> struct intel_crtc_config;
> struct intel_plane_config;
> struct intel_crtc;
> +struct intel_plane;
> struct intel_limit;
> struct dpll;
> struct intel_crtc_wm_config;
> +struct intel_plane_wm_parameters;
> +struct intel_crtc_wm_config;
>
> struct drm_i915_display_funcs {
> bool (*fbc_enabled)(struct drm_device *dev);
> @@ -434,10 +437,13 @@ struct drm_i915_display_funcs {
> struct dpll *match_clock,
> struct dpll *best_clock);
> void (*update_wm)(struct drm_crtc *crtc);
> - void (*update_sprite_wm)(struct drm_plane *plane,
> -struct drm_crtc *crtc,
> -uint32_t sprite_width, int pixel_size,
> -bool enable, bool scaled);
> + int (*update_primary_wm)(struct intel_crtc *crtc,
> +struct intel_crtc_wm_config *config);
> + int (*update_cursor_wm)(struct intel_crtc *crtc,
> + struct intel_crtc_wm_config *config);
> + int (*update_sprite_wm)(struct intel_plane *plane,
> + struct intel_crtc *crtc,
> + struct intel_crtc_wm_config *config);
> void (*program_wm_pre)(struct intel_crtc *crtc,
>const struct intel_crtc_wm_config *config);
> void (*program_wm_post)(struct intel_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 408b238..5bf1633 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2078,6 +2078,19 @@ void intel_flush_primary_plane(struct drm_i915_private
> *dev_priv,
> POSTING_READ(reg);
> }
>
> +static void update_pri
Re: [Intel-gfx] [PATCH v2 09/16] drm/i915: Actually perform the watermark update in two phases
On Tue, Jun 03, 2014 at 07:47:11PM -0300, Paulo Zanoni wrote:
> 2014-05-22 11:48 GMT-03:00 :
> > From: Ville Syrjälä
> >
> > Switch the code over to using the two phase watermark update. The steps
> > generally follow this pattern:
> >
> > 1. Calculate new plane parameters for changed planes
> > 2. Calculate new target and intermediate watermarks
> > 3. Check that both the target and intermediate watermarks are valid
> > 4. Program the hardware with the intermediate watermarks
> > 5. Program the plane registers
> > 6. Arm the vblank watermark update machinery for the next vblank
> > 7. Program the hardware with the target watermarks (after vblank)
> >
> > v2: Rebased, pass intel_crtc around, s/intel_crtc/crtc/
> >
> > Signed-off-by: Ville Syrjälä
>
> This patch is huge. If we bisect any regression to it, we will be
> completely lost and hopeless. Also, my tiny little brain doesn't have
> enough power to do a proper review of this patch with so many changes
> happening all at the same place. I tried, but I gave up in the middle.
>
> Please try to convert this patch into many smaller patches. I don't
> know if the following idea is actually possible, but it is what I
> could extract from what I read of your patch:
> - I'd start with the way you update watermarks parameters. Start by
> patching ilk_compute_wm_parameters() and making it directly call your
> new update_xxx_params() functions. You can even do separate patches
> for _pri, _cur and _spr patches since it seems the _spr code is
> different for most of your patch due to the old
> intel_update_sprite_watermarks() function.
> - Then you change the way you use to set your parameters (there's a
> comment below mentioning the specific line which I'm talking about
> here)
> - Then you can patch intel_display.c so you will only update the WM
> params when you actually change the HW (the _hw_plane and
> update_cursor functions). Again, you can even split _pri, _cur and
> _spr on separate patches.
> - Then you can introduce the update_cursor_wm() and
> update_primary_wm() functions, but still make them call the old-style
> intel_update_watermarks() or something similar.
> - You can also add the functions to deal with intermediate watermarks,
> but without using them. Or you could, for example, make the
> intermediate watermarks just be the same as the "old" ones, so the
> only real update will be the final one (which, I believe, will mean
> the code still behaves as it does today, no regressions).
> - Then you can add the final piece of the code that uses all the new
> infrastructure to actually generate and use the intermediate
> watermarks. And this would be a much smaller patch.
I guess I can try to chunk it up more. But in that case I'll have to
make all the intermediate stages as impotent as possible since no one
will be testing them. So I fear it's going to be more patches that
essentially do nothing.
>
> There are still some comments below:
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 14 ++-
> > drivers/gpu/drm/i915/intel_display.c | 58 -
> > drivers/gpu/drm/i915/intel_drv.h | 35 --
> > drivers/gpu/drm/i915/intel_pm.c | 229
> > +++
> > drivers/gpu/drm/i915/intel_sprite.c | 119 --
> > 5 files changed, 347 insertions(+), 108 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index d4f8ae8..5b1404e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -405,9 +405,12 @@ struct intel_connector;
> > struct intel_crtc_config;
> > struct intel_plane_config;
> > struct intel_crtc;
> > +struct intel_plane;
> > struct intel_limit;
> > struct dpll;
> > struct intel_crtc_wm_config;
> > +struct intel_plane_wm_parameters;
> > +struct intel_crtc_wm_config;
> >
> > struct drm_i915_display_funcs {
> > bool (*fbc_enabled)(struct drm_device *dev);
> > @@ -434,10 +437,13 @@ struct drm_i915_display_funcs {
> > struct dpll *match_clock,
> > struct dpll *best_clock);
> > void (*update_wm)(struct drm_crtc *crtc);
> > - void (*update_sprite_wm)(struct drm_plane *plane,
> > -struct drm_crtc *crtc,
> > -uint32_t sprite_width, int pixel_size,
> > -bool enable, bool scaled);
> > + int (*update_primary_wm)(struct intel_crtc *crtc,
> > +struct intel_crtc_wm_config *config);
> > + int (*update_cursor_wm)(struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config);
> > + int (*update_sprite_wm)(struct intel_plane *plane,
> > + struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config);
> > void (*program_wm_pre)(struct intel_crtc *crtc,
> >
