All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/skl: distribute DDB based on panel resolution
@ 2018-07-30 14:12 Mahesh Kumar
  2018-07-30 15:37 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Mahesh Kumar @ 2018-07-30 14:12 UTC (permalink / raw)
  To: intel-gfx

We distribute DDB equally among all pipes irrespective of display
buffer requirement of each pipe. This leads to a situation where high
resolution y-tiled display can not be enabled with 2 low resolution
displays.

Main contributing factor for DDB requirement is width of the display.
This patch make changes to distribute ddb based on display width.
So display with higher width will get bigger chunk of DDB.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107113
Cc: raviraj.p.sitaram@intel.com
Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7312ecb73415..e092f0deb93d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3814,8 +3814,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *for_crtc = cstate->base.crtc;
+	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
+	const struct drm_crtc_state *crtc_state;
+	const struct drm_crtc *crtc;
+	u32 pipe_width[I915_MAX_PIPES] = {0};
+	u32 total_width = 0, width_before_pipe = 0;
 	unsigned int pipe_size, ddb_size;
-	int nth_active_pipe;
+	u16 ddb_size_before_pipe;
+	u32 i;
 
 	if (WARN_ON(!state) || !cstate->base.active) {
 		alloc->start = 0;
@@ -3833,14 +3839,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				      *num_active, ddb);
 
 	/*
-	 * If the state doesn't change the active CRTC's, then there's
-	 * no need to recalculate; the existing pipe allocation limits
-	 * should remain unchanged.  Note that we're safe from racing
-	 * commits since any racing commit that changes the active CRTC
-	 * list would need to grab _all_ crtc locks, including the one
-	 * we currently hold.
+	 * If the state doesn't change the active CRTC's or there is no
+	 * modeset request, then there's no need to recalculate;
+	 * the existing pipe allocation limits should remain unchanged.
+	 * Note that we're safe from racing commits since any racing commit
+	 * that changes the active CRTC list or do modeset would need to
+	 * grab _all_ crtc locks, including the one we currently hold.
 	 */
-	if (!intel_state->active_pipe_changes) {
+	if (!intel_state->active_pipe_changes && !intel_state->modeset) {
 		/*
 		 * alloc may be cleared by clear_intel_crtc_state,
 		 * copy from old state to be sure
@@ -3849,10 +3855,33 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 		return;
 	}
 
-	nth_active_pipe = hweight32(intel_state->active_crtcs &
-				    (drm_crtc_mask(for_crtc) - 1));
-	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
-	alloc->start = nth_active_pipe * ddb_size / *num_active;
+	/*
+	 * Watermark/ddb requirement highly depends upon width of the
+	 * framebuffer, So instead of allocating DDB equally among pipes
+	 * distribute DDB based on resolution/width of the display.
+	 */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		const struct drm_display_mode *adjusted_mode;
+		int hdisplay, vdisplay;
+		enum pipe pipe;
+
+		if (!crtc_state->enable)
+			continue;
+
+		pipe = to_intel_crtc(crtc)->pipe;
+		adjusted_mode = &crtc_state->adjusted_mode;
+		drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
+		pipe_width[pipe] = hdisplay;
+		total_width += pipe_width[pipe];
+
+		if (pipe < for_pipe)
+			width_before_pipe += pipe_width[pipe];
+	}
+
+	ddb_size_before_pipe = div_u64(ddb_size * width_before_pipe,
+				       total_width);
+	pipe_size = div_u64(ddb_size * pipe_width[for_pipe], total_width);
+	alloc->start = ddb_size_before_pipe;
 	alloc->end = alloc->start + pipe_size;
 }
 
@@ -5259,7 +5288,7 @@ skl_ddb_add_affected_pipes(struct drm_atomic_state *state, bool *changed)
 	 * any other display updates race with this transaction, so we need
 	 * to grab the lock on *all* CRTC's.
 	 */
-	if (intel_state->active_pipe_changes) {
+	if (intel_state->active_pipe_changes || intel_state->modeset) {
 		realloc_pipes = ~0;
 		intel_state->wm_results.dirty_pipes = ~0;
 	}
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915/skl: distribute DDB based on panel resolution
  2018-07-30 14:12 [PATCH] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
@ 2018-07-30 15:37 ` Patchwork
  2018-07-30 15:38 ` [PATCH] " Chris Wilson
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-07-30 15:37 UTC (permalink / raw)
  To: Mahesh Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/skl: distribute DDB based on panel resolution
URL   : https://patchwork.freedesktop.org/series/47428/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4595 -> Patchwork_9811 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9811 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9811, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47428/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9811:

  === IGT changes ===

    ==== Warnings ====

    igt@drv_selftest@live_evict:
      fi-cnl-psr:         SKIP -> PASS +9

    
== Known issues ==

  Here are the changes found in Patchwork_9811 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_objects:
      fi-cnl-psr:         SKIP -> DMESG-FAIL (fdo#107398)

    igt@drv_selftest@live_workarounds:
      fi-kbl-7560u:       PASS -> DMESG-FAIL (fdo#107292)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    {igt@kms_psr@primary_mmap_gtt}:
      fi-cnl-psr:         PASS -> DMESG-WARN (fdo#107372)

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@drv_selftest@live_coherency:
      {fi-icl-u}:         DMESG-FAIL -> PASS

    igt@drv_selftest@live_requests:
      {fi-bsw-kefka}:     INCOMPLETE (fdo#105876) -> PASS

    igt@drv_selftest@live_workarounds:
      {fi-cfl-8109u}:     DMESG-FAIL (fdo#107292) -> PASS
      {fi-bsw-kefka}:     DMESG-FAIL (fdo#107292) -> PASS

    igt@kms_chamelium@dp-edid-read:
      fi-kbl-7500u:       FAIL (fdo#103841) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105876 https://bugs.freedesktop.org/show_bug.cgi?id=105876
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
  fdo#107398 https://bugs.freedesktop.org/show_bug.cgi?id=107398


== Participating hosts (52 -> 47) ==

  Additional (1): fi-byt-j1900 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper 


== Build changes ==

    * Linux: CI_DRM_4595 -> Patchwork_9811

  CI_DRM_4595: f133adaa57cf7118381b5ffc081bba3bbb1dbc83 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4581: f1c868dae24056ebc27e4f3c197724ce9b956a8a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9811: df35dc794b9449ef9dbd1692247d05513a95760f @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

df35dc794b94 drm/i915/skl: distribute DDB based on panel resolution

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9811/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915/skl: distribute DDB based on panel resolution
  2018-07-30 14:12 [PATCH] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
  2018-07-30 15:37 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-07-30 15:38 ` Chris Wilson
  2018-07-31  6:16   ` Kumar, Mahesh
  2018-07-30 17:04 ` ✓ Fi.CI.IGT: success for " Patchwork
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2018-07-30 15:38 UTC (permalink / raw)
  To: Mahesh Kumar, intel-gfx

Quoting Mahesh Kumar (2018-07-30 15:12:02)
> We distribute DDB equally among all pipes irrespective of display
> buffer requirement of each pipe. This leads to a situation where high
> resolution y-tiled display can not be enabled with 2 low resolution
> displays.
> 
> Main contributing factor for DDB requirement is width of the display.
> This patch make changes to distribute ddb based on display width.
> So display with higher width will get bigger chunk of DDB.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107113
> Cc: raviraj.p.sitaram@intel.com
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7312ecb73415..e092f0deb93d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3814,8 +3814,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>         struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>         struct drm_i915_private *dev_priv = to_i915(dev);
>         struct drm_crtc *for_crtc = cstate->base.crtc;
> +       enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
> +       const struct drm_crtc_state *crtc_state;
> +       const struct drm_crtc *crtc;
> +       u32 pipe_width[I915_MAX_PIPES] = {0};
> +       u32 total_width = 0, width_before_pipe = 0;
>         unsigned int pipe_size, ddb_size;
> -       int nth_active_pipe;
> +       u16 ddb_size_before_pipe;
> +       u32 i;
>  
>         if (WARN_ON(!state) || !cstate->base.active) {
>                 alloc->start = 0;
> @@ -3833,14 +3839,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>                                       *num_active, ddb);
>  
>         /*
> -        * If the state doesn't change the active CRTC's, then there's
> -        * no need to recalculate; the existing pipe allocation limits
> -        * should remain unchanged.  Note that we're safe from racing
> -        * commits since any racing commit that changes the active CRTC
> -        * list would need to grab _all_ crtc locks, including the one
> -        * we currently hold.
> +        * If the state doesn't change the active CRTC's or there is no
> +        * modeset request, then there's no need to recalculate;
> +        * the existing pipe allocation limits should remain unchanged.
> +        * Note that we're safe from racing commits since any racing commit
> +        * that changes the active CRTC list or do modeset would need to
> +        * grab _all_ crtc locks, including the one we currently hold.
>          */
> -       if (!intel_state->active_pipe_changes) {
> +       if (!intel_state->active_pipe_changes && !intel_state->modeset) {
>                 /*
>                  * alloc may be cleared by clear_intel_crtc_state,
>                  * copy from old state to be sure
> @@ -3849,10 +3855,33 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>                 return;
>         }
>  
> -       nth_active_pipe = hweight32(intel_state->active_crtcs &
> -                                   (drm_crtc_mask(for_crtc) - 1));
> -       pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> -       alloc->start = nth_active_pipe * ddb_size / *num_active;
> +       /*
> +        * Watermark/ddb requirement highly depends upon width of the
> +        * framebuffer, So instead of allocating DDB equally among pipes
> +        * distribute DDB based on resolution/width of the display.
> +        */
> +       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +               const struct drm_display_mode *adjusted_mode;
> +               int hdisplay, vdisplay;
> +               enum pipe pipe;
> +
> +               if (!crtc_state->enable)
> +                       continue;
> +
> +               pipe = to_intel_crtc(crtc)->pipe;
> +               adjusted_mode = &crtc_state->adjusted_mode;
> +               drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
> +               pipe_width[pipe] = hdisplay;
> +               total_width += pipe_width[pipe];
> +
> +               if (pipe < for_pipe)
> +                       width_before_pipe += pipe_width[pipe];
> +       }
> +
> +       ddb_size_before_pipe = div_u64(ddb_size * width_before_pipe,
> +                                      total_width);

ddb_size is unsigned int (u32)
width_before_pipe is u32
ddb_size_before_pipe is u16

That mismash of types is itself perplexing, but u32*u16 is only u32, you
need to cast it to u64 to avoid the overflow: i.e.
	div_u64(mul_u32_u32(ddb_size, width_before_pipe),
	        total_width);

But ddb_size_before_pipe obviously need to be the same type as ddb_size,
and if u16 is good enough, then you do not need a 64b divide!

> +       pipe_size = div_u64(ddb_size * pipe_width[for_pipe], total_width);

So why did you store all pipe_width?

And are not all the previous pipes stored in their respective cstate? So
alloc->start = &previous_cstate->wm.skl.ddb->end;

Looking at the earlier results seems far more robust.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915/skl: distribute DDB based on panel resolution
  2018-07-30 14:12 [PATCH] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
  2018-07-30 15:37 ` ✓ Fi.CI.BAT: success for " Patchwork
  2018-07-30 15:38 ` [PATCH] " Chris Wilson
@ 2018-07-30 17:04 ` Patchwork
  2018-07-31 15:13 ` ✓ Fi.CI.BAT: success for drm/i915/skl: distribute DDB based on panel resolution (rev2) Patchwork
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-07-30 17:04 UTC (permalink / raw)
  To: Mahesh Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/skl: distribute DDB based on panel resolution
URL   : https://patchwork.freedesktop.org/series/47428/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4595_full -> Patchwork_9811_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_9811_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          PASS -> DMESG-FAIL (fdo#106560, fdo#106947)

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
      shard-glk:          PASS -> FAIL (fdo#103375)

    igt@perf@polling:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-kbl:          FAIL (fdo#102887, fdo#105363) -> PASS

    igt@kms_flip@plain-flip-ts-check:
      shard-glk:          FAIL (fdo#100368) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4595 -> Patchwork_9811

  CI_DRM_4595: f133adaa57cf7118381b5ffc081bba3bbb1dbc83 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4581: f1c868dae24056ebc27e4f3c197724ce9b956a8a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9811: df35dc794b9449ef9dbd1692247d05513a95760f @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9811/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915/skl: distribute DDB based on panel resolution
  2018-07-30 15:38 ` [PATCH] " Chris Wilson
@ 2018-07-31  6:16   ` Kumar, Mahesh
  2018-07-31 14:24     ` [PATCH v2 0/2] " Mahesh Kumar
  0 siblings, 1 reply; 17+ messages in thread
From: Kumar, Mahesh @ 2018-07-31  6:16 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

Hi Chris,

Thanks for review.

On 7/30/2018 9:08 PM, Chris Wilson wrote:
> Quoting Mahesh Kumar (2018-07-30 15:12:02)
>> We distribute DDB equally among all pipes irrespective of display
>> buffer requirement of each pipe. This leads to a situation where high
>> resolution y-tiled display can not be enabled with 2 low resolution
>> displays.
>>
>> Main contributing factor for DDB requirement is width of the display.
>> This patch make changes to distribute ddb based on display width.
>> So display with higher width will get bigger chunk of DDB.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107113
>> Cc: raviraj.p.sitaram@intel.com
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++++++++----------
>>   1 file changed, 42 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 7312ecb73415..e092f0deb93d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3814,8 +3814,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>          struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>          struct drm_i915_private *dev_priv = to_i915(dev);
>>          struct drm_crtc *for_crtc = cstate->base.crtc;
>> +       enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
>> +       const struct drm_crtc_state *crtc_state;
>> +       const struct drm_crtc *crtc;
>> +       u32 pipe_width[I915_MAX_PIPES] = {0};
>> +       u32 total_width = 0, width_before_pipe = 0;
>>          unsigned int pipe_size, ddb_size;
>> -       int nth_active_pipe;
>> +       u16 ddb_size_before_pipe;
>> +       u32 i;
>>   
>>          if (WARN_ON(!state) || !cstate->base.active) {
>>                  alloc->start = 0;
>> @@ -3833,14 +3839,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>                                        *num_active, ddb);
>>   
>>          /*
>> -        * If the state doesn't change the active CRTC's, then there's
>> -        * no need to recalculate; the existing pipe allocation limits
>> -        * should remain unchanged.  Note that we're safe from racing
>> -        * commits since any racing commit that changes the active CRTC
>> -        * list would need to grab _all_ crtc locks, including the one
>> -        * we currently hold.
>> +        * If the state doesn't change the active CRTC's or there is no
>> +        * modeset request, then there's no need to recalculate;
>> +        * the existing pipe allocation limits should remain unchanged.
>> +        * Note that we're safe from racing commits since any racing commit
>> +        * that changes the active CRTC list or do modeset would need to
>> +        * grab _all_ crtc locks, including the one we currently hold.
>>           */
>> -       if (!intel_state->active_pipe_changes) {
>> +       if (!intel_state->active_pipe_changes && !intel_state->modeset) {
>>                  /*
>>                   * alloc may be cleared by clear_intel_crtc_state,
>>                   * copy from old state to be sure
>> @@ -3849,10 +3855,33 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>                  return;
>>          }
>>   
>> -       nth_active_pipe = hweight32(intel_state->active_crtcs &
>> -                                   (drm_crtc_mask(for_crtc) - 1));
>> -       pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
>> -       alloc->start = nth_active_pipe * ddb_size / *num_active;
>> +       /*
>> +        * Watermark/ddb requirement highly depends upon width of the
>> +        * framebuffer, So instead of allocating DDB equally among pipes
>> +        * distribute DDB based on resolution/width of the display.
>> +        */
>> +       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> +               const struct drm_display_mode *adjusted_mode;
>> +               int hdisplay, vdisplay;
>> +               enum pipe pipe;
>> +
>> +               if (!crtc_state->enable)
>> +                       continue;
>> +
>> +               pipe = to_intel_crtc(crtc)->pipe;
>> +               adjusted_mode = &crtc_state->adjusted_mode;
>> +               drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
>> +               pipe_width[pipe] = hdisplay;
>> +               total_width += pipe_width[pipe];
>> +
>> +               if (pipe < for_pipe)
>> +                       width_before_pipe += pipe_width[pipe];
>> +       }
>> +
>> +       ddb_size_before_pipe = div_u64(ddb_size * width_before_pipe,
>> +                                      total_width);
> ddb_size is unsigned int (u32)
> width_before_pipe is u32
> ddb_size_before_pipe is u16
>
> That mismash of types is itself perplexing, but u32*u16 is only u32, you
> need to cast it to u64 to avoid the overflow: i.e.
> 	div_u64(mul_u32_u32(ddb_size, width_before_pipe),
> 	        total_width);
>
> But ddb_size_before_pipe obviously need to be the same type as ddb_size,
> and if u16 is good enough, then you do not need a 64b divide!
hmm, ddb_size will not go beyond u16 as we have only 1024 blocks and in 
future also I'm not expecting it to overflow u16.
I don't wanted to change already used data-types , anyway will clean 
this part :)
>
>> +       pipe_size = div_u64(ddb_size * pipe_width[for_pipe], total_width);
> So why did you store all pipe_width?
hmm agree, we don't really need to store the width for each pipe.
>
> And are not all the previous pipes stored in their respective cstate? So
> alloc->start = &previous_cstate->wm.skl.ddb->end;
Actually this function may not get called in fixed sequence each time 
for each CRTC, It will be called first for the crtc which got added 
first in state.
I don't want to shuffle the DDB allocation during each modeset as in 
most of the cases we will be only changing refresh-rate of the panel not 
the resolution (dynamic media refresh rate switching kind of scenarios)

thanks,
-Mahesh
>
> Looking at the earlier results seems far more robust.
> -Chris

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 0/2] distribute DDB based on panel resolution
  2018-07-31  6:16   ` Kumar, Mahesh
@ 2018-07-31 14:24     ` Mahesh Kumar
  2018-07-31 14:24       ` [PATCH v1 1/2] drm/i915: ddb_size is of u16 type Mahesh Kumar
  2018-07-31 14:24       ` [PATCH v2 2/2] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
  0 siblings, 2 replies; 17+ messages in thread
From: Mahesh Kumar @ 2018-07-31 14:24 UTC (permalink / raw)
  To: intel-gfx

This series make changes to distribute DDB based on resolution of panel
instead of dividing equally among pipes.

Mahesh Kumar (2):
  drm/i915: ddb_size is of u16 type
  drm/i915/skl: distribute DDB based on panel resolution

 drivers/gpu/drm/i915/intel_pm.c | 66 +++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 19 deletions(-)

-- 
2.16.2

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v1 1/2] drm/i915: ddb_size is of u16 type
  2018-07-31 14:24     ` [PATCH v2 0/2] " Mahesh Kumar
@ 2018-07-31 14:24       ` Mahesh Kumar
  2018-07-31 21:54         ` Chris Wilson
  2018-07-31 14:24       ` [PATCH v2 2/2] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
  1 sibling, 1 reply; 17+ messages in thread
From: Mahesh Kumar @ 2018-07-31 14:24 UTC (permalink / raw)
  To: intel-gfx

ddb_size is u16 so use same return type for intel_get_ddb_size
wrapper.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7312ecb73415..91120560a61b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3771,11 +3771,11 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 	return true;
 }
 
-static unsigned int intel_get_ddb_size(struct drm_i915_private *dev_priv,
-				       const struct intel_crtc_state *cstate,
-				       const unsigned int total_data_rate,
-				       const int num_active,
-				       struct skl_ddb_allocation *ddb)
+static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
+			      const struct intel_crtc_state *cstate,
+			      const unsigned int total_data_rate,
+			      const int num_active,
+			      struct skl_ddb_allocation *ddb)
 {
 	const struct drm_display_mode *adjusted_mode;
 	u64 total_data_bw;
@@ -3814,7 +3814,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *for_crtc = cstate->base.crtc;
-	unsigned int pipe_size, ddb_size;
+	u16 pipe_size, ddb_size;
 	int nth_active_pipe;
 
 	if (WARN_ON(!state) || !cstate->base.active) {
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/2] drm/i915/skl: distribute DDB based on panel resolution
  2018-07-31 14:24     ` [PATCH v2 0/2] " Mahesh Kumar
  2018-07-31 14:24       ` [PATCH v1 1/2] drm/i915: ddb_size is of u16 type Mahesh Kumar
@ 2018-07-31 14:24       ` Mahesh Kumar
  2018-07-31 21:53         ` Chris Wilson
  1 sibling, 1 reply; 17+ messages in thread
From: Mahesh Kumar @ 2018-07-31 14:24 UTC (permalink / raw)
  To: intel-gfx

We distribute DDB equally among all pipes irrespective of display
buffer requirement of each pipe. This leads to a situation where high
resolution y-tiled display can not be enabled with 2 low resolution
displays.

Main contributing factor for DDB requirement is width of the display.
This patch make changes to distribute ddb based on display width.
So display with higher width will get bigger chunk of DDB.

Changes Since V1:
 - pipe_size/ddb_size will not overflow u16 so use appropriate
   data-types during computation (Chris)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107113
Cc: raviraj.p.sitaram@intel.com
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 54 +++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 91120560a61b..3a0b5a60f73c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3814,8 +3814,13 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *for_crtc = cstate->base.crtc;
+	const struct drm_crtc_state *crtc_state;
+	const struct drm_crtc *crtc;
+	u32 pipe_width = 0, total_width = 0, width_before_pipe = 0;
+	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
 	u16 pipe_size, ddb_size;
-	int nth_active_pipe;
+	u16 ddb_size_before_pipe;
+	u32 i;
 
 	if (WARN_ON(!state) || !cstate->base.active) {
 		alloc->start = 0;
@@ -3833,14 +3838,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				      *num_active, ddb);
 
 	/*
-	 * If the state doesn't change the active CRTC's, then there's
-	 * no need to recalculate; the existing pipe allocation limits
-	 * should remain unchanged.  Note that we're safe from racing
-	 * commits since any racing commit that changes the active CRTC
-	 * list would need to grab _all_ crtc locks, including the one
-	 * we currently hold.
+	 * If the state doesn't change the active CRTC's or there is no
+	 * modeset request, then there's no need to recalculate;
+	 * the existing pipe allocation limits should remain unchanged.
+	 * Note that we're safe from racing commits since any racing commit
+	 * that changes the active CRTC list or do modeset would need to
+	 * grab _all_ crtc locks, including the one we currently hold.
 	 */
-	if (!intel_state->active_pipe_changes) {
+	if (!intel_state->active_pipe_changes && !intel_state->modeset) {
 		/*
 		 * alloc may be cleared by clear_intel_crtc_state,
 		 * copy from old state to be sure
@@ -3849,10 +3854,33 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 		return;
 	}
 
-	nth_active_pipe = hweight32(intel_state->active_crtcs &
-				    (drm_crtc_mask(for_crtc) - 1));
-	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
-	alloc->start = nth_active_pipe * ddb_size / *num_active;
+	/*
+	 * Watermark/ddb requirement highly depends upon width of the
+	 * framebuffer, So instead of allocating DDB equally among pipes
+	 * distribute DDB based on resolution/width of the display.
+	 */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		const struct drm_display_mode *adjusted_mode;
+		int hdisplay, vdisplay;
+		enum pipe pipe;
+
+		if (!crtc_state->enable)
+			continue;
+
+		pipe = to_intel_crtc(crtc)->pipe;
+		adjusted_mode = &crtc_state->adjusted_mode;
+		drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
+		total_width += hdisplay;
+
+		if (pipe < for_pipe)
+			width_before_pipe += hdisplay;
+		else if (pipe == for_pipe)
+			pipe_width = hdisplay;
+	}
+
+	ddb_size_before_pipe = (ddb_size * width_before_pipe) / total_width;
+	pipe_size = (ddb_size * pipe_width) / total_width;
+	alloc->start = ddb_size_before_pipe;
 	alloc->end = alloc->start + pipe_size;
 }
 
@@ -5259,7 +5287,7 @@ skl_ddb_add_affected_pipes(struct drm_atomic_state *state, bool *changed)
 	 * any other display updates race with this transaction, so we need
 	 * to grab the lock on *all* CRTC's.
 	 */
-	if (intel_state->active_pipe_changes) {
+	if (intel_state->active_pipe_changes || intel_state->modeset) {
 		realloc_pipes = ~0;
 		intel_state->wm_results.dirty_pipes = ~0;
 	}
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915/skl: distribute DDB based on panel resolution (rev2)
  2018-07-30 14:12 [PATCH] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
                   ` (2 preceding siblings ...)
  2018-07-30 17:04 ` ✓ Fi.CI.IGT: success for " Patchwork
@ 2018-07-31 15:13 ` Patchwork
  2018-07-31 16:51 ` ✓ Fi.CI.IGT: " Patchwork
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-07-31 15:13 UTC (permalink / raw)
  To: Mahesh Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/skl: distribute DDB based on panel resolution (rev2)
URL   : https://patchwork.freedesktop.org/series/47428/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4598 -> Patchwork_9819 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47428/revisions/2/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9819 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7567u:       PASS -> DMESG-FAIL (fdo#106560, fdo#106947)

    igt@drv_selftest@live_workarounds:
      fi-cnl-psr:         PASS -> DMESG-FAIL (fdo#107292)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-skl-6260u:       INCOMPLETE (fdo#104108) -> PASS

    {igt@kms_psr@primary_mmap_gtt}:
      fi-cnl-psr:         DMESG-WARN (fdo#107372) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372


== Participating hosts (51 -> 46) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-byt-clapper fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4598 -> Patchwork_9819

  CI_DRM_4598: dc620be7ec801cea836a872f8a63469909562312 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4581: f1c868dae24056ebc27e4f3c197724ce9b956a8a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9819: 74e1f9e3424ca18587c45314cfe562dc76db33b4 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

74e1f9e3424c drm/i915/skl: distribute DDB based on panel resolution
2e027cb6e438 drm/i915: ddb_size is of u16 type

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9819/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915/skl: distribute DDB based on panel resolution (rev2)
  2018-07-30 14:12 [PATCH] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
                   ` (3 preceding siblings ...)
  2018-07-31 15:13 ` ✓ Fi.CI.BAT: success for drm/i915/skl: distribute DDB based on panel resolution (rev2) Patchwork
@ 2018-07-31 16:51 ` Patchwork
  2018-08-01 15:43 ` ✓ Fi.CI.BAT: success for drm/i915/skl: distribute DDB based on panel resolution (rev3) Patchwork
  2018-08-01 17:40 ` ✓ Fi.CI.IGT: " Patchwork
  6 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-07-31 16:51 UTC (permalink / raw)
  To: Mahesh Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/skl: distribute DDB based on panel resolution (rev2)
URL   : https://patchwork.freedesktop.org/series/47428/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4598_full -> Patchwork_9819_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_9819_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_isolation@vcs0-s3:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_universal_plane@cursor-fb-leak-pipe-b:
      shard-apl:          PASS -> FAIL (fdo#107241)

    
    ==== Possible fixes ====

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105189) -> PASS

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS +1

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#107241 https://bugs.freedesktop.org/show_bug.cgi?id=107241


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4598 -> Patchwork_9819

  CI_DRM_4598: dc620be7ec801cea836a872f8a63469909562312 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4581: f1c868dae24056ebc27e4f3c197724ce9b956a8a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9819: 74e1f9e3424ca18587c45314cfe562dc76db33b4 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9819/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/2] drm/i915/skl: distribute DDB based on panel resolution
  2018-07-31 14:24       ` [PATCH v2 2/2] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
@ 2018-07-31 21:53         ` Chris Wilson
  2018-08-01 15:11           ` [PATCH v3 " Mahesh Kumar
  0 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2018-07-31 21:53 UTC (permalink / raw)
  To: Mahesh Kumar, intel-gfx

Quoting Mahesh Kumar (2018-07-31 15:24:45)
> +       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +               const struct drm_display_mode *adjusted_mode;
> +               int hdisplay, vdisplay;
> +               enum pipe pipe;
> +
> +               if (!crtc_state->enable)
> +                       continue;
> +
> +               pipe = to_intel_crtc(crtc)->pipe;
> +               adjusted_mode = &crtc_state->adjusted_mode;
> +               drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);

You should check with Ville whether the adjusted_mode is already
adjusted. But at any rate hdisplay is not affected by hv_timing.

> +               total_width += hdisplay;
> +
> +               if (pipe < for_pipe)
> +                       width_before_pipe += hdisplay;
> +               else if (pipe == for_pipe)
> +                       pipe_width = hdisplay;
> +       }
> +
> +       ddb_size_before_pipe = (ddb_size * width_before_pipe) / total_width;

ddb_size_before_pipe can just be alloc->start.
(brackets here) are redundant, so have a discussion as to whether they
aide or hinder comprehension.

> +       pipe_size = (ddb_size * pipe_width) / total_width;
> +       alloc->start = ddb_size_before_pipe;
>         alloc->end = alloc->start + pipe_size;

To avoid truncation fun (and prev_crtc->end != this_crtc->start), use
alloc->end = ddb_size * (width_before_pipe + pipe_width) / total_width;
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 1/2] drm/i915: ddb_size is of u16 type
  2018-07-31 14:24       ` [PATCH v1 1/2] drm/i915: ddb_size is of u16 type Mahesh Kumar
@ 2018-07-31 21:54         ` Chris Wilson
  0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2018-07-31 21:54 UTC (permalink / raw)
  To: Mahesh Kumar, intel-gfx

Quoting Mahesh Kumar (2018-07-31 15:24:44)
> ddb_size is u16 so use same return type for intel_get_ddb_size
> wrapper.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/2] drm/i915/skl: distribute DDB based on panel resolution
  2018-08-01 15:11           ` [PATCH v3 " Mahesh Kumar
@ 2018-08-01 15:11             ` Chris Wilson
  2018-08-07  8:35               ` Maarten Lankhorst
  0 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2018-08-01 15:11 UTC (permalink / raw)
  To: Mahesh Kumar, intel-gfx

Quoting Mahesh Kumar (2018-08-01 16:11:13)
> We distribute DDB equally among all pipes irrespective of display
> buffer requirement of each pipe. This leads to a situation where high
> resolution y-tiled display can not be enabled with 2 low resolution
> displays.
> 
> Main contributing factor for DDB requirement is width of the display.
> This patch make changes to distribute ddb based on display width.
> So display with higher width will get bigger chunk of DDB.
> 
> Changes Since V1:
>  - pipe_size/ddb_size will not overflow u16 so use appropriate
>    data-types during computation (Chris)
> Changes Since V2:
>  - avoid redundancy and possible truncation errors (Chris)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107113
> Cc: raviraj.p.sitaram@intel.com
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 2/2] drm/i915/skl: distribute DDB based on panel resolution
  2018-07-31 21:53         ` Chris Wilson
@ 2018-08-01 15:11           ` Mahesh Kumar
  2018-08-01 15:11             ` Chris Wilson
  0 siblings, 1 reply; 17+ messages in thread
From: Mahesh Kumar @ 2018-08-01 15:11 UTC (permalink / raw)
  To: intel-gfx

We distribute DDB equally among all pipes irrespective of display
buffer requirement of each pipe. This leads to a situation where high
resolution y-tiled display can not be enabled with 2 low resolution
displays.

Main contributing factor for DDB requirement is width of the display.
This patch make changes to distribute ddb based on display width.
So display with higher width will get bigger chunk of DDB.

Changes Since V1:
 - pipe_size/ddb_size will not overflow u16 so use appropriate
   data-types during computation (Chris)
Changes Since V2:
 - avoid redundancy and possible truncation errors (Chris)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107113
Cc: raviraj.p.sitaram@intel.com
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 55 ++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 91120560a61b..382efc048fd8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3814,8 +3814,12 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *for_crtc = cstate->base.crtc;
-	u16 pipe_size, ddb_size;
-	int nth_active_pipe;
+	const struct drm_crtc_state *crtc_state;
+	const struct drm_crtc *crtc;
+	u32 pipe_width = 0, total_width = 0, width_before_pipe = 0;
+	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
+	u16 ddb_size;
+	u32 i;
 
 	if (WARN_ON(!state) || !cstate->base.active) {
 		alloc->start = 0;
@@ -3833,14 +3837,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				      *num_active, ddb);
 
 	/*
-	 * If the state doesn't change the active CRTC's, then there's
-	 * no need to recalculate; the existing pipe allocation limits
-	 * should remain unchanged.  Note that we're safe from racing
-	 * commits since any racing commit that changes the active CRTC
-	 * list would need to grab _all_ crtc locks, including the one
-	 * we currently hold.
+	 * If the state doesn't change the active CRTC's or there is no
+	 * modeset request, then there's no need to recalculate;
+	 * the existing pipe allocation limits should remain unchanged.
+	 * Note that we're safe from racing commits since any racing commit
+	 * that changes the active CRTC list or do modeset would need to
+	 * grab _all_ crtc locks, including the one we currently hold.
 	 */
-	if (!intel_state->active_pipe_changes) {
+	if (!intel_state->active_pipe_changes && !intel_state->modeset) {
 		/*
 		 * alloc may be cleared by clear_intel_crtc_state,
 		 * copy from old state to be sure
@@ -3849,11 +3853,32 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 		return;
 	}
 
-	nth_active_pipe = hweight32(intel_state->active_crtcs &
-				    (drm_crtc_mask(for_crtc) - 1));
-	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
-	alloc->start = nth_active_pipe * ddb_size / *num_active;
-	alloc->end = alloc->start + pipe_size;
+	/*
+	 * Watermark/ddb requirement highly depends upon width of the
+	 * framebuffer, So instead of allocating DDB equally among pipes
+	 * distribute DDB based on resolution/width of the display.
+	 */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		const struct drm_display_mode *adjusted_mode;
+		int hdisplay, vdisplay;
+		enum pipe pipe;
+
+		if (!crtc_state->enable)
+			continue;
+
+		pipe = to_intel_crtc(crtc)->pipe;
+		adjusted_mode = &crtc_state->adjusted_mode;
+		drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
+		total_width += hdisplay;
+
+		if (pipe < for_pipe)
+			width_before_pipe += hdisplay;
+		else if (pipe == for_pipe)
+			pipe_width = hdisplay;
+	}
+
+	alloc->start = ddb_size * width_before_pipe / total_width;
+	alloc->end = ddb_size * (width_before_pipe + pipe_width) / total_width;
 }
 
 static unsigned int skl_cursor_allocation(int num_active)
@@ -5259,7 +5284,7 @@ skl_ddb_add_affected_pipes(struct drm_atomic_state *state, bool *changed)
 	 * any other display updates race with this transaction, so we need
 	 * to grab the lock on *all* CRTC's.
 	 */
-	if (intel_state->active_pipe_changes) {
+	if (intel_state->active_pipe_changes || intel_state->modeset) {
 		realloc_pipes = ~0;
 		intel_state->wm_results.dirty_pipes = ~0;
 	}
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915/skl: distribute DDB based on panel resolution (rev3)
  2018-07-30 14:12 [PATCH] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
                   ` (4 preceding siblings ...)
  2018-07-31 16:51 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-08-01 15:43 ` Patchwork
  2018-08-01 17:40 ` ✓ Fi.CI.IGT: " Patchwork
  6 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-08-01 15:43 UTC (permalink / raw)
  To: Mahesh Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/skl: distribute DDB based on panel resolution (rev3)
URL   : https://patchwork.freedesktop.org/series/47428/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4603 -> Patchwork_9834 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47428/revisions/3/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9834 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_workarounds:
      {fi-cfl-8109u}:     PASS -> DMESG-FAIL (fdo#107292)

    
    ==== Possible fixes ====

    {igt@kms_psr@primary_mmap_gtt}:
      fi-cnl-psr:         DMESG-WARN (fdo#107372) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372


== Participating hosts (52 -> 45) ==

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u fi-byt-clapper 


== Build changes ==

    * Linux: CI_DRM_4603 -> Patchwork_9834

  CI_DRM_4603: b9b3d5bcbd907a90f052b47810fb6752198090f8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4582: 263ca16e4d8909f475d32a28fc0e5972bac214fb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9834: 0043da92c5bcd4d78518e8a16eb525abce0c2a0c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

0043da92c5bc drm/i915/skl: distribute DDB based on panel resolution
c804f0c117fb drm/i915: ddb_size is of u16 type

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9834/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915/skl: distribute DDB based on panel resolution (rev3)
  2018-07-30 14:12 [PATCH] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
                   ` (5 preceding siblings ...)
  2018-08-01 15:43 ` ✓ Fi.CI.BAT: success for drm/i915/skl: distribute DDB based on panel resolution (rev3) Patchwork
@ 2018-08-01 17:40 ` Patchwork
  6 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-08-01 17:40 UTC (permalink / raw)
  To: Mahesh Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/skl: distribute DDB based on panel resolution (rev3)
URL   : https://patchwork.freedesktop.org/series/47428/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4603_full -> Patchwork_9834_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_9834_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      shard-snb:          NOTRUN -> INCOMPLETE (fdo#105411)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          DMESG-FAIL (fdo#106947, fdo#106560) -> PASS

    igt@drv_suspend@shrink:
      shard-apl:          FAIL (fdo#106886) -> PASS

    igt@gem_userptr_blits@create-destroy-sync:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
      shard-hsw:          FAIL (fdo#105767) -> PASS

    igt@kms_draw_crc@draw-method-xrgb8888-pwrite-ytiled:
      shard-glk:          FAIL (fdo#107401) -> PASS

    igt@kms_flip@dpms-vs-vblank-race:
      shard-hsw:          DMESG-WARN (fdo#102614) -> PASS

    igt@kms_setmode@basic:
      shard-glk:          FAIL (fdo#99912) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107401 https://bugs.freedesktop.org/show_bug.cgi?id=107401
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4603 -> Patchwork_9834

  CI_DRM_4603: b9b3d5bcbd907a90f052b47810fb6752198090f8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4582: 263ca16e4d8909f475d32a28fc0e5972bac214fb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9834: 0043da92c5bcd4d78518e8a16eb525abce0c2a0c @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9834/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/2] drm/i915/skl: distribute DDB based on panel resolution
  2018-08-01 15:11             ` Chris Wilson
@ 2018-08-07  8:35               ` Maarten Lankhorst
  0 siblings, 0 replies; 17+ messages in thread
From: Maarten Lankhorst @ 2018-08-07  8:35 UTC (permalink / raw)
  To: Chris Wilson, Mahesh Kumar, intel-gfx

Op 01-08-18 om 17:11 schreef Chris Wilson:
> Quoting Mahesh Kumar (2018-08-01 16:11:13)
>> We distribute DDB equally among all pipes irrespective of display
>> buffer requirement of each pipe. This leads to a situation where high
>> resolution y-tiled display can not be enabled with 2 low resolution
>> displays.
>>
>> Main contributing factor for DDB requirement is width of the display.
>> This patch make changes to distribute ddb based on display width.
>> So display with higher width will get bigger chunk of DDB.
>>
>> Changes Since V1:
>>  - pipe_size/ddb_size will not overflow u16 so use appropriate
>>    data-types during computation (Chris)
>> Changes Since V2:
>>  - avoid redundancy and possible truncation errors (Chris)
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107113
>> Cc: raviraj.p.sitaram@intel.com
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Thanks, reviewed and pushed. :)

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-08-07  8:35 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-30 14:12 [PATCH] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
2018-07-30 15:37 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-07-30 15:38 ` [PATCH] " Chris Wilson
2018-07-31  6:16   ` Kumar, Mahesh
2018-07-31 14:24     ` [PATCH v2 0/2] " Mahesh Kumar
2018-07-31 14:24       ` [PATCH v1 1/2] drm/i915: ddb_size is of u16 type Mahesh Kumar
2018-07-31 21:54         ` Chris Wilson
2018-07-31 14:24       ` [PATCH v2 2/2] drm/i915/skl: distribute DDB based on panel resolution Mahesh Kumar
2018-07-31 21:53         ` Chris Wilson
2018-08-01 15:11           ` [PATCH v3 " Mahesh Kumar
2018-08-01 15:11             ` Chris Wilson
2018-08-07  8:35               ` Maarten Lankhorst
2018-07-30 17:04 ` ✓ Fi.CI.IGT: success for " Patchwork
2018-07-31 15:13 ` ✓ Fi.CI.BAT: success for drm/i915/skl: distribute DDB based on panel resolution (rev2) Patchwork
2018-07-31 16:51 ` ✓ Fi.CI.IGT: " Patchwork
2018-08-01 15:43 ` ✓ Fi.CI.BAT: success for drm/i915/skl: distribute DDB based on panel resolution (rev3) Patchwork
2018-08-01 17:40 ` ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.