[PATCH 0/9] .rodata diet 2 (non-disruptive version)

[PATCH 0/9] .rodata diet 2 (non-disruptive version)

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Drive by shrinkage of various static const tables. Also, Joonas complained about
some unsightly casts and too casual data type usage in the watermark code so
I've added a few fixes for that as well.

Series altogether saves around 3.5KiB of combined .text and .rodata.

   text    data     bss     dec     hex filename
1069462   23256     576 1093294  10aeae i915.ko.nightly-161007
1065785   23256     576 1089617  10a051 i915.ko.rodatawm

Tvrtko Ursulin (9):
  drm/i915: Shrink cxsr_latency_table
  drm/i915: Shrink sdvo_cmd_names
  drm/i915: Shrink per-platform watermark configuration
  drm/i915: Shrink TV modes const data
  drm/i915: Use unsigned int for latencies
  drm/i915: unsigned int is enough for crtc clock
  drm/i915: Convert get_fifo_size return from int to unsigned int
  drm/i915: Make intel_calculate_wm return unsigned int
  drm/i915: Tidy watermark computation local types

 drivers/gpu/drm/i915/i915_drv.h   |   2 +-
 drivers/gpu/drm/i915/intel_drv.h  |  26 +++----
 drivers/gpu/drm/i915/intel_pm.c   | 146 ++++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_sdvo.c |   2 +-
 drivers/gpu/drm/i915/intel_tv.c   |  50 +++++++------
 5 files changed, 115 insertions(+), 111 deletions(-)

-- 
2.7.4

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

[PATCH 1/9] drm/i915: Shrink cxsr_latency_table

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

unsigned long is too wide - use smaller types in
struct cxsr_latency to save 800-something bytes of .rodata.

v2: All data even fits in u16 for even more saving. (Ville Syrjala)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_pm.c  |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f48e79ae2ac6..f618a1c4b9a3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -807,14 +807,14 @@ struct intel_watermark_params {
 };
 
 struct cxsr_latency {
-	int is_desktop;
-	int is_ddr3;
-	unsigned long fsb_freq;
-	unsigned long mem_freq;
-	unsigned long display_sr;
-	unsigned long display_hpll_disable;
-	unsigned long cursor_sr;
-	unsigned long cursor_hpll_disable;
+	bool is_desktop : 1;
+	bool is_ddr3 : 1;
+	u16 fsb_freq;
+	u16 mem_freq;
+	u16 display_sr;
+	u16 display_hpll_disable;
+	u16 cursor_sr;
+	u16 cursor_hpll_disable;
 };
 
 #define to_intel_atomic_state(x) container_of(x, struct intel_atomic_state, base)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7f1748a1e614..3b1f0b40ccb9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -252,8 +252,8 @@ static const struct cxsr_latency cxsr_latency_table[] = {
 	{0, 1, 400, 800, 6042, 36042, 6584, 36584},    /* DDR3-800 SC */
 };
 
-static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
-							 int is_ddr3,
+static const struct cxsr_latency *intel_get_cxsr_latency(bool is_desktop,
+							 bool is_ddr3,
 							 int fsb,
 							 int mem)
 {
-- 
2.7.4

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

[PATCH 2/9] drm/i915: Shrink sdvo_cmd_names

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Pack the struct _sdvo_cmd_name to save 736 bytes of .rodata.

This is fine since the name pointers are used only for debug.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index a061b0029797..9f352aac9526 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -307,7 +307,7 @@ static bool intel_sdvo_read_byte(struct intel_sdvo *intel_sdvo, u8 addr, u8 *ch)
 static const struct _sdvo_cmd_name {
 	u8 cmd;
 	const char *name;
-} sdvo_cmd_names[] = {
+} __attribute__ ((packed)) sdvo_cmd_names[] = {
 	SDVO_CMD_NAME_ENTRY(SDVO_CMD_RESET),
 	SDVO_CMD_NAME_ENTRY(SDVO_CMD_GET_DEVICE_CAPS),
 	SDVO_CMD_NAME_ENTRY(SDVO_CMD_GET_FIRMWARE_REV),
-- 
2.7.4

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

[PATCH 3/9] drm/i915: Shrink per-platform watermark configuration

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Use types of more appropriate size in struct
intel_watermark_params to save 512 bytes of .rodata.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 10 +++++-----
 drivers/gpu/drm/i915/intel_pm.c  |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f618a1c4b9a3..2ab417c73ad8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -799,11 +799,11 @@ struct intel_plane {
 };
 
 struct intel_watermark_params {
-	unsigned long fifo_size;
-	unsigned long max_wm;
-	unsigned long default_wm;
-	unsigned long guard_size;
-	unsigned long cacheline_size;
+	u16 fifo_size;
+	u16 max_wm;
+	u8 default_wm;
+	u8 guard_size;
+	u8 cacheline_size;
 };
 
 struct cxsr_latency {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3b1f0b40ccb9..96d0c57c816c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -775,13 +775,13 @@ static bool g4x_check_srwm(struct drm_device *dev,
 		      display_wm, cursor_wm);
 
 	if (display_wm > display->max_wm) {
-		DRM_DEBUG_KMS("display watermark is too large(%d/%ld), disabling\n",
+		DRM_DEBUG_KMS("display watermark is too large(%d/%u), disabling\n",
 			      display_wm, display->max_wm);
 		return false;
 	}
 
 	if (cursor_wm > cursor->max_wm) {
-		DRM_DEBUG_KMS("cursor watermark is too large(%d/%ld), disabling\n",
+		DRM_DEBUG_KMS("cursor watermark is too large(%d/%u), disabling\n",
 			      cursor_wm, cursor->max_wm);
 		return false;
 	}
-- 
2.7.4

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

[PATCH 4/9] drm/i915: Shrink TV modes const data

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Make struct video_levels and struct tv_mode use data types
of sufficient width to save approximately one kilobyte in
the .rodata section.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_tv.c | 50 ++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 3988c45f9e5f..fd4d59341897 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -86,7 +86,8 @@ struct intel_tv {
 };
 
 struct video_levels {
-	int blank, black, burst;
+	u16 blank, black;
+	u8  burst;
 };
 
 struct color_conversion {
@@ -339,34 +340,43 @@ static const struct video_levels component_levels = {
 
 struct tv_mode {
 	const char *name;
-	int clock;
-	int refresh; /* in millihertz (for precision) */
-	u32 oversample;
-	int hsync_end, hblank_start, hblank_end, htotal;
-	bool progressive, trilevel_sync, component_only;
-	int vsync_start_f1, vsync_start_f2, vsync_len;
-	bool veq_ena;
-	int veq_start_f1, veq_start_f2, veq_len;
-	int vi_end_f1, vi_end_f2, nbr_end;
-	bool burst_ena;
-	int hburst_start, hburst_len;
-	int vburst_start_f1, vburst_end_f1;
-	int vburst_start_f2, vburst_end_f2;
-	int vburst_start_f3, vburst_end_f3;
-	int vburst_start_f4, vburst_end_f4;
+
+	u32  clock;
+	u16  refresh; /* in millihertz (for precision) */
+	u32  oversample;
+	u8   hsync_end;
+	u16  hblank_start, hblank_end, htotal;
+	bool progressive : 1, trilevel_sync : 1, component_only : 1;
+	u8   vsync_start_f1, vsync_start_f2, vsync_len;
+	bool veq_ena : 1;
+	u8   veq_start_f1, veq_start_f2, veq_len;
+	u8   vi_end_f1, vi_end_f2;
+	u16  nbr_end;
+	bool burst_ena : 1;
+	u8   hburst_start, hburst_len;
+	u8   vburst_start_f1;
+	u16  vburst_end_f1;
+	u8   vburst_start_f2;
+	u16  vburst_end_f2;
+	u8   vburst_start_f3;
+	u16  vburst_end_f3;
+	u8   vburst_start_f4;
+	u16  vburst_end_f4;
 	/*
 	 * subcarrier programming
 	 */
-	int dda2_size, dda3_size, dda1_inc, dda2_inc, dda3_inc;
-	u32 sc_reset;
-	bool pal_burst;
+	u16  dda2_size, dda3_size;
+	u8   dda1_inc;
+	u16  dda2_inc, dda3_inc;
+	u32  sc_reset;
+	bool pal_burst : 1;
 	/*
 	 * blank/black levels
 	 */
 	const struct video_levels *composite_levels, *svideo_levels;
 	const struct color_conversion *composite_color, *svideo_color;
 	const u32 *filter_table;
-	int max_srcw;
+	u16   max_srcw;
 };
 
 
-- 
2.7.4

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

[PATCH 5/9] drm/i915: Use unsigned int for latencies

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 96d0c57c816c..68b3614c6a0b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -372,7 +372,7 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
  * A value of 5us seems to be a good balance; safe for very low end
  * platforms but not overly aggressive on lower latency configs.
  */
-static const int pessimal_latency_ns = 5000;
+static const unsigned int pessimal_latency_ns = 5000;
 
 #define VLV_FIFO_START(dsparb, dsparb2, lo_shift, hi_shift) \
 	((((dsparb) >> (lo_shift)) & 0xff) | ((((dsparb2) >> (hi_shift)) & 0x1) << 8))
@@ -585,7 +585,7 @@ static const struct intel_watermark_params i845_wm_info = {
 static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
 					const struct intel_watermark_params *wm,
 					int fifo_size, int cpp,
-					unsigned long latency_ns)
+					unsigned int latency_ns)
 {
 	long entries_required, wm_size;
 
@@ -709,9 +709,9 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
 static bool g4x_compute_wm0(struct drm_device *dev,
 			    int plane,
 			    const struct intel_watermark_params *display,
-			    int display_latency_ns,
+			    unsigned int display_latency_ns,
 			    const struct intel_watermark_params *cursor,
-			    int cursor_latency_ns,
+			    unsigned int cursor_latency_ns,
 			    int *plane_wm,
 			    int *cursor_wm)
 {
@@ -796,7 +796,7 @@ static bool g4x_check_srwm(struct drm_device *dev,
 
 static bool g4x_compute_srwm(struct drm_device *dev,
 			     int plane,
-			     int latency_ns,
+			     unsigned int latency_ns,
 			     const struct intel_watermark_params *display,
 			     const struct intel_watermark_params *cursor,
 			     int *display_wm, int *cursor_wm)
@@ -1385,7 +1385,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 static void g4x_update_wm(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	static const int sr_latency_ns = 12000;
+	static const unsigned int sr_latency_ns = 12000;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
 	int plane_sr, cursor_sr;
@@ -1453,7 +1453,7 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
 	crtc = single_enabled_crtc(dev);
 	if (crtc) {
 		/* self-refresh has much higher latency */
-		static const int sr_latency_ns = 12000;
+		static const unsigned int sr_latency_ns = 12000;
 		const struct drm_display_mode *adjusted_mode = &to_intel_crtc(crtc)->config->base.adjusted_mode;
 		int clock = adjusted_mode->crtc_clock;
 		int htotal = adjusted_mode->crtc_htotal;
@@ -1600,7 +1600,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	/* Calc sr entries for one plane configs */
 	if (HAS_FW_BLC(dev) && enabled) {
 		/* self-refresh has much higher latency */
-		static const int sr_latency_ns = 6000;
+		static const unsigned int sr_latency_ns = 6000;
 		const struct drm_display_mode *adjusted_mode = &to_intel_crtc(enabled)->config->base.adjusted_mode;
 		int clock = adjusted_mode->crtc_clock;
 		int htotal = adjusted_mode->crtc_htotal;
-- 
2.7.4

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

[PATCH 6/9] drm/i915: unsigned int is enough for crtc clock

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 68b3614c6a0b..56726777b4d3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -582,7 +582,7 @@ static const struct intel_watermark_params i845_wm_info = {
  * past the watermark point.  If the FIFO drains completely, a FIFO underrun
  * will occur, and a display engine hang could result.
  */
-static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
+static unsigned long intel_calculate_wm(unsigned int clock_in_khz,
 					const struct intel_watermark_params *wm,
 					int fifo_size, int cpp,
 					unsigned int latency_ns)
-- 
2.7.4

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

[PATCH 7/9] drm/i915: Convert get_fifo_size return from int to unsigned int

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Unsigned is a more appropriate data type in this case.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/intel_pm.c | 36 ++++++++++++++++++------------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a219a3534750..59a2a9598c4b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -492,7 +492,7 @@ struct dpll;
 
 struct drm_i915_display_funcs {
 	int (*get_display_clock_speed)(struct drm_device *dev);
-	int (*get_fifo_size)(struct drm_device *dev, int plane);
+	unsigned int (*get_fifo_size)(struct drm_device *dev, int plane);
 	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
 	int (*compute_intermediate_wm)(struct drm_device *dev,
 				       struct intel_crtc *intel_crtc,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 56726777b4d3..faa379e54322 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -377,11 +377,11 @@ static const unsigned int pessimal_latency_ns = 5000;
 #define VLV_FIFO_START(dsparb, dsparb2, lo_shift, hi_shift) \
 	((((dsparb) >> (lo_shift)) & 0xff) | ((((dsparb2) >> (hi_shift)) & 0x1) << 8))
 
-static int vlv_get_fifo_size(struct drm_device *dev,
-			      enum pipe pipe, int plane)
+static unsigned int vlv_get_fifo_size(struct drm_device *dev,
+				      enum pipe pipe, int plane)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int sprite0_start, sprite1_start, size;
+	unsigned int sprite0_start, sprite1_start, size;
 
 	switch (pipe) {
 		uint32_t dsparb, dsparb2, dsparb3;
@@ -421,7 +421,7 @@ static int vlv_get_fifo_size(struct drm_device *dev,
 		return 0;
 	}
 
-	DRM_DEBUG_KMS("Pipe %c %s %c FIFO size: %d\n",
+	DRM_DEBUG_KMS("Pipe %c %s %c FIFO size: %u\n",
 		      pipe_name(pipe), plane == 0 ? "primary" : "sprite",
 		      plane == 0 ? plane_name(pipe) : sprite_name(pipe, plane - 1),
 		      size);
@@ -429,49 +429,49 @@ static int vlv_get_fifo_size(struct drm_device *dev,
 	return size;
 }
 
-static int i9xx_get_fifo_size(struct drm_device *dev, int plane)
+static unsigned int i9xx_get_fifo_size(struct drm_device *dev, int plane)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	uint32_t dsparb = I915_READ(DSPARB);
-	int size;
+	u32 dsparb = I915_READ(DSPARB);
+	unsigned int size;
 
 	size = dsparb & 0x7f;
 	if (plane)
 		size = ((dsparb >> DSPARB_CSTART_SHIFT) & 0x7f) - size;
 
-	DRM_DEBUG_KMS("FIFO size - (0x%08x) %s: %d\n", dsparb,
+	DRM_DEBUG_KMS("FIFO size - (0x%08x) %s: %u\n", dsparb,
 		      plane ? "B" : "A", size);
 
 	return size;
 }
 
-static int i830_get_fifo_size(struct drm_device *dev, int plane)
+static unsigned int i830_get_fifo_size(struct drm_device *dev, int plane)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	uint32_t dsparb = I915_READ(DSPARB);
-	int size;
+	u32 dsparb = I915_READ(DSPARB);
+	unsigned int size;
 
 	size = dsparb & 0x1ff;
 	if (plane)
 		size = ((dsparb >> DSPARB_BEND_SHIFT) & 0x1ff) - size;
 	size >>= 1; /* Convert to cachelines */
 
-	DRM_DEBUG_KMS("FIFO size - (0x%08x) %s: %d\n", dsparb,
+	DRM_DEBUG_KMS("FIFO size - (0x%08x) %s: %u\n", dsparb,
 		      plane ? "B" : "A", size);
 
 	return size;
 }
 
-static int i845_get_fifo_size(struct drm_device *dev, int plane)
+static unsigned int i845_get_fifo_size(struct drm_device *dev, int plane)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	uint32_t dsparb = I915_READ(DSPARB);
-	int size;
+	u32 dsparb = I915_READ(DSPARB);
+	unsigned int size;
 
 	size = dsparb & 0x7f;
 	size >>= 2; /* Convert to cachelines */
 
-	DRM_DEBUG_KMS("FIFO size - (0x%08x) %s: %d\n", dsparb,
+	DRM_DEBUG_KMS("FIFO size - (0x%08x) %s: %u\n", dsparb,
 		      plane ? "B" : "A",
 		      size);
 
@@ -584,7 +584,7 @@ static const struct intel_watermark_params i845_wm_info = {
  */
 static unsigned long intel_calculate_wm(unsigned int clock_in_khz,
 					const struct intel_watermark_params *wm,
-					int fifo_size, int cpp,
+					unsigned int fifo_size, int cpp,
 					unsigned int latency_ns)
 {
 	long entries_required, wm_size;
@@ -1522,7 +1522,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	uint32_t fwater_lo;
 	uint32_t fwater_hi;
 	int cwm, srwm = 1;
-	int fifo_size;
+	unsigned int fifo_size;
 	int planea_wm, planeb_wm;
 	struct drm_crtc *crtc, *enabled = NULL;
 
-- 
2.7.4

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

[PATCH 8/9] drm/i915: Make intel_calculate_wm return unsigned int

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Either unsigned int is enough or it isn't. There is no point in
using an unsigned long here.

Also replace local long variables with integers. No need to have
a difference between 32- and 64-bit build in any case.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index faa379e54322..308edc4378fa 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -582,12 +582,12 @@ static const struct intel_watermark_params i845_wm_info = {
  * past the watermark point.  If the FIFO drains completely, a FIFO underrun
  * will occur, and a display engine hang could result.
  */
-static unsigned long intel_calculate_wm(unsigned int clock_in_khz,
-					const struct intel_watermark_params *wm,
-					unsigned int fifo_size, int cpp,
-					unsigned int latency_ns)
+static unsigned int intel_calculate_wm(unsigned int clock_in_khz,
+				       const struct intel_watermark_params *wm,
+				       unsigned int fifo_size, int cpp,
+				       unsigned int latency_ns)
 {
-	long entries_required, wm_size;
+	int entries_required, wm_size;
 
 	/*
 	 * Note: we need to make sure we don't overflow for various clock &
@@ -595,18 +595,17 @@ static unsigned long intel_calculate_wm(unsigned int clock_in_khz,
 	 * clocks go from a few thousand to several hundred thousand.
 	 * latency is usually a few thousand
 	 */
-	entries_required = ((clock_in_khz / 1000) * cpp * latency_ns) /
-		1000;
+	entries_required = ((clock_in_khz / 1000) * cpp * latency_ns) / 1000;
 	entries_required = DIV_ROUND_UP(entries_required, wm->cacheline_size);
 
-	DRM_DEBUG_KMS("FIFO entries required for mode: %ld\n", entries_required);
+	DRM_DEBUG_KMS("FIFO entries required for mode: %d\n", entries_required);
 
 	wm_size = fifo_size - (entries_required + wm->guard_size);
 
-	DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
+	DRM_DEBUG_KMS("FIFO watermark level: %d\n", wm_size);
 
 	/* Don't promote wm_size to unsigned... */
-	if (wm_size > (long)wm->max_wm)
+	if (wm_size > (int)wm->max_wm)
 		wm_size = wm->max_wm;
 	if (wm_size <= 0)
 		wm_size = wm->default_wm;
@@ -646,7 +645,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
 	struct drm_crtc *crtc;
 	const struct cxsr_latency *latency;
 	u32 reg;
-	unsigned long wm;
+	unsigned int wm;
 
 	latency = intel_get_cxsr_latency(IS_PINEVIEW_G(dev), dev_priv->is_ddr3,
 					 dev_priv->fsb_freq, dev_priv->mem_freq);
@@ -1522,8 +1521,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	uint32_t fwater_lo;
 	uint32_t fwater_hi;
 	int cwm, srwm = 1;
-	unsigned int fifo_size;
-	int planea_wm, planeb_wm;
+	unsigned int fifo_size, planea_wm, planeb_wm;
 	struct drm_crtc *crtc, *enabled = NULL;
 
 	if (IS_I945GM(dev))
@@ -1548,7 +1546,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 		enabled = crtc;
 	} else {
 		planea_wm = fifo_size - wm_info->guard_size;
-		if (planea_wm > (long)wm_info->max_wm)
+		if (planea_wm > wm_info->max_wm)
 			planea_wm = wm_info->max_wm;
 	}
 
@@ -1573,11 +1571,11 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 			enabled = NULL;
 	} else {
 		planeb_wm = fifo_size - wm_info->guard_size;
-		if (planeb_wm > (long)wm_info->max_wm)
+		if (planeb_wm > wm_info->max_wm)
 			planeb_wm = wm_info->max_wm;
 	}
 
-	DRM_DEBUG_KMS("FIFO watermarks - A: %d, B: %d\n", planea_wm, planeb_wm);
+	DRM_DEBUG_KMS("FIFO watermarks - A: %u, B: %u\n", planea_wm, planeb_wm);
 
 	if (IS_I915GM(dev) && enabled) {
 		struct drm_i915_gem_object *obj;
@@ -1653,8 +1651,8 @@ static void i845_update_wm(struct drm_crtc *unused_crtc)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *crtc;
 	const struct drm_display_mode *adjusted_mode;
-	uint32_t fwater_lo;
-	int planea_wm;
+	u32 fwater_lo;
+	unsigned int planea_wm;
 
 	crtc = single_enabled_crtc(dev);
 	if (crtc == NULL)
@@ -1668,7 +1666,7 @@ static void i845_update_wm(struct drm_crtc *unused_crtc)
 	fwater_lo = I915_READ(FW_BLC) & ~0xfff;
 	fwater_lo |= (3<<8) | planea_wm;
 
-	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d\n", planea_wm);
+	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %u\n", planea_wm);
 
 	I915_WRITE(FW_BLC, fwater_lo);
 }
-- 
2.7.4

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

[PATCH 9/9] drm/i915: Tidy watermark computation local types

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Convert to from signed to unsigned and from longs
to integers where appropriate.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 60 +++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 308edc4378fa..1203a7a80e9e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -711,14 +711,14 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 			    unsigned int display_latency_ns,
 			    const struct intel_watermark_params *cursor,
 			    unsigned int cursor_latency_ns,
-			    int *plane_wm,
-			    int *cursor_wm)
+			    unsigned int *plane_wm,
+			    unsigned int *cursor_wm)
 {
 	struct drm_crtc *crtc;
 	const struct drm_display_mode *adjusted_mode;
 	int htotal, hdisplay, clock, cpp;
-	int line_time_us, line_count;
-	int entries, tlb_miss;
+	unsigned int line_time_us, line_count, entries;
+	int tlb_miss;
 
 	crtc = intel_get_crtc_for_plane(dev, plane);
 	if (!intel_crtc_active(crtc)) {
@@ -740,7 +740,7 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 		entries += tlb_miss;
 	entries = DIV_ROUND_UP(entries, display->cacheline_size);
 	*plane_wm = entries + display->guard_size;
-	if (*plane_wm > (int)display->max_wm)
+	if (*plane_wm > display->max_wm)
 		*plane_wm = display->max_wm;
 
 	/* Use the large buffer method to calculate cursor watermark */
@@ -752,8 +752,8 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 		entries += tlb_miss;
 	entries = DIV_ROUND_UP(entries, cursor->cacheline_size);
 	*cursor_wm = entries + cursor->guard_size;
-	if (*cursor_wm > (int)cursor->max_wm)
-		*cursor_wm = (int)cursor->max_wm;
+	if (*cursor_wm > cursor->max_wm)
+		*cursor_wm = cursor->max_wm;
 
 	return true;
 }
@@ -766,21 +766,21 @@ static bool g4x_compute_wm0(struct drm_device *dev,
  * must be disabled.
  */
 static bool g4x_check_srwm(struct drm_device *dev,
-			   int display_wm, int cursor_wm,
+			   unsigned int display_wm, unsigned int cursor_wm,
 			   const struct intel_watermark_params *display,
 			   const struct intel_watermark_params *cursor)
 {
-	DRM_DEBUG_KMS("SR watermark: display plane %d, cursor %d\n",
+	DRM_DEBUG_KMS("SR watermark: display plane %u, cursor %u\n",
 		      display_wm, cursor_wm);
 
 	if (display_wm > display->max_wm) {
-		DRM_DEBUG_KMS("display watermark is too large(%d/%u), disabling\n",
+		DRM_DEBUG_KMS("display watermark is too large(%u/%u), disabling\n",
 			      display_wm, display->max_wm);
 		return false;
 	}
 
 	if (cursor_wm > cursor->max_wm) {
-		DRM_DEBUG_KMS("cursor watermark is too large(%d/%u), disabling\n",
+		DRM_DEBUG_KMS("cursor watermark is too large(%u/%u), disabling\n",
 			      cursor_wm, cursor->max_wm);
 		return false;
 	}
@@ -798,15 +798,13 @@ static bool g4x_compute_srwm(struct drm_device *dev,
 			     unsigned int latency_ns,
 			     const struct intel_watermark_params *display,
 			     const struct intel_watermark_params *cursor,
-			     int *display_wm, int *cursor_wm)
+			     unsigned int *display_wm, unsigned int *cursor_wm)
 {
 	struct drm_crtc *crtc;
 	const struct drm_display_mode *adjusted_mode;
 	int hdisplay, htotal, cpp, clock;
-	unsigned long line_time_us;
-	int line_count, line_size;
-	int small, large;
-	int entries;
+	unsigned int line_time_us, line_count, line_size;
+	unsigned int small, large, entries;
 
 	if (!latency_ns) {
 		*display_wm = *cursor_wm = 0;
@@ -836,9 +834,7 @@ static bool g4x_compute_srwm(struct drm_device *dev,
 	entries = DIV_ROUND_UP(entries, cursor->cacheline_size);
 	*cursor_wm = entries + cursor->guard_size;
 
-	return g4x_check_srwm(dev,
-			      *display_wm, *cursor_wm,
-			      display, cursor);
+	return g4x_check_srwm(dev, *display_wm, *cursor_wm, display, cursor);
 }
 
 #define FW_WM_VLV(value, plane) \
@@ -1386,8 +1382,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	static const unsigned int sr_latency_ns = 12000;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
-	int plane_sr, cursor_sr;
+	unsigned int planea_wm, planeb_wm;
+	unsigned int cursora_wm, cursorb_wm;
+	unsigned int plane_sr, cursor_sr;
 	unsigned int enabled = 0;
 	bool cxsr_enabled;
 
@@ -1416,8 +1413,8 @@ static void g4x_update_wm(struct drm_crtc *crtc)
 		plane_sr = cursor_sr = 0;
 	}
 
-	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
-		      "B: plane=%d, cursor=%d, SR: plane=%d, cursor=%d\n",
+	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%u, cursor=%u, "
+		      "B: plane=%u, cursor=%u, SR: plane=%u, cursor=%u\n",
 		      planea_wm, cursora_wm,
 		      planeb_wm, cursorb_wm,
 		      plane_sr, cursor_sr);
@@ -1458,8 +1455,7 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
 		int htotal = adjusted_mode->crtc_htotal;
 		int hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
 		int cpp = drm_format_plane_cpp(crtc->primary->state->fb->pixel_format, 0);
-		unsigned long line_time_us;
-		int entries;
+		unsigned int line_time_us, entries;
 
 		line_time_us = max(htotal * 1000 / clock, 1);
 
@@ -1518,9 +1514,10 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	struct drm_device *dev = unused_crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	const struct intel_watermark_params *wm_info;
-	uint32_t fwater_lo;
-	uint32_t fwater_hi;
-	int cwm, srwm = 1;
+	u32 fwater_lo;
+	u32 fwater_hi;
+	unsigned int cwm;
+	int srwm = 1;
 	unsigned int fifo_size, planea_wm, planeb_wm;
 	struct drm_crtc *crtc, *enabled = NULL;
 
@@ -1604,8 +1601,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 		int htotal = adjusted_mode->crtc_htotal;
 		int hdisplay = to_intel_crtc(enabled)->config->pipe_src_w;
 		int cpp = drm_format_plane_cpp(enabled->primary->state->fb->pixel_format, 0);
-		unsigned long line_time_us;
-		int entries;
+		unsigned int line_time_us, entries;
 
 		if (IS_I915GM(dev) || IS_I945GM(dev))
 			cpp = 4;
@@ -1616,7 +1612,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 		entries = (((sr_latency_ns / line_time_us) + 1000) / 1000) *
 			cpp * hdisplay;
 		entries = DIV_ROUND_UP(entries, wm_info->cacheline_size);
-		DRM_DEBUG_KMS("self-refresh entries: %d\n", entries);
+		DRM_DEBUG_KMS("self-refresh entries: %u\n", entries);
 		srwm = wm_info->fifo_size - entries;
 		if (srwm < 0)
 			srwm = 1;
@@ -1628,7 +1624,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 			I915_WRITE(FW_BLC_SELF, srwm & 0x3f);
 	}
 
-	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d, B: %d, C: %d, SR %d\n",
+	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %u, B: %u, C: %u, SR %d\n",
 		      planea_wm, planeb_wm, cwm, srwm);
 
 	fwater_lo = ((planeb_wm & 0x3f) << 16) | (planea_wm & 0x3f);
-- 
2.7.4

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

Re: [PATCH 9/9] drm/i915: Tidy watermark computation local types

[Ville Syrjälä]

On Fri, Oct 07, 2016 at 02:34:12PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Convert to from signed to unsigned and from longs

Ddi you check that we can't get negative values? Depending on how you do
things that can be possible on gmch platforms on account of the
watermarks being inverted. IIRC we might have places were negative
values might be present in temporary results.

> to integers where appropriate.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 60 +++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 308edc4378fa..1203a7a80e9e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -711,14 +711,14 @@ static bool g4x_compute_wm0(struct drm_device *dev,
>  			    unsigned int display_latency_ns,
>  			    const struct intel_watermark_params *cursor,
>  			    unsigned int cursor_latency_ns,
> -			    int *plane_wm,
> -			    int *cursor_wm)
> +			    unsigned int *plane_wm,
> +			    unsigned int *cursor_wm)
>  {
>  	struct drm_crtc *crtc;
>  	const struct drm_display_mode *adjusted_mode;
>  	int htotal, hdisplay, clock, cpp;
> -	int line_time_us, line_count;
> -	int entries, tlb_miss;
> +	unsigned int line_time_us, line_count, entries;
> +	int tlb_miss;
>  
>  	crtc = intel_get_crtc_for_plane(dev, plane);
>  	if (!intel_crtc_active(crtc)) {
> @@ -740,7 +740,7 @@ static bool g4x_compute_wm0(struct drm_device *dev,
>  		entries += tlb_miss;
>  	entries = DIV_ROUND_UP(entries, display->cacheline_size);
>  	*plane_wm = entries + display->guard_size;
> -	if (*plane_wm > (int)display->max_wm)
> +	if (*plane_wm > display->max_wm)
>  		*plane_wm = display->max_wm;
>  
>  	/* Use the large buffer method to calculate cursor watermark */
> @@ -752,8 +752,8 @@ static bool g4x_compute_wm0(struct drm_device *dev,
>  		entries += tlb_miss;
>  	entries = DIV_ROUND_UP(entries, cursor->cacheline_size);
>  	*cursor_wm = entries + cursor->guard_size;
> -	if (*cursor_wm > (int)cursor->max_wm)
> -		*cursor_wm = (int)cursor->max_wm;
> +	if (*cursor_wm > cursor->max_wm)
> +		*cursor_wm = cursor->max_wm;
>  
>  	return true;
>  }
> @@ -766,21 +766,21 @@ static bool g4x_compute_wm0(struct drm_device *dev,
>   * must be disabled.
>   */
>  static bool g4x_check_srwm(struct drm_device *dev,
> -			   int display_wm, int cursor_wm,
> +			   unsigned int display_wm, unsigned int cursor_wm,
>  			   const struct intel_watermark_params *display,
>  			   const struct intel_watermark_params *cursor)
>  {
> -	DRM_DEBUG_KMS("SR watermark: display plane %d, cursor %d\n",
> +	DRM_DEBUG_KMS("SR watermark: display plane %u, cursor %u\n",
>  		      display_wm, cursor_wm);
>  
>  	if (display_wm > display->max_wm) {
> -		DRM_DEBUG_KMS("display watermark is too large(%d/%u), disabling\n",
> +		DRM_DEBUG_KMS("display watermark is too large(%u/%u), disabling\n",
>  			      display_wm, display->max_wm);
>  		return false;
>  	}
>  
>  	if (cursor_wm > cursor->max_wm) {
> -		DRM_DEBUG_KMS("cursor watermark is too large(%d/%u), disabling\n",
> +		DRM_DEBUG_KMS("cursor watermark is too large(%u/%u), disabling\n",
>  			      cursor_wm, cursor->max_wm);
>  		return false;
>  	}
> @@ -798,15 +798,13 @@ static bool g4x_compute_srwm(struct drm_device *dev,
>  			     unsigned int latency_ns,
>  			     const struct intel_watermark_params *display,
>  			     const struct intel_watermark_params *cursor,
> -			     int *display_wm, int *cursor_wm)
> +			     unsigned int *display_wm, unsigned int *cursor_wm)
>  {
>  	struct drm_crtc *crtc;
>  	const struct drm_display_mode *adjusted_mode;
>  	int hdisplay, htotal, cpp, clock;
> -	unsigned long line_time_us;
> -	int line_count, line_size;
> -	int small, large;
> -	int entries;
> +	unsigned int line_time_us, line_count, line_size;
> +	unsigned int small, large, entries;
>  
>  	if (!latency_ns) {
>  		*display_wm = *cursor_wm = 0;
> @@ -836,9 +834,7 @@ static bool g4x_compute_srwm(struct drm_device *dev,
>  	entries = DIV_ROUND_UP(entries, cursor->cacheline_size);
>  	*cursor_wm = entries + cursor->guard_size;
>  
> -	return g4x_check_srwm(dev,
> -			      *display_wm, *cursor_wm,
> -			      display, cursor);
> +	return g4x_check_srwm(dev, *display_wm, *cursor_wm, display, cursor);
>  }
>  
>  #define FW_WM_VLV(value, plane) \
> @@ -1386,8 +1382,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
>  	struct drm_device *dev = crtc->dev;
>  	static const unsigned int sr_latency_ns = 12000;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
> -	int plane_sr, cursor_sr;
> +	unsigned int planea_wm, planeb_wm;
> +	unsigned int cursora_wm, cursorb_wm;
> +	unsigned int plane_sr, cursor_sr;
>  	unsigned int enabled = 0;
>  	bool cxsr_enabled;
>  
> @@ -1416,8 +1413,8 @@ static void g4x_update_wm(struct drm_crtc *crtc)
>  		plane_sr = cursor_sr = 0;
>  	}
>  
> -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> -		      "B: plane=%d, cursor=%d, SR: plane=%d, cursor=%d\n",
> +	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%u, cursor=%u, "
> +		      "B: plane=%u, cursor=%u, SR: plane=%u, cursor=%u\n",
>  		      planea_wm, cursora_wm,
>  		      planeb_wm, cursorb_wm,
>  		      plane_sr, cursor_sr);
> @@ -1458,8 +1455,7 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
>  		int htotal = adjusted_mode->crtc_htotal;
>  		int hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
>  		int cpp = drm_format_plane_cpp(crtc->primary->state->fb->pixel_format, 0);
> -		unsigned long line_time_us;
> -		int entries;
> +		unsigned int line_time_us, entries;
>  
>  		line_time_us = max(htotal * 1000 / clock, 1);
>  
> @@ -1518,9 +1514,10 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>  	struct drm_device *dev = unused_crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	const struct intel_watermark_params *wm_info;
> -	uint32_t fwater_lo;
> -	uint32_t fwater_hi;
> -	int cwm, srwm = 1;
> +	u32 fwater_lo;
> +	u32 fwater_hi;
> +	unsigned int cwm;
> +	int srwm = 1;
>  	unsigned int fifo_size, planea_wm, planeb_wm;
>  	struct drm_crtc *crtc, *enabled = NULL;
>  
> @@ -1604,8 +1601,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>  		int htotal = adjusted_mode->crtc_htotal;
>  		int hdisplay = to_intel_crtc(enabled)->config->pipe_src_w;
>  		int cpp = drm_format_plane_cpp(enabled->primary->state->fb->pixel_format, 0);
> -		unsigned long line_time_us;
> -		int entries;
> +		unsigned int line_time_us, entries;
>  
>  		if (IS_I915GM(dev) || IS_I945GM(dev))
>  			cpp = 4;
> @@ -1616,7 +1612,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>  		entries = (((sr_latency_ns / line_time_us) + 1000) / 1000) *
>  			cpp * hdisplay;
>  		entries = DIV_ROUND_UP(entries, wm_info->cacheline_size);
> -		DRM_DEBUG_KMS("self-refresh entries: %d\n", entries);
> +		DRM_DEBUG_KMS("self-refresh entries: %u\n", entries);
>  		srwm = wm_info->fifo_size - entries;
>  		if (srwm < 0)
>  			srwm = 1;
> @@ -1628,7 +1624,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>  			I915_WRITE(FW_BLC_SELF, srwm & 0x3f);
>  	}
>  
> -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d, B: %d, C: %d, SR %d\n",
> +	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %u, B: %u, C: %u, SR %d\n",
>  		      planea_wm, planeb_wm, cwm, srwm);
>  
>  	fwater_lo = ((planeb_wm & 0x3f) << 16) | (planea_wm & 0x3f);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915: Tidy watermark computation local types

[Tvrtko Ursulin]

On 07/10/2016 14:48, Ville Syrjälä wrote:
> On Fri, Oct 07, 2016 at 02:34:12PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Convert to from signed to unsigned and from longs
> Ddi you check that we can't get negative values? Depending on how you do
> things that can be possible on gmch platforms on account of the
> watermarks being inverted. IIRC we might have places were negative
> values might be present in temporary results.

I tried to be careful, but it is of course possible I've missed 
something. Could you give me a more precise pointer on where exactly to 
look? In this particular patch?

Regards,

Tvrtko

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

Re: [PATCH 9/9] drm/i915: Tidy watermark computation local types

[Ville Syrjälä]

On Fri, Oct 07, 2016 at 03:51:50PM +0100, Tvrtko Ursulin wrote:
> 
> On 07/10/2016 14:48, Ville Syrjälä wrote:
> > On Fri, Oct 07, 2016 at 02:34:12PM +0100, Tvrtko Ursulin wrote:
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> Convert to from signed to unsigned and from longs
> > Ddi you check that we can't get negative values? Depending on how you do
> > things that can be possible on gmch platforms on account of the
> > watermarks being inverted. IIRC we might have places were negative
> > values might be present in temporary results.
> 
> I tried to be careful, but it is of course possible I've missed 
> something. Could you give me a more precise pointer on where exactly to 
> look? In this particular patch?

I don't have specifics in mind right now. But in general I've become a
bit wary of unsigned in my older days on account of integer promotions
and arithmetic conversions. It's far too easy to end up with an unsigned
value where signed was needed.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: Shrink TV modes const data

[Jani Nikula]

On Fri, 07 Oct 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Make struct video_levels and struct tv_mode use data types
> of sufficient width to save approximately one kilobyte in
> the .rodata section.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_tv.c | 50 ++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 3988c45f9e5f..fd4d59341897 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -86,7 +86,8 @@ struct intel_tv {
>  };
>  
>  struct video_levels {
> -	int blank, black, burst;
> +	u16 blank, black;
> +	u8  burst;
>  };
>  
>  struct color_conversion {
> @@ -339,34 +340,43 @@ static const struct video_levels component_levels = {
>  
>  struct tv_mode {
>  	const char *name;
> -	int clock;
> -	int refresh; /* in millihertz (for precision) */
> -	u32 oversample;
> -	int hsync_end, hblank_start, hblank_end, htotal;
> -	bool progressive, trilevel_sync, component_only;
> -	int vsync_start_f1, vsync_start_f2, vsync_len;
> -	bool veq_ena;
> -	int veq_start_f1, veq_start_f2, veq_len;
> -	int vi_end_f1, vi_end_f2, nbr_end;
> -	bool burst_ena;
> -	int hburst_start, hburst_len;
> -	int vburst_start_f1, vburst_end_f1;
> -	int vburst_start_f2, vburst_end_f2;
> -	int vburst_start_f3, vburst_end_f3;
> -	int vburst_start_f4, vburst_end_f4;
> +
> +	u32  clock;
> +	u16  refresh; /* in millihertz (for precision) */
> +	u32  oversample;
> +	u8   hsync_end;
> +	u16  hblank_start, hblank_end, htotal;
> +	bool progressive : 1, trilevel_sync : 1, component_only : 1;
> +	u8   vsync_start_f1, vsync_start_f2, vsync_len;
> +	bool veq_ena : 1;
> +	u8   veq_start_f1, veq_start_f2, veq_len;
> +	u8   vi_end_f1, vi_end_f2;
> +	u16  nbr_end;
> +	bool burst_ena : 1;
> +	u8   hburst_start, hburst_len;
> +	u8   vburst_start_f1;
> +	u16  vburst_end_f1;
> +	u8   vburst_start_f2;
> +	u16  vburst_end_f2;
> +	u8   vburst_start_f3;
> +	u16  vburst_end_f3;
> +	u8   vburst_start_f4;
> +	u16  vburst_end_f4;

Not convinced about the indentation change.

BR,
Jani.

>  	/*
>  	 * subcarrier programming
>  	 */
> -	int dda2_size, dda3_size, dda1_inc, dda2_inc, dda3_inc;
> -	u32 sc_reset;
> -	bool pal_burst;
> +	u16  dda2_size, dda3_size;
> +	u8   dda1_inc;
> +	u16  dda2_inc, dda3_inc;
> +	u32  sc_reset;
> +	bool pal_burst : 1;
>  	/*
>  	 * blank/black levels
>  	 */
>  	const struct video_levels *composite_levels, *svideo_levels;
>  	const struct color_conversion *composite_color, *svideo_color;
>  	const u32 *filter_table;
> -	int max_srcw;
> +	u16   max_srcw;
>  };

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/9] drm/i915: Shrink cxsr_latency_table

[Joonas Lahtinen]

On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
> @@ -807,14 +807,14 @@ struct intel_watermark_params {
> 
>  };
>  
>  struct cxsr_latency {
> -	int is_desktop;
> -	int is_ddr3;
> -	unsigned long fsb_freq;
> -	unsigned long mem_freq;
> -	unsigned long display_sr;
> -	unsigned long display_hpll_disable;
> -	unsigned long cursor_sr;
> -	unsigned long cursor_hpll_disable;
> +	bool is_desktop : 1;
> +	bool is_ddr3 : 1;

bitfields to the end of struct, with that;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915: Shrink sdvo_cmd_names

[Joonas Lahtinen]

On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Pack the struct _sdvo_cmd_name to save 736 bytes of .rodata.
> 
> This is fine since the name pointers are used only for debug.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Shrink per-platform watermark configuration

[Joonas Lahtinen]

On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Use types of more appropriate size in struct
> intel_watermark_params to save 512 bytes of .rodata.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

No changelog, so assuming equal to trybot one;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: Shrink TV modes const data

[Joonas Lahtinen]

On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Make struct video_levels and struct tv_mode use data types
> of sufficient width to save approximately one kilobyte in
> the .rodata section.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Would it hurt us to make the struct packed?

I can see why not to reorder the struct (although, it uses designated
initializers, so it should not matter in that sense).

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915: Use unsigned int for latencies

[Joonas Lahtinen]

On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

And maybe Suggested-by? :)

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915: unsigned int is enough for crtc clock

[Joonas Lahtinen]

On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/i915: Convert get_fifo_size return from int to unsigned int

[Joonas Lahtinen]

On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Unsigned is a more appropriate data type in this case.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915: Make intel_calculate_wm return unsigned int

[Joonas Lahtinen]

On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
> @@ -595,18 +595,17 @@ static unsigned long intel_calculate_wm(unsigned int clock_in_khz,
>  	 * clocks go from a few thousand to several hundred thousand.
>  	 * latency is usually a few thousand
>  	 */
> -	entries_required = ((clock_in_khz / 1000) * cpp * latency_ns) /
> -		1000;
> +	entries_required = ((clock_in_khz / 1000) * cpp * latency_ns) / 1000;

Is the intermediary result always within int?

>  	entries_required = DIV_ROUND_UP(entries_required, wm->cacheline_size);
>  
> -	DRM_DEBUG_KMS("FIFO entries required for mode: %ld\n", entries_required);
> +	DRM_DEBUG_KMS("FIFO entries required for mode: %d\n", entries_required);
>  
>  	wm_size = fifo_size - (entries_required + wm->guard_size);
>  
> -	DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
> +	DRM_DEBUG_KMS("FIFO watermark level: %d\n", wm_size);
>  
>  	/* Don't promote wm_size to unsigned... */
> -	if (wm_size > (long)wm->max_wm)
> +	if (wm_size > (int)wm->max_wm)
>  		wm_size = wm->max_wm;
>  	if (wm_size <= 0)
>  		wm_size = wm->default_wm;

This could be

	if (wm_size <= 0)
		wm_size = wm->default_wm;
	else if (wm_size > U16_MAX || (u16)wm_size > wm->max_wm)
		wm_size = wm->max_wm;

or something?

Other than that, types look better that they used to.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

The whole type landscape in watermark code seems bit sloppy to me.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915: Tidy watermark computation local types

[Joonas Lahtinen]

On pe, 2016-10-07 at 18:16 +0300, Ville Syrjälä wrote:
> On Fri, Oct 07, 2016 at 03:51:50PM +0100, Tvrtko Ursulin wrote:
> >
> > I tried to be careful, but it is of course possible I've missed 
> > something. Could you give me a more precise pointer on where exactly to 
> > look? In this particular patch?
> 
> I don't have specifics in mind right now. But in general I've become a
> bit wary of unsigned in my older days on account of integer promotions
> and arithmetic conversions. It's far too easy to end up with an unsigned
> value where signed was needed.

Yes, if one is not paying attention to the maximum expected values. And
looking back a few months, paying attention is what our watermark code
needs badly. By quick look, some types are random and there're quite a
few casts. Try "git grep max_wm" for example.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for .rodata diet 2 (non-disruptive version)

[Patchwork]

== Series Details ==

Series: .rodata diet 2 (non-disruptive version)
URL   : https://patchwork.freedesktop.org/series/13445/
State : warning

== Summary ==

Series 13445v1 .rodata diet 2 (non-disruptive version)
https://patchwork.freedesktop.org/api/1.0/series/13445/revisions/1/mbox/


Results at /archive/results/CI_IGT_test/Patchwork_2648/

0f2a32f6bdc7590370d79a8ec5be27a8762f1d91 drm-intel-nightly: 2016y-10m-10d-05h-29m-53s UTC integration manifest
cdb5478 drm/i915: Tidy watermark computation local types
4e9225d drm/i915: Make intel_calculate_wm return unsigned int
08ef44d drm/i915: Convert get_fifo_size return from int to unsigned int
359176c drm/i915: unsigned int is enough for crtc clock
2442f60 drm/i915: Use unsigned int for latencies
cbb8eab drm/i915: Shrink TV modes const data
40148c9 drm/i915: Shrink per-platform watermark configuration
b0fbaa3 drm/i915: Shrink sdvo_cmd_names
34dbd0b drm/i915: Shrink cxsr_latency_table

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

✗ Fi.CI.BAT: failure for .rodata diet 2 (non-disruptive version)

[Patchwork]

== Series Details ==

Series: .rodata diet 2 (non-disruptive version)
URL   : https://patchwork.freedesktop.org/series/13445/
State : failure

== Summary ==

Series 13445v1 .rodata diet 2 (non-disruptive version)
https://patchwork.freedesktop.org/api/1.0/series/13445/revisions/1/mbox/

Test gem_busy:
        Subgroup basic-hang-default:
                pass       -> FAIL       (fi-hsw-4770r)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-byt-j1900)
Test vgem_basic:
        Subgroup unload:
                pass       -> SKIP       (fi-ilk-650)
                skip       -> PASS       (fi-ivb-3770)
                skip       -> PASS       (fi-bsw-n3050)
                pass       -> SKIP       (fi-hsw-4770)
                skip       -> PASS       (fi-skl-6700k)
                pass       -> SKIP       (fi-ivb-3520m)

fi-bdw-5557u     total:248  pass:231  dwarn:0   dfail:0   fail:0   skip:17 
fi-bsw-n3050     total:248  pass:205  dwarn:0   dfail:0   fail:0   skip:43 
fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-j1900     total:248  pass:213  dwarn:2   dfail:0   fail:1   skip:32 
fi-byt-n2820     total:248  pass:211  dwarn:0   dfail:0   fail:1   skip:36 
fi-hsw-4770      total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-hsw-4770r     total:248  pass:223  dwarn:0   dfail:0   fail:1   skip:24 
fi-ilk-650       total:248  pass:184  dwarn:0   dfail:0   fail:2   skip:62 
fi-ivb-3520m     total:248  pass:221  dwarn:0   dfail:0   fail:0   skip:27 
fi-ivb-3770      total:248  pass:208  dwarn:0   dfail:0   fail:0   skip:40 
fi-kbl-7200u     total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-6260u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-skl-6700hq    total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6700k     total:248  pass:222  dwarn:1   dfail:0   fail:0   skip:25 
fi-skl-6770hq    total:248  pass:230  dwarn:1   dfail:0   fail:1   skip:16 
fi-snb-2520m     total:248  pass:211  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:248  pass:209  dwarn:0   dfail:0   fail:0   skip:39 

Results at /archive/results/CI_IGT_test/Patchwork_2648/

0f2a32f6bdc7590370d79a8ec5be27a8762f1d91 drm-intel-nightly: 2016y-10m-10d-05h-29m-53s UTC integration manifest
cdb5478 drm/i915: Tidy watermark computation local types
4e9225d drm/i915: Make intel_calculate_wm return unsigned int
08ef44d drm/i915: Convert get_fifo_size return from int to unsigned int
359176c drm/i915: unsigned int is enough for crtc clock
2442f60 drm/i915: Use unsigned int for latencies
cbb8eab drm/i915: Shrink TV modes const data
40148c9 drm/i915: Shrink per-platform watermark configuration
b0fbaa3 drm/i915: Shrink sdvo_cmd_names
34dbd0b drm/i915: Shrink cxsr_latency_table

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

Re: [PATCH 8/9] drm/i915: Make intel_calculate_wm return unsigned int

[Tvrtko Ursulin]

On 10/10/2016 09:02, Joonas Lahtinen wrote:
> On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
>> @@ -595,18 +595,17 @@ static unsigned long intel_calculate_wm(unsigned int clock_in_khz,
>>   	 * clocks go from a few thousand to several hundred thousand.
>>   	 * latency is usually a few thousand
>>   	 */
>> -	entries_required = ((clock_in_khz / 1000) * cpp * latency_ns) /
>> -		1000;
>> +	entries_required = ((clock_in_khz / 1000) * cpp * latency_ns) / 1000;
> Is the intermediary result always within int?

I certainly hope so! Display clock in MHz * cpp * latency which is u16. 
So 2^31 / 2^16 / cpp=8 leaves 4GHz for the clock before it would overflow.

>>   	entries_required = DIV_ROUND_UP(entries_required, wm->cacheline_size);
>>   
>> -	DRM_DEBUG_KMS("FIFO entries required for mode: %ld\n", entries_required);
>> +	DRM_DEBUG_KMS("FIFO entries required for mode: %d\n", entries_required);
>>   
>>   	wm_size = fifo_size - (entries_required + wm->guard_size);
>>   
>> -	DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
>> +	DRM_DEBUG_KMS("FIFO watermark level: %d\n", wm_size);
>>   
>>   	/* Don't promote wm_size to unsigned... */
>> -	if (wm_size > (long)wm->max_wm)
>> +	if (wm_size > (int)wm->max_wm)
>>   		wm_size = wm->max_wm;
>>   	if (wm_size <= 0)
>>   		wm_size = wm->default_wm;
> This could be
>
> 	if (wm_size <= 0)
> 		wm_size = wm->default_wm;
> 	else if (wm_size > U16_MAX || (u16)wm_size > wm->max_wm)
> 		wm_size = wm->max_wm;
>
> or something?
>
> Other than that, types look better that they used to.
>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> The whole type landscape in watermark code seems bit sloppy to me.

Yes, but I would also like not to introduce regressions.  So I am a bit 
reluctant to push forward with the second half of this series. :I

Regards,

Tvrtko

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

Re: [PATCH 5/9] drm/i915: Use unsigned int for latencies

[Tvrtko Ursulin]

On 10/10/2016 08:24, Joonas Lahtinen wrote:
> On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> And maybe Suggested-by? :)

Definitely, yes, should have put it on all of the related ones. Will 
correct.

Regards,

Tvrtko

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

Re: [PATCH 4/9] drm/i915: Shrink TV modes const data

[Tvrtko Ursulin]

On 10/10/2016 07:49, Jani Nikula wrote:
> On Fri, 07 Oct 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Make struct video_levels and struct tv_mode use data types
>> of sufficient width to save approximately one kilobyte in
>> the .rodata section.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_tv.c | 50 ++++++++++++++++++++++++-----------------
>>   1 file changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>> index 3988c45f9e5f..fd4d59341897 100644
>> --- a/drivers/gpu/drm/i915/intel_tv.c
>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>> @@ -86,7 +86,8 @@ struct intel_tv {
>>   };
>>   
>>   struct video_levels {
>> -	int blank, black, burst;
>> +	u16 blank, black;
>> +	u8  burst;
>>   };
>>   
>>   struct color_conversion {
>> @@ -339,34 +340,43 @@ static const struct video_levels component_levels = {
>>   
>>   struct tv_mode {
>>   	const char *name;
>> -	int clock;
>> -	int refresh; /* in millihertz (for precision) */
>> -	u32 oversample;
>> -	int hsync_end, hblank_start, hblank_end, htotal;
>> -	bool progressive, trilevel_sync, component_only;
>> -	int vsync_start_f1, vsync_start_f2, vsync_len;
>> -	bool veq_ena;
>> -	int veq_start_f1, veq_start_f2, veq_len;
>> -	int vi_end_f1, vi_end_f2, nbr_end;
>> -	bool burst_ena;
>> -	int hburst_start, hburst_len;
>> -	int vburst_start_f1, vburst_end_f1;
>> -	int vburst_start_f2, vburst_end_f2;
>> -	int vburst_start_f3, vburst_end_f3;
>> -	int vburst_start_f4, vburst_end_f4;
>> +
>> +	u32  clock;
>> +	u16  refresh; /* in millihertz (for precision) */
>> +	u32  oversample;
>> +	u8   hsync_end;
>> +	u16  hblank_start, hblank_end, htotal;
>> +	bool progressive : 1, trilevel_sync : 1, component_only : 1;
>> +	u8   vsync_start_f1, vsync_start_f2, vsync_len;
>> +	bool veq_ena : 1;
>> +	u8   veq_start_f1, veq_start_f2, veq_len;
>> +	u8   vi_end_f1, vi_end_f2;
>> +	u16  nbr_end;
>> +	bool burst_ena : 1;
>> +	u8   hburst_start, hburst_len;
>> +	u8   vburst_start_f1;
>> +	u16  vburst_end_f1;
>> +	u8   vburst_start_f2;
>> +	u16  vburst_end_f2;
>> +	u8   vburst_start_f3;
>> +	u16  vburst_end_f3;
>> +	u8   vburst_start_f4;
>> +	u16  vburst_end_f4;
> Not convinced about the indentation change.

I found it much more readable like that in this case. I still do, but 
thinking about it more, maybe it is just because my editor does no 
syntax highlighting for u32 types. I need to fix that.

Regards,

Tvrtko


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