[PATCH 00/20] skylake display scalers

[PATCH 00/20] skylake display scalers

[Chandra Konduru]

primary changes in this version are:
- moved changes to state from commit patch to check path (Matt)
- squashed few patches into others (Matt)
- rebased colorkey related patches ontop of recent updates (Daniel)
- rebased all patches onto today's drm-intel-nightly (me)
- initialized colorkey to NONE (me)
- changed primary plane src values to regular integers align with sprite (me)
- used updated get display clock function (me)

Numbers of patches updated in this version:
 4, 6, 8, 9, 11, 18, 19, 20

Sending full patch series for completeness. Individual patch headers
have additional details on changes. This series should cleanly
merge to latest drm-intel-nighly.

Chandra Konduru (20):
  drm/i915: Adding drm helper function drm_plane_from_index().
  drm/i915: Register definitions for skylake scalers
  drm/i915: skylake scaler structure definitions
  drm/i915: Initialize plane colorkey to NONE
  drm/i915: Initialize skylake scalers
  drm/i915: Convert primary plane 16.16 values to regular ints
  drm/i915: Dump scaler_state too as part of dumping crtc_state
  drm/i915: Helper function to update skylake scaling ratio.
  drm/i915: Add helper function to update scaler_users in crtc_state
  drm/i915: Add atomic function to setup scalers scalers for a crtc.
  drm/i915: Helper function to detach a scaler from a plane or crtc
  drm/i915: Preserve scaler state when clearing crtc_state
  drm/i915: use current scaler state during readout_hw_state.
  drm/i915: Update scaling ratio as part of crtc_compute_config
  drm/i915: Ensure setting up scalers into staged crtc_state
  drm/i915: copy staged scaler state from drm state to crtc->config.
  drm/i915: stage panel fitting scaler request for fixed mode panel
  drm/i915: Enable skylake panel fitting using skylake shared scalers
  drm/i915: Enable skylake primary plane scaling using shared scalers
  drm/i915: Enable skylake sprite plane scaling using shared scalers

 drivers/gpu/drm/drm_crtc.c           |   22 ++
 drivers/gpu/drm/i915/i915_reg.h      |  115 ++++++++
 drivers/gpu/drm/i915/intel_atomic.c  |  168 ++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  489 +++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c      |    8 +
 drivers/gpu/drm/i915/intel_drv.h     |   97 +++++++
 drivers/gpu/drm/i915/intel_sprite.c  |   71 ++++-
 include/drm/drm_crtc.h               |    1 +
 8 files changed, 923 insertions(+), 48 deletions(-)

-- 
1.7.9.5

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

[PATCH 01/20] drm/i915: Adding drm helper function drm_plane_from_index().

[Chandra Konduru]

Adding drm helper function to return plane pointer from index where
index is a returned by drm_plane_index.

v2:
-avoided nested loop by adding loop count (Daniel)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   22 ++++++++++++++++++++++
 include/drm/drm_crtc.h     |    1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d576a4d..0f0159b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1289,6 +1289,28 @@ unsigned int drm_plane_index(struct drm_plane *plane)
 EXPORT_SYMBOL(drm_plane_index);
 
 /**
+ * drm_plane_from_index - find the registered plane at an index
+ * @idx: index of registered plane to find for
+ *
+ * Given a plane index, return the registered plane from DRM device's
+ * list of planes with matching index.
+ */
+struct drm_plane *
+drm_plane_from_index(struct drm_device *dev, int idx)
+{
+	struct drm_plane *plane;
+	unsigned int i = 0;
+
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		if (i == idx)
+			return plane;
+		i++;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(drm_plane_from_index);
+
+/**
  * drm_plane_force_disable - Forcibly disable a plane
  * @plane: plane to disable
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7b5c661..6b30036 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1264,6 +1264,7 @@ extern int drm_plane_init(struct drm_device *dev,
 			  bool is_primary);
 extern void drm_plane_cleanup(struct drm_plane *plane);
 extern unsigned int drm_plane_index(struct drm_plane *plane);
+extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx);
 extern void drm_plane_force_disable(struct drm_plane *plane);
 extern int drm_plane_check_pixel_format(const struct drm_plane *plane,
 					u32 format);
-- 
1.7.9.5

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

Re: [PATCH 01/20] drm/i915: Adding drm helper function drm_plane_from_index().

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:30PM -0700, Chandra Konduru wrote:
> Adding drm helper function to return plane pointer from index where
> index is a returned by drm_plane_index.
> 
> v2:
> -avoided nested loop by adding loop count (Daniel)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

This should just have a "drm:" headline prefix and should have a Cc:
to dri-devel here since it's touching DRM core code.

Otherwise,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


> ---
>  drivers/gpu/drm/drm_crtc.c |   22 ++++++++++++++++++++++
>  include/drm/drm_crtc.h     |    1 +
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d576a4d..0f0159b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1289,6 +1289,28 @@ unsigned int drm_plane_index(struct drm_plane *plane)
>  EXPORT_SYMBOL(drm_plane_index);
>  
>  /**
> + * drm_plane_from_index - find the registered plane at an index
> + * @idx: index of registered plane to find for
> + *
> + * Given a plane index, return the registered plane from DRM device's
> + * list of planes with matching index.
> + */
> +struct drm_plane *
> +drm_plane_from_index(struct drm_device *dev, int idx)
> +{
> +	struct drm_plane *plane;
> +	unsigned int i = 0;
> +
> +	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> +		if (i == idx)
> +			return plane;
> +		i++;
> +	}
> +	return NULL;
> +}
> +EXPORT_SYMBOL(drm_plane_from_index);
> +
> +/**
>   * drm_plane_force_disable - Forcibly disable a plane
>   * @plane: plane to disable
>   *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 7b5c661..6b30036 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1264,6 +1264,7 @@ extern int drm_plane_init(struct drm_device *dev,
>  			  bool is_primary);
>  extern void drm_plane_cleanup(struct drm_plane *plane);
>  extern unsigned int drm_plane_index(struct drm_plane *plane);
> +extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx);
>  extern void drm_plane_force_disable(struct drm_plane *plane);
>  extern int drm_plane_check_pixel_format(const struct drm_plane *plane,
>  					u32 format);
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/20] drm/i915: Register definitions for skylake scalers

[Chandra Konduru]

Adding register definitions for skylake scalers.
v2:
-add #define for plane selection mask (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  115 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b134fa3..6ad1932 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5096,6 +5096,121 @@ enum skl_disp_power_wells {
 #define PS_WIN_SZ(pipe)		_PIPE(pipe, _PSA_WIN_SZ, _PSB_WIN_SZ)
 #define PS_WIN_POS(pipe)	_PIPE(pipe, _PSA_WIN_POS, _PSB_WIN_POS)
 
+/*
+ * Skylake scalers
+ */
+#define _PS_1A_CTRL      0x68180
+#define _PS_2A_CTRL      0x68280
+#define _PS_1B_CTRL      0x68980
+#define _PS_2B_CTRL      0x68A80
+#define _PS_1C_CTRL      0x69180
+#define PS_SCALER_EN        (1 << 31)
+#define PS_SCALER_MODE_MASK (3 << 28)
+#define PS_SCALER_MODE_DYN  (0 << 28)
+#define PS_SCALER_MODE_HQ  (1 << 28)
+#define PS_PLANE_SEL_MASK  (7 << 25)
+#define PS_PLANE_SEL(plane) ((plane + 1) << 25)
+#define PS_FILTER_MASK         (3 << 23)
+#define PS_FILTER_MEDIUM       (0 << 23)
+#define PS_FILTER_EDGE_ENHANCE (2 << 23)
+#define PS_FILTER_BILINEAR     (3 << 23)
+#define PS_VERT3TAP            (1 << 21)
+#define PS_VERT_INT_INVERT_FIELD1 (0 << 20)
+#define PS_VERT_INT_INVERT_FIELD0 (1 << 20)
+#define PS_PWRUP_PROGRESS         (1 << 17)
+#define PS_V_FILTER_BYPASS        (1 << 8)
+#define PS_VADAPT_EN              (1 << 7)
+#define PS_VADAPT_MODE_MASK        (3 << 5)
+#define PS_VADAPT_MODE_LEAST_ADAPT (0 << 5)
+#define PS_VADAPT_MODE_MOD_ADAPT   (1 << 5)
+#define PS_VADAPT_MODE_MOST_ADAPT  (3 << 5)
+
+#define _PS_PWR_GATE_1A     0x68160
+#define _PS_PWR_GATE_2A     0x68260
+#define _PS_PWR_GATE_1B     0x68960
+#define _PS_PWR_GATE_2B     0x68A60
+#define _PS_PWR_GATE_1C     0x69160
+#define PS_PWR_GATE_DIS_OVERRIDE       (1 << 31)
+#define PS_PWR_GATE_SETTLING_TIME_32   (0 << 3)
+#define PS_PWR_GATE_SETTLING_TIME_64   (1 << 3)
+#define PS_PWR_GATE_SETTLING_TIME_96   (2 << 3)
+#define PS_PWR_GATE_SETTLING_TIME_128  (3 << 3)
+#define PS_PWR_GATE_SLPEN_8             0
+#define PS_PWR_GATE_SLPEN_16            1
+#define PS_PWR_GATE_SLPEN_24            2
+#define PS_PWR_GATE_SLPEN_32            3
+
+#define _PS_WIN_POS_1A      0x68170
+#define _PS_WIN_POS_2A      0x68270
+#define _PS_WIN_POS_1B      0x68970
+#define _PS_WIN_POS_2B      0x68A70
+#define _PS_WIN_POS_1C      0x69170
+
+#define _PS_WIN_SZ_1A       0x68174
+#define _PS_WIN_SZ_2A       0x68274
+#define _PS_WIN_SZ_1B       0x68974
+#define _PS_WIN_SZ_2B       0x68A74
+#define _PS_WIN_SZ_1C       0x69174
+
+#define _PS_VSCALE_1A       0x68184
+#define _PS_VSCALE_2A       0x68284
+#define _PS_VSCALE_1B       0x68984
+#define _PS_VSCALE_2B       0x68A84
+#define _PS_VSCALE_1C       0x69184
+
+#define _PS_HSCALE_1A       0x68190
+#define _PS_HSCALE_2A       0x68290
+#define _PS_HSCALE_1B       0x68990
+#define _PS_HSCALE_2B       0x68A90
+#define _PS_HSCALE_1C       0x69190
+
+#define _PS_VPHASE_1A       0x68188
+#define _PS_VPHASE_2A       0x68288
+#define _PS_VPHASE_1B       0x68988
+#define _PS_VPHASE_2B       0x68A88
+#define _PS_VPHASE_1C       0x69188
+
+#define _PS_HPHASE_1A       0x68194
+#define _PS_HPHASE_2A       0x68294
+#define _PS_HPHASE_1B       0x68994
+#define _PS_HPHASE_2B       0x68A94
+#define _PS_HPHASE_1C       0x69194
+
+#define _PS_ECC_STAT_1A     0x681D0
+#define _PS_ECC_STAT_2A     0x682D0
+#define _PS_ECC_STAT_1B     0x689D0
+#define _PS_ECC_STAT_2B     0x68AD0
+#define _PS_ECC_STAT_1C     0x691D0
+
+#define _ID(id, a, b) ((a) + (id)*((b)-(a)))
+#define SKL_PS_CTRL(pipe, id) _PIPE(pipe,        \
+			_ID(id, _PS_1A_CTRL, _PS_2A_CTRL),       \
+			_ID(id, _PS_1B_CTRL, _PS_2B_CTRL))
+#define SKL_PS_PWR_GATE(pipe, id) _PIPE(pipe,    \
+			_ID(id, _PS_PWR_GATE_1A, _PS_PWR_GATE_2A), \
+			_ID(id, _PS_PWR_GATE_1B, _PS_PWR_GATE_2B))
+#define SKL_PS_WIN_POS(pipe, id) _PIPE(pipe,     \
+			_ID(id, _PS_WIN_POS_1A, _PS_WIN_POS_2A), \
+			_ID(id, _PS_WIN_POS_1B, _PS_WIN_POS_2B))
+#define SKL_PS_WIN_SZ(pipe, id)  _PIPE(pipe,     \
+			_ID(id, _PS_WIN_SZ_1A, _PS_WIN_SZ_2A),   \
+			_ID(id, _PS_WIN_SZ_1B, _PS_WIN_SZ_2B))
+#define SKL_PS_VSCALE(pipe, id)  _PIPE(pipe,     \
+			_ID(id, _PS_VSCALE_1A, _PS_VSCALE_2A),   \
+			_ID(id, _PS_VSCALE_1B, _PS_VSCALE_2B))
+#define SKL_PS_HSCALE(pipe, id)  _PIPE(pipe,     \
+			_ID(id, _PS_HSCALE_1A, _PS_HSCALE_2A),   \
+			_ID(id, _PS_HSCALE_1B, _PS_HSCALE_2B))
+#define SKL_PS_VPHASE(pipe, id)  _PIPE(pipe,     \
+			_ID(id, _PS_VPHASE_1A, _PS_VPHASE_2A),   \
+			_ID(id, _PS_VPHASE_1B, _PS_VPHASE_2B))
+#define SKL_PS_HPHASE(pipe, id)  _PIPE(pipe,     \
+			_ID(id, _PS_HPHASE_1A, _PS_HPHASE_2A),   \
+			_ID(id, _PS_HPHASE_1B, _PS_HPHASE_2B))
+#define SKL_PS_ECC_STAT(pipe, id)  _PIPE(pipe,     \
+			_ID(id, _PS_ECC_STAT_1A, _PS_ECC_STAT_2A),   \
+			_ID(id, _PS_ECC_STAT_1B, _PS_ECC_STAT_2B)
+
 /* legacy palette */
 #define _LGC_PALETTE_A           0x4a000
 #define _LGC_PALETTE_B           0x4a800
-- 
1.7.9.5

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

[PATCH 03/20] drm/i915: skylake scaler structure definitions

[Chandra Konduru]

skylake scaler structure definitions. scalers live in crtc_state as
they are pipe resources. They can be used either as plane scaler or
panel fitter.

scaler assigned to either plane (for plane scaling) or crtc (for panel
fitting) is saved in scaler_id in plane_state or crtc_state respectively.

scaler_id is used instead of scaler pointer in plane or crtc state
to avoid updating scaler pointer everytime a new crtc_state is created.

v2:
-made single copy of min/max values for scalers (Matt)

v3:
-updated commentary for scaler_id (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |   90 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4799b11..e8140c7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -256,6 +256,26 @@ struct intel_plane_state {
 	 * enable/disable the primary plane
 	 */
 	bool hides_primary;
+
+	/*
+	 * scaler_id
+	 *    = -1 : not using a scaler
+	 *    >=  0 : using a scalers
+	 *
+	 * plane requiring a scaler:
+	 *   - During check_plane, its bit is set in
+	 *     crtc_state->scaler_state.scaler_users by calling helper function
+	 *     update_scaler_users.
+	 *   - scaler_id indicates the scaler it got assigned.
+	 *
+	 * plane doesn't require a scaler:
+	 *   - this can happen when scaling is no more required or plane simply
+	 *     got disabled.
+	 *   - During check_plane, corresponding bit is reset in
+	 *     crtc_state->scaler_state.scaler_users by calling helper function
+	 *     update_scaler_users.
+	 */
+	int scaler_id;
 };
 
 struct intel_initial_plane_config {
@@ -265,6 +285,74 @@ struct intel_initial_plane_config {
 	u32 base;
 };
 
+struct intel_scaler {
+	int id;
+	int in_use;
+	uint32_t mode;
+	uint32_t filter;
+};
+
+struct intel_crtc_scaler_state {
+#define INTEL_MAX_SCALERS 2
+#define SKL_NUM_SCALERS INTEL_MAX_SCALERS
+	/* scalers available on this crtc */
+	int num_scalers;
+	struct intel_scaler scalers[INTEL_MAX_SCALERS];
+
+	/*
+	 * scaler_users: keeps track of users requesting scalers on this crtc.
+	 *
+	 *     If a bit is set, a user is using a scaler.
+	 *     Here user can be a plane or crtc as defined below:
+	 *       bits 0-30 - plane (bit position is index from drm_plane_index)
+	 *       bit 31    - crtc
+	 *
+	 * Instead of creating a new index to cover planes and crtc, using
+	 * existing drm_plane_index for planes which is well less than 31
+	 * planes and bit 31 for crtc. This should be fine to cover all
+	 * our platforms.
+	 *
+	 * intel_atomic_setup_scalers will setup available scalers to users
+	 * requesting scalers. It will gracefully fail if request exceeds
+	 * avilability.
+	 */
+#define SKL_CRTC_INDEX 31
+	unsigned scaler_users;
+
+	/* scaler used by crtc for panel fitting purpose */
+	int scaler_id;
+
+	/*
+	 * Supported scaling ratio is represented as a range in [min max]
+	 * variables. This range covers both up and downscaling
+	 * where scaling ratio = (dst * 100)/src.
+	 * In above range any value:
+	 *    < 100 represents downscaling coverage
+	 *    > 100 represents upscaling coverage
+	 *    = 100 represents no-scaling (i.e., 1:1)
+	 * e.g., a min value = 50 means -> supports upto 50% of original image
+	 *       a max value = 200 means -> supports upto 200% of original image
+	 *
+	 * if incoming flip requires scaling in the supported [min max] range
+	 * then requested scaling will be performed.
+	 */
+	uint32_t min_hsr;
+	uint32_t max_hsr;
+	uint32_t min_vsr;
+	uint32_t max_vsr;
+	uint32_t min_hvsr;
+	uint32_t max_hvsr;
+
+	uint32_t min_src_w;
+	uint32_t max_src_w;
+	uint32_t min_src_h;
+	uint32_t max_src_h;
+	uint32_t min_dst_w;
+	uint32_t max_dst_w;
+	uint32_t min_dst_h;
+	uint32_t max_dst_h;
+};
+
 struct intel_crtc_state {
 	struct drm_crtc_state base;
 
@@ -391,6 +479,8 @@ struct intel_crtc_state {
 
 	bool dp_encoder_is_mst;
 	int pbn;
+
+	struct intel_crtc_scaler_state scaler_state;
 };
 
 struct intel_pipe_wm {
-- 
1.7.9.5

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

Re: [PATCH 03/20] drm/i915: skylake scaler structure definitions

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:32PM -0700, Chandra Konduru wrote:
> skylake scaler structure definitions. scalers live in crtc_state as
> they are pipe resources. They can be used either as plane scaler or
> panel fitter.
> 
> scaler assigned to either plane (for plane scaling) or crtc (for panel
> fitting) is saved in scaler_id in plane_state or crtc_state respectively.
> 
> scaler_id is used instead of scaler pointer in plane or crtc state
> to avoid updating scaler pointer everytime a new crtc_state is created.
> 
> v2:
> -made single copy of min/max values for scalers (Matt)
> 
> v3:
> -updated commentary for scaler_id (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

It seems like some of the things that were called out in the previous
review cycle still haven't been addressed here.  Repeating them below.


> ---
...
> +struct intel_scaler {
> +	int id;
> +	int in_use;
> +	uint32_t mode;
> +	uint32_t filter;

As we noted in the last round of review, filter is constant in this
patch series, so we don't need this field for now.  We should only add
this field if/when we decide to actually expose the other hardware
filter types.

> +};
> +
> +struct intel_crtc_scaler_state {
> +#define INTEL_MAX_SCALERS 2
> +#define SKL_NUM_SCALERS INTEL_MAX_SCALERS
> +	/* scalers available on this crtc */
> +	int num_scalers;

This is an unchanging hardware trait, not dynamic state, so we noted
that this should move to the CRTC itself.  The goal is to keep the state
structure to things that truly are dynamic (and not trivial to
recalculate from other state).

> +	struct intel_scaler scalers[INTEL_MAX_SCALERS];
> +
> +	/*
> +	 * scaler_users: keeps track of users requesting scalers on this crtc.
> +	 *
> +	 *     If a bit is set, a user is using a scaler.
> +	 *     Here user can be a plane or crtc as defined below:
> +	 *       bits 0-30 - plane (bit position is index from drm_plane_index)
> +	 *       bit 31    - crtc
> +	 *
> +	 * Instead of creating a new index to cover planes and crtc, using
> +	 * existing drm_plane_index for planes which is well less than 31
> +	 * planes and bit 31 for crtc. This should be fine to cover all
> +	 * our platforms.
> +	 *
> +	 * intel_atomic_setup_scalers will setup available scalers to users
> +	 * requesting scalers. It will gracefully fail if request exceeds
> +	 * avilability.
> +	 */
> +#define SKL_CRTC_INDEX 31
> +	unsigned scaler_users;
> +
> +	/* scaler used by crtc for panel fitting purpose */
> +	int scaler_id;
> +
> +	/*
> +	 * Supported scaling ratio is represented as a range in [min max]
> +	 * variables. This range covers both up and downscaling
> +	 * where scaling ratio = (dst * 100)/src.
> +	 * In above range any value:
> +	 *    < 100 represents downscaling coverage
> +	 *    > 100 represents upscaling coverage
> +	 *    = 100 represents no-scaling (i.e., 1:1)
> +	 * e.g., a min value = 50 means -> supports upto 50% of original image
> +	 *       a max value = 200 means -> supports upto 200% of original image
> +	 *
> +	 * if incoming flip requires scaling in the supported [min max] range
> +	 * then requested scaling will be performed.
> +	 */
> +	uint32_t min_hsr;
> +	uint32_t max_hsr;
> +	uint32_t min_vsr;
> +	uint32_t max_vsr;
> +	uint32_t min_hvsr;
> +	uint32_t max_hvsr;

We noted these could be dropped in the last review cycle.

> +
> +	uint32_t min_src_w;
> +	uint32_t max_src_w;
> +	uint32_t min_src_h;
> +	uint32_t max_src_h;
> +	uint32_t min_dst_w;
> +	uint32_t max_dst_w;
> +	uint32_t min_dst_h;
> +	uint32_t max_dst_h;

And we noted that these should just be #define's.

> +};
> +
>  struct intel_crtc_state {
>  	struct drm_crtc_state base;
>  
> @@ -391,6 +479,8 @@ struct intel_crtc_state {
>  
>  	bool dp_encoder_is_mst;
>  	int pbn;
> +
> +	struct intel_crtc_scaler_state scaler_state;
>  };
>  
>  struct intel_pipe_wm {
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/20] drm/i915: Initialize plane colorkey to NONE

[Chandra Konduru]

This patch initializes plane colorkey to NONE.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    1 +
 drivers/gpu/drm/i915/intel_sprite.c  |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 84f5b41..e2507f5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12771,6 +12771,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	primary->plane = pipe;
 	primary->check_plane = intel_check_primary_plane;
 	primary->commit_plane = intel_commit_primary_plane;
+	primary->ckey.flags = I915_SET_COLORKEY_NONE;
 	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
 		primary->plane = !pipe;
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index e9ff6fc..66b424b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1276,6 +1276,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	intel_plane->plane = plane;
 	intel_plane->check_plane = intel_check_sprite_plane;
 	intel_plane->commit_plane = intel_commit_sprite_plane;
+	intel_plane->ckey.flags = I915_SET_COLORKEY_NONE;
 	possible_crtcs = (1 << pipe);
 	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
 				       &intel_plane_funcs,
-- 
1.7.9.5

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

[PATCH 05/20] drm/i915: Initialize skylake scalers

[Chandra Konduru]

Initializing scalers with supported values during crtc init.

v2:
-initialize single copy of min/max values (Matt)

v3:
-moved gen check to callsite (Matt)

v4:
-squashed planes begin with no scaler to here (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   54 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c  |    1 +
 2 files changed, 55 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e2507f5..ee71bde 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -103,6 +103,8 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
 static void intel_begin_crtc_commit(struct drm_crtc *crtc);
 static void intel_finish_crtc_commit(struct drm_crtc *crtc);
+static void skl_init_scalers(struct drm_device *dev, int pipe,
+	struct intel_crtc_state *crtc_state);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -12767,6 +12769,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 
 	primary->can_scale = false;
 	primary->max_downscale = 1;
+	state->scaler_id = -1;
 	primary->pipe = pipe;
 	primary->plane = pipe;
 	primary->check_plane = intel_check_primary_plane;
@@ -12922,6 +12925,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
 	cursor->plane = pipe;
+	state->scaler_id = -1;
 	cursor->check_plane = intel_check_cursor_plane;
 	cursor->commit_plane = intel_commit_cursor_plane;
 
@@ -12948,6 +12952,52 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	return &cursor->base;
 }
 
+static void skl_init_scalers(struct drm_device *dev, int pipe,
+	struct intel_crtc_state *crtc_state)
+{
+	int i;
+	struct intel_scaler *intel_scaler;
+	struct intel_crtc_scaler_state *scaler_state = &crtc_state->scaler_state;
+
+	scaler_state->num_scalers = SKL_NUM_SCALERS;
+	for (i = 0; i < SKL_NUM_SCALERS; i++) {
+		intel_scaler = &scaler_state->scalers[i];
+		intel_scaler->in_use = 0;
+		intel_scaler->id = i;
+
+		intel_scaler->mode = PS_SCALER_MODE_DYN;
+		intel_scaler->filter = PS_FILTER_MEDIUM;
+	}
+
+	/* down scaling ratio: 2.99 --> 1, i.e., 34% of original */
+	scaler_state->min_hsr = 34;
+	scaler_state->min_vsr = 34;
+	scaler_state->max_hsr = 500;
+	scaler_state->max_vsr = 500;
+
+	/* down scaling ratio: 2.99x2.99 --> 1x1, i.e., 12% of original */
+	scaler_state->min_hvsr = 12;
+	scaler_state->max_hvsr = 2500;
+
+	/* src_w & dst_w range 8 - 4096 */
+	scaler_state->min_src_w = 8;
+	scaler_state->max_src_w = 4096;
+	scaler_state->min_dst_w = 8;
+	scaler_state->max_dst_w = 4096;
+
+	/* src_h & dst_h range 8 - 2304 */
+	scaler_state->min_src_h = 8;
+	scaler_state->max_src_h = 2304;
+	scaler_state->min_dst_h = 8;
+	scaler_state->max_dst_h = 2304;
+
+	/* pipe C has one scaler */
+	if (pipe == PIPE_C) {
+		scaler_state->num_scalers = 1;
+	}
+	scaler_state->scaler_id = -1;
+}
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -12967,6 +13017,10 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_crtc_set_state(intel_crtc, crtc_state);
 	crtc_state->base.crtc = &intel_crtc->base;
 
+	/* initialize shared scalers */
+	if (INTEL_INFO(dev)->gen >= 9)
+		skl_init_scalers(dev, pipe, crtc_state);
+
 	primary = intel_primary_plane_create(dev, pipe);
 	if (!primary)
 		goto fail;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 66b424b..ac4aa68 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1263,6 +1263,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		intel_plane->max_downscale = 1;
 		intel_plane->update_plane = skl_update_plane;
 		intel_plane->disable_plane = skl_disable_plane;
+		state->scaler_id = -1;
 
 		plane_formats = skl_plane_formats;
 		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
-- 
1.7.9.5

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

[PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints

[Chandra Konduru]

This patch converts intel_plane_state->src rect from 16.16
values into regular ints.

This approach aligns with sprite_plane_state->src rects
which are already in regular ints.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ee71bde..dddbe11 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12590,6 +12590,15 @@ intel_check_primary_plane(struct drm_plane *plane,
 			intel_crtc->atomic.update_wm = true;
 	}
 
+	/*
+	 * Hardware doesn't handle subpixel coordinates.
+	 * Adjust to (macro)pixel boundary.
+	 */
+	src->x1 >>= 16;
+	src->x2 >>= 16;
+	src->y1 >>= 16;
+	src->y2 >>= 16;
+
 	return 0;
 }
 
@@ -12608,8 +12617,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	intel_crtc = to_intel_crtc(crtc);
 
 	plane->fb = fb;
-	crtc->x = src->x1 >> 16;
-	crtc->y = src->y1 >> 16;
+	crtc->x = src->x1;
+	crtc->y = src->y1;
 
 	if (intel_crtc->active) {
 		if (state->visible) {
-- 
1.7.9.5

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

Re: [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote:
> This patch converts intel_plane_state->src rect from 16.16
> values into regular ints.
> 
> This approach aligns with sprite_plane_state->src rects
> which are already in regular ints.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

You're not touching cursor state here, so I guess it stays in 16.16 form
always?  Does it need to be updated to behave the same way?

Looking at our sprite code today, it treats intel_state->src as 16.16
for most of the check function, then re-writes it as integer pixels near
the end, which I guess matches the type of change you're doing here.  I
still find this pretty confusing that our structure is sometimes
interpreted in one way and other times interpreted a different way.

Personally I think it would be less error-prone if we just treated src
as 16.16 always, but if you to keep the current logic which changes the
meaning at the end of the check() stage, can you add some comments to
struct intel_plane_state clarifying that?

With some extra comments to avoid future confusion, you can consider this
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> ---
>  drivers/gpu/drm/i915/intel_display.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ee71bde..dddbe11 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12590,6 +12590,15 @@ intel_check_primary_plane(struct drm_plane *plane,
>  			intel_crtc->atomic.update_wm = true;
>  	}
>  
> +	/*
> +	 * Hardware doesn't handle subpixel coordinates.
> +	 * Adjust to (macro)pixel boundary.
> +	 */
> +	src->x1 >>= 16;
> +	src->x2 >>= 16;
> +	src->y1 >>= 16;
> +	src->y2 >>= 16;
> +
>  	return 0;
>  }
>  
> @@ -12608,8 +12617,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	plane->fb = fb;
> -	crtc->x = src->x1 >> 16;
> -	crtc->y = src->y1 >> 16;
> +	crtc->x = src->x1;
> +	crtc->y = src->y1;
>  
>  	if (intel_crtc->active) {
>  		if (state->visible) {
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints

[Daniel Vetter]

On Thu, Apr 02, 2015 at 04:03:22PM -0700, Matt Roper wrote:
> On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote:
> > This patch converts intel_plane_state->src rect from 16.16
> > values into regular ints.
> > 
> > This approach aligns with sprite_plane_state->src rects
> > which are already in regular ints.
> > 
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> 
> You're not touching cursor state here, so I guess it stays in 16.16 form
> always?  Does it need to be updated to behave the same way?
> 
> Looking at our sprite code today, it treats intel_state->src as 16.16
> for most of the check function, then re-writes it as integer pixels near
> the end, which I guess matches the type of change you're doing here.  I
> still find this pretty confusing that our structure is sometimes
> interpreted in one way and other times interpreted a different way.
> 
> Personally I think it would be less error-prone if we just treated src
> as 16.16 always, but if you to keep the current logic which changes the
> meaning at the end of the check() stage, can you add some comments to
> struct intel_plane_state clarifying that?

Rewriting intel_plane_state won't work since on duplicate_state we'd need
to undo that again. That's a bit too brittle imo. I'd go with Matt's
suggestion to just use 16.16 everywhere.
-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: [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, April 07, 2015 1:44 AM
> To: Roper, Matthew D
> Cc: Konduru, Chandra; Vetter, Daniel; intel-gfx@lists.freedesktop.org;
> Conselvan De Oliveira, Ander
> Subject: Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16
> values to regular ints
> 
> On Thu, Apr 02, 2015 at 04:03:22PM -0700, Matt Roper wrote:
> > On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote:
> > > This patch converts intel_plane_state->src rect from 16.16 values
> > > into regular ints.
> > >
> > > This approach aligns with sprite_plane_state->src rects which are
> > > already in regular ints.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> >
> > You're not touching cursor state here, so I guess it stays in 16.16
> > form always?  Does it need to be updated to behave the same way?
> >
> > Looking at our sprite code today, it treats intel_state->src as 16.16
> > for most of the check function, then re-writes it as integer pixels
> > near the end, which I guess matches the type of change you're doing
> > here.  I still find this pretty confusing that our structure is
> > sometimes interpreted in one way and other times interpreted a different way.
> >
> > Personally I think it would be less error-prone if we just treated src
> > as 16.16 always, but if you to keep the current logic which changes
> > the meaning at the end of the check() stage, can you add some comments
> > to struct intel_plane_state clarifying that?
> 
> Rewriting intel_plane_state won't work since on duplicate_state we'd need to
> undo that again. That's a bit too brittle imo. I'd go with Matt's suggestion to just
> use 16.16 everywhere.
> -Daniel

Recently in upstream, sprite plane src rect changed to regular int at end of check
before entering commit but left primary src rect as 16.16.

This is causing issue for having any common helper function to handle sprites
and primary. So my patch is aligning both primary and sprite's src rect as regular int
after checks are done. 

Earlier Matt commented that it is upto i915's implementation how to keep
its internal src rect values in its state which is his response to my earlier 
change to fix a bug when passing src rect keeping them in 16.16 format.  
I am fine whichever format you want. Can you or Matt make it clear which
format to keep them?

> --
> 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: [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints

[Matt Roper]

On Tue, Apr 07, 2015 at 11:29:06AM -0700, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, April 07, 2015 1:44 AM
> > To: Roper, Matthew D
> > Cc: Konduru, Chandra; Vetter, Daniel; intel-gfx@lists.freedesktop.org;
> > Conselvan De Oliveira, Ander
> > Subject: Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16
> > values to regular ints
> > 
> > On Thu, Apr 02, 2015 at 04:03:22PM -0700, Matt Roper wrote:
> > > On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote:
> > > > This patch converts intel_plane_state->src rect from 16.16 values
> > > > into regular ints.
> > > >
> > > > This approach aligns with sprite_plane_state->src rects which are
> > > > already in regular ints.
> > > >
> > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > >
> > > You're not touching cursor state here, so I guess it stays in 16.16
> > > form always?  Does it need to be updated to behave the same way?
> > >
> > > Looking at our sprite code today, it treats intel_state->src as 16.16
> > > for most of the check function, then re-writes it as integer pixels
> > > near the end, which I guess matches the type of change you're doing
> > > here.  I still find this pretty confusing that our structure is
> > > sometimes interpreted in one way and other times interpreted a different way.
> > >
> > > Personally I think it would be less error-prone if we just treated src
> > > as 16.16 always, but if you to keep the current logic which changes
> > > the meaning at the end of the check() stage, can you add some comments
> > > to struct intel_plane_state clarifying that?
> > 
> > Rewriting intel_plane_state won't work since on duplicate_state we'd need to
> > undo that again. That's a bit too brittle imo. I'd go with Matt's suggestion to just
> > use 16.16 everywhere.
> > -Daniel
> 
> Recently in upstream, sprite plane src rect changed to regular int at end of check
> before entering commit but left primary src rect as 16.16.
> 
> This is causing issue for having any common helper function to handle sprites
> and primary. So my patch is aligning both primary and sprite's src rect as regular int
> after checks are done. 
> 
> Earlier Matt commented that it is upto i915's implementation how to keep
> its internal src rect values in its state which is his response to my earlier 
> change to fix a bug when passing src rect keeping them in 16.16 format.  
> I am fine whichever format you want. Can you or Matt make it clear which
> format to keep them?

The internal src/dest rect are derived state that the driver tracks for
its own purposes, so yeah, it's up to us how we decide to store it.  The
confusing (and error-prone) part is that our sprite check code treats it
as 16.16 whereas our commit code treats it as integer; to make matters
worse, we aren't even consistent with this scheme across the various
plane types (primary, sprite, cursor).

Even though the state gets copied in duplicate_state(), we don't have
any immediate problems with the existing sprite code today because it
does wind up getting blown away and recomputed before we have a chance
to mix up the meaning of the values.  Your patch here looks like it
would work without problems today too for the same reason.  But simply
the inconsistency of what the value means at various points in the
process bothers me because it is likely to cause mistakes as people
write code in the future.  Since the check phase is the more complex
logic here, and since it depends on the 16.16 storage, it seems natural
to me to just preserve that 16.16 format throughout and just shift as
necessary in the commit step.

From what I can see, the mid-operation switch between 16.16 and integer
format in the sprite code originated with commit

        commit 96d61a7f267ff355a401ca23a732810027d10ba2
        Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
        Date:   Fri Sep 5 17:04:47 2014 -0300

            drm/i915: split intel_update_plane into check() and commit()

when the check and commit steps were first split apart.  The change
isn't directly referenced in the commit message, so I think it just sort
of snuck in under the radar.


Matt

> 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints

[Konduru, Chandra]

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, April 07, 2015 11:46 AM
> To: Konduru, Chandra
> Cc: Daniel Vetter; Vetter, Daniel; intel-gfx@lists.freedesktop.org; Conselvan De
> Oliveira, Ander
> Subject: Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16
> values to regular ints
> 
> On Tue, Apr 07, 2015 at 11:29:06AM -0700, Konduru, Chandra wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Tuesday, April 07, 2015 1:44 AM
> > > To: Roper, Matthew D
> > > Cc: Konduru, Chandra; Vetter, Daniel;
> > > intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander
> > > Subject: Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary
> > > plane 16.16 values to regular ints
> > >
> > > On Thu, Apr 02, 2015 at 04:03:22PM -0700, Matt Roper wrote:
> > > > On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote:
> > > > > This patch converts intel_plane_state->src rect from 16.16
> > > > > values into regular ints.
> > > > >
> > > > > This approach aligns with sprite_plane_state->src rects which
> > > > > are already in regular ints.
> > > > >
> > > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > >
> > > > You're not touching cursor state here, so I guess it stays in
> > > > 16.16 form always?  Does it need to be updated to behave the same way?
> > > >
> > > > Looking at our sprite code today, it treats intel_state->src as
> > > > 16.16 for most of the check function, then re-writes it as integer
> > > > pixels near the end, which I guess matches the type of change
> > > > you're doing here.  I still find this pretty confusing that our
> > > > structure is sometimes interpreted in one way and other times interpreted
> a different way.
> > > >
> > > > Personally I think it would be less error-prone if we just treated
> > > > src as 16.16 always, but if you to keep the current logic which
> > > > changes the meaning at the end of the check() stage, can you add
> > > > some comments to struct intel_plane_state clarifying that?
> > >
> > > Rewriting intel_plane_state won't work since on duplicate_state we'd
> > > need to undo that again. That's a bit too brittle imo. I'd go with
> > > Matt's suggestion to just use 16.16 everywhere.
> > > -Daniel
> >
> > Recently in upstream, sprite plane src rect changed to regular int at
> > end of check before entering commit but left primary src rect as 16.16.
> >
> > This is causing issue for having any common helper function to handle
> > sprites and primary. So my patch is aligning both primary and sprite's
> > src rect as regular int after checks are done.
> >
> > Earlier Matt commented that it is upto i915's implementation how to
> > keep its internal src rect values in its state which is his response
> > to my earlier change to fix a bug when passing src rect keeping them in 16.16
> format.
> > I am fine whichever format you want. Can you or Matt make it clear
> > which format to keep them?
> 
> The internal src/dest rect are derived state that the driver tracks for its own
> purposes, so yeah, it's up to us how we decide to store it.  The confusing (and
> error-prone) part is that our sprite check code treats it as 16.16 whereas our
> commit code treats it as integer; to make matters worse, we aren't even
> consistent with this scheme across the various plane types (primary, sprite,
> cursor).
> 
> Even though the state gets copied in duplicate_state(), we don't have any
> immediate problems with the existing sprite code today because it does wind up
> getting blown away and recomputed before we have a chance to mix up the
> meaning of the values.  Your patch here looks like it would work without
> problems today too for the same reason.  But simply the inconsistency of what
> the value means at various points in the process bothers me because it is likely
> to cause mistakes as people write code in the future.  Since the check phase is
> the more complex logic here, and since it depends on the 16.16 storage, it
> seems natural to me to just preserve that 16.16 format throughout and just shift
> as necessary in the commit step.
> 
> From what I can see, the mid-operation switch between 16.16 and integer
> format in the sprite code originated with commit
> 
>         commit 96d61a7f267ff355a401ca23a732810027d10ba2
>         Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>         Date:   Fri Sep 5 17:04:47 2014 -0300
> 
>             drm/i915: split intel_update_plane into check() and commit()
> 
> when the check and commit steps were first split apart.  The change isn't directly
> referenced in the commit message, so I think it just sort of snuck in under the
> radar.

Today platform_update_plane(x, y, src_w, src_h) and 
platform_update_primary_plane(x, y)
parameters are in regular integer format. 
keeping them as is.

Will bring src rect back into 16.16 format. And any consumer of src
will do >> 16 on src values as needed.

> 
> 
> Matt
> 
> >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation http://blog.ffwll.ch
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/20] drm/i915: Dump scaler_state too as part of dumping crtc_state

[Chandra Konduru]

Dumps scaler state as part of dumping crtc_state.

v2:
-use regular ints from plane_state->src (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   47 ++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dddbe11..316c4c2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10558,8 +10558,14 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 				   struct intel_crtc_state *pipe_config,
 				   const char *context)
 {
-	DRM_DEBUG_KMS("[CRTC:%d]%s config for pipe %c\n", crtc->base.base.id,
-		      context, pipe_name(crtc->pipe));
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_plane *plane;
+	struct intel_plane *intel_plane;
+	struct intel_plane_state *state;
+	struct drm_framebuffer *fb;
+
+	DRM_DEBUG_KMS("[CRTC:%d]%s config %p for pipe %c\n", crtc->base.base.id,
+		      context, pipe_config, pipe_name(crtc->pipe));
 
 	DRM_DEBUG_KMS("cpu_transcoder: %c\n", transcoder_name(pipe_config->cpu_transcoder));
 	DRM_DEBUG_KMS("pipe bpp: %i, dithering: %i\n",
@@ -10596,6 +10602,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 	DRM_DEBUG_KMS("port clock: %d\n", pipe_config->port_clock);
 	DRM_DEBUG_KMS("pipe src size: %dx%d\n",
 		      pipe_config->pipe_src_w, pipe_config->pipe_src_h);
+	DRM_DEBUG_KMS("num_scalers: %d\n", pipe_config->scaler_state.num_scalers);
+	DRM_DEBUG_KMS("scaler_users: 0x%x\n", pipe_config->scaler_state.scaler_users);
+	DRM_DEBUG_KMS("scaler id: %d\n", pipe_config->scaler_state.scaler_id);
 	DRM_DEBUG_KMS("gmch pfit: control: 0x%08x, ratios: 0x%08x, lvds border: 0x%08x\n",
 		      pipe_config->gmch_pfit.control,
 		      pipe_config->gmch_pfit.pgm_ratios,
@@ -10606,6 +10615,40 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 		      pipe_config->pch_pfit.enabled ? "enabled" : "disabled");
 	DRM_DEBUG_KMS("ips: %i\n", pipe_config->ips_enabled);
 	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
+
+	DRM_DEBUG_KMS("planes on this crtc\n");
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		intel_plane = to_intel_plane(plane);
+		if (intel_plane->pipe != crtc->pipe)
+			continue;
+
+		state = to_intel_plane_state(plane->state);
+		fb = state->base.fb;
+		if (!fb) {
+			DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d "
+				"disabled, scaler_id = %d\n",
+				plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
+				plane->base.id, intel_plane->pipe,
+				(crtc->base.primary == plane) ? 0 : intel_plane->plane + 1,
+				drm_plane_index(plane), state->scaler_id);
+			continue;
+		}
+
+		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
+			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
+			plane->base.id, intel_plane->pipe,
+			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
+			drm_plane_index(plane));
+		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
+			fb->base.id, fb->width, fb->height, fb->pixel_format);
+		DRM_DEBUG_KMS("\tscaler:%d src (%u, %u) %ux%u dst (%u, %u) %ux%u\n",
+			state->scaler_id,
+			state->src.x1, state->src.y1,
+			drm_rect_width(&state->src),
+			drm_rect_height(&state->src),
+			state->dst.x1, state->dst.y1,
+			drm_rect_width(&state->dst), drm_rect_height(&state->dst));
+	}
 }
 
 static bool encoders_cloneable(const struct intel_encoder *a,
-- 
1.7.9.5

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

[PATCH 08/20] drm/i915: Helper function to update skylake scaling ratio.

[Chandra Konduru]

Helper function updates supported scaling ratios based on cdclk and
crtc clocks.

v2:
-update single copy of scaling ratios (Matt)

v3:
-min scaling ratio is limited by either display engine limit or clocks,
 it is not related to previous ratio (Matt, me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 316c4c2..8b2eff4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4616,6 +4616,31 @@ static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
 	intel_wait_for_vblank(dev, other_active_crtc->pipe);
 }
 
+static void skl_update_scaling_ratio(struct drm_device *dev,
+	struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t crtc_clock, cdclk;
+	struct intel_crtc_scaler_state *scaler_state;
+
+	if (!crtc_state)
+		return;
+
+	crtc_clock = (uint32_t) crtc_state->base.adjusted_mode.crtc_clock;
+	cdclk = (uint32_t) dev_priv->display.get_display_clock_speed(dev);
+
+	if (!crtc_clock || !cdclk)
+		return;
+
+	scaler_state = &crtc_state->scaler_state;
+	scaler_state->min_hsr = max((uint32_t)34, (crtc_clock * 100)/cdclk);
+	scaler_state->min_vsr = max((uint32_t)34, (crtc_clock * 100)/cdclk);
+	scaler_state->min_hvsr = max((uint32_t)12, (crtc_clock * 100)/cdclk);
+
+	DRM_DEBUG_KMS("for crtc_state = %p crtc_clock = %d cdclk = %d\n", crtc_state,
+		crtc_clock, cdclk);
+}
+
 static void haswell_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-- 
1.7.9.5

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

Re: [PATCH 08/20] drm/i915: Helper function to update skylake scaling ratio.

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:37PM -0700, Chandra Konduru wrote:
> Helper function updates supported scaling ratios based on cdclk and
> crtc clocks.
> 
> v2:
> -update single copy of scaling ratios (Matt)
> 
> v3:
> -min scaling ratio is limited by either display engine limit or clocks,
>  it is not related to previous ratio (Matt, me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

As noted on an earlier patch (and on the previous review cycle), min_vsr
and min_hvsr are never used, so we should just drop them completely for
now.  min_hsr is only used in a couple trivial places, so we should
probably just replace that usage with a direct calculation of max(CONST,
clockval).  That would allow us to drop this patch completely.


Matt

> ---
>  drivers/gpu/drm/i915/intel_display.c |   25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 316c4c2..8b2eff4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4616,6 +4616,31 @@ static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
>  	intel_wait_for_vblank(dev, other_active_crtc->pipe);
>  }
>  
> +static void skl_update_scaling_ratio(struct drm_device *dev,
> +	struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t crtc_clock, cdclk;
> +	struct intel_crtc_scaler_state *scaler_state;
> +
> +	if (!crtc_state)
> +		return;
> +
> +	crtc_clock = (uint32_t) crtc_state->base.adjusted_mode.crtc_clock;
> +	cdclk = (uint32_t) dev_priv->display.get_display_clock_speed(dev);
> +
> +	if (!crtc_clock || !cdclk)
> +		return;
> +
> +	scaler_state = &crtc_state->scaler_state;
> +	scaler_state->min_hsr = max((uint32_t)34, (crtc_clock * 100)/cdclk);
> +	scaler_state->min_vsr = max((uint32_t)34, (crtc_clock * 100)/cdclk);
> +	scaler_state->min_hvsr = max((uint32_t)12, (crtc_clock * 100)/cdclk);
> +
> +	DRM_DEBUG_KMS("for crtc_state = %p crtc_clock = %d cdclk = %d\n", crtc_state,
> +		crtc_clock, cdclk);
> +}
> +
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/20] drm/i915: Add helper function to update scaler_users in crtc_state

[Chandra Konduru]

This helper function stages a scaler request for a plane/crtc into
crtc_state->scaler_users (which is a bit field). It also performs
required checks before staging any change into scaler_state.

v2:
-updates to use single copy of scaler limits (Matt)
-added force detach parameter for pfit disable purpose (me)

v3:
-updated function header to kerneldoc format (Matt)
-dropped need_scaling checks (Matt)

v4:
-move clearing of scaler id from commit path to check path (Matt)
-updated colorkey checks based on recent updates (me)
-squashed scaler check while enabling colorkey to here (me)
-use values in plane_state->src as regular integers (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  144 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |    3 +
 drivers/gpu/drm/i915/intel_sprite.c  |    9 +++
 3 files changed, 156 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8b2eff4..603a2dc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12594,6 +12594,150 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	}
 }
 
+/**
+ * skl_update_scaler_users - Stages update to crtc's scaler state
+ * @intel_crtc: crtc
+ * @crtc_state: crtc_state
+ * @plane: plane (NULL indicates crtc is requesting update)
+ * @plane_state: plane's state
+ * @force_detach: request unconditional detachment of scaler
+ *
+ * This function updates scaler state for requested plane or crtc.
+ * To request scaler usage update for a plane, caller shall pass plane pointer.
+ * To request scaler usage update for crtc, caller shall pass plane pointer
+ * as NULL.
+ *
+ * Return
+ *     0 - scaler_usage updated successfully
+ *    error - requested scaling cannot be supported or other error condition
+ */
+int
+skl_update_scaler_users(
+	struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state,
+	struct intel_plane *intel_plane, struct intel_plane_state *plane_state,
+	int force_detach)
+{
+	int need_scaling;
+	int idx;
+	int src_w, src_h, dst_w, dst_h;
+	int *scaler_id;
+	struct drm_framebuffer *fb;
+	struct intel_crtc_scaler_state *scaler_state;
+
+	if (!intel_crtc || !crtc_state ||
+		(intel_plane && intel_plane->base.type == DRM_PLANE_TYPE_CURSOR))
+		return 0;
+
+	scaler_state = &crtc_state->scaler_state;
+
+	if (!scaler_state->num_scalers) {
+		DRM_DEBUG_KMS("crtc_state = %p, num_scalers = %d\n", crtc_state,
+			scaler_state->num_scalers);
+		return 0;
+	}
+
+	idx = intel_plane ? drm_plane_index(&intel_plane->base) : SKL_CRTC_INDEX;
+	fb = intel_plane ? plane_state->base.fb : NULL;
+
+	if (intel_plane) {
+		src_w = drm_rect_width(&plane_state->src);
+		src_h = drm_rect_height(&plane_state->src);
+		dst_w = drm_rect_width(&plane_state->dst);
+		dst_h = drm_rect_height(&plane_state->dst);
+		scaler_id = &plane_state->scaler_id;
+	} else {
+		struct drm_display_mode *adjusted_mode =
+			&crtc_state->base.adjusted_mode;
+		src_w = crtc_state->pipe_src_w;
+		src_h = crtc_state->pipe_src_h;
+		dst_w = adjusted_mode->hdisplay;
+		dst_h = adjusted_mode->vdisplay;
+		scaler_id = &scaler_state->scaler_id;
+	}
+	need_scaling = (src_w != dst_w || src_h != dst_h);
+
+	/*
+	 * if plane is being disabled or scaler is no more required or force detach
+	 *  - free scaler binded to this plane/crtc
+	 *  - in order to do this, update crtc->scaler_usage
+	 *
+	 * Here scaler state in crtc_state is set free so that
+	 * scaler can be assigned to other user. Actual register
+	 * update to free the scaler is done in plane/panel-fit programming.
+	 * For this purpose crtc/plane_state->scaler_id isn't reset here.
+	 */
+	if (force_detach || !need_scaling || (intel_plane &&
+		(!fb || !plane_state->visible))) {
+		if (*scaler_id >= 0) {
+			scaler_state->scaler_users &= ~(1 << idx);
+			scaler_state->scalers[*scaler_id].in_use = 0;
+			*scaler_id = -1;
+
+			DRM_DEBUG_KMS("Staged freeing scaler id %u.%u from %s:%d "
+				"crtc_state = %p scaler_users = 0x%x\n",
+				intel_crtc->pipe, *scaler_id, intel_plane ? "PLANE" : "CRTC",
+				intel_plane ? intel_plane->base.base.id :
+				intel_crtc->base.base.id, crtc_state,
+				scaler_state->scaler_users);
+		}
+		return 0;
+	}
+
+	/* range checks */
+	if (src_w < scaler_state->min_src_w || src_h < scaler_state->min_src_h ||
+		dst_w < scaler_state->min_dst_w || dst_h < scaler_state->min_dst_h ||
+
+		src_w > scaler_state->max_src_w || src_h > scaler_state->max_src_h ||
+		dst_w > scaler_state->max_dst_w || dst_h > scaler_state->max_dst_h) {
+		DRM_DEBUG_KMS("%s:%d scaler_user index %u.%u: src %ux%u dst %ux%u "
+			"size is out of scaler range\n",
+			intel_plane ? "PLANE" : "CRTC",
+			intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
+			intel_crtc->pipe, idx, src_w, src_h, dst_w, dst_h);
+		return -EINVAL;
+	}
+
+	/* check colorkey */
+	if (intel_plane && intel_plane->ckey.flags != I915_SET_COLORKEY_NONE) {
+		DRM_DEBUG_KMS("PLANE:%d scaling with color key not allowed",
+			intel_plane->base.base.id);
+		return -EINVAL;
+	}
+
+	/* Check src format */
+	if (intel_plane) {
+		switch (fb->pixel_format) {
+		case DRM_FORMAT_RGB565:
+		case DRM_FORMAT_XBGR8888:
+		case DRM_FORMAT_XRGB8888:
+		case DRM_FORMAT_ABGR8888:
+		case DRM_FORMAT_ARGB8888:
+		case DRM_FORMAT_XRGB2101010:
+		case DRM_FORMAT_ARGB2101010:
+		case DRM_FORMAT_XBGR2101010:
+		case DRM_FORMAT_ABGR2101010:
+		case DRM_FORMAT_YUYV:
+		case DRM_FORMAT_YVYU:
+		case DRM_FORMAT_UYVY:
+		case DRM_FORMAT_VYUY:
+			break;
+		default:
+			DRM_DEBUG_KMS("PLANE:%d FB:%d unsupported scaling format 0x%x\n",
+				intel_plane->base.base.id, fb->base.id, fb->pixel_format);
+			return -EINVAL;
+		}
+	}
+
+	/* mark this plane as a scaler user in crtc_state */
+	scaler_state->scaler_users |= (1 << idx);
+	DRM_DEBUG_KMS("%s:%d staged scaling request for %ux%u->%ux%u "
+		"crtc_state = %p scaler_users = 0x%x\n",
+		intel_plane ? "PLANE" : "CRTC",
+		intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
+		src_w, src_h, dst_w, dst_h, crtc_state, scaler_state->scaler_users);
+	return 0;
+}
+
 static int
 intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e8140c7..e20ddd5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1143,6 +1143,9 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_state *pipe_config);
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
 void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
+int skl_update_scaler_users(struct intel_crtc *intel_crtc,
+	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
+	struct intel_plane_state *plane_state, int force_detach);
 
 unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 				     struct drm_i915_gem_object *obj);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ac4aa68..c8feff7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1119,6 +1119,15 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 	}
 
 	intel_plane = to_intel_plane(plane);
+
+	if (INTEL_INFO(dev)->gen >= 9) {
+		/* plane scaling and colorkey are mutually exclusive */
+		if (to_intel_plane_state(plane->state)->scaler_id >= 0) {
+			DRM_ERROR("colorkey not allowed with scaler\n");
+			return -EINVAL;
+		}
+	}
+
 	intel_plane->ckey = *set;
 
 	/*
-- 
1.7.9.5

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

Re: [PATCH 09/20] drm/i915: Add helper function to update scaler_users in crtc_state

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:38PM -0700, Chandra Konduru wrote:
> This helper function stages a scaler request for a plane/crtc into
> crtc_state->scaler_users (which is a bit field). It also performs
> required checks before staging any change into scaler_state.
> 
> v2:
> -updates to use single copy of scaler limits (Matt)
> -added force detach parameter for pfit disable purpose (me)
> 
> v3:
> -updated function header to kerneldoc format (Matt)
> -dropped need_scaling checks (Matt)
> 
> v4:
> -move clearing of scaler id from commit path to check path (Matt)
> -updated colorkey checks based on recent updates (me)
> -squashed scaler check while enabling colorkey to here (me)
> -use values in plane_state->src as regular integers (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

As noted in the previous review cycle, this should at least be squashed
with patch 17, which is when you start calling this.

> ---
>  drivers/gpu/drm/i915/intel_display.c |  144 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    3 +
>  drivers/gpu/drm/i915/intel_sprite.c  |    9 +++
>  3 files changed, 156 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8b2eff4..603a2dc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12594,6 +12594,150 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	}
>  }
>  
> +/**
> + * skl_update_scaler_users - Stages update to crtc's scaler state
> + * @intel_crtc: crtc
> + * @crtc_state: crtc_state
> + * @plane: plane (NULL indicates crtc is requesting update)
> + * @plane_state: plane's state
> + * @force_detach: request unconditional detachment of scaler
> + *
> + * This function updates scaler state for requested plane or crtc.
> + * To request scaler usage update for a plane, caller shall pass plane pointer.
> + * To request scaler usage update for crtc, caller shall pass plane pointer
> + * as NULL.
> + *
> + * Return
> + *     0 - scaler_usage updated successfully
> + *    error - requested scaling cannot be supported or other error condition
> + */
> +int
> +skl_update_scaler_users(
> +	struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state,
> +	struct intel_plane *intel_plane, struct intel_plane_state *plane_state,
> +	int force_detach)
> +{
> +	int need_scaling;
> +	int idx;
> +	int src_w, src_h, dst_w, dst_h;
> +	int *scaler_id;
> +	struct drm_framebuffer *fb;
> +	struct intel_crtc_scaler_state *scaler_state;
> +
> +	if (!intel_crtc || !crtc_state ||
> +		(intel_plane && intel_plane->base.type == DRM_PLANE_TYPE_CURSOR))

It doesn't look possible to get here with a cursor plane to me.  Maybe
wrap that plane type test in a WARN_ON() so that we notice if we screw
up?

> +		return 0;
> +
> +	scaler_state = &crtc_state->scaler_state;
> +
> +	if (!scaler_state->num_scalers) {
> +		DRM_DEBUG_KMS("crtc_state = %p, num_scalers = %d\n", crtc_state,
> +			scaler_state->num_scalers);
> +		return 0;
> +	}
> +
> +	idx = intel_plane ? drm_plane_index(&intel_plane->base) : SKL_CRTC_INDEX;
> +	fb = intel_plane ? plane_state->base.fb : NULL;
> +
> +	if (intel_plane) {
> +		src_w = drm_rect_width(&plane_state->src);
> +		src_h = drm_rect_height(&plane_state->src);
> +		dst_w = drm_rect_width(&plane_state->dst);
> +		dst_h = drm_rect_height(&plane_state->dst);
> +		scaler_id = &plane_state->scaler_id;
> +	} else {
> +		struct drm_display_mode *adjusted_mode =
> +			&crtc_state->base.adjusted_mode;
> +		src_w = crtc_state->pipe_src_w;
> +		src_h = crtc_state->pipe_src_h;
> +		dst_w = adjusted_mode->hdisplay;
> +		dst_h = adjusted_mode->vdisplay;
> +		scaler_id = &scaler_state->scaler_id;
> +	}
> +	need_scaling = (src_w != dst_w || src_h != dst_h);
> +
> +	/*
> +	 * if plane is being disabled or scaler is no more required or force detach
> +	 *  - free scaler binded to this plane/crtc
> +	 *  - in order to do this, update crtc->scaler_usage
> +	 *
> +	 * Here scaler state in crtc_state is set free so that
> +	 * scaler can be assigned to other user. Actual register
> +	 * update to free the scaler is done in plane/panel-fit programming.
> +	 * For this purpose crtc/plane_state->scaler_id isn't reset here.
> +	 */
> +	if (force_detach || !need_scaling || (intel_plane &&

I guess this force_detach approach is a bit of a necessary evil because
we haven't converted to atomic for full modesets yet (or have real
crtc_state tracking yet).  I'm a little bit worried that it will make
the eventual atomic conversion a bit trickier, but I guess there isn't
much we can do about that if we're planning to merge this series on
today's driver rather than waiting to finish atomic conversion.

Ander is Cc'd on this, and it intersects mainly with his ongoing work,
so if he's okay with how the scaler assignment state is managed with
this, then I am too.

> +		(!fb || !plane_state->visible))) {
> +		if (*scaler_id >= 0) {
> +			scaler_state->scaler_users &= ~(1 << idx);
> +			scaler_state->scalers[*scaler_id].in_use = 0;
> +			*scaler_id = -1;
> +
> +			DRM_DEBUG_KMS("Staged freeing scaler id %u.%u from %s:%d "
> +				"crtc_state = %p scaler_users = 0x%x\n",
> +				intel_crtc->pipe, *scaler_id, intel_plane ? "PLANE" : "CRTC",
> +				intel_plane ? intel_plane->base.base.id :
> +				intel_crtc->base.base.id, crtc_state,
> +				scaler_state->scaler_users);
> +		}
> +		return 0;
> +	}
> +
> +	/* range checks */
> +	if (src_w < scaler_state->min_src_w || src_h < scaler_state->min_src_h ||
> +		dst_w < scaler_state->min_dst_w || dst_h < scaler_state->min_dst_h ||
> +
> +		src_w > scaler_state->max_src_w || src_h > scaler_state->max_src_h ||
> +		dst_w > scaler_state->max_dst_w || dst_h > scaler_state->max_dst_h) {
> +		DRM_DEBUG_KMS("%s:%d scaler_user index %u.%u: src %ux%u dst %ux%u "
> +			"size is out of scaler range\n",
> +			intel_plane ? "PLANE" : "CRTC",
> +			intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
> +			intel_crtc->pipe, idx, src_w, src_h, dst_w, dst_h);
> +		return -EINVAL;
> +	}
> +
> +	/* check colorkey */
> +	if (intel_plane && intel_plane->ckey.flags != I915_SET_COLORKEY_NONE) {
> +		DRM_DEBUG_KMS("PLANE:%d scaling with color key not allowed",
> +			intel_plane->base.base.id);
> +		return -EINVAL;
> +	}
> +
> +	/* Check src format */
> +	if (intel_plane) {
> +		switch (fb->pixel_format) {
> +		case DRM_FORMAT_RGB565:
> +		case DRM_FORMAT_XBGR8888:
> +		case DRM_FORMAT_XRGB8888:
> +		case DRM_FORMAT_ABGR8888:
> +		case DRM_FORMAT_ARGB8888:
> +		case DRM_FORMAT_XRGB2101010:
> +		case DRM_FORMAT_ARGB2101010:
> +		case DRM_FORMAT_XBGR2101010:
> +		case DRM_FORMAT_ABGR2101010:
> +		case DRM_FORMAT_YUYV:
> +		case DRM_FORMAT_YVYU:
> +		case DRM_FORMAT_UYVY:
> +		case DRM_FORMAT_VYUY:
> +			break;
> +		default:
> +			DRM_DEBUG_KMS("PLANE:%d FB:%d unsupported scaling format 0x%x\n",
> +				intel_plane->base.base.id, fb->base.id, fb->pixel_format);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* mark this plane as a scaler user in crtc_state */
> +	scaler_state->scaler_users |= (1 << idx);
> +	DRM_DEBUG_KMS("%s:%d staged scaling request for %ux%u->%ux%u "
> +		"crtc_state = %p scaler_users = 0x%x\n",
> +		intel_plane ? "PLANE" : "CRTC",
> +		intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
> +		src_w, src_h, dst_w, dst_h, crtc_state, scaler_state->scaler_users);
> +	return 0;
> +}
> +
>  static int
>  intel_check_primary_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e8140c7..e20ddd5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1143,6 +1143,9 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_state *pipe_config);
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
> +int skl_update_scaler_users(struct intel_crtc *intel_crtc,
> +	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
> +	struct intel_plane_state *plane_state, int force_detach);
>  
>  unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>  				     struct drm_i915_gem_object *obj);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ac4aa68..c8feff7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1119,6 +1119,15 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  	}
>  
>  	intel_plane = to_intel_plane(plane);
> +
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		/* plane scaling and colorkey are mutually exclusive */
> +		if (to_intel_plane_state(plane->state)->scaler_id >= 0) {
> +			DRM_ERROR("colorkey not allowed with scaler\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	intel_plane->ckey = *set;
>  
>  	/*
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/20] drm/i915: Add atomic function to setup scalers scalers for a crtc.

[Chandra Konduru]

intel_atomic_setup_scalers sets up scalers based on staged scaling
requests coming from a crtc and its planes. This function should be
called from crtc level check path.

If staged requests are supportable, function assigns scalers to
requested planes and crtc. This function also takes into account
the current planes using scalers but not being part of this
atomic state for optimal operation of scalers. Note that the scaler
assignement itself is staged into crtc_state and respective
plane_states for later commit after all checks have been done.

overall high level flow:
 - scaler requests are staged into crtc_state by planes/crtc
 - check whether staged scaling requests can be supported
 - add planes using scalers that aren't in current transaction
 - assign scalers to requested users
 - as part of plane commit, scalers will be committed
   (i.e., either attached or detached) to respective planes in hw
 - as part of crtc_commit, scaler will be either attached or detached
   to crtc in hw

v2:
-removed a log message (me)
-changed input parameter to crtc_state (me)

v3:
-remove assigning plane_state returned by drm_atomic_get_plane_state (Matt)
-fail if there is an error from drm_atomic_get_plane_state (Matt)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c |  142 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |    3 +
 2 files changed, 145 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 3903b90..fab1f13 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -241,3 +241,145 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
 {
 	drm_atomic_helper_crtc_destroy_state(crtc, state);
 }
+
+/**
+ * intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
+ * @dev: DRM device
+ * @crtc: intel crtc
+ * @crtc_state: incoming crtc_state to validate and setup scalers
+ *
+ * This function sets up scalers based on staged scaling requests for
+ * a @crtc and its planes. It is called from crtc level check path. If request
+ * is a supportable request, it attaches scalers to requested planes and crtc.
+ *
+ * This function takes into account the current scaler(s) in use by any planes
+ * not being part of this atomic state
+ *
+ *  Returns:
+ *         0 - scalers were setup succesfully
+ *         error code - otherwise
+ */
+int intel_atomic_setup_scalers(struct drm_device *dev,
+	struct intel_crtc *intel_crtc,
+	struct intel_crtc_state *crtc_state)
+{
+	struct drm_plane *plane = NULL;
+	struct intel_plane *intel_plane;
+	struct intel_plane_state *plane_state = NULL;
+	struct intel_crtc_scaler_state *scaler_state;
+	struct drm_atomic_state *drm_state;
+	int num_scalers_need;
+	int i, j;
+
+	if (INTEL_INFO(dev)->gen < 9 || !intel_crtc || !crtc_state)
+		return 0;
+
+	scaler_state = &crtc_state->scaler_state;
+	drm_state = crtc_state->base.state;
+
+	num_scalers_need = hweight32(scaler_state->scaler_users);
+	DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users = 0x%x\n",
+		crtc_state, num_scalers_need, scaler_state->num_scalers,
+		scaler_state->scaler_users);
+
+	/* if there is no change in scaler configuration, return */
+	if (scaler_state->scaler_users ==
+		intel_crtc->config->scaler_state.scaler_users)
+		return 0;
+
+	/*
+	 * High level flow:
+	 * - staged scaler requests are already in scaler_state->scaler_users
+	 * - check whether staged scaling requests can be supported
+	 * - add planes using scalers that aren't in current transaction
+	 * - assign scalers to requested users
+	 * - as part of plane commit, scalers will be committed
+	 *   (i.e., either attached or detached) to respective planes in hw
+	 * - as part of crtc_commit, scaler will be either attached or detached
+	 *   to crtc in hw
+	 */
+
+	/* fail if required scalers > available scalers */
+	if (num_scalers_need > scaler_state->num_scalers){
+		DRM_DEBUG_KMS("Too many scaling requests %d > %d\n",
+			num_scalers_need, scaler_state->num_scalers);
+		return -EINVAL;
+	}
+
+	/* walkthrough scaler_users bits and start assigning scalers */
+	for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
+		int *scaler_id;
+
+		/* skip if scaler not required */
+		if (!(scaler_state->scaler_users & (1 << i)))
+			continue;
+
+		if (i == SKL_CRTC_INDEX) {
+			/* panel fitter case: assign as a crtc scaler */
+			scaler_id = &scaler_state->scaler_id;
+		} else {
+			if (!drm_state)
+				continue;
+
+			/* plane scaler case: assign as a plane scaler */
+			/* find the plane that set the bit as scaler_user */
+			plane = drm_state->planes[i];
+
+			/*
+			 * to enable/disable hq mode, add planes that are using scaler
+			 * into this transaction
+			 */
+			if (!plane) {
+				struct drm_plane_state *state;
+				plane = drm_plane_from_index(dev, i);
+				state = drm_atomic_get_plane_state(drm_state, plane);
+				if (IS_ERR(state)) {
+					DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state\n",
+						plane->base.id);
+					return PTR_ERR(state);
+				}
+			}
+
+			intel_plane = to_intel_plane(plane);
+
+			/* plane on different crtc cannot be a scaler user of this crtc */
+			if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
+				continue;
+			}
+
+			plane_state = to_intel_plane_state(drm_state->plane_states[i]);
+			scaler_id = &plane_state->scaler_id;
+		}
+
+		if (*scaler_id < 0) {
+			/* find a free scaler */
+			for (j = 0; j < scaler_state->num_scalers; j++) {
+				if (!scaler_state->scalers[j].in_use) {
+					scaler_state->scalers[j].in_use = 1;
+					*scaler_id = scaler_state->scalers[j].id;
+					DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
+						intel_crtc->pipe,
+						i == SKL_CRTC_INDEX ? scaler_state->scaler_id :
+							plane_state->scaler_id,
+						i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
+						i == SKL_CRTC_INDEX ?  intel_crtc->base.base.id :
+						plane->base.id);
+					break;
+				}
+			}
+		}
+
+		if (WARN_ON(*scaler_id < 0)) {
+			DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n",
+				i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
+				i == SKL_CRTC_INDEX ? intel_crtc->base.base.id:plane->base.id);
+			continue;
+		}
+
+		/* set scaler mode */
+		scaler_state->scalers[*scaler_id].mode = (num_scalers_need == 1) ?
+			PS_SCALER_MODE_HQ : PS_SCALER_MODE_DYN;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e20ddd5..1381d11 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1407,6 +1407,9 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 
 	return to_intel_crtc_state(crtc_state);
 }
+int intel_atomic_setup_scalers(struct drm_device *dev,
+	struct intel_crtc *intel_crtc,
+	struct intel_crtc_state *crtc_state);
 
 /* intel_atomic_plane.c */
 struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
-- 
1.7.9.5

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

Re: [PATCH 10/20] drm/i915: Add atomic function to setup scalers scalers for a crtc.

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:39PM -0700, Chandra Konduru wrote:
> intel_atomic_setup_scalers sets up scalers based on staged scaling
> requests coming from a crtc and its planes. This function should be
> called from crtc level check path.
> 
> If staged requests are supportable, function assigns scalers to
> requested planes and crtc. This function also takes into account
> the current planes using scalers but not being part of this
> atomic state for optimal operation of scalers. Note that the scaler
> assignement itself is staged into crtc_state and respective
> plane_states for later commit after all checks have been done.
> 
> overall high level flow:
>  - scaler requests are staged into crtc_state by planes/crtc
>  - check whether staged scaling requests can be supported
>  - add planes using scalers that aren't in current transaction
>  - assign scalers to requested users
>  - as part of plane commit, scalers will be committed
>    (i.e., either attached or detached) to respective planes in hw
>  - as part of crtc_commit, scaler will be either attached or detached
>    to crtc in hw
> 
> v2:
> -removed a log message (me)
> -changed input parameter to crtc_state (me)
> 
> v3:
> -remove assigning plane_state returned by drm_atomic_get_plane_state (Matt)
> -fail if there is an error from drm_atomic_get_plane_state (Matt)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

So looking ahead through the patch series, it looks like the places you
call this are:
 * intel_crtc_compute_config() --- Will presumably move to check_crtc()
   once we're farther along with atomic conversion.
 * intel_atomic_check() --- Handles updates via atomic ioctl (will also
   handle legacy plane updates once we switch to full atomic helpers)
 * skylake_pfit_update()

Since we're on transitional plane helpers today (which don't create a
top-level atomic transaction and thus never call intel_atomic_check),
does this ever get called for legacy SetPlane() operations on today's
driver?  Is it correct to assume that the switch back to full atomic
helpers is a prereq for merging this, or am I overlooking something?


Matt

> ---
>  drivers/gpu/drm/i915/intel_atomic.c |  142 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |    3 +
>  2 files changed, 145 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 3903b90..fab1f13 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -241,3 +241,145 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
>  {
>  	drm_atomic_helper_crtc_destroy_state(crtc, state);
>  }
> +
> +/**
> + * intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
> + * @dev: DRM device
> + * @crtc: intel crtc
> + * @crtc_state: incoming crtc_state to validate and setup scalers
> + *
> + * This function sets up scalers based on staged scaling requests for
> + * a @crtc and its planes. It is called from crtc level check path. If request
> + * is a supportable request, it attaches scalers to requested planes and crtc.
> + *
> + * This function takes into account the current scaler(s) in use by any planes
> + * not being part of this atomic state
> + *
> + *  Returns:
> + *         0 - scalers were setup succesfully
> + *         error code - otherwise
> + */
> +int intel_atomic_setup_scalers(struct drm_device *dev,
> +	struct intel_crtc *intel_crtc,
> +	struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_plane *plane = NULL;
> +	struct intel_plane *intel_plane;
> +	struct intel_plane_state *plane_state = NULL;
> +	struct intel_crtc_scaler_state *scaler_state;
> +	struct drm_atomic_state *drm_state;
> +	int num_scalers_need;
> +	int i, j;
> +
> +	if (INTEL_INFO(dev)->gen < 9 || !intel_crtc || !crtc_state)
> +		return 0;
> +
> +	scaler_state = &crtc_state->scaler_state;
> +	drm_state = crtc_state->base.state;
> +
> +	num_scalers_need = hweight32(scaler_state->scaler_users);
> +	DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users = 0x%x\n",
> +		crtc_state, num_scalers_need, scaler_state->num_scalers,
> +		scaler_state->scaler_users);
> +
> +	/* if there is no change in scaler configuration, return */
> +	if (scaler_state->scaler_users ==
> +		intel_crtc->config->scaler_state.scaler_users)
> +		return 0;
> +
> +	/*
> +	 * High level flow:
> +	 * - staged scaler requests are already in scaler_state->scaler_users
> +	 * - check whether staged scaling requests can be supported
> +	 * - add planes using scalers that aren't in current transaction
> +	 * - assign scalers to requested users
> +	 * - as part of plane commit, scalers will be committed
> +	 *   (i.e., either attached or detached) to respective planes in hw
> +	 * - as part of crtc_commit, scaler will be either attached or detached
> +	 *   to crtc in hw
> +	 */
> +
> +	/* fail if required scalers > available scalers */
> +	if (num_scalers_need > scaler_state->num_scalers){
> +		DRM_DEBUG_KMS("Too many scaling requests %d > %d\n",
> +			num_scalers_need, scaler_state->num_scalers);
> +		return -EINVAL;
> +	}
> +
> +	/* walkthrough scaler_users bits and start assigning scalers */
> +	for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
> +		int *scaler_id;
> +
> +		/* skip if scaler not required */
> +		if (!(scaler_state->scaler_users & (1 << i)))
> +			continue;
> +
> +		if (i == SKL_CRTC_INDEX) {
> +			/* panel fitter case: assign as a crtc scaler */
> +			scaler_id = &scaler_state->scaler_id;
> +		} else {
> +			if (!drm_state)
> +				continue;
> +
> +			/* plane scaler case: assign as a plane scaler */
> +			/* find the plane that set the bit as scaler_user */
> +			plane = drm_state->planes[i];
> +
> +			/*
> +			 * to enable/disable hq mode, add planes that are using scaler
> +			 * into this transaction
> +			 */
> +			if (!plane) {
> +				struct drm_plane_state *state;
> +				plane = drm_plane_from_index(dev, i);
> +				state = drm_atomic_get_plane_state(drm_state, plane);
> +				if (IS_ERR(state)) {
> +					DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state\n",
> +						plane->base.id);
> +					return PTR_ERR(state);
> +				}
> +			}
> +
> +			intel_plane = to_intel_plane(plane);
> +
> +			/* plane on different crtc cannot be a scaler user of this crtc */
> +			if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
> +				continue;
> +			}
> +
> +			plane_state = to_intel_plane_state(drm_state->plane_states[i]);
> +			scaler_id = &plane_state->scaler_id;
> +		}
> +
> +		if (*scaler_id < 0) {
> +			/* find a free scaler */
> +			for (j = 0; j < scaler_state->num_scalers; j++) {
> +				if (!scaler_state->scalers[j].in_use) {
> +					scaler_state->scalers[j].in_use = 1;
> +					*scaler_id = scaler_state->scalers[j].id;
> +					DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
> +						intel_crtc->pipe,
> +						i == SKL_CRTC_INDEX ? scaler_state->scaler_id :
> +							plane_state->scaler_id,
> +						i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
> +						i == SKL_CRTC_INDEX ?  intel_crtc->base.base.id :
> +						plane->base.id);
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (WARN_ON(*scaler_id < 0)) {
> +			DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n",
> +				i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
> +				i == SKL_CRTC_INDEX ? intel_crtc->base.base.id:plane->base.id);
> +			continue;
> +		}
> +
> +		/* set scaler mode */
> +		scaler_state->scalers[*scaler_id].mode = (num_scalers_need == 1) ?
> +			PS_SCALER_MODE_HQ : PS_SCALER_MODE_DYN;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e20ddd5..1381d11 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1407,6 +1407,9 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
>  
>  	return to_intel_crtc_state(crtc_state);
>  }
> +int intel_atomic_setup_scalers(struct drm_device *dev,
> +	struct intel_crtc *intel_crtc,
> +	struct intel_crtc_state *crtc_state);
>  
>  /* intel_atomic_plane.c */
>  struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/20] drm/i915: Add atomic function to setup scalers scalers for a crtc.

[Konduru, Chandra]

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Thursday, April 02, 2015 4:05 PM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander
> Subject: Re: [PATCH 10/20] drm/i915: Add atomic function to setup scalers
> scalers for a crtc.
> 
> On Wed, Apr 01, 2015 at 07:59:39PM -0700, Chandra Konduru wrote:
> > intel_atomic_setup_scalers sets up scalers based on staged scaling
> > requests coming from a crtc and its planes. This function should be
> > called from crtc level check path.
> >
> > If staged requests are supportable, function assigns scalers to
> > requested planes and crtc. This function also takes into account the
> > current planes using scalers but not being part of this atomic state
> > for optimal operation of scalers. Note that the scaler assignement
> > itself is staged into crtc_state and respective plane_states for later
> > commit after all checks have been done.
> >
> > overall high level flow:
> >  - scaler requests are staged into crtc_state by planes/crtc
> >  - check whether staged scaling requests can be supported
> >  - add planes using scalers that aren't in current transaction
> >  - assign scalers to requested users
> >  - as part of plane commit, scalers will be committed
> >    (i.e., either attached or detached) to respective planes in hw
> >  - as part of crtc_commit, scaler will be either attached or detached
> >    to crtc in hw
> >
> > v2:
> > -removed a log message (me)
> > -changed input parameter to crtc_state (me)
> >
> > v3:
> > -remove assigning plane_state returned by drm_atomic_get_plane_state
> > (Matt) -fail if there is an error from drm_atomic_get_plane_state
> > (Matt)
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> 
> So looking ahead through the patch series, it looks like the places you call this
> are:
>  * intel_crtc_compute_config() --- Will presumably move to check_crtc()
>    once we're farther along with atomic conversion.
>  * intel_atomic_check() --- Handles updates via atomic ioctl (will also
>    handle legacy plane updates once we switch to full atomic helpers)
>  * skylake_pfit_update()
> 
> Since we're on transitional plane helpers today (which don't create a top-level
> atomic transaction and thus never call intel_atomic_check), does this ever get
> called for legacy SetPlane() operations on today's driver?  Is it correct to assume
> that the switch back to full atomic helpers is a prereq for merging this, or am I
> overlooking something?

With current state of tree:
- it is required and called for panel fitting.
- it will be called for plane scaling once update_plane ptr switched to full atomic 
   helpers. Until then not called.

> 
> 
> Matt
> 
> > ---
> >  drivers/gpu/drm/i915/intel_atomic.c |  142
> +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h    |    3 +
> >  2 files changed, 145 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > b/drivers/gpu/drm/i915/intel_atomic.c
> > index 3903b90..fab1f13 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -241,3 +241,145 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> > {
> >  	drm_atomic_helper_crtc_destroy_state(crtc, state);  }
> > +
> > +/**
> > + * intel_atomic_setup_scalers() - setup scalers for crtc per staged
> > +requests
> > + * @dev: DRM device
> > + * @crtc: intel crtc
> > + * @crtc_state: incoming crtc_state to validate and setup scalers
> > + *
> > + * This function sets up scalers based on staged scaling requests for
> > + * a @crtc and its planes. It is called from crtc level check path.
> > +If request
> > + * is a supportable request, it attaches scalers to requested planes and crtc.
> > + *
> > + * This function takes into account the current scaler(s) in use by
> > +any planes
> > + * not being part of this atomic state
> > + *
> > + *  Returns:
> > + *         0 - scalers were setup succesfully
> > + *         error code - otherwise
> > + */
> > +int intel_atomic_setup_scalers(struct drm_device *dev,
> > +	struct intel_crtc *intel_crtc,
> > +	struct intel_crtc_state *crtc_state) {
> > +	struct drm_plane *plane = NULL;
> > +	struct intel_plane *intel_plane;
> > +	struct intel_plane_state *plane_state = NULL;
> > +	struct intel_crtc_scaler_state *scaler_state;
> > +	struct drm_atomic_state *drm_state;
> > +	int num_scalers_need;
> > +	int i, j;
> > +
> > +	if (INTEL_INFO(dev)->gen < 9 || !intel_crtc || !crtc_state)
> > +		return 0;
> > +
> > +	scaler_state = &crtc_state->scaler_state;
> > +	drm_state = crtc_state->base.state;
> > +
> > +	num_scalers_need = hweight32(scaler_state->scaler_users);
> > +	DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users =
> 0x%x\n",
> > +		crtc_state, num_scalers_need, scaler_state->num_scalers,
> > +		scaler_state->scaler_users);
> > +
> > +	/* if there is no change in scaler configuration, return */
> > +	if (scaler_state->scaler_users ==
> > +		intel_crtc->config->scaler_state.scaler_users)
> > +		return 0;
> > +
> > +	/*
> > +	 * High level flow:
> > +	 * - staged scaler requests are already in scaler_state->scaler_users
> > +	 * - check whether staged scaling requests can be supported
> > +	 * - add planes using scalers that aren't in current transaction
> > +	 * - assign scalers to requested users
> > +	 * - as part of plane commit, scalers will be committed
> > +	 *   (i.e., either attached or detached) to respective planes in hw
> > +	 * - as part of crtc_commit, scaler will be either attached or detached
> > +	 *   to crtc in hw
> > +	 */
> > +
> > +	/* fail if required scalers > available scalers */
> > +	if (num_scalers_need > scaler_state->num_scalers){
> > +		DRM_DEBUG_KMS("Too many scaling requests %d > %d\n",
> > +			num_scalers_need, scaler_state->num_scalers);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* walkthrough scaler_users bits and start assigning scalers */
> > +	for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
> > +		int *scaler_id;
> > +
> > +		/* skip if scaler not required */
> > +		if (!(scaler_state->scaler_users & (1 << i)))
> > +			continue;
> > +
> > +		if (i == SKL_CRTC_INDEX) {
> > +			/* panel fitter case: assign as a crtc scaler */
> > +			scaler_id = &scaler_state->scaler_id;
> > +		} else {
> > +			if (!drm_state)
> > +				continue;
> > +
> > +			/* plane scaler case: assign as a plane scaler */
> > +			/* find the plane that set the bit as scaler_user */
> > +			plane = drm_state->planes[i];
> > +
> > +			/*
> > +			 * to enable/disable hq mode, add planes that are using
> scaler
> > +			 * into this transaction
> > +			 */
> > +			if (!plane) {
> > +				struct drm_plane_state *state;
> > +				plane = drm_plane_from_index(dev, i);
> > +				state =
> drm_atomic_get_plane_state(drm_state, plane);
> > +				if (IS_ERR(state)) {
> > +					DRM_DEBUG_KMS("Failed to add
> [PLANE:%d] to drm_state\n",
> > +						plane->base.id);
> > +					return PTR_ERR(state);
> > +				}
> > +			}
> > +
> > +			intel_plane = to_intel_plane(plane);
> > +
> > +			/* plane on different crtc cannot be a scaler user of this
> crtc */
> > +			if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
> > +				continue;
> > +			}
> > +
> > +			plane_state = to_intel_plane_state(drm_state-
> >plane_states[i]);
> > +			scaler_id = &plane_state->scaler_id;
> > +		}
> > +
> > +		if (*scaler_id < 0) {
> > +			/* find a free scaler */
> > +			for (j = 0; j < scaler_state->num_scalers; j++) {
> > +				if (!scaler_state->scalers[j].in_use) {
> > +					scaler_state->scalers[j].in_use = 1;
> > +					*scaler_id = scaler_state->scalers[j].id;
> > +					DRM_DEBUG_KMS("Attached scaler id
> %u.%u to %s:%d\n",
> > +						intel_crtc->pipe,
> > +						i == SKL_CRTC_INDEX ?
> scaler_state->scaler_id :
> > +							plane_state-
> >scaler_id,
> > +						i == SKL_CRTC_INDEX ? "CRTC"
> : "PLANE",
> > +						i == SKL_CRTC_INDEX ?
> intel_crtc->base.base.id :
> > +						plane->base.id);
> > +					break;
> > +				}
> > +			}
> > +		}
> > +
> > +		if (WARN_ON(*scaler_id < 0)) {
> > +			DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n",
> > +				i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
> > +				i == SKL_CRTC_INDEX ? intel_crtc-
> >base.base.id:plane->base.id);
> > +			continue;
> > +		}
> > +
> > +		/* set scaler mode */
> > +		scaler_state->scalers[*scaler_id].mode = (num_scalers_need ==
> 1) ?
> > +			PS_SCALER_MODE_HQ : PS_SCALER_MODE_DYN;
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index e20ddd5..1381d11 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1407,6 +1407,9 @@ intel_atomic_get_crtc_state(struct
> > drm_atomic_state *state,
> >
> >  	return to_intel_crtc_state(crtc_state);  }
> > +int intel_atomic_setup_scalers(struct drm_device *dev,
> > +	struct intel_crtc *intel_crtc,
> > +	struct intel_crtc_state *crtc_state);
> >
> >  /* intel_atomic_plane.c */
> >  struct intel_plane_state *intel_create_plane_state(struct drm_plane
> > *plane);
> > --
> > 1.7.9.5
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/20] drm/i915: Helper function to detach a scaler from a plane or crtc

[Chandra Konduru]

This function is called from commit path of a plane or crtc.
It programs scaler registers to detach (aka. unbinds) scaler
from requested plane or crtc if it isn't in use. It also resets
scaler_id in crtc/plane state.

v2:
-improved a log message (me)

v3:
-improved commentary (Matt)
-added a case where scaler id needs to be reset (me)

v4:
-changes made not to modify state in commit path (Matt)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   29 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 603a2dc..8cf0d0e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2938,6 +2938,35 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 	return i915_gem_obj_ggtt_offset_view(obj, view);
 }
 
+/*
+ * This function detaches (aka. unbinds) unused scalers in hardware
+ */
+void skl_detach_scalers(struct intel_crtc *intel_crtc)
+{
+	struct drm_device *dev;
+	struct drm_i915_private *dev_priv;
+	struct intel_crtc_scaler_state *scaler_state;
+	int i;
+
+	if (!intel_crtc || !intel_crtc->config)
+		return;
+
+	dev = intel_crtc->base.dev;
+	dev_priv = dev->dev_private;
+	scaler_state = &intel_crtc->config->scaler_state;
+
+	/* loop through and disable scalers that aren't in use */
+	for (i = 0; i < scaler_state->num_scalers; i++) {
+		if (!scaler_state->scalers[i].in_use) {
+			I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, i), 0);
+			I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, i), 0);
+			I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, i), 0);
+			DRM_DEBUG_KMS("CRTC:%d Disabled scaler id %u.%u\n",
+				intel_crtc->base.base.id, intel_crtc->pipe, i);
+		}
+	}
+}
+
 static void skylake_update_primary_plane(struct drm_crtc *crtc,
 					 struct drm_framebuffer *fb,
 					 int x, int y)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1381d11..7bb4c44 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1146,6 +1146,7 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
 int skl_update_scaler_users(struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
 	struct intel_plane_state *plane_state, int force_detach);
+void skl_detach_scalers(struct intel_crtc *intel_crtc);
 
 unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 				     struct drm_i915_gem_object *obj);
-- 
1.7.9.5

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

Re: [PATCH 11/20] drm/i915: Helper function to detach a scaler from a plane or crtc

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:40PM -0700, Chandra Konduru wrote:
> This function is called from commit path of a plane or crtc.
> It programs scaler registers to detach (aka. unbinds) scaler
> from requested plane or crtc if it isn't in use. It also resets
> scaler_id in crtc/plane state.

The last sentence here is no longer true, so you should probably remove
it to avoid confusion.

Otherwise,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> 
> v2:
> -improved a log message (me)
> 
> v3:
> -improved commentary (Matt)
> -added a case where scaler id needs to be reset (me)
> 
> v4:
> -changes made not to modify state in commit path (Matt)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 603a2dc..8cf0d0e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2938,6 +2938,35 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>  	return i915_gem_obj_ggtt_offset_view(obj, view);
>  }
>  
> +/*
> + * This function detaches (aka. unbinds) unused scalers in hardware
> + */
> +void skl_detach_scalers(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_device *dev;
> +	struct drm_i915_private *dev_priv;
> +	struct intel_crtc_scaler_state *scaler_state;
> +	int i;
> +
> +	if (!intel_crtc || !intel_crtc->config)
> +		return;
> +
> +	dev = intel_crtc->base.dev;
> +	dev_priv = dev->dev_private;
> +	scaler_state = &intel_crtc->config->scaler_state;
> +
> +	/* loop through and disable scalers that aren't in use */
> +	for (i = 0; i < scaler_state->num_scalers; i++) {
> +		if (!scaler_state->scalers[i].in_use) {
> +			I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, i), 0);
> +			I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, i), 0);
> +			I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, i), 0);
> +			DRM_DEBUG_KMS("CRTC:%d Disabled scaler id %u.%u\n",
> +				intel_crtc->base.base.id, intel_crtc->pipe, i);
> +		}
> +	}
> +}
> +
>  static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  					 struct drm_framebuffer *fb,
>  					 int x, int y)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1381d11..7bb4c44 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1146,6 +1146,7 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  int skl_update_scaler_users(struct intel_crtc *intel_crtc,
>  	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
>  	struct intel_plane_state *plane_state, int force_detach);
> +void skl_detach_scalers(struct intel_crtc *intel_crtc);
>  
>  unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>  				     struct drm_i915_gem_object *obj);
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/20] drm/i915: Preserve scaler state when clearing crtc_state

[Chandra Konduru]

crtc_state is cleared during mode set which wipes out complete
scaler state too. This is causing issues. To fix, ensure scaler
state is preserved because it contains not only crtc
scaler usage, but also planes using scalers on this crtc.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8cf0d0e..aae4a22 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10791,11 +10791,14 @@ static void
 clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 {
 	struct drm_crtc_state tmp_state;
+	struct intel_crtc_scaler_state scaler_state;
 
-	/* Clear only the intel specific part of the crtc state */
+	/* Clear only the intel specific part of the crtc state excluding scalers */
 	tmp_state = crtc_state->base;
+	scaler_state = crtc_state->scaler_state;
 	memset(crtc_state, 0, sizeof *crtc_state);
 	crtc_state->base = tmp_state;
+	crtc_state->scaler_state = scaler_state;
 }
 
 static struct intel_crtc_state *
-- 
1.7.9.5

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

[PATCH 13/20] drm/i915: use current scaler state during readout_hw_state.

[Chandra Konduru]

During readout_hw_state, rebuild crtc scaler_state from hw state:
 - crtc scaler id
 - scaler users
 - scaling ratios

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index aae4a22..834ea46 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8756,6 +8756,11 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 
 	intel_get_pipe_timings(crtc, pipe_config);
 
+	if (INTEL_INFO(dev)->gen >= 9) {
+		skl_init_scalers(dev, crtc->pipe, pipe_config);
+		skl_update_scaling_ratio(dev, pipe_config);
+	}
+
 	pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
 	if (intel_display_power_is_enabled(dev_priv, pfit_domain)) {
 		if (IS_SKYLAKE(dev))
-- 
1.7.9.5

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

Re: [PATCH 13/20] drm/i915: use current scaler state during readout_hw_state.

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:42PM -0700, Chandra Konduru wrote:
> During readout_hw_state, rebuild crtc scaler_state from hw state:
>  - crtc scaler id
>  - scaler users

This patch doesn't look like it actually does what you're advertising
here.  If your firmware or bootloader or whatever has programmed the
display in a way that some scalers are in use, I think your state
variables are still going to show all scalers as free after you're done
here, right?

>  - scaling ratios

I think this is the only thing that will actually match the hardware
state, but we've already noted that those state fields can probably just
go away since we can trivially calculate the proper values in the places
we're currently using them.


Matt

> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index aae4a22..834ea46 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8756,6 +8756,11 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  
>  	intel_get_pipe_timings(crtc, pipe_config);
>  
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		skl_init_scalers(dev, crtc->pipe, pipe_config);
> +		skl_update_scaling_ratio(dev, pipe_config);
> +	}
> +
>  	pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>  	if (intel_display_power_is_enabled(dev_priv, pfit_domain)) {
>  		if (IS_SKYLAKE(dev))
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/20] drm/i915: use current scaler state during readout_hw_state.

[Konduru, Chandra]

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Thursday, April 02, 2015 4:05 PM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander
> Subject: Re: [PATCH 13/20] drm/i915: use current scaler state during
> readout_hw_state.
> 
> On Wed, Apr 01, 2015 at 07:59:42PM -0700, Chandra Konduru wrote:
> > During readout_hw_state, rebuild crtc scaler_state from hw state:
> >  - crtc scaler id
> >  - scaler users
> 
> This patch doesn't look like it actually does what you're advertising here.  If your
> firmware or bootloader or whatever has programmed the display in a way that
> some scalers are in use, I think your state variables are still going to show all
> scalers as free after you're done here, right?

No, the readout changes were made in other patch (17). 
Due to revisions it left out like this. Will squash this into 17.

> 
> >  - scaling ratios
> 
> I think this is the only thing that will actually match the hardware state, but
> we've already noted that those state fields can probably just go away since we
> can trivially calculate the proper values in the places we're currently using them.
> 
> 
> Matt
> 
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index aae4a22..834ea46 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8756,6 +8756,11 @@ static bool haswell_get_pipe_config(struct
> > intel_crtc *crtc,
> >
> >  	intel_get_pipe_timings(crtc, pipe_config);
> >
> > +	if (INTEL_INFO(dev)->gen >= 9) {
> > +		skl_init_scalers(dev, crtc->pipe, pipe_config);
> > +		skl_update_scaling_ratio(dev, pipe_config);
> > +	}
> > +
> >  	pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> >  	if (intel_display_power_is_enabled(dev_priv, pfit_domain)) {
> >  		if (IS_SKYLAKE(dev))
> > --
> > 1.7.9.5
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 14/20] drm/i915: Update scaling ratio as part of crtc_compute_config

[Chandra Konduru]

Based on computed crtc config, stage any updates to scaling ratios.

Also call intel_atomic_setup_scalers() to stage scaler assignments
if crtc compute config staged any changes to its scaler needs.

Above actions should be moved to atomic crtc once it is available.

v2:
-moved gen comparision check to caller (Matt)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 834ea46..166b9d6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5863,6 +5863,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
+	int ret;
 
 	/* FIXME should check pixel clock limits on all platforms */
 	if (INTEL_INFO(dev)->gen < 4) {
@@ -5917,7 +5918,17 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 	if (pipe_config->has_pch_encoder)
 		return ironlake_fdi_compute_config(crtc, pipe_config);
 
-	return 0;
+	if (INTEL_INFO(dev)->gen >= 9)
+		skl_update_scaling_ratio(dev, pipe_config);
+
+	/* FIXME: remove below call once atomic mode set is place and all crtc
+	 * related checks called from atomic_crtc_check function */
+	ret = 0;
+	DRM_DEBUG_KMS("intel_crtc = %p drm_state (pipe_config->base.state) = %p\n",
+		crtc, pipe_config->base.state);
+	ret = intel_atomic_setup_scalers(dev, crtc, pipe_config);
+
+	return ret;
 }
 
 static int skylake_get_display_clock_speed(struct drm_device *dev)
-- 
1.7.9.5

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

[PATCH 15/20] drm/i915: Ensure setting up scalers into staged crtc_state

[Chandra Konduru]

From intel_atomic_check, call intel_atomic_setup_scalers() to
assign scalers based on staged scaling requests. Fail the
transaction if setup returns error.

Setting up of scalers should be moved to atomic crtc check  once
atomic crtc is ready.

v2:
-updated parameter passing to setup_scalers (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index fab1f13..0b21cd6 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -48,6 +48,8 @@ int intel_atomic_check(struct drm_device *dev,
 	int ncrtcs = dev->mode_config.num_crtc;
 	int nconnectors = dev->mode_config.num_connector;
 	enum pipe nuclear_pipe = INVALID_PIPE;
+	struct intel_crtc *nuclear_crtc = NULL;
+	struct intel_crtc_state *crtc_state = NULL;
 	int ret;
 	int i;
 	bool not_nuclear = false;
@@ -78,6 +80,10 @@ int intel_atomic_check(struct drm_device *dev,
 		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
 		if (crtc && crtc->pipe != nuclear_pipe)
 			not_nuclear = true;
+		if (crtc && crtc->pipe == nuclear_pipe) {
+			nuclear_crtc = crtc;
+			crtc_state = to_intel_crtc_state(state->crtc_states[i]);
+		}
 	}
 	for (i = 0; i < nconnectors; i++)
 		if (state->connectors[i] != NULL)
@@ -92,6 +98,11 @@ int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	/* FIXME: move to crtc atomic check function once it is ready */
+	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
+	if (ret)
+		return ret;
+
 	return ret;
 }
 
-- 
1.7.9.5

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

[PATCH 16/20] drm/i915: copy staged scaler state from drm state to crtc->config.

[Chandra Konduru]

This is required for commit to perform as per staged assignment
of scalers until atomic crtc commit function is available.

As a place holder doing this copy from intel_atomic_commit for
scaling to operate correctly.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 0b21cd6..7b8efd4 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -166,6 +166,18 @@ int intel_atomic_commit(struct drm_device *dev,
 		swap(state->plane_states[i], plane->state);
 		plane->state->state = NULL;
 	}
+
+	/* swap crtc_state */
+	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+		struct drm_crtc *crtc = state->crtcs[i];
+		if (!crtc) {
+			continue;
+		}
+
+		to_intel_crtc(crtc)->config->scaler_state =
+			to_intel_crtc_state(state->crtc_states[i])->scaler_state;
+	}
+
 	drm_atomic_helper_commit_planes(dev, state);
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
-- 
1.7.9.5

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

[PATCH 17/20] drm/i915: stage panel fitting scaler request for fixed mode panel

[Chandra Konduru]

This patch calls skl_update_scaler_users() to stage a panel fitting
request for fixed mode panel.

v2:
-move gen check to caller (Matt)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7936155..c19698f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1351,6 +1351,14 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
 		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
 				       adjusted_mode);
+
+		if (INTEL_INFO(dev)->gen >= 9) {
+			int ret;
+			ret = skl_update_scaler_users(intel_crtc, pipe_config, NULL, NULL, 0);
+			if (ret)
+				return ret;
+		}
+
 		if (!HAS_PCH_SPLIT(dev))
 			intel_gmch_panel_fitting(intel_crtc, pipe_config,
 						 intel_connector->panel.fitting_mode);
-- 
1.7.9.5

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

[PATCH 18/20] drm/i915: Enable skylake panel fitting using skylake shared scalers

[Chandra Konduru]

Modify skylake panel fitting implementation to use shared scalers.

v2:
-added log message in pfit enable/disable (me)
-read crtc scaler state from hw state (Daniel)
-replaced both skylake_pfit_enable and disable with skylake_pfit_update (me)
-added scaler id check to intel_pipe_config_compare (Daniel)

v3:
-use updated detach scalers function (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   81 ++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 166b9d6..664b7fb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4282,16 +4282,39 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
 	}
 }
 
-static void skylake_pfit_enable(struct intel_crtc *crtc)
+static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe = crtc->pipe;
+	struct intel_crtc_scaler_state *scaler_state =
+		&crtc->config->scaler_state;
+
+	DRM_DEBUG_KMS("for crtc_state = %p\n", crtc->config);
+
+	/* To update pfit, first update scaler state */
+	skl_update_scaler_users(crtc, crtc->config, NULL, NULL, !enable);
+	intel_atomic_setup_scalers(crtc->base.dev, crtc, crtc->config);
+	skl_detach_scalers(crtc);
+	if (!enable)
+		return;
 
 	if (crtc->config->pch_pfit.enabled) {
-		I915_WRITE(PS_CTL(pipe), PS_ENABLE);
-		I915_WRITE(PS_WIN_POS(pipe), crtc->config->pch_pfit.pos);
-		I915_WRITE(PS_WIN_SZ(pipe), crtc->config->pch_pfit.size);
+		int id;
+
+		if (WARN_ON(crtc->config->scaler_state.scaler_id < 0)) {
+			DRM_ERROR("Requesting pfit without getting a scaler first\n");
+			return;
+		}
+
+		id = scaler_state->scaler_id;
+		I915_WRITE(SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
+			scaler_state->scalers[id].mode |
+			scaler_state->scalers[id].filter);
+		I915_WRITE(SKL_PS_WIN_POS(pipe, id), crtc->config->pch_pfit.pos);
+		I915_WRITE(SKL_PS_WIN_SZ(pipe, id), crtc->config->pch_pfit.size);
+
+		DRM_DEBUG_KMS("for crtc_state = %p scaler_id = %d\n", crtc->config, id);
 	}
 }
 
@@ -4721,7 +4744,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_ddi_enable_pipe_clock(intel_crtc);
 
 	if (IS_SKYLAKE(dev))
-		skylake_pfit_enable(intel_crtc);
+		skylake_pfit_update(intel_crtc, 1);
 	else
 		ironlake_pfit_enable(intel_crtc);
 
@@ -4757,21 +4780,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_enable_planes(crtc);
 }
 
-static void skylake_pfit_disable(struct intel_crtc *crtc)
-{
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipe = crtc->pipe;
-
-	/* To avoid upsetting the power well on haswell only disable the pfit if
-	 * it's in use. The hw state code will make sure we get this right. */
-	if (crtc->config->pch_pfit.enabled) {
-		I915_WRITE(PS_CTL(pipe), 0);
-		I915_WRITE(PS_WIN_POS(pipe), 0);
-		I915_WRITE(PS_WIN_SZ(pipe), 0);
-	}
-}
-
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -4884,7 +4892,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
 
 	if (IS_SKYLAKE(dev))
-		skylake_pfit_disable(intel_crtc);
+		skylake_pfit_update(intel_crtc, 0);
 	else
 		ironlake_pfit_disable(intel_crtc);
 
@@ -8126,14 +8134,28 @@ static void skylake_get_pfit_config(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t tmp;
+	struct intel_crtc_scaler_state *scaler_state = &pipe_config->scaler_state;
+	uint32_t ps_ctrl = 0;
+	int id = -1;
+	int i;
 
-	tmp = I915_READ(PS_CTL(crtc->pipe));
+	/* find scaler attached to this pipe */
+	for (i = 0; i < scaler_state->num_scalers; i++) {
+		ps_ctrl = I915_READ(SKL_PS_CTRL(crtc->pipe, i));
+		if (ps_ctrl & PS_SCALER_EN && !(ps_ctrl & PS_PLANE_SEL_MASK)) {
+			id = i;
+			pipe_config->pch_pfit.enabled = true;
+			pipe_config->pch_pfit.pos = I915_READ(SKL_PS_WIN_POS(crtc->pipe, i));
+			pipe_config->pch_pfit.size = I915_READ(SKL_PS_WIN_SZ(crtc->pipe, i));
+			break;
+		}
+	}
 
-	if (tmp & PS_ENABLE) {
-		pipe_config->pch_pfit.enabled = true;
-		pipe_config->pch_pfit.pos = I915_READ(PS_WIN_POS(crtc->pipe));
-		pipe_config->pch_pfit.size = I915_READ(PS_WIN_SZ(crtc->pipe));
+	scaler_state->scaler_id = id;
+	if (id >= 0) {
+		scaler_state->scaler_users |= (1 << SKL_CRTC_INDEX);
+	} else {
+		scaler_state->scaler_users &= ~(1 << SKL_CRTC_INDEX);
 	}
 }
 
@@ -8778,6 +8800,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 			skylake_get_pfit_config(crtc, pipe_config);
 		else
 			ironlake_get_pfit_config(crtc, pipe_config);
+	} else {
+		pipe_config->scaler_state.scaler_id = -1;
+		pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX);
 	}
 
 	if (IS_HASWELL(dev))
@@ -11284,6 +11309,8 @@ intel_pipe_config_compare(struct drm_device *dev,
 		PIPE_CONF_CHECK_I(pch_pfit.size);
 	}
 
+	PIPE_CONF_CHECK_I(scaler_state.scaler_id);
+
 	/* BDW+ don't expose a synchronous way to read the state */
 	if (IS_HASWELL(dev))
 		PIPE_CONF_CHECK_I(ips_enabled);
-- 
1.7.9.5

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

[PATCH 19/20] drm/i915: Enable skylake primary plane scaling using shared scalers

[Chandra Konduru]

This patch enables skylake primary plane display scaling using shared
scalers atomic desgin.

v2:
-use single copy of scaler limits (Matt)

v3:
-move detach_scalers to crtc commit path (Matt)
-use values in plane_state->src as regular integers (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |    5 ++-
 drivers/gpu/drm/i915/intel_display.c |   72 +++++++++++++++++++++++++++++++---
 2 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 7b8efd4..f14c60e 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -167,7 +167,7 @@ int intel_atomic_commit(struct drm_device *dev,
 		plane->state->state = NULL;
 	}
 
-	/* swap crtc_state */
+	/* swap crtc_scaler_state */
 	for (i = 0; i < dev->mode_config.num_crtc; i++) {
 		struct drm_crtc *crtc = state->crtcs[i];
 		if (!crtc) {
@@ -176,6 +176,9 @@ int intel_atomic_commit(struct drm_device *dev,
 
 		to_intel_crtc(crtc)->config->scaler_state =
 			to_intel_crtc_state(state->crtc_states[i])->scaler_state;
+
+		if (INTEL_INFO(dev)->gen >= 9)
+			skl_detach_scalers(to_intel_crtc(crtc));
 	}
 
 	drm_atomic_helper_commit_planes(dev, state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 664b7fb..4e9d9f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2978,6 +2978,14 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl, stride_div;
 	unsigned long surf_addr;
+	struct intel_crtc_state *crtc_state = intel_crtc->config;
+	struct intel_plane_state *plane_state;
+	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
+	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
+	int scaler_id = -1;
+
+	plane_state = crtc->primary ?
+		to_intel_plane_state(crtc->primary->state) : NULL;
 
 	if (!intel_crtc->primary_enabled) {
 		I915_WRITE(PLANE_CTL(pipe, 0), 0);
@@ -3046,12 +3054,41 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 					       fb->pixel_format);
 	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
 
+	if (plane_state) {
+		scaler_id = plane_state->scaler_id;
+		src_x = plane_state->src.x1;
+		src_y = plane_state->src.y1;
+		src_w = drm_rect_width(&plane_state->src);
+		src_h = drm_rect_height(&plane_state->src);
+		dst_x = plane_state->dst.x1;
+		dst_y = plane_state->dst.y1;
+		dst_w = drm_rect_width(&plane_state->dst);
+		dst_h = drm_rect_height(&plane_state->dst);
+	}
+
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_POS(pipe, 0), 0);
+
+	if (src_w && src_h && dst_w && dst_h && scaler_id >= 0) {
+		uint32_t ps_ctrl = 0;
+
+		WARN_ON(x != src_x || y != src_y);
+		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(0) |
+			crtc_state->scaler_state.scalers[scaler_id].mode |
+			crtc_state->scaler_state.scalers[scaler_id].filter;
+		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
+		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
+		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (dst_x << 16) | dst_y);
+		I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id), (dst_w << 16) | dst_h);
+
+		I915_WRITE(PLANE_SIZE(pipe, 0), ((src_h - 1) << 16) | (src_w - 1));
+	} else {
+		I915_WRITE(PLANE_SIZE(pipe, 0),
+			(intel_crtc->config->pipe_src_h - 1) << 16 |
+			(intel_crtc->config->pipe_src_w - 1));
+	}
+
 	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
-	I915_WRITE(PLANE_SIZE(pipe, 0),
-		   (intel_crtc->config->pipe_src_h - 1) << 16 |
-		   (intel_crtc->config->pipe_src_w - 1));
 	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
 	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
 
@@ -12821,19 +12858,33 @@ intel_check_primary_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *crtc_state;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
+	struct intel_crtc_scaler_state *scaler_state;
+	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
+	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
 	int ret;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
+	crtc_state = state->base.state ?
+		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
+	scaler_state = crtc_state ? &crtc_state->scaler_state : NULL;
+
+	if (INTEL_INFO(dev)->gen >= 9) {
+		if (scaler_state && scaler_state->num_scalers) {
+			min_scale = 1;
+			max_scale = (100 << 16) / scaler_state->min_hsr;
+		}
+	}
 
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    DRM_PLANE_HELPER_NO_SCALING,
+					    min_scale,
+					    max_scale,
 					    false, true, &state->visible);
 	if (ret)
 		return ret;
@@ -12886,6 +12937,13 @@ intel_check_primary_plane(struct drm_plane *plane,
 	src->y1 >>= 16;
 	src->y2 >>= 16;
 
+	if (INTEL_INFO(dev)->gen >= 9) {
+		ret = skl_update_scaler_users(intel_crtc, crtc_state,
+			to_intel_plane(plane), state, 0);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -13065,6 +13123,10 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 
 	primary->can_scale = false;
 	primary->max_downscale = 1;
+	if (INTEL_INFO(dev)->gen >= 9) {
+		primary->can_scale = true;
+		primary->max_downscale = 2; /* updated later */
+	}
 	state->scaler_id = -1;
 	primary->pipe = pipe;
 	primary->plane = pipe;
-- 
1.7.9.5

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

Re: [PATCH 19/20] drm/i915: Enable skylake primary plane scaling using shared scalers

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:48PM -0700, Chandra Konduru wrote:
> This patch enables skylake primary plane display scaling using shared
> scalers atomic desgin.
> 
> v2:
> -use single copy of scaler limits (Matt)
> 
> v3:
> -move detach_scalers to crtc commit path (Matt)
> -use values in plane_state->src as regular integers (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |    5 ++-
>  drivers/gpu/drm/i915/intel_display.c |   72 +++++++++++++++++++++++++++++++---
>  2 files changed, 71 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 7b8efd4..f14c60e 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -167,7 +167,7 @@ int intel_atomic_commit(struct drm_device *dev,
>  		plane->state->state = NULL;
>  	}
>  
> -	/* swap crtc_state */
> +	/* swap crtc_scaler_state */
>  	for (i = 0; i < dev->mode_config.num_crtc; i++) {
>  		struct drm_crtc *crtc = state->crtcs[i];
>  		if (!crtc) {
> @@ -176,6 +176,9 @@ int intel_atomic_commit(struct drm_device *dev,
>  
>  		to_intel_crtc(crtc)->config->scaler_state =
>  			to_intel_crtc_state(state->crtc_states[i])->scaler_state;
> +
> +		if (INTEL_INFO(dev)->gen >= 9)
> +			skl_detach_scalers(to_intel_crtc(crtc));
>  	}
>  
>  	drm_atomic_helper_commit_planes(dev, state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 664b7fb..4e9d9f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2978,6 +2978,14 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl, stride_div;
>  	unsigned long surf_addr;
> +	struct intel_crtc_state *crtc_state = intel_crtc->config;
> +	struct intel_plane_state *plane_state;
> +	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
> +	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
> +	int scaler_id = -1;
> +
> +	plane_state = crtc->primary ?
> +		to_intel_plane_state(crtc->primary->state) : NULL;
>  
>  	if (!intel_crtc->primary_enabled) {
>  		I915_WRITE(PLANE_CTL(pipe, 0), 0);
> @@ -3046,12 +3054,41 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  					       fb->pixel_format);
>  	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
>  
> +	if (plane_state) {
> +		scaler_id = plane_state->scaler_id;
> +		src_x = plane_state->src.x1;
> +		src_y = plane_state->src.y1;
> +		src_w = drm_rect_width(&plane_state->src);
> +		src_h = drm_rect_height(&plane_state->src);
> +		dst_x = plane_state->dst.x1;
> +		dst_y = plane_state->dst.y1;
> +		dst_w = drm_rect_width(&plane_state->dst);
> +		dst_h = drm_rect_height(&plane_state->dst);
> +	}
> +
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_POS(pipe, 0), 0);
> +
> +	if (src_w && src_h && dst_w && dst_h && scaler_id >= 0) {
> +		uint32_t ps_ctrl = 0;
> +
> +		WARN_ON(x != src_x || y != src_y);
> +		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(0) |
> +			crtc_state->scaler_state.scalers[scaler_id].mode |
> +			crtc_state->scaler_state.scalers[scaler_id].filter;
> +		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
> +		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
> +		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (dst_x << 16) | dst_y);
> +		I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id), (dst_w << 16) | dst_h);
> +
> +		I915_WRITE(PLANE_SIZE(pipe, 0), ((src_h - 1) << 16) | (src_w - 1));
> +	} else {
> +		I915_WRITE(PLANE_SIZE(pipe, 0),
> +			(intel_crtc->config->pipe_src_h - 1) << 16 |
> +			(intel_crtc->config->pipe_src_w - 1));
> +	}
> +
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> -	I915_WRITE(PLANE_SIZE(pipe, 0),
> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
> -		   (intel_crtc->config->pipe_src_w - 1));
>  	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
>  	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
>  
> @@ -12821,19 +12858,33 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state;
>  	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> +	struct intel_crtc_scaler_state *scaler_state;
> +	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
> +	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
>  	int ret;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
> +	crtc_state = state->base.state ?
> +		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
> +	scaler_state = crtc_state ? &crtc_state->scaler_state : NULL;
> +
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		if (scaler_state && scaler_state->num_scalers) {
> +			min_scale = 1;
> +			max_scale = (100 << 16) / scaler_state->min_hsr;
> +		}
> +	}
>  
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
> -					    DRM_PLANE_HELPER_NO_SCALING,
> -					    DRM_PLANE_HELPER_NO_SCALING,
> +					    min_scale,
> +					    max_scale,
>  					    false, true, &state->visible);
>  	if (ret)
>  		return ret;
> @@ -12886,6 +12937,13 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	src->y1 >>= 16;
>  	src->y2 >>= 16;
>  
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		ret = skl_update_scaler_users(intel_crtc, crtc_state,
> +			to_intel_plane(plane), state, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -13065,6 +13123,10 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  
>  	primary->can_scale = false;
>  	primary->max_downscale = 1;
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		primary->can_scale = true;
> +		primary->max_downscale = 2; /* updated later */

I don't see this ever updated again in this patch series?  As far as I
can tell, this value only gets used in intel_check_sprite_plane(), so
it's never actually used for primary plane code.  Can we just drop the
assignment completely?


Matt

> +	}
>  	state->scaler_id = -1;
>  	primary->pipe = pipe;
>  	primary->plane = pipe;
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 20/20] drm/i915: Enable skylake sprite plane scaling using shared scalers

[Chandra Konduru]

This patch enables skylake sprite plane display scaling using shared
scalers atomic desgin.

v2:
-use single copy of scaler limits (Matt)

v3:
-detaching scalers moved to crtc commit path (Matt)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c |   60 +++++++++++++++++++++++++++++------
 1 file changed, 50 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c8feff7..562f90c 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -33,6 +33,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_plane_helper.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
@@ -194,6 +195,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
 	unsigned long surf_addr;
+	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
+	int scaler_id;
 
 	plane_ctl = PLANE_CTL_ENABLE |
 		PLANE_CTL_PIPE_CSC_ENABLE;
@@ -264,6 +267,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
 					       fb->pixel_format);
 
+	scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
+
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
@@ -283,10 +288,30 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 
 	surf_addr = intel_plane_obj_offset(intel_plane, obj);
 
+	/* program plane scaler */
+	if (scaler_id >= 0) {
+		uint32_t ps_ctrl = 0;
+
+		DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n", plane,
+			PS_PLANE_SEL(plane));
+		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) |
+			crtc_state->scaler_state.scalers[scaler_id].mode |
+			crtc_state->scaler_state.scalers[scaler_id].filter;
+		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
+		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
+		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
+		I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id),
+			((crtc_w + 1) << 16)|(crtc_h + 1));
+
+		I915_WRITE(PLANE_POS(pipe, plane), 0);
+		I915_WRITE(PLANE_SIZE(pipe, plane), (src_h << 16) | src_w);
+	} else {
+		I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
+		I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
+	}
+
 	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
 	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
-	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
-	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
 	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
 	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
 	POSTING_READ(PLANE_SURF(pipe, plane));
@@ -863,7 +888,9 @@ static int
 intel_check_sprite_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
 {
+	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
+	struct intel_crtc_state *crtc_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	int crtc_x, crtc_y;
@@ -872,11 +899,16 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	struct drm_rect *dst = &state->dst;
 	const struct drm_rect *clip = &state->clip;
+	struct intel_crtc_scaler_state *scaler_state;
 	int hscale, vscale;
 	int max_scale, min_scale;
 	int pixel_size;
+	int ret;
 
 	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
+	crtc_state = state->base.state ?
+		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
+	scaler_state = crtc_state ? &crtc_state->scaler_state : NULL;
 
 	if (!fb) {
 		state->visible = false;
@@ -903,6 +935,11 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	max_scale = intel_plane->max_downscale << 16;
 	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
 
+	if (INTEL_INFO(dev)->gen >= 9 && scaler_state && scaler_state->num_scalers) {
+		min_scale = 1;
+		max_scale = (100 << 16) / scaler_state->min_hsr;
+	}
+
 	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
 			state->base.rotation);
 
@@ -998,8 +1035,8 @@ intel_check_sprite_plane(struct drm_plane *plane,
 		width_bytes = ((src_x * pixel_size) & 63) +
 					src_w * pixel_size;
 
-		if (src_w > 2048 || src_h > 2048 ||
-		    width_bytes > 4096 || fb->pitches[0] > 4096) {
+		if (INTEL_INFO(dev)->gen < 9 && (src_w > 2048 || src_h > 2048 ||
+		    width_bytes > 4096 || fb->pitches[0] > 4096)) {
 			DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
 			return -EINVAL;
 		}
@@ -1053,6 +1090,13 @@ finish:
 		}
 	}
 
+	if (INTEL_INFO(dev)->gen >= 9) {
+		ret = skl_update_scaler_users(intel_crtc, crtc_state, intel_plane,
+			state, 0);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -1264,12 +1308,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		}
 		break;
 	case 9:
-		/*
-		 * FIXME: Skylake planes can be scaled (with some restrictions),
-		 * but this is for another time.
-		 */
-		intel_plane->can_scale = false;
-		intel_plane->max_downscale = 1;
+		intel_plane->can_scale = true;
+		intel_plane->max_downscale = 2; /* updated later */
 		intel_plane->update_plane = skl_update_plane;
 		intel_plane->disable_plane = skl_disable_plane;
 		state->scaler_id = -1;
-- 
1.7.9.5

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

Re: [PATCH 20/20] drm/i915: Enable skylake sprite plane scaling using shared scalers

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6118
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              272/272              271/272
ILK                                  302/302              302/302
SNB                                  303/303              303/303
IVB                                  338/338              338/338
BYT                 -1              287/287              286/287
HSW                                  361/361              361/361
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt@gen3_render_tiledx_blits      FAIL(6)PASS(3)      FAIL(2)
*BYT  igt@gem_exec_bad_domains@conflicting-write-domain      PASS(9)      FAIL(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 20/20] drm/i915: Enable skylake sprite plane scaling using shared scalers

[Konduru, Chandra]

Hi Shuang,
Looking at starting with '*':
*BYT  igt@gem_exec_bad_domains@conflicting-write-domain      PASS(9) FAIL(1)PASS(1)

Above failure seems unrelated to my patch series. I suspect this 
pre-exist before my changes.
Can you double check whether above failure is pre-existing before 
any action is taken from my side?

Also I just ran it on skl (that's the system I have) without any failures:
~/gfx/sources/intel-gpu-tools/tests$ sudo ./gem_exec_bad_domains 
IGT-Version: 1.10-g4f076bc (x86_64) (Linux: 4.0.0-rc3+ x86_64)
Subtest cpu-domain: SUCCESS (0.004s)
Subtest gtt-domain: SUCCESS (0.000s)
Subtest conflicting-write-domain: SUCCESS (0.000s)
Subtest double-write-domain: SUCCESS (0.000s)
Subtest invalid-gpu-domain: SUCCESS (0.000s)
~/gfx/sources/intel-gpu-tools/tests$


-Chandra

> -----Original Message-----
> From: He, Shuang
> Sent: Thursday, April 02, 2015 7:45 AM
> To: He, Shuang; Gao, Ethan; intel-gfx@lists.freedesktop.org; Konduru, Chandra
> Subject: RE: [Intel-gfx] [PATCH 20/20] drm/i915: Enable skylake sprite plane
> scaling using shared scalers
> 
> Tested-By: PRC QA PRTS (Patch Regression Test System Contact:
> shuang.he@intel.com) Task id: 6118
> -------------------------------------Summary-------------------------------------
> Platform          Delta          drm-intel-nightly          Series Applied
> PNV                 -1              272/272              271/272
> ILK                                  302/302              302/302
> SNB                                  303/303              303/303
> IVB                                  338/338              338/338
> BYT                 -1              287/287              286/287
> HSW                                  361/361              361/361
> BDW                                  308/308              308/308
> -------------------------------------Detailed-------------------------------------
> Platform  Test                                drm-intel-nightly          Series Applied
>  PNV  igt@gen3_render_tiledx_blits      FAIL(6)PASS(3)      FAIL(2)
> *BYT  igt@gem_exec_bad_domains@conflicting-write-domain      PASS(9)
> FAIL(1)PASS(1)
> Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 20/20] drm/i915: Enable skylake sprite plane scaling using shared scalers

[He, Shuang]

Hi, Chandra
	Yes, I agree
	There is some noise pre-existing bugs, you can ignore it for now. We're figuring out an way to filter them out

Thanks
	--Shuang

> -----Original Message-----
> From: Konduru, Chandra
> Sent: Friday, April 3, 2015 1:21 AM
> To: He, Shuang; Gao, Ethan; intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 20/20] drm/i915: Enable skylake sprite plane
> scaling using shared scalers
> 
> Hi Shuang,
> Looking at starting with '*':
> *BYT  igt@gem_exec_bad_domains@conflicting-write-domain      PASS(9)
> FAIL(1)PASS(1)
> 
> Above failure seems unrelated to my patch series. I suspect this
> pre-exist before my changes.
> Can you double check whether above failure is pre-existing before
> any action is taken from my side?
> 
> Also I just ran it on skl (that's the system I have) without any failures:
> ~/gfx/sources/intel-gpu-tools/tests$ sudo ./gem_exec_bad_domains
> IGT-Version: 1.10-g4f076bc (x86_64) (Linux: 4.0.0-rc3+ x86_64)
> Subtest cpu-domain: SUCCESS (0.004s)
> Subtest gtt-domain: SUCCESS (0.000s)
> Subtest conflicting-write-domain: SUCCESS (0.000s)
> Subtest double-write-domain: SUCCESS (0.000s)
> Subtest invalid-gpu-domain: SUCCESS (0.000s)
> ~/gfx/sources/intel-gpu-tools/tests$
> 
> 
> -Chandra
> 
> > -----Original Message-----
> > From: He, Shuang
> > Sent: Thursday, April 02, 2015 7:45 AM
> > To: He, Shuang; Gao, Ethan; intel-gfx@lists.freedesktop.org; Konduru,
> Chandra
> > Subject: RE: [Intel-gfx] [PATCH 20/20] drm/i915: Enable skylake sprite plane
> > scaling using shared scalers
> >
> > Tested-By: PRC QA PRTS (Patch Regression Test System Contact:
> > shuang.he@intel.com) Task id: 6118
> > -------------------------------------Summary-------------------------------------
> > Platform          Delta          drm-intel-nightly          Series Applied
> > PNV                 -1              272/272              271/272
> > ILK                                  302/302              302/302
> > SNB                                  303/303              303/303
> > IVB                                  338/338              338/338
> > BYT                 -1              287/287              286/287
> > HSW                                  361/361              361/361
> > BDW                                  308/308              308/308
> > -------------------------------------Detailed-------------------------------------
> > Platform  Test                                drm-intel-nightly          Series Applied
> >  PNV  igt@gen3_render_tiledx_blits      FAIL(6)PASS(3)      FAIL(2)
> > *BYT  igt@gem_exec_bad_domains@conflicting-write-domain      PASS(9)
> > FAIL(1)PASS(1)
> > Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 20/20] drm/i915: Enable skylake sprite plane scaling using shared scalers

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:49PM -0700, Chandra Konduru wrote:
> This patch enables skylake sprite plane display scaling using shared
> scalers atomic desgin.
> 
> v2:
> -use single copy of scaler limits (Matt)
> 
> v3:
> -detaching scalers moved to crtc commit path (Matt)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c |   60 +++++++++++++++++++++++++++++------
>  1 file changed, 50 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c8feff7..562f90c 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -33,6 +33,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_rect.h>
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_plane_helper.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
> @@ -194,6 +195,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
>  	unsigned long surf_addr;
> +	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
> +	int scaler_id;
>  
>  	plane_ctl = PLANE_CTL_ENABLE |
>  		PLANE_CTL_PIPE_CSC_ENABLE;
> @@ -264,6 +267,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>  					       fb->pixel_format);
>  
> +	scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
> +
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -283,10 +288,30 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  
>  	surf_addr = intel_plane_obj_offset(intel_plane, obj);
>  
> +	/* program plane scaler */
> +	if (scaler_id >= 0) {
> +		uint32_t ps_ctrl = 0;
> +
> +		DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n", plane,
> +			PS_PLANE_SEL(plane));
> +		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) |
> +			crtc_state->scaler_state.scalers[scaler_id].mode |
> +			crtc_state->scaler_state.scalers[scaler_id].filter;
> +		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
> +		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
> +		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
> +		I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id),
> +			((crtc_w + 1) << 16)|(crtc_h + 1));
> +
> +		I915_WRITE(PLANE_POS(pipe, plane), 0);
> +		I915_WRITE(PLANE_SIZE(pipe, plane), (src_h << 16) | src_w);
> +	} else {
> +		I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> +		I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> +	}
> +
>  	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
>  	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
> -	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
>  	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
>  	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
>  	POSTING_READ(PLANE_SURF(pipe, plane));
> @@ -863,7 +888,9 @@ static int
>  intel_check_sprite_plane(struct drm_plane *plane,
>  			 struct intel_plane_state *state)
>  {
> +	struct drm_device *dev = plane->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> +	struct intel_crtc_state *crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	int crtc_x, crtc_y;
> @@ -872,11 +899,16 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	struct drm_rect *src = &state->src;
>  	struct drm_rect *dst = &state->dst;
>  	const struct drm_rect *clip = &state->clip;
> +	struct intel_crtc_scaler_state *scaler_state;
>  	int hscale, vscale;
>  	int max_scale, min_scale;
>  	int pixel_size;
> +	int ret;
>  
>  	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> +	crtc_state = state->base.state ?
> +		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
> +	scaler_state = crtc_state ? &crtc_state->scaler_state : NULL;
>  
>  	if (!fb) {
>  		state->visible = false;
> @@ -903,6 +935,11 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	max_scale = intel_plane->max_downscale << 16;
>  	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
>  
> +	if (INTEL_INFO(dev)->gen >= 9 && scaler_state && scaler_state->num_scalers) {
> +		min_scale = 1;
> +		max_scale = (100 << 16) / scaler_state->min_hsr;
> +	}
> +
>  	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
>  			state->base.rotation);
>  
> @@ -998,8 +1035,8 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  		width_bytes = ((src_x * pixel_size) & 63) +
>  					src_w * pixel_size;
>  
> -		if (src_w > 2048 || src_h > 2048 ||
> -		    width_bytes > 4096 || fb->pitches[0] > 4096) {
> +		if (INTEL_INFO(dev)->gen < 9 && (src_w > 2048 || src_h > 2048 ||
> +		    width_bytes > 4096 || fb->pitches[0] > 4096)) {
>  			DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
>  			return -EINVAL;
>  		}
> @@ -1053,6 +1090,13 @@ finish:
>  		}
>  	}
>  
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		ret = skl_update_scaler_users(intel_crtc, crtc_state, intel_plane,
> +			state, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1264,12 +1308,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  		}
>  		break;
>  	case 9:
> -		/*
> -		 * FIXME: Skylake planes can be scaled (with some restrictions),
> -		 * but this is for another time.
> -		 */
> -		intel_plane->can_scale = false;
> -		intel_plane->max_downscale = 1;
> +		intel_plane->can_scale = true;
> +		intel_plane->max_downscale = 2; /* updated later */

Again, not true about being updated later.  We just wind up not using
the value on SKL.

>  		intel_plane->update_plane = skl_update_plane;
>  		intel_plane->disable_plane = skl_disable_plane;
>  		state->scaler_id = -1;
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/20] skylake display scalers

[Matt Roper]

On Wed, Apr 01, 2015 at 07:59:29PM -0700, Chandra Konduru wrote:
> primary changes in this version are:
> - moved changes to state from commit patch to check path (Matt)
> - squashed few patches into others (Matt)
> - rebased colorkey related patches ontop of recent updates (Daniel)
> - rebased all patches onto today's drm-intel-nightly (me)
> - initialized colorkey to NONE (me)
> - changed primary plane src values to regular integers align with sprite (me)
> - used updated get display clock function (me)
> 
> Numbers of patches updated in this version:
>  4, 6, 8, 9, 11, 18, 19, 20
> 
> Sending full patch series for completeness. Individual patch headers
> have additional details on changes. This series should cleanly
> merge to latest drm-intel-nighly.

I've sent review feedback to several of the patches and some of those
have my r-b assuming minor updates to the commit message or comments are
made.  You can also consider 2, 4, 5, 7, 12, and 14-18 to be

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


A couple other notes/questions:
 * What is the high-level strategy for merging this?  Do we need to wait
   for full atomic helpers to be switched back on?  It looked to me like
   one of the patches here wouldn't work properly / get called for
   SetPlane() updates while using the transitional plane helpers.

 * I think it would be good to get Ander's thoughts on this as well
   since a lot of this directly intersects with the type of things he's
   working on.  In a couple places it feels a little bit like we have to
   do things the "wrong" way here because it's the best we can do on the
   current codebase, so we'll have to backtrack and re-write things a
   bit once we have full atomic modesetting working through the full
   check/commit model.  So Ander's high level ack would be good as well,
   since it may make his work more challenging.


Matt



> 
> Chandra Konduru (20):
>   drm/i915: Adding drm helper function drm_plane_from_index().
>   drm/i915: Register definitions for skylake scalers
>   drm/i915: skylake scaler structure definitions
>   drm/i915: Initialize plane colorkey to NONE
>   drm/i915: Initialize skylake scalers
>   drm/i915: Convert primary plane 16.16 values to regular ints
>   drm/i915: Dump scaler_state too as part of dumping crtc_state
>   drm/i915: Helper function to update skylake scaling ratio.
>   drm/i915: Add helper function to update scaler_users in crtc_state
>   drm/i915: Add atomic function to setup scalers scalers for a crtc.
>   drm/i915: Helper function to detach a scaler from a plane or crtc
>   drm/i915: Preserve scaler state when clearing crtc_state
>   drm/i915: use current scaler state during readout_hw_state.
>   drm/i915: Update scaling ratio as part of crtc_compute_config
>   drm/i915: Ensure setting up scalers into staged crtc_state
>   drm/i915: copy staged scaler state from drm state to crtc->config.
>   drm/i915: stage panel fitting scaler request for fixed mode panel
>   drm/i915: Enable skylake panel fitting using skylake shared scalers
>   drm/i915: Enable skylake primary plane scaling using shared scalers
>   drm/i915: Enable skylake sprite plane scaling using shared scalers
> 
>  drivers/gpu/drm/drm_crtc.c           |   22 ++
>  drivers/gpu/drm/i915/i915_reg.h      |  115 ++++++++
>  drivers/gpu/drm/i915/intel_atomic.c  |  168 ++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  489 +++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_dp.c      |    8 +
>  drivers/gpu/drm/i915/intel_drv.h     |   97 +++++++
>  drivers/gpu/drm/i915/intel_sprite.c  |   71 ++++-
>  include/drm/drm_crtc.h               |    1 +
>  8 files changed, 923 insertions(+), 48 deletions(-)
> 
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx