[PATCH 00/21] skylake display scalers

[PATCH 00/21] skylake display scalers

[Chandra Konduru]

This patch enables skylake display scalers in atomic framework.
Skylake scalers are sharable within a pipe and can be used as a panel
fitter or plane scaler. Two scalers cannot be ganged to a single plane
to get higher scale factor but simultaneous use of one as plane scaler
and one scaler as panel fitter is allowed. Reformatted previous patch
series into smaller patches and addressed previous feedback inputs.
Performed some initial testing and more testing is in works.
Testing is done applying these patches on top of Ander's v2
atomic crtc patches.
As several atomic crtc is in flight, will revisit scalers and perform
any additional testing after atomic crtc is in place.

Thanks,
Chandra

Chandra Konduru (21):
  drm/i915: Adding drm helper function drm_plane_from_index().
  drm/i915: Register definitions for skylake scalers
  drm/i915: Enable get_colorkey functions for primary plane.
  drm/i915: skylake scaler structure definitions
  drm/i915: Initialize skylake scalers
  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: Ensure planes begin with no scaler.
  drm/i915: Ensure colorkey and scaling aren't enabled at same time
  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           |   20 ++
 drivers/gpu/drm/i915/i915_reg.h      |  114 +++++++++
 drivers/gpu/drm/i915/intel_atomic.c  |  157 ++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  442 +++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c      |    7 +
 drivers/gpu/drm/i915/intel_drv.h     |  109 +++++++++
 drivers/gpu/drm/i915/intel_sprite.c  |   95 ++++++--
 include/drm/drm_crtc.h               |    1 +
 8 files changed, 895 insertions(+), 50 deletions(-)

-- 
1.7.9.5

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

[PATCH 01/21] 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.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9f970c2..bbe573e1 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1286,6 +1286,26 @@ 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;
+
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		if (drm_plane_index(plane) == idx)
+			return plane;
+	}
+	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

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

[Chandra Konduru]

Adding register definitions for skylake scalers.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cc8ebab..08cfdf9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5094,6 +5094,120 @@ 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(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/21] drm/i915: Enable get_colorkey functions for primary plane.

[Chandra Konduru]

Made intel_colorkey_enabled and skl_get_colorkey functions
available for primary plane.

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ec99046..3f7d05e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1263,6 +1263,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
+bool intel_colorkey_enabled(struct intel_plane *intel_plane);
+void skl_get_colorkey(struct drm_plane *drm_plane,
+                struct drm_intel_sprite_colorkey *key);
 bool intel_pipe_update_start(struct intel_crtc *crtc,
 			     uint32_t *start_vbl_count);
 void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a828736..9ee12d0 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -336,7 +336,7 @@ skl_update_colorkey(struct drm_plane *drm_plane,
 	return 0;
 }
 
-static void
+void
 skl_get_colorkey(struct drm_plane *drm_plane,
 		 struct drm_intel_sprite_colorkey *key)
 {
@@ -1068,11 +1068,12 @@ ilk_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
 		key->flags = I915_SET_COLORKEY_NONE;
 }
 
-static bool colorkey_enabled(struct intel_plane *intel_plane)
+bool intel_colorkey_enabled(struct intel_plane *intel_plane)
 {
 	struct drm_intel_sprite_colorkey key;
 
-	intel_plane->get_colorkey(&intel_plane->base, &key);
+	if (intel_plane->get_colorkey)
+		intel_plane->get_colorkey(&intel_plane->base, &key);
 
 	return key.flags != I915_SET_COLORKEY_NONE;
 }
@@ -1241,7 +1242,7 @@ finish:
 	 * we can disable the primary and save power.
 	 */
 	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
-		!colorkey_enabled(intel_plane);
+		!intel_colorkey_enabled(intel_plane);
 	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
 
 	if (intel_crtc->active) {
-- 
1.7.9.5

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

[PATCH 04/21] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3f7d05e..d9a3b64 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -256,6 +256,35 @@ 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.
+	 *
+	 *   There are two scenarios:
+	 *   1. the freed up scaler is assigned to crtc or some other plane
+	 *      In this case, as part of plane programming scaler_id will be set
+	 *      to -1 using helper function detach_scalers
+	 *   2. the freed up scaler is not assigned to anyone
+	 *      In this case, as part of plane programming scaler registers will
+	 *      be reset and scaler_id will also be reset to -1 using the same
+	 *      helper function detach_scalers
+	 */
+	int scaler_id;
 };
 
 struct intel_initial_plane_config {
@@ -265,6 +294,74 @@ struct intel_initial_plane_config {
 	u32 base;
 };
 
+struct intel_scaler {
+	int id;
+	int in_use;
+	uint32_t mode;
+	uint32_t filter;
+
+	/*
+	 * 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_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;
+};
+
 struct intel_crtc_state {
 	struct drm_crtc_state base;
 
@@ -391,6 +488,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

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

[Chandra Konduru]

Initializing scalers with supported values during crtc init.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8dba8e8..ccddd81 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12721,6 +12721,54 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	return &cursor->base;
 }
 
+static void skl_scalers_init(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;
+	if (INTEL_INFO(dev)->gen < 9)
+		return;
+
+	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;
+
+		/* down scaling ratio: 2.99 --> 1, i.e., 34% of original */
+		intel_scaler->min_hsr = 34;
+		intel_scaler->min_vsr = 34;
+		intel_scaler->max_hsr = 500;
+		intel_scaler->max_vsr = 500;
+
+		/* down scaling ratio: 2.99x2.99 --> 1x1, i.e., 12% of original */
+		intel_scaler->min_hvsr = 12;
+		intel_scaler->max_hvsr = 2500;
+
+		/* src_w & dst_w range 8 - 4096 */
+		intel_scaler->min_src_w = 8;
+		intel_scaler->max_src_w = 4096;
+		intel_scaler->min_dst_w = 8;
+		intel_scaler->max_dst_w = 4096;
+
+		/* src_h & dst_h range 8 - 2304 */
+		intel_scaler->min_src_h = 8;
+		intel_scaler->max_src_h = 2304;
+		intel_scaler->min_dst_h = 8;
+		intel_scaler->max_dst_h = 2304;
+
+		intel_scaler->mode = PS_SCALER_MODE_DYN;
+		intel_scaler->filter = PS_FILTER_MEDIUM;
+	}
+
+	/* 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;
@@ -12740,6 +12788,9 @@ 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 */
+	skl_scalers_init(dev, pipe, crtc_state);
+
 	primary = intel_primary_plane_create(dev, pipe);
 	if (!primary)
 		goto fail;
-- 
1.7.9.5

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

[PATCH 06/21] drm/i915: Dump scaler_state too as part of dumping crtc_state

[Chandra Konduru]

Dumps scaler state as part of dumping crtc_state.

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 ccddd81..da78e77 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10374,8 +10374,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",
@@ -10412,6 +10418,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,
@@ -10422,6 +10431,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) >> 16,
+			drm_rect_height(&state->src) >> 16,
+			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 07/21] drm/i915: Helper function to update skylake scaling ratio.

[Chandra Konduru]

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

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 da78e77..5591282 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4511,6 +4511,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;
+	int i;
+
+	if (INTEL_INFO(dev)->gen < 9 || !crtc_state)
+		return;
+
+	crtc_clock = (uint32_t) crtc_state->base.adjusted_mode.crtc_clock;
+	cdclk = (uint32_t) intel_ddi_get_cdclk_freq(dev_priv);
+
+	if (!crtc_clock || !cdclk)
+		return;
+
+	for (i = 0; i < crtc_state->scaler_state.num_scalers; i++) {
+		struct intel_scaler *scaler = &crtc_state->scaler_state.scalers[i];
+
+		scaler->min_hsr = max(scaler->min_hsr, (crtc_clock * 100)/cdclk);
+		scaler->min_vsr = max(scaler->min_hsr, (crtc_clock * 100)/cdclk);
+		scaler->min_hvsr = max(scaler->min_hsr, (crtc_clock * 100)/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

[PATCH 08/21] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5591282..0eb120d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12356,6 +12356,148 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	}
 }
 
+/*
+ * Update crtc_state->scaler_users for requested plane or crtc based on state.
+ *
+ * plane (in)
+ *     NULL - for crtc
+ *     not NULL - for plane
+ * 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)
+{
+	struct drm_device *dev = intel_crtc->base.dev;
+	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;
+	struct intel_scaler *scaler;
+
+	if (!intel_crtc || !crtc_state || INTEL_INFO(dev)->gen < 9 ||
+		(intel_plane && intel_plane->base.type == DRM_PLANE_TYPE_CURSOR))
+		return 0;
+
+	scaler_state = &crtc_state->scaler_state;
+	scaler = &scaler_state->scalers[0];
+
+	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) >> 16;
+		src_h = drm_rect_height(&plane_state->src) >> 16;
+		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
+	 *  - 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 (!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;
+
+			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;
+	}
+
+	/*
+	 * check for rect size:
+	 *     min sizes in case of scaling involved
+	 *     max sizes in all cases
+	 */
+	if ((need_scaling &&
+		(src_w < scaler->min_src_w || src_h < scaler->min_src_h ||
+		 dst_w < scaler->min_dst_w || dst_h < scaler->min_dst_h)) ||
+
+		 src_w > scaler->max_src_w || src_h > scaler->max_src_h ||
+		 dst_w > scaler->max_dst_w || dst_h > scaler->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 && need_scaling && intel_colorkey_enabled(intel_plane)) {
+		DRM_DEBUG_KMS("PLANE:%d scaling with color key not allowed",
+			intel_plane->base.base.id);
+		return -EINVAL;
+	}
+
+	/* Check src format */
+	if (intel_plane && need_scaling) {
+		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 format 0x%x not supported\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 d9a3b64..b56baba 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1138,6 +1138,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);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
-- 
1.7.9.5

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

[PATCH 09/21] 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

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 3903b90..421fb04 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -241,3 +241,140 @@ 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
+ * @new_drm_state: incoming new drm state to validate
+ *
+ * This function setups 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 drm_atomic_state *new_drm_state)
+{
+	struct drm_plane *plane = NULL;
+	struct intel_plane *intel_plane;
+	struct intel_crtc_state *crtc_state;
+	struct intel_plane_state *plane_state = NULL;
+	struct intel_crtc_scaler_state *scaler_state;
+	int num_scalers_need;
+	int i, j;
+
+	if (INTEL_INFO(dev)->gen < 9 || !intel_crtc)
+		return 0;
+
+	crtc_state = intel_atomic_get_crtc_state(new_drm_state, intel_crtc);
+	scaler_state = &crtc_state->scaler_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 {
+			/* plane scaler case: assign as a plane scaler */
+			/* find the plane that set the bit as scaler_user */
+			plane = new_drm_state->planes[i];
+
+			/*
+			 * to enable/disable hq mode, add planes that are using scaler
+			 * into this transaction
+			 */
+			if (!plane) {
+				plane = drm_plane_from_index(dev, i);
+				new_drm_state->planes[i] = plane;
+				new_drm_state->plane_states[i] =
+					drm_atomic_get_plane_state(new_drm_state, plane);
+			}
+
+			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(new_drm_state->plane_states[i]);
+			scaler_id = &plane_state->scaler_id;
+		}
+
+		DRM_ERROR("*scaler_id = %d\n", *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 b56baba..b01c836 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1401,6 +1401,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 drm_atomic_state *new_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

[PATCH 10/21] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0eb120d..277c895 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2834,6 +2834,45 @@ u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
 	}
 }
 
+/*
+ * This function detaches (aka. unbinds) a scaler from plane or crtc
+ * if scaler is not in use.
+ * It resets scaler_id in plane or crtc
+ * To request detach a scaler from crtc, call plane as NULL
+ */
+void skl_detach_scaler(struct drm_crtc *crtc, struct drm_plane *plane)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc_state *crtc_state;
+	struct intel_crtc *intel_crtc;
+	struct intel_plane *intel_plane;
+	struct intel_plane_state *plane_state;
+	int *scaler_id;
+
+	intel_crtc = to_intel_crtc(crtc);
+	intel_plane = plane ? to_intel_plane(plane) : NULL;
+	crtc_state = intel_crtc->config;
+	plane_state = plane ? to_intel_plane_state(plane->state) : NULL;
+
+	scaler_id = plane ? (plane_state ? &plane_state->scaler_id : NULL) :
+		&crtc_state->scaler_state.scaler_id;
+
+	if (!scaler_id || (scaler_id && *scaler_id < 0))
+		return;
+
+	/* if scaler is not in use, free */
+	if (!crtc_state->scaler_state.scalers[*scaler_id].in_use) {
+		I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, (*scaler_id)), 0);
+		I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, (*scaler_id)), 0);
+		I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, (*scaler_id)), 0);
+		DRM_DEBUG_KMS("Detached scaler id %u.%u from %s:%d\n",
+			intel_crtc->pipe, *scaler_id, plane ? "PLANE" : "CRTC",
+			plane ? plane->base.id : crtc->base.id);
+		*scaler_id = -1;
+	}
+}
+
 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 b01c836..745ebda 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1141,6 +1141,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);
+void skl_detach_scaler(struct drm_crtc *crtc, struct drm_plane *plane);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
-- 
1.7.9.5

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

[PATCH 11/21] drm/i915: Ensure planes begin with no scaler.

[Chandra Konduru]

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 277c895..e07c24e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12787,6 +12787,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;
@@ -12944,6 +12945,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;
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9ee12d0..c010528 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1497,6 +1497,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		intel_plane->disable_plane = skl_disable_plane;
 		intel_plane->update_colorkey = skl_update_colorkey;
 		intel_plane->get_colorkey = skl_get_colorkey;
+		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 12/21] drm/i915: Ensure colorkey and scaling aren't enabled at same time

[Chandra Konduru]

Plane scaling and colorkey are mutually exclusive. Ensure scaling
isn't active at the time of enabling colorkey.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c010528..0194390 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -319,6 +319,12 @@ skl_update_colorkey(struct drm_plane *drm_plane,
 	const int plane = intel_plane->plane;
 	u32 plane_ctl;
 
+	/* plane scaling and colorkey are mutually exclusive */
+	if (to_intel_plane_state(drm_plane->state)->scaler_id >= 0) {
+		DRM_ERROR("colorkey not allowed with scaler\n");
+		return -EINVAL;
+	}
+
 	I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
 	I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
 	I915_WRITE(PLANE_KEYMSK(pipe, plane), key->channel_mask);
-- 
1.7.9.5

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

[PATCH 13/21] 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 e07c24e..9c3c6b1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10617,11 +10617,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 int
-- 
1.7.9.5

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

[PATCH 14/21] drm/i915: use current scaler state during readout_hw_state.

[Chandra Konduru]

During readout_hw_state, whole pipe_config is built reading hw.
But rebuilding scaler state from hw requires,
 - reading all planes and find its corresponding index in order to set
   its bits in scaler_users
 - reading cdclk and adjusted mode crtc clk in order to regenerate
   min scaling ratios.
 - some values directly from bspec

To simplify things, for now using sw scaler state as readout state.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9c3c6b1..b1d7036 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14292,7 +14292,10 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	int i;
 
 	for_each_intel_crtc(dev, crtc) {
+		struct intel_crtc_scaler_state scaler_state;
+		scaler_state = crtc->config->scaler_state;
 		memset(crtc->config, 0, sizeof(*crtc->config));
+		crtc->config->scaler_state = scaler_state;
 
 		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
 
-- 
1.7.9.5

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

[PATCH 15/21] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b1d7036..8ab9624 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5788,6 +5788,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) {
@@ -5842,7 +5843,16 @@ 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;
+	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_ERROR("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->base.state);
+
+	return ret;
 }
 
 static int valleyview_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 16/21] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 421fb04..d078800b 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -48,6 +48,7 @@ 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;
 	int ret;
 	int i;
 	bool not_nuclear = false;
@@ -78,6 +79,8 @@ 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;
 	}
 	for (i = 0; i < nconnectors; i++)
 		if (state->connectors[i] != NULL)
@@ -92,6 +95,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, 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 17/21] 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 d078800b..35d3b4d 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -163,6 +163,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 18/21] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 22bf6f2..82e42fc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1303,8 +1303,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	pipe_config->has_audio = intel_dp->has_audio;
 
 	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
+		int ret;
+
 		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
 				       adjusted_mode);
+
+		ret = skl_update_scaler_users(intel_crtc, pipe_config, NULL, NULL);
+		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 19/21] drm/i915: Enable skylake panel fitting using skylake shared scalers

[Chandra Konduru]

Modify skylake panel fitting implementation to use shared scalers.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ab9624..8deebb7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4192,11 +4192,23 @@ static void skylake_pfit_enable(struct intel_crtc *crtc)
 	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;
 
 	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);
 	}
 }
 
@@ -4664,17 +4676,7 @@ static void haswell_crtc_enable(struct drm_crtc *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);
-	}
+	skl_detach_scaler(&crtc->base, NULL);
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
@@ -7953,13 +7955,14 @@ 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;
+	int id = crtc->config->scaler_state.scaler_id;
 
-	tmp = I915_READ(PS_CTL(crtc->pipe));
+	tmp = id >= 0 ? I915_READ(SKL_PS_CTRL(crtc->pipe, id)) : 0;
 
-	if (tmp & PS_ENABLE) {
+	if (tmp & PS_SCALER_EN) {
 		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));
+		pipe_config->pch_pfit.pos = I915_READ(SKL_PS_WIN_POS(crtc->pipe, id));
+		pipe_config->pch_pfit.size = I915_READ(SKL_PS_WIN_SZ(crtc->pipe, id));
 	}
 }
 
-- 
1.7.9.5

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

[PATCH 20/21] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8deebb7..d63be8e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2883,6 +2883,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj;
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl, stride_div;
+	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;
+
+	skl_detach_scaler(crtc, crtc->primary);
 
 	if (!intel_crtc->primary_enabled) {
 		I915_WRITE(PLANE_CTL(pipe, 0), 0);
@@ -2950,13 +2960,42 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
 					       fb->pixel_format);
 
+	if (plane_state) {
+		scaler_id = plane_state->scaler_id;
+		src_x = plane_state->src.x1 >> 16;
+		src_y = plane_state->src.y1 >> 16;
+		src_w = drm_rect_width(&plane_state->src) >> 16;
+		src_h = drm_rect_height(&plane_state->src) >> 16;
+		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_POS(pipe, 0), 0);
+		I915_WRITE(PLANE_SIZE(pipe, 0), ((src_h - 1) << 16) | (src_w - 1));
+	} else {
+		I915_WRITE(PLANE_POS(pipe, 0), 0);
+		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), i915_gem_obj_ggtt_offset(obj));
 
@@ -12561,19 +12600,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->scalers[0].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;
@@ -12620,6 +12673,13 @@ intel_check_primary_plane(struct drm_plane *plane,
 			intel_crtc->atomic.update_wm = true;
 	}
 
+	if (INTEL_INFO(dev)->gen >= 9) {
+		ret = skl_update_scaler_users(intel_crtc, crtc_state,
+			to_intel_plane(plane), state);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -12803,6 +12863,11 @@ 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 */
+		primary->get_colorkey = skl_get_colorkey;
+	}
 	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

[PATCH 21/21] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0194390..e395a02 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>
@@ -191,6 +192,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	const int plane = intel_plane->plane + 1;
 	u32 plane_ctl, stride_div;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
+	int scaler_id;
 
 	plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
 
@@ -274,16 +277,39 @@ 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);
 
+	skl_detach_scaler(crtc, drm_plane);
+	scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
+
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
 	crtc_w--;
 	crtc_h--;
 
+	/* 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), i915_gem_obj_ggtt_offset(obj));
 	POSTING_READ(PLANE_SURF(pipe, plane));
@@ -305,6 +331,8 @@ skl_disable_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc)
 	I915_WRITE(PLANE_CTL(pipe, plane), 0);
 	POSTING_READ(PLANE_CTL(pipe, plane));
 
+	skl_detach_scaler(crtc, drm_plane);
+
 	intel_update_sprite_watermarks(drm_plane, crtc, 0, 0, 0, false, false);
 }
 
@@ -1088,7 +1116,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;
@@ -1097,11 +1127,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;
@@ -1128,6 +1163,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->scalers[0].min_hsr;
+	}
+
 	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
 			state->base.rotation);
 
@@ -1223,18 +1263,18 @@ 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;
 		}
 	}
 
 	if (state->visible) {
-		src->x1 = src_x;
-		src->x2 = src_x + src_w;
-		src->y1 = src_y;
-		src->y2 = src_y + src_h;
+		src->x1 = src_x << 16;
+		src->x2 = (src_x + src_w) << 16;
+		src->y1 = src_y << 16;
+		src->y2 = (src_y + src_h) << 16;
 	}
 
 	dst->x1 = crtc_x;
@@ -1271,6 +1311,13 @@ finish:
 			intel_crtc->atomic.update_wm = true;
 	}
 
+	if (INTEL_INFO(dev)->gen >= 9) {
+		ret = skl_update_scaler_users(intel_crtc, crtc_state, intel_plane,
+			state);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -1301,10 +1348,10 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 			crtc_y = state->dst.y1;
 			crtc_w = drm_rect_width(&state->dst);
 			crtc_h = drm_rect_height(&state->dst);
-			src_x = state->src.x1;
-			src_y = state->src.y1;
-			src_w = drm_rect_width(&state->src);
-			src_h = drm_rect_height(&state->src);
+			src_x = state->src.x1 >> 16;
+			src_y = state->src.y1 >> 16;
+			src_w = drm_rect_width(&state->src) >> 16;
+			src_h = drm_rect_height(&state->src) >> 16;
 			intel_plane->update_plane(plane, crtc, fb, obj,
 						  crtc_x, crtc_y, crtc_w, crtc_h,
 						  src_x, src_y, src_w, src_h);
@@ -1493,12 +1540,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;
 		intel_plane->update_colorkey = skl_update_colorkey;
-- 
1.7.9.5

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

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

[Daniel Vetter]

On Sat, Mar 14, 2015 at 10:55:26PM -0700, Chandra Konduru wrote:
> Adding drm helper function to return plane pointer from index where
> index is a returned by drm_plane_index.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |   20 ++++++++++++++++++++
>  include/drm/drm_crtc.h     |    1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 9f970c2..bbe573e1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1286,6 +1286,26 @@ 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;
> +
> +	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> +		if (drm_plane_index(plane) == idx)
> +			return plane;

Just a bikeshed, but you can do the same counting loop as in
drm_plane_index and then return the plane as soon as idx = i. Avoids a
nested double-loop on the plane_list.
-Daniel

> +	}
> +	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

-- 
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 03/21] drm/i915: Enable get_colorkey functions for primary plane.

[Daniel Vetter]

On Sat, Mar 14, 2015 at 10:55:28PM -0700, Chandra Konduru wrote:
> Made intel_colorkey_enabled and skl_get_colorkey functions
> available for primary plane.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

This is a legacy plane attribute which we first need to convert over to
properties. Do we really need this for skl scaler support? The problem
here is that this directly looks at hw state, so breaks the check/commit
split atomic needs.

Imo best would be if we could push this ouf of the scaler series. I
haven't checked for usage though.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_drv.h    |    3 +++
>  drivers/gpu/drm/i915/intel_sprite.c |    9 +++++----
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ec99046..3f7d05e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1263,6 +1263,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
>  int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
> +bool intel_colorkey_enabled(struct intel_plane *intel_plane);
> +void skl_get_colorkey(struct drm_plane *drm_plane,
> +                struct drm_intel_sprite_colorkey *key);
>  bool intel_pipe_update_start(struct intel_crtc *crtc,
>  			     uint32_t *start_vbl_count);
>  void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a828736..9ee12d0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -336,7 +336,7 @@ skl_update_colorkey(struct drm_plane *drm_plane,
>  	return 0;
>  }
>  
> -static void
> +void
>  skl_get_colorkey(struct drm_plane *drm_plane,
>  		 struct drm_intel_sprite_colorkey *key)
>  {
> @@ -1068,11 +1068,12 @@ ilk_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
>  		key->flags = I915_SET_COLORKEY_NONE;
>  }
>  
> -static bool colorkey_enabled(struct intel_plane *intel_plane)
> +bool intel_colorkey_enabled(struct intel_plane *intel_plane)
>  {
>  	struct drm_intel_sprite_colorkey key;
>  
> -	intel_plane->get_colorkey(&intel_plane->base, &key);
> +	if (intel_plane->get_colorkey)
> +		intel_plane->get_colorkey(&intel_plane->base, &key);
>  
>  	return key.flags != I915_SET_COLORKEY_NONE;
>  }
> @@ -1241,7 +1242,7 @@ finish:
>  	 * we can disable the primary and save power.
>  	 */
>  	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
> -		!colorkey_enabled(intel_plane);
> +		!intel_colorkey_enabled(intel_plane);
>  	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
>  
>  	if (intel_crtc->active) {
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 12/21] drm/i915: Ensure colorkey and scaling aren't enabled at same time

[Daniel Vetter]

On Sat, Mar 14, 2015 at 10:55:37PM -0700, Chandra Konduru wrote:
> Plane scaling and colorkey are mutually exclusive. Ensure scaling
> isn't active at the time of enabling colorkey.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c010528..0194390 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -319,6 +319,12 @@ skl_update_colorkey(struct drm_plane *drm_plane,
>  	const int plane = intel_plane->plane;
>  	u32 plane_ctl;
>  
> +	/* plane scaling and colorkey are mutually exclusive */
> +	if (to_intel_plane_state(drm_plane->state)->scaler_id >= 0) {
> +		DRM_ERROR("colorkey not allowed with scaler\n");
> +		return -EINVAL;
> +	}

Yeah this is a bit trouble because of the interactions between colorkey
state in hw and the atomic scaler state in the atomic state structures.

Since colorkey isn't support with atomic atm anyway and we want to move
towards atomic: Do we really have anyone who cares about legacy sprite
ioctls on skl? If we'd just add a 

	if (gen >= 9)
		return -ENODEV;

to all the sprite ioctls then we won't have any problems. And we also
don't need to add temporary code like the above.
-Daniel

> +
>  	I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
>  	I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
>  	I915_WRITE(PLANE_KEYMSK(pipe, plane), key->channel_mask);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 12/21] drm/i915: Ensure colorkey and scaling aren't enabled at same time

[Chris Wilson]

On Tue, Mar 17, 2015 at 03:16:01PM +0100, Daniel Vetter wrote:
> On Sat, Mar 14, 2015 at 10:55:37PM -0700, Chandra Konduru wrote:
> > Plane scaling and colorkey are mutually exclusive. Ensure scaling
> > isn't active at the time of enabling colorkey.
> > 
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index c010528..0194390 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -319,6 +319,12 @@ skl_update_colorkey(struct drm_plane *drm_plane,
> >  	const int plane = intel_plane->plane;
> >  	u32 plane_ctl;
> >  
> > +	/* plane scaling and colorkey are mutually exclusive */
> > +	if (to_intel_plane_state(drm_plane->state)->scaler_id >= 0) {
> > +		DRM_ERROR("colorkey not allowed with scaler\n");
> > +		return -EINVAL;
> > +	}
> 
> Yeah this is a bit trouble because of the interactions between colorkey
> state in hw and the atomic scaler state in the atomic state structures.
> 
> Since colorkey isn't support with atomic atm anyway and we want to move
> towards atomic: Do we really have anyone who cares about legacy sprite
> ioctls on skl? If we'd just add a 

Yes. Current userspace uses sprites and colorkey.
-Chris

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

Re: [PATCH 13/21] drm/i915: Preserve scaler state when clearing crtc_state

[Daniel Vetter]

On Sat, Mar 14, 2015 at 10:55:38PM -0700, Chandra Konduru wrote:
> 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>

I guess this is going to conflict with Ander's crtc state series. But
since you're signed up to review that I guess you will know what needs to
be changed here. Hopefully Ander's series will just take care of this.
-Daniel

> ---
>  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 e07c24e..9c3c6b1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10617,11 +10617,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 int
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 14/21] drm/i915: use current scaler state during readout_hw_state.

[Daniel Vetter]

On Sat, Mar 14, 2015 at 10:55:39PM -0700, Chandra Konduru wrote:
> During readout_hw_state, whole pipe_config is built reading hw.
> But rebuilding scaler state from hw requires,
>  - reading all planes and find its corresponding index in order to set
>    its bits in scaler_users
>  - reading cdclk and adjusted mode crtc clk in order to regenerate
>    min scaling ratios.
>  - some values directly from bspec
> 
> To simplify things, for now using sw scaler state as readout state.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

At least the crtc scaler might get used by the bios for boot-up with
non-native resolution. I do think we need to properly track those parts
(and also double-check the hw state in pipe config compare function like
we do for the old pfit state).

Maybe we need to always compute limits directly instead of storing the in
the state structures?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9c3c6b1..b1d7036 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14292,7 +14292,10 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	int i;
>  
>  	for_each_intel_crtc(dev, crtc) {
> +		struct intel_crtc_scaler_state scaler_state;
> +		scaler_state = crtc->config->scaler_state;
>  		memset(crtc->config, 0, sizeof(*crtc->config));
> +		crtc->config->scaler_state = scaler_state;
>  
>  		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 19/21] drm/i915: Enable skylake panel fitting using skylake shared scalers

[Daniel Vetter]

On Sat, Mar 14, 2015 at 10:55:44PM -0700, Chandra Konduru wrote:
> Modify skylake panel fitting implementation to use shared scalers.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

Ah here's the real pfit state, and the scaler state is just readout. Still
we recover this from the bios, I do think we need to make sure that at
least the crtc scaler is correctly assigned. Can't we recompute the scaler
state after readout to make sure it fits with pfit state here?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |   39 ++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ab9624..8deebb7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4192,11 +4192,23 @@ static void skylake_pfit_enable(struct intel_crtc *crtc)
>  	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;
>  
>  	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);
>  	}
>  }
>  
> @@ -4664,17 +4676,7 @@ static void haswell_crtc_enable(struct drm_crtc *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);
> -	}
> +	skl_detach_scaler(&crtc->base, NULL);
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -7953,13 +7955,14 @@ 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;
> +	int id = crtc->config->scaler_state.scaler_id;
>  
> -	tmp = I915_READ(PS_CTL(crtc->pipe));
> +	tmp = id >= 0 ? I915_READ(SKL_PS_CTRL(crtc->pipe, id)) : 0;
>  
> -	if (tmp & PS_ENABLE) {
> +	if (tmp & PS_SCALER_EN) {
>  		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));
> +		pipe_config->pch_pfit.pos = I915_READ(SKL_PS_WIN_POS(crtc->pipe, id));
> +		pipe_config->pch_pfit.size = I915_READ(SKL_PS_WIN_SZ(crtc->pipe, id));
>  	}
>  }
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 00/21] skylake display scalers

[Daniel Vetter]

On Sat, Mar 14, 2015 at 10:55:25PM -0700, Chandra Konduru wrote:
> This patch enables skylake display scalers in atomic framework.
> Skylake scalers are sharable within a pipe and can be used as a panel
> fitter or plane scaler. Two scalers cannot be ganged to a single plane
> to get higher scale factor but simultaneous use of one as plane scaler
> and one scaler as panel fitter is allowed. Reformatted previous patch
> series into smaller patches and addressed previous feedback inputs.
> Performed some initial testing and more testing is in works.
> Testing is done applying these patches on top of Ander's v2
> atomic crtc patches.
> As several atomic crtc is in flight, will revisit scalers and perform
> any additional testing after atomic crtc is in place.

Ok sprinkled a few smaller comments over a few patches, looks good overall
I think. And we discussed a bunch of the more tricky bits already in a 1:1
code walkthrough.

A few high-level bits:
- When splitting up patches pls dont make patches which just add
  functions/structs without using them. That makes it a lot harder to
  understand the big picture. The only exception is register #defines
  since reviewing those against Bspec can be done mostly independently of
  the code. So for functions make sure you have at least 1 use-site so
  that it's clear what it does. For structures just start out with the
  basic structure, then add members as you start using them.

  Since this amounts to a full rewrite of the patch series and this one
  here isn't too tricky I think it's ok to not do that here. But please
  follow this bkm for the next one.

- Testcase: We definitely don't have testcases for primary plane scaling
  (and panning fwiw), and iirc we also don't have any function testcases
  for sprite scaling.

- Testcases 2: I think we're also missing tests for the crtc pfit stuff.
  I think it'd be good to exercise the various resolutions and different
  boxing modes supported. I don't think we can do a full functional
  testcase here with crc.

- Unfortunately we can't yet test the sharing logic fully due to lack of
  atomic. But that's an open that we need to address once atomic has
  landed. Especially to make sure that extreme cases (e.g. all scalers
  used by planes <-> pfit enabled) work correctly.

Overall I think this is ready for detailed review. Since we lack igts I
recommend that the reviewer also does the igt coverage, that's a good way
to make sure all issues are caught.

Thanks, Daniel


> 
> Thanks,
> Chandra
> 
> Chandra Konduru (21):
>   drm/i915: Adding drm helper function drm_plane_from_index().
>   drm/i915: Register definitions for skylake scalers
>   drm/i915: Enable get_colorkey functions for primary plane.
>   drm/i915: skylake scaler structure definitions
>   drm/i915: Initialize skylake scalers
>   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: Ensure planes begin with no scaler.
>   drm/i915: Ensure colorkey and scaling aren't enabled at same time
>   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           |   20 ++
>  drivers/gpu/drm/i915/i915_reg.h      |  114 +++++++++
>  drivers/gpu/drm/i915/intel_atomic.c  |  157 ++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  442 +++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_dp.c      |    7 +
>  drivers/gpu/drm/i915/intel_drv.h     |  109 +++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  |   95 ++++++--
>  include/drm/drm_crtc.h               |    1 +
>  8 files changed, 895 insertions(+), 50 deletions(-)
> 
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 12/21] drm/i915: Ensure colorkey and scaling aren't enabled at same time

[Daniel Vetter]

On Tue, Mar 17, 2015 at 02:16:33PM +0000, Chris Wilson wrote:
> On Tue, Mar 17, 2015 at 03:16:01PM +0100, Daniel Vetter wrote:
> > On Sat, Mar 14, 2015 at 10:55:37PM -0700, Chandra Konduru wrote:
> > > Plane scaling and colorkey are mutually exclusive. Ensure scaling
> > > isn't active at the time of enabling colorkey.
> > > 
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_sprite.c |    6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index c010528..0194390 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -319,6 +319,12 @@ skl_update_colorkey(struct drm_plane *drm_plane,
> > >  	const int plane = intel_plane->plane;
> > >  	u32 plane_ctl;
> > >  
> > > +	/* plane scaling and colorkey are mutually exclusive */
> > > +	if (to_intel_plane_state(drm_plane->state)->scaler_id >= 0) {
> > > +		DRM_ERROR("colorkey not allowed with scaler\n");
> > > +		return -EINVAL;
> > > +	}
> > 
> > Yeah this is a bit trouble because of the interactions between colorkey
> > state in hw and the atomic scaler state in the atomic state structures.
> > 
> > Since colorkey isn't support with atomic atm anyway and we want to move
> > towards atomic: Do we really have anyone who cares about legacy sprite
> > ioctls on skl? If we'd just add a 
> 
> Yes. Current userspace uses sprites and colorkey.

Yes I know we have the code around and it'll keep working. This was more a
question of priority and whether we really should keep it working or
whether cros/android don't really care. Since sooner or later we need to
convert it to atomic/properties if we want to keep it working. Right now
(since skl is still prelimary support) we can still do that I hope ...
-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 12/21] drm/i915: Ensure colorkey and scaling aren't enabled at same time

[Chris Wilson]

On Tue, Mar 17, 2015 at 03:38:28PM +0100, Daniel Vetter wrote:
> On Tue, Mar 17, 2015 at 02:16:33PM +0000, Chris Wilson wrote:
> > On Tue, Mar 17, 2015 at 03:16:01PM +0100, Daniel Vetter wrote:
> > > On Sat, Mar 14, 2015 at 10:55:37PM -0700, Chandra Konduru wrote:
> > > > Plane scaling and colorkey are mutually exclusive. Ensure scaling
> > > > isn't active at the time of enabling colorkey.
> > > > 
> > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_sprite.c |    6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index c010528..0194390 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -319,6 +319,12 @@ skl_update_colorkey(struct drm_plane *drm_plane,
> > > >  	const int plane = intel_plane->plane;
> > > >  	u32 plane_ctl;
> > > >  
> > > > +	/* plane scaling and colorkey are mutually exclusive */
> > > > +	if (to_intel_plane_state(drm_plane->state)->scaler_id >= 0) {
> > > > +		DRM_ERROR("colorkey not allowed with scaler\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > 
> > > Yeah this is a bit trouble because of the interactions between colorkey
> > > state in hw and the atomic scaler state in the atomic state structures.
> > > 
> > > Since colorkey isn't support with atomic atm anyway and we want to move
> > > towards atomic: Do we really have anyone who cares about legacy sprite
> > > ioctls on skl? If we'd just add a 
> > 
> > Yes. Current userspace uses sprites and colorkey.
> 
> Yes I know we have the code around and it'll keep working. This was more a
> question of priority and whether we really should keep it working or
> whether cros/android don't really care. Since sooner or later we need to
> convert it to atomic/properties if we want to keep it working. Right now
> (since skl is still prelimary support) we can still do that I hope ...

Code was written a couple of years ago with the intent that it would
work for the next 5 years across multiple hardware releases.
-Chris

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

Re: [PATCH 04/21] drm/i915: skylake scaler structure definitions

[Matt Roper]

On Sat, Mar 14, 2015 at 10:55:29PM -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.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |   99 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3f7d05e..d9a3b64 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -256,6 +256,35 @@ 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.
> +	 *
> +	 *   There are two scenarios:
> +	 *   1. the freed up scaler is assigned to crtc or some other plane
> +	 *      In this case, as part of plane programming scaler_id will be set
> +	 *      to -1 using helper function detach_scalers
> +	 *   2. the freed up scaler is not assigned to anyone
> +	 *      In this case, as part of plane programming scaler registers will
> +	 *      be reset and scaler_id will also be reset to -1 using the same
> +	 *      helper function detach_scalers
> +	 */
> +	int scaler_id;
>  };
>  
>  struct intel_initial_plane_config {
> @@ -265,6 +294,74 @@ struct intel_initial_plane_config {
>  	u32 base;
>  };
>  
> +struct intel_scaler {
> +	int id;
> +	int in_use;
> +	uint32_t mode;
> +	uint32_t filter;
> +
> +	/*
> +	 * 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.
> +	 */

I've only skimmed a little ahead in the series, so I might have missed
something, but do we really need to track these on a per-scaler basis?
When you use the values here, I think you're always pulling the values
from scaler[0] unconditionally from what I saw so duplicating the values
for each scaler doesn't really help us.

Is it possible to keep just one copy of these min/max values?  On SKL
all of our scalers are homogeneous, so it doesn't feel like there's too
much value to duplicating these values.  If we have a future platform
with heterogeneous scalers, it seems like we can figure out how to
handle that appropriately if/when we get there.

> +	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_scaler_state {
> +#define INTEL_MAX_SCALERS 2
> +#define SKL_NUM_SCALERS INTEL_MAX_SCALERS
> +	/* scalers available on this crtc */
> +	int num_scalers;

Maybe add .num_scalers to the device_info struct?  I know it doesn't
make much of a difference, but it feels cleaner to have immutable traits
of the hardware in device_info or even the base intel_crtc structure and
leave the state variable for tracking things that can change at runtime.

> +	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;

Might be slightly preferable to use a type with a specific size when
creating a bitmask?  I.e., uint32_t or uint64_t, just to be explicit.

> +
> +	/* scaler used by crtc for panel fitting purpose */
> +	int scaler_id;
> +};
> +
>  struct intel_crtc_state {
>  	struct drm_crtc_state base;
>  
> @@ -391,6 +488,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

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

[Matt Roper]

On Sat, Mar 14, 2015 at 10:55:32PM -0700, Chandra Konduru wrote:
> Helper function updates supported scaling ratios based on cdclk and
> crtc clocks.

It might be worth squashing this into patch 15, which I believe is when
you first start using this.  It's a little but harder to review new
functions like this without the context of how/why/when they're called.

> 
> 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 da78e77..5591282 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4511,6 +4511,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;
> +	int i;
> +
> +	if (INTEL_INFO(dev)->gen < 9 || !crtc_state)
> +		return;

I wouldn't expect a skl_ function to even get called on gen<9.  Or for a
NULL CRTC state to be passed in.


> +
> +	crtc_clock = (uint32_t) crtc_state->base.adjusted_mode.crtc_clock;
> +	cdclk = (uint32_t) intel_ddi_get_cdclk_freq(dev_priv);
> +
> +	if (!crtc_clock || !cdclk)
> +		return;
> +
> +	for (i = 0; i < crtc_state->scaler_state.num_scalers; i++) {
> +		struct intel_scaler *scaler = &crtc_state->scaler_state.scalers[i];
> +
> +		scaler->min_hsr = max(scaler->min_hsr, (crtc_clock * 100)/cdclk);
> +		scaler->min_vsr = max(scaler->min_hsr, (crtc_clock * 100)/cdclk);
> +		scaler->min_hvsr = max(scaler->min_hsr, (crtc_clock * 100)/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

Re: [PATCH 12/21] drm/i915: Ensure colorkey and scaling aren't enabled at same time

[Konduru, Chandra]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Tuesday, March 17, 2015 8:13 AM
> To: Daniel Vetter
> Cc: Konduru, Chandra; Conselvan De Oliveira, Ander; intel-
> gfx@lists.freedesktop.org; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 12/21] drm/i915: Ensure colorkey and scaling
> aren't enabled at same time
> 
> On Tue, Mar 17, 2015 at 03:38:28PM +0100, Daniel Vetter wrote:
> > On Tue, Mar 17, 2015 at 02:16:33PM +0000, Chris Wilson wrote:
> > > On Tue, Mar 17, 2015 at 03:16:01PM +0100, Daniel Vetter wrote:
> > > > On Sat, Mar 14, 2015 at 10:55:37PM -0700, Chandra Konduru wrote:
> > > > > Plane scaling and colorkey are mutually exclusive. Ensure
> > > > > scaling isn't active at the time of enabling colorkey.
> > > > >
> > > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_sprite.c |    6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > index c010528..0194390 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > @@ -319,6 +319,12 @@ skl_update_colorkey(struct drm_plane
> *drm_plane,
> > > > >  	const int plane = intel_plane->plane;
> > > > >  	u32 plane_ctl;
> > > > >
> > > > > +	/* plane scaling and colorkey are mutually exclusive */
> > > > > +	if (to_intel_plane_state(drm_plane->state)->scaler_id >= 0) {
> > > > > +		DRM_ERROR("colorkey not allowed with scaler\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > >
> > > > Yeah this is a bit trouble because of the interactions between
> > > > colorkey state in hw and the atomic scaler state in the atomic state
> structures.
> > > >
> > > > Since colorkey isn't support with atomic atm anyway and we want to
> > > > move towards atomic: Do we really have anyone who cares about
> > > > legacy sprite ioctls on skl? If we'd just add a
> > >
> > > Yes. Current userspace uses sprites and colorkey.
> >
> > Yes I know we have the code around and it'll keep working. This was
> > more a question of priority and whether we really should keep it
> > working or whether cros/android don't really care. Since sooner or
> > later we need to convert it to atomic/properties if we want to keep it
> > working. Right now (since skl is still prelimary support) we can still do that I
> hope ...
> 
> Code was written a couple of years ago with the intent that it would work for
> the next 5 years across multiple hardware releases.
> -Chris

Reading color key from hw doesn't harm until sprite colorkey becomes atomic. 
So keeping this patch as there are users using colorkey.

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

Re: [PATCH 13/21] drm/i915: Preserve scaler state when clearing crtc_state

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, March 17, 2015 7:17 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 13/21] drm/i915: Preserve scaler state when
> clearing crtc_state
> 
> On Sat, Mar 14, 2015 at 10:55:38PM -0700, Chandra Konduru wrote:
> > 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>
> 
> I guess this is going to conflict with Ander's crtc state series. But since you're
> signed up to review that I guess you will know what needs to be changed here.
> Hopefully Ander's series will just take care of this.

Planes may have changed its scaling needs and updated scaler_users
is in crtc_state. Wiping out crtc state will wipeout staged scaler_state 
and cause incorrect behavior. So preserving computed (or staged) 
scaler state into crtc state.

Ander, 
Can you pls check for any issue here?

> -Daniel
> 
> > ---
> >  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 e07c24e..9c3c6b1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10617,11 +10617,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 int
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, March 17, 2015 7:05 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 01/21] drm/i915: Adding drm helper function
> drm_plane_from_index().
> 
> On Sat, Mar 14, 2015 at 10:55:26PM -0700, Chandra Konduru wrote:
> > Adding drm helper function to return plane pointer from index where
> > index is a returned by drm_plane_index.
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c |   20 ++++++++++++++++++++
> >  include/drm/drm_crtc.h     |    1 +
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 9f970c2..bbe573e1 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -1286,6 +1286,26 @@ 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;
> > +
> > +	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > +		if (drm_plane_index(plane) == idx)
> > +			return plane;
> 
> Just a bikeshed, but you can do the same counting loop as in drm_plane_index
> and then return the plane as soon as idx = i. Avoids a nested double-loop on the
> plane_list.
> -Daniel

Agree, will send out updated patch to avoid nested loop.

> 
> > +	}
> > +	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
> 
> --
> 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 14/21] drm/i915: use current scaler state during readout_hw_state.

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, March 17, 2015 7:20 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 14/21] drm/i915: use current scaler state during
> readout_hw_state.
> 
> On Sat, Mar 14, 2015 at 10:55:39PM -0700, Chandra Konduru wrote:
> > During readout_hw_state, whole pipe_config is built reading hw.
> > But rebuilding scaler state from hw requires,
> >  - reading all planes and find its corresponding index in order to set
> >    its bits in scaler_users
> >  - reading cdclk and adjusted mode crtc clk in order to regenerate
> >    min scaling ratios.
> >  - some values directly from bspec
> >
> > To simplify things, for now using sw scaler state as readout state.
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> 
> At least the crtc scaler might get used by the bios for boot-up with non-native
> resolution. I do think we need to properly track those parts (and also double-
> check the hw state in pipe config compare function like we do for the old pfit
> state).

To cover above case, will populate the crtc scaler state.

But population of scalers used for planes is tricky, because 
in general there is no population of planes or its states from hw.
From scaler point of this may be ok because there aren't any 
scalers in use for planes.

> 
> Maybe we need to always compute limits directly instead of storing the in the
> state structures?

We are adjusting/computing some of limits from hw but not all
limits. 
Along with crtc_scaler population, will add code to populate
limits that can be computed from hw. Rest will carry from previous
state.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 9c3c6b1..b1d7036 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14292,7 +14292,10 @@ static void
> intel_modeset_readout_hw_state(struct drm_device *dev)
> >  	int i;
> >
> >  	for_each_intel_crtc(dev, crtc) {
> > +		struct intel_crtc_scaler_state scaler_state;
> > +		scaler_state = crtc->config->scaler_state;
> >  		memset(crtc->config, 0, sizeof(*crtc->config));
> > +		crtc->config->scaler_state = scaler_state;
> >
> >  		crtc->config->quirks |=
> PIPE_CONFIG_QUIRK_INHERITED_MODE;
> >
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 19/21] drm/i915: Enable skylake panel fitting using skylake shared scalers

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, March 17, 2015 7:22 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 19/21] drm/i915: Enable skylake panel fitting
> using skylake shared scalers
> 
> On Sat, Mar 14, 2015 at 10:55:44PM -0700, Chandra Konduru wrote:
> > Modify skylake panel fitting implementation to use shared scalers.
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> 
> Ah here's the real pfit state, and the scaler state is just readout. Still we recover
> this from the bios, I do think we need to make sure that at least the crtc scaler is
> correctly assigned. Can't we recompute the scaler state after readout to make
> sure it fits with pfit state here?
> -Daniel

Crtc scaler can be computed from hw readout. As replied earlier, will be making 
changes to update crtc scaler_id and appropriately set the scaler_users. 

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   39 ++++++++++++++++++---------------
> -
> >  1 file changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 8ab9624..8deebb7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4192,11 +4192,23 @@ static void skylake_pfit_enable(struct intel_crtc
> *crtc)
> >  	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;
> >
> >  	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);
> >  	}
> >  }
> >
> > @@ -4664,17 +4676,7 @@ static void haswell_crtc_enable(struct drm_crtc
> > *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);
> > -	}
> > +	skl_detach_scaler(&crtc->base, NULL);
> >  }
> >
> >  static void ironlake_pfit_disable(struct intel_crtc *crtc) @@
> > -7953,13 +7955,14 @@ 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;
> > +	int id = crtc->config->scaler_state.scaler_id;
> >
> > -	tmp = I915_READ(PS_CTL(crtc->pipe));
> > +	tmp = id >= 0 ? I915_READ(SKL_PS_CTRL(crtc->pipe, id)) : 0;
> >
> > -	if (tmp & PS_ENABLE) {
> > +	if (tmp & PS_SCALER_EN) {
> >  		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));
> > +		pipe_config->pch_pfit.pos = I915_READ(SKL_PS_WIN_POS(crtc-
> >pipe, id));
> > +		pipe_config->pch_pfit.size = I915_READ(SKL_PS_WIN_SZ(crtc-
> >pipe,
> > +id));
> >  	}
> >  }
> >
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/21] skylake display scalers

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, March 17, 2015 7:33 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 00/21] skylake display scalers
> 
> On Sat, Mar 14, 2015 at 10:55:25PM -0700, Chandra Konduru wrote:
> > This patch enables skylake display scalers in atomic framework.
> > Skylake scalers are sharable within a pipe and can be used as a panel
> > fitter or plane scaler. Two scalers cannot be ganged to a single plane
> > to get higher scale factor but simultaneous use of one as plane scaler
> > and one scaler as panel fitter is allowed. Reformatted previous patch
> > series into smaller patches and addressed previous feedback inputs.
> > Performed some initial testing and more testing is in works.
> > Testing is done applying these patches on top of Ander's v2 atomic
> > crtc patches.
> > As several atomic crtc is in flight, will revisit scalers and perform
> > any additional testing after atomic crtc is in place.
> 
> Ok sprinkled a few smaller comments over a few patches, looks good overall I
> think. And we discussed a bunch of the more tricky bits already in a 1:1 code
> walkthrough.
> 
> A few high-level bits:
> - When splitting up patches pls dont make patches which just add
>   functions/structs without using them. That makes it a lot harder to
>   understand the big picture. The only exception is register #defines
>   since reviewing those against Bspec can be done mostly independently of
>   the code. So for functions make sure you have at least 1 use-site so
>   that it's clear what it does. For structures just start out with the
>   basic structure, then add members as you start using them.
> 
>   Since this amounts to a full rewrite of the patch series and this one
>   here isn't too tricky I think it's ok to not do that here. But please
>   follow this bkm for the next one.

Sure.

> 
> - Testcase: We definitely don't have testcases for primary plane scaling
>   (and panning fwiw), and iirc we also don't have any function testcases
>   for sprite scaling.
> 
> - Testcases 2: I think we're also missing tests for the crtc pfit stuff.
>   I think it'd be good to exercise the various resolutions and different
>   boxing modes supported. I don't think we can do a full functional
>   testcase here with crc.

I have written kms tests to cover, crtc pfit, primary plane scaling and
sprite scaling and some combinations. Will be sending out once I take 
care of the review feedback and test them one more time.

> 
> - Unfortunately we can't yet test the sharing logic fully due to lack of
>   atomic. But that's an open that we need to address once atomic has
>   landed. Especially to make sure that extreme cases (e.g. all scalers
>   used by planes <-> pfit enabled) work correctly.

I'm covering test cases but not using atomic. Agree that once atomic
is landed, scalers can be tested in atomic fashion.

> 
> Overall I think this is ready for detailed review. Since we lack igts I recommend
> that the reviewer also does the igt coverage, that's a good way to make sure all
> issues are caught.
> 
> Thanks, Daniel
> 
> 
> >
> > Thanks,
> > Chandra
> >
> > Chandra Konduru (21):
> >   drm/i915: Adding drm helper function drm_plane_from_index().
> >   drm/i915: Register definitions for skylake scalers
> >   drm/i915: Enable get_colorkey functions for primary plane.
> >   drm/i915: skylake scaler structure definitions
> >   drm/i915: Initialize skylake scalers
> >   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: Ensure planes begin with no scaler.
> >   drm/i915: Ensure colorkey and scaling aren't enabled at same time
> >   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           |   20 ++
> >  drivers/gpu/drm/i915/i915_reg.h      |  114 +++++++++
> >  drivers/gpu/drm/i915/intel_atomic.c  |  157 ++++++++++++
> > drivers/gpu/drm/i915/intel_display.c |  442
> +++++++++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_dp.c      |    7 +
> >  drivers/gpu/drm/i915/intel_drv.h     |  109 +++++++++
> >  drivers/gpu/drm/i915/intel_sprite.c  |   95 ++++++--
> >  include/drm/drm_crtc.h               |    1 +
> >  8 files changed, 895 insertions(+), 50 deletions(-)
> >
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/21] drm/i915: skylake scaler structure definitions

[Konduru, Chandra]

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, March 17, 2015 9:13 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander
> Subject: Re: [PATCH 04/21] drm/i915: skylake scaler structure definitions
> 
> On Sat, Mar 14, 2015 at 10:55:29PM -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.
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |   99
> ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 99 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 3f7d05e..d9a3b64 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -256,6 +256,35 @@ 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.
> > +	 *
> > +	 *   There are two scenarios:
> > +	 *   1. the freed up scaler is assigned to crtc or some other plane
> > +	 *      In this case, as part of plane programming scaler_id will be set
> > +	 *      to -1 using helper function detach_scalers
> > +	 *   2. the freed up scaler is not assigned to anyone
> > +	 *      In this case, as part of plane programming scaler registers will
> > +	 *      be reset and scaler_id will also be reset to -1 using the same
> > +	 *      helper function detach_scalers
> > +	 */
> > +	int scaler_id;
> >  };
> >
> >  struct intel_initial_plane_config {
> > @@ -265,6 +294,74 @@ struct intel_initial_plane_config {
> >  	u32 base;
> >  };
> >
> > +struct intel_scaler {
> > +	int id;
> > +	int in_use;
> > +	uint32_t mode;
> > +	uint32_t filter;
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> 
> I've only skimmed a little ahead in the series, so I might have missed something,
> but do we really need to track these on a per-scaler basis?
> When you use the values here, I think you're always pulling the values from
> scaler[0] unconditionally from what I saw so duplicating the values for each
> scaler doesn't really help us.
> 
> Is it possible to keep just one copy of these min/max values?  On SKL all of our
> scalers are homogeneous, so it doesn't feel like there's too much value to
> duplicating these values.  If we have a future platform with heterogeneous
> scalers, it seems like we can figure out how to handle that appropriately if/when
> we get there.

I put them per scaler basis for future scalability, but can make them as one copy.
It has some cascading effect on other patches so send out this one and other
impacted ones.

> 
> > +	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_scaler_state {
> > +#define INTEL_MAX_SCALERS 2
> > +#define SKL_NUM_SCALERS INTEL_MAX_SCALERS
> > +	/* scalers available on this crtc */
> > +	int num_scalers;
> 
> Maybe add .num_scalers to the device_info struct?  I know it doesn't make
> much of a difference, but it feels cleaner to have immutable traits of the
> hardware in device_info or even the base intel_crtc structure and leave the state
> variable for tracking things that can change at runtime.

num_scalers isn't symmetric across pipes, so we need maintain it per crtc.

> 
> > +	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;
> 
> Might be slightly preferable to use a type with a specific size when creating a
> bitmask?  I.e., uint32_t or uint64_t, just to be explicit.

I looked at other variables carrying bits, for example, disable_pipes, 
prepare_pipes, modeset_pipes, disabled_planes etc. and all are 
using just like above. And followed the same.

> 
> > +
> > +	/* scaler used by crtc for panel fitting purpose */
> > +	int scaler_id;
> > +};
> > +
> >  struct intel_crtc_state {
> >  	struct drm_crtc_state base;
> >
> > @@ -391,6 +488,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

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

[Konduru, Chandra]

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, March 17, 2015 9:35 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander
> Subject: Re: [PATCH 07/21] drm/i915: Helper function to update skylake scaling
> ratio.
> 
> On Sat, Mar 14, 2015 at 10:55:32PM -0700, Chandra Konduru wrote:
> > Helper function updates supported scaling ratios based on cdclk and
> > crtc clocks.
> 
> It might be worth squashing this into patch 15, which I believe is when you first
> start using this.  It's a little but harder to review new functions like this without
> the context of how/why/when they're called.

Daniel also gave similar feedback. For now, I am keeping it as is.
But will follow for the next one.

> 
> >
> > 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 da78e77..5591282 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4511,6 +4511,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;
> > +	int i;
> > +
> > +	if (INTEL_INFO(dev)->gen < 9 || !crtc_state)
> > +		return;
> 
> I wouldn't expect a skl_ function to even get called on gen<9.  Or for a NULL
> CRTC state to be passed in.

With atomicity influx and many things going on, I am little bit
cautious to make them robust.

> 
> 
> > +
> > +	crtc_clock = (uint32_t) crtc_state->base.adjusted_mode.crtc_clock;
> > +	cdclk = (uint32_t) intel_ddi_get_cdclk_freq(dev_priv);
> > +
> > +	if (!crtc_clock || !cdclk)
> > +		return;
> > +
> > +	for (i = 0; i < crtc_state->scaler_state.num_scalers; i++) {
> > +		struct intel_scaler *scaler = &crtc_state-
> >scaler_state.scalers[i];
> > +
> > +		scaler->min_hsr = max(scaler->min_hsr, (crtc_clock *
> 100)/cdclk);
> > +		scaler->min_vsr = max(scaler->min_hsr, (crtc_clock *
> 100)/cdclk);
> > +		scaler->min_hvsr = max(scaler->min_hsr, (crtc_clock *
> 100)/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

Re: [PATCH 13/21] drm/i915: Preserve scaler state when clearing crtc_state

[Ander Conselvan De Oliveira]

On Tue, 2015-03-17 at 18:11 +0000, Konduru, Chandra wrote:
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, March 17, 2015 7:17 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> > Subject: Re: [Intel-gfx] [PATCH 13/21] drm/i915: Preserve scaler state when
> > clearing crtc_state
> > 
> > On Sat, Mar 14, 2015 at 10:55:38PM -0700, Chandra Konduru wrote:
> > > 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>
> > 
> > I guess this is going to conflict with Ander's crtc state series. But since you're
> > signed up to review that I guess you will know what needs to be changed here.
> > Hopefully Ander's series will just take care of this.
> 
> Planes may have changed its scaling needs and updated scaler_users
> is in crtc_state. Wiping out crtc state will wipeout staged scaler_state 
> and cause incorrect behavior. So preserving computed (or staged) 
> scaler state into crtc state.
> 
> Ander, 
> Can you pls check for any issue here?

As is your patch shouldn't cause any trouble. The problem we have is
that the legacy modeset path assumes the new pipe_config is kzalloc'd. I
need to inspect all that code and check what really depends on the state
being zeroed, but I haven't got to it yet. Once that's done, we could
just duplicate the whole crtc state and be done with it.

Ander

> 
> > -Daniel
> > 
> > > ---
> > >  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 e07c24e..9c3c6b1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -10617,11 +10617,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 int
> > > --
> > > 1.7.9.5
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch


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

Re: [PATCH 04/21] drm/i915: skylake scaler structure definitions

[Daniel Vetter]

On Tue, Mar 17, 2015 at 09:20:11PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Roper, Matthew D
> > Sent: Tuesday, March 17, 2015 9:13 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander
> > Subject: Re: [PATCH 04/21] drm/i915: skylake scaler structure definitions
> > 
> > On Sat, Mar 14, 2015 at 10:55:29PM -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.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_drv.h |   99
> > ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 99 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 3f7d05e..d9a3b64 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -256,6 +256,35 @@ 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.
> > > +	 *
> > > +	 *   There are two scenarios:
> > > +	 *   1. the freed up scaler is assigned to crtc or some other plane
> > > +	 *      In this case, as part of plane programming scaler_id will be set
> > > +	 *      to -1 using helper function detach_scalers
> > > +	 *   2. the freed up scaler is not assigned to anyone
> > > +	 *      In this case, as part of plane programming scaler registers will
> > > +	 *      be reset and scaler_id will also be reset to -1 using the same
> > > +	 *      helper function detach_scalers
> > > +	 */
> > > +	int scaler_id;
> > >  };
> > >
> > >  struct intel_initial_plane_config {
> > > @@ -265,6 +294,74 @@ struct intel_initial_plane_config {
> > >  	u32 base;
> > >  };
> > >
> > > +struct intel_scaler {
> > > +	int id;
> > > +	int in_use;
> > > +	uint32_t mode;
> > > +	uint32_t filter;
> > > +
> > > +	/*
> > > +	 * 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.
> > > +	 */
> > 
> > I've only skimmed a little ahead in the series, so I might have missed something,
> > but do we really need to track these on a per-scaler basis?
> > When you use the values here, I think you're always pulling the values from
> > scaler[0] unconditionally from what I saw so duplicating the values for each
> > scaler doesn't really help us.
> > 
> > Is it possible to keep just one copy of these min/max values?  On SKL all of our
> > scalers are homogeneous, so it doesn't feel like there's too much value to
> > duplicating these values.  If we have a future platform with heterogeneous
> > scalers, it seems like we can figure out how to handle that appropriately if/when
> > we get there.
> 
> I put them per scaler basis for future scalability, but can make them as one copy.
> It has some cascading effect on other patches so send out this one and other
> impacted ones.

In my experience adding flexibility in the design for which we don't yet
have a clear use-case established is a tricky design mistake: It always
sounds good and very often turns out to be a costly endeavor. I much
prefer to be as lazy as possible in code design and if there's no need yet
to not write code.

Also, can't we recompute these limits at runtime? I can't quickly check
since the code is in some other patches, but iirc that would resolve some
of the issues I've pointed out later on with keeping these in sync.

I think as a design rule atomic state structures should be constrained to
the input values (we need them by design of the atomic interfaces) and the
computed values we want to put into the hw. Intermediate results,
especially for constraint checking just complicate all the state handling.
There are some exceptions (e.g. pll computation where we have intermediate
dotclock values), but adding state to the permanent state structures has
costs. Imo better to avoid it.
-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 14/21] drm/i915: use current scaler state during readout_hw_state.

[Daniel Vetter]

On Tue, Mar 17, 2015 at 06:54:30PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, March 17, 2015 7:20 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> > Subject: Re: [Intel-gfx] [PATCH 14/21] drm/i915: use current scaler state during
> > readout_hw_state.
> > 
> > On Sat, Mar 14, 2015 at 10:55:39PM -0700, Chandra Konduru wrote:
> > > During readout_hw_state, whole pipe_config is built reading hw.
> > > But rebuilding scaler state from hw requires,
> > >  - reading all planes and find its corresponding index in order to set
> > >    its bits in scaler_users
> > >  - reading cdclk and adjusted mode crtc clk in order to regenerate
> > >    min scaling ratios.
> > >  - some values directly from bspec
> > >
> > > To simplify things, for now using sw scaler state as readout state.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > 
> > At least the crtc scaler might get used by the bios for boot-up with non-native
> > resolution. I do think we need to properly track those parts (and also double-
> > check the hw state in pipe config compare function like we do for the old pfit
> > state).
> 
> To cover above case, will populate the crtc scaler state.
> 
> But population of scalers used for planes is tricky, because 
> in general there is no population of planes or its states from hw.
> From scaler point of this may be ok because there aren't any 
> scalers in use for planes.

Yeah plane scaler state is completely different. Imo it's ok to not care
about that - as you spotted we almost completely lack state readout
support for planes. No need to fix that in your series ;-)

> > Maybe we need to always compute limits directly instead of storing the in the
> > state structures?
> 
> We are adjusting/computing some of limits from hw but not all
> limits. 
> Along with crtc_scaler population, will add code to populate
> limits that can be computed from hw. Rest will carry from previous
> state.

My suggestion was to completely remove the limit fields from the scaler
state and always recompute them when needed. I think that would simplify
the code in a few other places, and reduce changes that these computed
values would get out of sync.
-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 04/21] drm/i915: skylake scaler structure definitions

[Daniel Vetter]

On Wed, Mar 18, 2015 at 09:00:20AM +0100, Daniel Vetter wrote:
> On Tue, Mar 17, 2015 at 09:20:11PM +0000, Konduru, Chandra wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Roper, Matthew D
> > > Sent: Tuesday, March 17, 2015 9:13 AM
> > > To: Konduru, Chandra
> > > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander
> > > Subject: Re: [PATCH 04/21] drm/i915: skylake scaler structure definitions
> > > 
> > > On Sat, Mar 14, 2015 at 10:55:29PM -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.
> > > >
> > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_drv.h |   99
> > > ++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 99 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 3f7d05e..d9a3b64 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -256,6 +256,35 @@ 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.
> > > > +	 *
> > > > +	 *   There are two scenarios:
> > > > +	 *   1. the freed up scaler is assigned to crtc or some other plane
> > > > +	 *      In this case, as part of plane programming scaler_id will be set
> > > > +	 *      to -1 using helper function detach_scalers
> > > > +	 *   2. the freed up scaler is not assigned to anyone
> > > > +	 *      In this case, as part of plane programming scaler registers will
> > > > +	 *      be reset and scaler_id will also be reset to -1 using the same
> > > > +	 *      helper function detach_scalers
> > > > +	 */
> > > > +	int scaler_id;
> > > >  };
> > > >
> > > >  struct intel_initial_plane_config {
> > > > @@ -265,6 +294,74 @@ struct intel_initial_plane_config {
> > > >  	u32 base;
> > > >  };
> > > >
> > > > +struct intel_scaler {
> > > > +	int id;
> > > > +	int in_use;
> > > > +	uint32_t mode;
> > > > +	uint32_t filter;
> > > > +
> > > > +	/*
> > > > +	 * 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.
> > > > +	 */
> > > 
> > > I've only skimmed a little ahead in the series, so I might have missed something,
> > > but do we really need to track these on a per-scaler basis?
> > > When you use the values here, I think you're always pulling the values from
> > > scaler[0] unconditionally from what I saw so duplicating the values for each
> > > scaler doesn't really help us.
> > > 
> > > Is it possible to keep just one copy of these min/max values?  On SKL all of our
> > > scalers are homogeneous, so it doesn't feel like there's too much value to
> > > duplicating these values.  If we have a future platform with heterogeneous
> > > scalers, it seems like we can figure out how to handle that appropriately if/when
> > > we get there.
> > 
> > I put them per scaler basis for future scalability, but can make them as one copy.
> > It has some cascading effect on other patches so send out this one and other
> > impacted ones.
> 
> In my experience adding flexibility in the design for which we don't yet
> have a clear use-case established is a tricky design mistake: It always
> sounds good and very often turns out to be a costly endeavor. I much
> prefer to be as lazy as possible in code design and if there's no need yet
> to not write code.
> 
> Also, can't we recompute these limits at runtime? I can't quickly check
> since the code is in some other patches, but iirc that would resolve some
> of the issues I've pointed out later on with keeping these in sync.
> 
> I think as a design rule atomic state structures should be constrained to
> the input values (we need them by design of the atomic interfaces) and the
> computed values we want to put into the hw. Intermediate results,
> especially for constraint checking just complicate all the state handling.
> There are some exceptions (e.g. pll computation where we have intermediate
> dotclock values), but adding state to the permanent state structures has
> costs. Imo better to avoid it.

Ok forget about all this, somehow I got really confused with the split
between intel_scaler and intel_scaler_state. Dunno why but I thought some
of the limits are in scaler_state and need to be recomputed. I'll
follow-up with new comments.
-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 08/21] drm/i915: Add helper function to update scaler_users in crtc_state

[Daniel Vetter]

On Sun, Mar 15, 2015 at 6:55 AM, Chandra Konduru
<chandra.konduru@intel.com> wrote:
> +       /*
> +        * check for rect size:
> +        *     min sizes in case of scaling involved
> +        *     max sizes in all cases
> +        */
> +       if ((need_scaling &&
> +               (src_w < scaler->min_src_w || src_h < scaler->min_src_h ||
> +                dst_w < scaler->min_dst_w || dst_h < scaler->min_dst_h)) ||
> +
> +                src_w > scaler->max_src_w || src_h > scaler->max_src_h ||
> +                dst_w > scaler->max_dst_w || dst_h > scaler->max_dst_h) {

Ok let's hope I've traced all the min/max stuff in your patches
correctly. It looks like we only ever initialized them to fixed
values, never changed them and only use them here. Spreading out the
values like that without having a real need for this flexibility makes
review really hard imo.

What about instead adding a skl_check_scale_limits functions which
does all these checks here and uses hardcoded values? That way you
could move the commits about the various values (e.g. only 34% scaling
and the other easier-to-understand limits) right next to the code that
checks these limits?

There's also some confusion with the overly generic (imo) old sprite
code and its scaling limit checks. Imo we can look at that later on.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 19/21] drm/i915: Enable skylake panel fitting using skylake shared scalers

[Daniel Vetter]

On Tue, Mar 17, 2015 at 08:43:02PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, March 17, 2015 7:22 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> > Subject: Re: [Intel-gfx] [PATCH 19/21] drm/i915: Enable skylake panel fitting
> > using skylake shared scalers
> > 
> > On Sat, Mar 14, 2015 at 10:55:44PM -0700, Chandra Konduru wrote:
> > > Modify skylake panel fitting implementation to use shared scalers.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > 
> > Ah here's the real pfit state, and the scaler state is just readout. Still we recover
> > this from the bios, I do think we need to make sure that at least the crtc scaler is
> > correctly assigned. Can't we recompute the scaler state after readout to make
> > sure it fits with pfit state here?
> > -Daniel
> 
> Crtc scaler can be computed from hw readout. As replied earlier, will be making 
> changes to update crtc scaler_id and appropriately set the scaler_users. 

When you add state readout suppoort for crtc scaler id, please also add
corresponding self-check code to intel_pipe_config_compare. That way we
can exercise all the fastboot code with normal igt testcases which
massively improves automated test coverage.
-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 08/21] drm/i915: Add helper function to update scaler_users in crtc_state

[Konduru, Chandra]

> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, March 18, 2015 1:16 AM
> To: Konduru, Chandra
> Cc: intel-gfx; Conselvan De Oliveira, Ander; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 08/21] drm/i915: Add helper function to update
> scaler_users in crtc_state
> 
> On Sun, Mar 15, 2015 at 6:55 AM, Chandra Konduru
> <chandra.konduru@intel.com> wrote:
> > +       /*
> > +        * check for rect size:
> > +        *     min sizes in case of scaling involved
> > +        *     max sizes in all cases
> > +        */
> > +       if ((need_scaling &&
> > +               (src_w < scaler->min_src_w || src_h < scaler->min_src_h ||
> > +                dst_w < scaler->min_dst_w || dst_h <
> > + scaler->min_dst_h)) ||
> > +
> > +                src_w > scaler->max_src_w || src_h > scaler->max_src_h ||
> > +                dst_w > scaler->max_dst_w || dst_h >
> > + scaler->max_dst_h) {
> 
> Ok let's hope I've traced all the min/max stuff in your patches correctly. It looks
> like we only ever initialized them to fixed values, never changed them and only
> use them here. Spreading out the values like that without having a real need for
> this flexibility makes review really hard imo.
> 
> What about instead adding a skl_check_scale_limits functions which does all
> these checks here and uses hardcoded values? That way you could move the
> commits about the various values (e.g. only 34% scaling and the other easier-to-
> understand limits) right next to the code that checks these limits?

I agree that some of limits are fixed, ratios aren't fixed. So kept them
in state and updated during modeset. There isn't much complexity with current 
approach.

> 
> There's also some confusion with the overly generic (imo) old sprite code and its
> scaling limit checks. Imo we can look at that later on.

Agree. 

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Mar 19, 2015 at 05:43:24PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > Daniel Vetter
> > Sent: Wednesday, March 18, 2015 1:16 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx; Conselvan De Oliveira, Ander; Vetter, Daniel
> > Subject: Re: [Intel-gfx] [PATCH 08/21] drm/i915: Add helper function to update
> > scaler_users in crtc_state
> > 
> > On Sun, Mar 15, 2015 at 6:55 AM, Chandra Konduru
> > <chandra.konduru@intel.com> wrote:
> > > +       /*
> > > +        * check for rect size:
> > > +        *     min sizes in case of scaling involved
> > > +        *     max sizes in all cases
> > > +        */
> > > +       if ((need_scaling &&
> > > +               (src_w < scaler->min_src_w || src_h < scaler->min_src_h ||
> > > +                dst_w < scaler->min_dst_w || dst_h <
> > > + scaler->min_dst_h)) ||
> > > +
> > > +                src_w > scaler->max_src_w || src_h > scaler->max_src_h ||
> > > +                dst_w > scaler->max_dst_w || dst_h >
> > > + scaler->max_dst_h) {
> > 
> > Ok let's hope I've traced all the min/max stuff in your patches correctly. It looks
> > like we only ever initialized them to fixed values, never changed them and only
> > use them here. Spreading out the values like that without having a real need for
> > this flexibility makes review really hard imo.
> > 
> > What about instead adding a skl_check_scale_limits functions which does all
> > these checks here and uses hardcoded values? That way you could move the
> > commits about the various values (e.g. only 34% scaling and the other easier-to-
> > understand limits) right next to the code that checks these limits?
> 
> I agree that some of limits are fixed, ratios aren't fixed. So kept them
> in state and updated during modeset. There isn't much complexity with current 
> approach.

Hm, where are the ratios? I haven't found that part in your series ...
-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