Re: [Intel-gfx] [PATCH 7/8] drm/i915: Move programming plane scaler to its own function.

2018-09-21 Thread Ville Syrjälä 
On Thu, Sep 20, 2018 at 12:27:10PM +0200, Maarten Lankhorst wrote:
> This cleans the code up slightly, and will make other changes easier.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 90 +
>  1 file changed, 52 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index d4c8e10fc90b..7d3c7469d271 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -280,13 +280,63 @@ skl_plane_max_stride(struct intel_plane *plane,
>   return min(8192 * cpp, 32768);
>  }
>  
> +static void
> +skl_program_scaler(struct drm_i915_private *dev_priv,
> +struct intel_plane *plane,

Could have dug these out from the states rather than passing in
explicitly. I prefer minimlism when it comes to function arguments.

> +const struct intel_crtc_state *crtc_state,
> +const struct intel_plane_state *plane_state)
> +{
> + enum plane_id plane_id = plane->id;
> + enum pipe pipe = plane->pipe;
> + int scaler_id = plane_state->scaler_id;
> + const struct intel_scaler *scaler =
> + _state->scaler_state.scalers[scaler_id];
> + int crtc_x = plane_state->base.dst.x1;
> + int crtc_y = plane_state->base.dst.y1;
> + uint32_t crtc_w = drm_rect_width(_state->base.dst);
> + uint32_t crtc_h = drm_rect_height(_state->base.dst);
> + u16 y_hphase, uv_rgb_hphase;
> + u16 y_vphase, uv_rgb_vphase;
> +
> + /* Sizes are 0 based */
> + crtc_w--;
> + crtc_h--;
> +
> + /* TODO: handle sub-pixel coordinates */
> + if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
> + y_hphase = skl_scaler_calc_phase(1, false);
> + y_vphase = skl_scaler_calc_phase(1, false);
> +
> + /* MPEG2 chroma siting convention */
> + uv_rgb_hphase = skl_scaler_calc_phase(2, true);
> + uv_rgb_vphase = skl_scaler_calc_phase(2, false);
> + } else {
> + /* not used */
> + y_hphase = 0;
> + y_vphase = 0;
> +
> + uv_rgb_hphase = skl_scaler_calc_phase(1, false);
> + uv_rgb_vphase = skl_scaler_calc_phase(1, false);
> + }
> +
> + I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
> +   PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
> + I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
> + I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
> +   PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> + I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
> +   PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> + I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
> + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
> +   ((crtc_w + 1) << 16)|(crtc_h + 1));
> +}
> +
>  void
>  skl_update_plane(struct intel_plane *plane,
>const struct intel_crtc_state *crtc_state,
>const struct intel_plane_state *plane_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> - const struct drm_framebuffer *fb = plane_state->base.fb;
>   enum plane_id plane_id = plane->id;
>   enum pipe pipe = plane->pipe;
>   u32 plane_ctl = plane_state->ctl;
> @@ -296,8 +346,6 @@ skl_update_plane(struct intel_plane *plane,
>   u32 aux_stride = skl_plane_stride(plane_state, 1);
>   int crtc_x = plane_state->base.dst.x1;
>   int crtc_y = plane_state->base.dst.y1;
> - uint32_t crtc_w = drm_rect_width(_state->base.dst);
> - uint32_t crtc_h = drm_rect_height(_state->base.dst);
>   uint32_t x = plane_state->color_plane[0].x;
>   uint32_t y = plane_state->color_plane[0].y;
>   uint32_t src_w = drm_rect_width(_state->base.src) >> 16;
> @@ -307,8 +355,6 @@ skl_update_plane(struct intel_plane *plane,
>   /* Sizes are 0 based */
>   src_w--;
>   src_h--;
> - crtc_w--;
> - crtc_h--;
>  
>   spin_lock_irqsave(_priv->uncore.lock, irqflags);
>  
> @@ -333,39 +379,7 @@ skl_update_plane(struct intel_plane *plane,
>  
>   /* program plane scaler */
>   if (plane_state->scaler_id >= 0) {
> - int scaler_id = plane_state->scaler_id;
> - const struct intel_scaler *scaler =
> - _state->scaler_state.scalers[scaler_id];
> - u16 y_hphase, uv_rgb_hphase;
> - u16 y_vphase, uv_rgb_vphase;
> -
> - /* TODO: handle sub-pixel coordinates */
> - if (fb->format->format == DRM_FORMAT_NV12) {
> - y_hphase = skl_scaler_calc_phase(1, false);
> - y_vphase = skl_scaler_calc_phase(1, false);
> -
> - /* MPEG2 chroma siting convention */
> - uv_rgb_hphase = skl_scaler_calc_phase(2, true);
> -

Re: [Intel-gfx] [PATCH 7/8] drm/i915: Move programming plane scaler to its own function.

2018-09-20 Thread Matt Roper 
On Thu, Sep 20, 2018 at 12:27:10PM +0200, Maarten Lankhorst wrote:
> This cleans the code up slightly, and will make other changes easier.
> 
> Signed-off-by: Maarten Lankhorst 

Reviewed-by: Matt Roper 

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 90 +
>  1 file changed, 52 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index d4c8e10fc90b..7d3c7469d271 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -280,13 +280,63 @@ skl_plane_max_stride(struct intel_plane *plane,
>   return min(8192 * cpp, 32768);
>  }
>  
> +static void
> +skl_program_scaler(struct drm_i915_private *dev_priv,
> +struct intel_plane *plane,
> +const struct intel_crtc_state *crtc_state,
> +const struct intel_plane_state *plane_state)
> +{
> + enum plane_id plane_id = plane->id;
> + enum pipe pipe = plane->pipe;
> + int scaler_id = plane_state->scaler_id;
> + const struct intel_scaler *scaler =
> + _state->scaler_state.scalers[scaler_id];
> + int crtc_x = plane_state->base.dst.x1;
> + int crtc_y = plane_state->base.dst.y1;
> + uint32_t crtc_w = drm_rect_width(_state->base.dst);
> + uint32_t crtc_h = drm_rect_height(_state->base.dst);
> + u16 y_hphase, uv_rgb_hphase;
> + u16 y_vphase, uv_rgb_vphase;
> +
> + /* Sizes are 0 based */
> + crtc_w--;
> + crtc_h--;
> +
> + /* TODO: handle sub-pixel coordinates */
> + if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
> + y_hphase = skl_scaler_calc_phase(1, false);
> + y_vphase = skl_scaler_calc_phase(1, false);
> +
> + /* MPEG2 chroma siting convention */
> + uv_rgb_hphase = skl_scaler_calc_phase(2, true);
> + uv_rgb_vphase = skl_scaler_calc_phase(2, false);
> + } else {
> + /* not used */
> + y_hphase = 0;
> + y_vphase = 0;
> +
> + uv_rgb_hphase = skl_scaler_calc_phase(1, false);
> + uv_rgb_vphase = skl_scaler_calc_phase(1, false);
> + }
> +
> + I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
> +   PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
> + I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
> + I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
> +   PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> + I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
> +   PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> + I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
> + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
> +   ((crtc_w + 1) << 16)|(crtc_h + 1));
> +}
> +
>  void
>  skl_update_plane(struct intel_plane *plane,
>const struct intel_crtc_state *crtc_state,
>const struct intel_plane_state *plane_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> - const struct drm_framebuffer *fb = plane_state->base.fb;
>   enum plane_id plane_id = plane->id;
>   enum pipe pipe = plane->pipe;
>   u32 plane_ctl = plane_state->ctl;
> @@ -296,8 +346,6 @@ skl_update_plane(struct intel_plane *plane,
>   u32 aux_stride = skl_plane_stride(plane_state, 1);
>   int crtc_x = plane_state->base.dst.x1;
>   int crtc_y = plane_state->base.dst.y1;
> - uint32_t crtc_w = drm_rect_width(_state->base.dst);
> - uint32_t crtc_h = drm_rect_height(_state->base.dst);
>   uint32_t x = plane_state->color_plane[0].x;
>   uint32_t y = plane_state->color_plane[0].y;
>   uint32_t src_w = drm_rect_width(_state->base.src) >> 16;
> @@ -307,8 +355,6 @@ skl_update_plane(struct intel_plane *plane,
>   /* Sizes are 0 based */
>   src_w--;
>   src_h--;
> - crtc_w--;
> - crtc_h--;
>  
>   spin_lock_irqsave(_priv->uncore.lock, irqflags);
>  
> @@ -333,39 +379,7 @@ skl_update_plane(struct intel_plane *plane,
>  
>   /* program plane scaler */
>   if (plane_state->scaler_id >= 0) {
> - int scaler_id = plane_state->scaler_id;
> - const struct intel_scaler *scaler =
> - _state->scaler_state.scalers[scaler_id];
> - u16 y_hphase, uv_rgb_hphase;
> - u16 y_vphase, uv_rgb_vphase;
> -
> - /* TODO: handle sub-pixel coordinates */
> - if (fb->format->format == DRM_FORMAT_NV12) {
> - y_hphase = skl_scaler_calc_phase(1, false);
> - y_vphase = skl_scaler_calc_phase(1, false);
> -
> - /* MPEG2 chroma siting convention */
> - uv_rgb_hphase = skl_scaler_calc_phase(2, true);
> - uv_rgb_vphase = skl_scaler_calc_phase(2, false);
> - } else {
> - /* not used

[Intel-gfx] [PATCH 7/8] drm/i915: Move programming plane scaler to its own function.

2018-09-20 Thread Maarten Lankhorst 
This cleans the code up slightly, and will make other changes easier.

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/i915/intel_sprite.c | 90 +
 1 file changed, 52 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index d4c8e10fc90b..7d3c7469d271 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -280,13 +280,63 @@ skl_plane_max_stride(struct intel_plane *plane,
return min(8192 * cpp, 32768);
 }
 
+static void
+skl_program_scaler(struct drm_i915_private *dev_priv,
+  struct intel_plane *plane,
+  const struct intel_crtc_state *crtc_state,
+  const struct intel_plane_state *plane_state)
+{
+   enum plane_id plane_id = plane->id;
+   enum pipe pipe = plane->pipe;
+   int scaler_id = plane_state->scaler_id;
+   const struct intel_scaler *scaler =
+   _state->scaler_state.scalers[scaler_id];
+   int crtc_x = plane_state->base.dst.x1;
+   int crtc_y = plane_state->base.dst.y1;
+   uint32_t crtc_w = drm_rect_width(_state->base.dst);
+   uint32_t crtc_h = drm_rect_height(_state->base.dst);
+   u16 y_hphase, uv_rgb_hphase;
+   u16 y_vphase, uv_rgb_vphase;
+
+   /* Sizes are 0 based */
+   crtc_w--;
+   crtc_h--;
+
+   /* TODO: handle sub-pixel coordinates */
+   if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
+   y_hphase = skl_scaler_calc_phase(1, false);
+   y_vphase = skl_scaler_calc_phase(1, false);
+
+   /* MPEG2 chroma siting convention */
+   uv_rgb_hphase = skl_scaler_calc_phase(2, true);
+   uv_rgb_vphase = skl_scaler_calc_phase(2, false);
+   } else {
+   /* not used */
+   y_hphase = 0;
+   y_vphase = 0;
+
+   uv_rgb_hphase = skl_scaler_calc_phase(1, false);
+   uv_rgb_vphase = skl_scaler_calc_phase(1, false);
+   }
+
+   I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
+ PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
+   I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
+   I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
+ PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
+   I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
+ PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
+   I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
+   I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
+ ((crtc_w + 1) << 16)|(crtc_h + 1));
+}
+
 void
 skl_update_plane(struct intel_plane *plane,
 const struct intel_crtc_state *crtc_state,
 const struct intel_plane_state *plane_state)
 {
struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
-   const struct drm_framebuffer *fb = plane_state->base.fb;
enum plane_id plane_id = plane->id;
enum pipe pipe = plane->pipe;
u32 plane_ctl = plane_state->ctl;
@@ -296,8 +346,6 @@ skl_update_plane(struct intel_plane *plane,
u32 aux_stride = skl_plane_stride(plane_state, 1);
int crtc_x = plane_state->base.dst.x1;
int crtc_y = plane_state->base.dst.y1;
-   uint32_t crtc_w = drm_rect_width(_state->base.dst);
-   uint32_t crtc_h = drm_rect_height(_state->base.dst);
uint32_t x = plane_state->color_plane[0].x;
uint32_t y = plane_state->color_plane[0].y;
uint32_t src_w = drm_rect_width(_state->base.src) >> 16;
@@ -307,8 +355,6 @@ skl_update_plane(struct intel_plane *plane,
/* Sizes are 0 based */
src_w--;
src_h--;
-   crtc_w--;
-   crtc_h--;
 
spin_lock_irqsave(_priv->uncore.lock, irqflags);
 
@@ -333,39 +379,7 @@ skl_update_plane(struct intel_plane *plane,
 
/* program plane scaler */
if (plane_state->scaler_id >= 0) {
-   int scaler_id = plane_state->scaler_id;
-   const struct intel_scaler *scaler =
-   _state->scaler_state.scalers[scaler_id];
-   u16 y_hphase, uv_rgb_hphase;
-   u16 y_vphase, uv_rgb_vphase;
-
-   /* TODO: handle sub-pixel coordinates */
-   if (fb->format->format == DRM_FORMAT_NV12) {
-   y_hphase = skl_scaler_calc_phase(1, false);
-   y_vphase = skl_scaler_calc_phase(1, false);
-
-   /* MPEG2 chroma siting convention */
-   uv_rgb_hphase = skl_scaler_calc_phase(2, true);
-   uv_rgb_vphase = skl_scaler_calc_phase(2, false);
-   } else {
-   /* not used */
-   y_hphase = 0;
-   y_vphase = 0;
-
-   uv_rgb_hphase = skl_scaler_calc_phase(1, false);
-