[PATCH 1/2] drm/i915/cnl: Allow 2 pixel per clock on Cannonlake.

[PATCH 1/2] drm/i915/cnl: Allow 2 pixel per clock on Cannonlake.

[Rodrigo Vivi]

This is heavily based on a initial patch provided by Ville
plus all changes provided later by Ander.

As Geminilake, Cannonlake also supports 2 pixels per clock.

Different from Geminilake we are not implementing the 99% Wa.
But we can revisit that decision later if we find out
any limitation on later CNL SKUs.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_pm.c    |  3 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 1241e5891b29..6b1d805fb755 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1422,9 +1422,9 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
 
 static int cnl_calc_cdclk(int max_pixclk)
 {
-	if (max_pixclk > 336000)
+	if (max_pixclk > 2 * 336000)
 		return 528000;
-	else if (max_pixclk > 168000)
+	else if (max_pixclk > 2 * 168000)
 		return 336000;
 	else
 		return 168000;
@@ -1752,9 +1752,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
 	    crtc_state->has_audio &&
 	    crtc_state->port_clock >= 540000 &&
 	    crtc_state->lane_count == 4) {
-		if (IS_CANNONLAKE(dev_priv))
-			pixel_rate = max(316800, pixel_rate);
-		else if (IS_GEMINILAKE(dev_priv))
+		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
 			pixel_rate = max(2 * 316800, pixel_rate);
 		else
 			pixel_rate = max(432000, pixel_rate);
@@ -1766,7 +1764,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
 	 * two pixels per clock.
 	 */
 	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
-		if (IS_GEMINILAKE(dev_priv))
+		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
 			pixel_rate = max(2 * 2 * 96000, pixel_rate);
 		else
 			pixel_rate = max(2 * 96000, pixel_rate);
@@ -1999,6 +1997,8 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
 {
 	int max_cdclk_freq = dev_priv->max_cdclk_freq;
 
+	if (IS_CANNONLAKE(dev_priv))
+		return 2 * max_cdclk_freq;
 	if (IS_GEMINILAKE(dev_priv))
 		/*
 		 * FIXME: Limiting to 99% as a temporary workaround. See
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ed662937ec3c..42f753df30cb 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3969,7 +3969,8 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
 	crtc_clock = crtc_state->adjusted_mode.crtc_clock;
 	dotclk = to_intel_atomic_state(state)->cdclk.logical.cdclk;
 
-	if (IS_GEMINILAKE(to_i915(intel_crtc->base.dev)))
+	if (IS_GEMINILAKE(to_i915(intel_crtc->base.dev)) ||
+	    IS_CANNONLAKE(to_i915(intel_crtc->base.dev)))
 		dotclk *= 2;
 
 	pipe_max_pixel_rate = div_round_up_u32_fixed16(dotclk, pipe_downscale);
-- 
2.13.2

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

[PATCH 2/2] drm/i915: Introduce HAS_2PPC.

[Rodrigo Vivi]

Let's make it easier to add platforms that supports 2 pixel per
clock.

With spread checks per platform it was easy to miss one or
another spot leading to loose some time on debug.

Hopefully this check would save some cases in the future.

No functional change.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    | 4 ++++
 drivers/gpu/drm/i915/i915_pci.c    | 2 ++
 drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
 drivers/gpu/drm/i915/intel_pm.c    | 3 +--
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6c25c8520c87..94f5e6522e5e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -748,6 +748,7 @@ struct intel_csr {
 	func(is_lp); \
 	func(is_alpha_support); \
 	/* Keep has_* in alphabetical order */ \
+	func(has_2ppc); \
 	func(has_64bit_reloc); \
 	func(has_aliasing_ppgtt); \
 	func(has_csr); \
@@ -3025,6 +3026,9 @@ intel_info(const struct drm_i915_private *dev_priv)
 #define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
 	(IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
 
+/* Supports 2 pixel per clock */
+#define HAS_2PPC(dev_priv) ((dev_priv)->info.has_2ppc)
+
 /*
  * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
  * even when in MSI mode. This results in spurious interrupt warnings if the
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 09d97e0990b7..df84025579cf 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -405,6 +405,7 @@ static const struct intel_device_info intel_geminilake_info = {
 	GEN9_LP_FEATURES,
 	.platform = INTEL_GEMINILAKE,
 	.ddb_size = 1024,
+	.has_2ppc = 1,
 	.color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 }
 };
 
@@ -450,6 +451,7 @@ static const struct intel_device_info intel_cannonlake_info = {
 	.gen = 10,
 	.ddb_size = 1024,
 	.has_csr = 1,
+	.has_2ppc = 1,
 	.color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 }
 };
 
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 6b1d805fb755..edbccda40f0c 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1752,7 +1752,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
 	    crtc_state->has_audio &&
 	    crtc_state->port_clock >= 540000 &&
 	    crtc_state->lane_count == 4) {
-		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
+		if (HAS_2PPC(dev_priv))
 			pixel_rate = max(2 * 316800, pixel_rate);
 		else
 			pixel_rate = max(432000, pixel_rate);
@@ -1764,7 +1764,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
 	 * two pixels per clock.
 	 */
 	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
-		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
+		if (HAS_2PPC(dev_priv))
 			pixel_rate = max(2 * 2 * 96000, pixel_rate);
 		else
 			pixel_rate = max(2 * 96000, pixel_rate);
@@ -1997,14 +1997,14 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
 {
 	int max_cdclk_freq = dev_priv->max_cdclk_freq;
 
-	if (IS_CANNONLAKE(dev_priv))
-		return 2 * max_cdclk_freq;
 	if (IS_GEMINILAKE(dev_priv))
 		/*
 		 * FIXME: Limiting to 99% as a temporary workaround. See
 		 * glk_calc_cdclk() for details.
 		 */
 		return 2 * max_cdclk_freq * 99 / 100;
+	else if (HAS_2PPC(dev_priv))
+		return 2 * max_cdclk_freq;
 	else if (INTEL_INFO(dev_priv)->gen >= 9 ||
 		 IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		return max_cdclk_freq;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 42f753df30cb..c8da6ca4e8df 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3969,8 +3969,7 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
 	crtc_clock = crtc_state->adjusted_mode.crtc_clock;
 	dotclk = to_intel_atomic_state(state)->cdclk.logical.cdclk;
 
-	if (IS_GEMINILAKE(to_i915(intel_crtc->base.dev)) ||
-	    IS_CANNONLAKE(to_i915(intel_crtc->base.dev)))
+	if (HAS_2PPC(to_i915(intel_crtc->base.dev)))
 		dotclk *= 2;
 
 	pipe_max_pixel_rate = div_round_up_u32_fixed16(dotclk, pipe_downscale);
-- 
2.13.2

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/cnl: Allow 2 pixel per clock on Cannonlake.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/cnl: Allow 2 pixel per clock on Cannonlake.
URL   : https://patchwork.freedesktop.org/series/28890/
State : success

== Summary ==

Series 28890v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/28890/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#101705

fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:453s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:441s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:356s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:552s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:519s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:525s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:514s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:614s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:446s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:422s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:428s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:508s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:476s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:476s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:599s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:598s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:526s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:468s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:477s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:489s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:444s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:489s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:545s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:410s

ada53b43f81fe618f3f0f1dfbd3dd776bb277323 drm-tip: 2017y-08m-16d-15h-18m-56s UTC integration manifest
36dd9803628a drm/i915: Introduce HAS_2PPC.
82245cda4c5c drm/i915/cnl: Allow 2 pixel per clock on Cannonlake.

== Logs ==

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

Re: [PATCH 1/2] drm/i915/cnl: Allow 2 pixel per clock on Cannonlake.

[Pandiyan, Dhinakaran]

On Wed, 2017-08-16 at 17:54 -0700, Rodrigo Vivi wrote:
> This is heavily based on a initial patch provided by Ville
> plus all changes provided later by Ander.
> 

Ville abandoned this change last time stating - "Assume 1 pixel per
clock for the purposes of max pixel rate calculation until DDI clock
voltage scaling is handled"

If you can confirm that change was implemented, this patch is
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


Also, I see that Ville's patch to change the pixel rate checks to use
cdclk has not been merged



> As Geminilake, Cannonlake also supports 2 pixels per clock.
> 
> Different from Geminilake we are not implementing the 99% Wa.
> But we can revisit that decision later if we find out
> any limitation on later CNL SKUs.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_pm.c    |  3 ++-
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 1241e5891b29..6b1d805fb755 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1422,9 +1422,9 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
>  
>  static int cnl_calc_cdclk(int max_pixclk)
>  {
> -	if (max_pixclk > 336000)
> +	if (max_pixclk > 2 * 336000)
>  		return 528000;
> -	else if (max_pixclk > 168000)
> +	else if (max_pixclk > 2 * 168000)
>  		return 336000;
>  	else
>  		return 168000;
> @@ -1752,9 +1752,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
>  	    crtc_state->has_audio &&
>  	    crtc_state->port_clock >= 540000 &&
>  	    crtc_state->lane_count == 4) {
> -		if (IS_CANNONLAKE(dev_priv))
> -			pixel_rate = max(316800, pixel_rate);
> -		else if (IS_GEMINILAKE(dev_priv))
> +		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
>  			pixel_rate = max(2 * 316800, pixel_rate);
>  		else
>  			pixel_rate = max(432000, pixel_rate);
> @@ -1766,7 +1764,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
>  	 * two pixels per clock.
>  	 */
>  	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
> -		if (IS_GEMINILAKE(dev_priv))
> +		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
>  			pixel_rate = max(2 * 2 * 96000, pixel_rate);
>  		else
>  			pixel_rate = max(2 * 96000, pixel_rate);
> @@ -1999,6 +1997,8 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
>  {
>  	int max_cdclk_freq = dev_priv->max_cdclk_freq;
>  
> +	if (IS_CANNONLAKE(dev_priv))
> +		return 2 * max_cdclk_freq;
>  	if (IS_GEMINILAKE(dev_priv))
>  		/*
>  		 * FIXME: Limiting to 99% as a temporary workaround. See
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ed662937ec3c..42f753df30cb 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3969,7 +3969,8 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>  	crtc_clock = crtc_state->adjusted_mode.crtc_clock;
>  	dotclk = to_intel_atomic_state(state)->cdclk.logical.cdclk;
>  
> -	if (IS_GEMINILAKE(to_i915(intel_crtc->base.dev)))
> +	if (IS_GEMINILAKE(to_i915(intel_crtc->base.dev)) ||
> +	    IS_CANNONLAKE(to_i915(intel_crtc->base.dev)))
>  		dotclk *= 2;
>  
>  	pipe_max_pixel_rate = div_round_up_u32_fixed16(dotclk, pipe_downscale);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/cnl: Allow 2 pixel per clock on Cannonlake.

[Ville Syrjälä]

On Wed, Aug 30, 2017 at 06:14:03PM +0000, Pandiyan, Dhinakaran wrote:
> 
> On Wed, 2017-08-16 at 17:54 -0700, Rodrigo Vivi wrote:
> > This is heavily based on a initial patch provided by Ville
> > plus all changes provided later by Ander.
> > 
> 
> Ville abandoned this change last time stating - "Assume 1 pixel per
> clock for the purposes of max pixel rate calculation until DDI clock
> voltage scaling is handled"
> 
> If you can confirm that change was implemented, this patch is
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> 
> Also, I see that Ville's patch to change the pixel rate checks to use
> cdclk has not been merged

I totally forgot about that one TBH. I've just reposted first patch to
get a fresh CI run for it.

I think this CNL thing might look cleaner if redone on top of my stuff,
but if people want this in ASAP I can rebase my stuff as well.

> 
> 
> 
> > As Geminilake, Cannonlake also supports 2 pixels per clock.
> > 
> > Different from Geminilake we are not implementing the 99% Wa.
> > But we can revisit that decision later if we find out
> > any limitation on later CNL SKUs.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 12 ++++++------
> >  drivers/gpu/drm/i915/intel_pm.c    |  3 ++-
> >  2 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 1241e5891b29..6b1d805fb755 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1422,9 +1422,9 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
> >  
> >  static int cnl_calc_cdclk(int max_pixclk)
> >  {
> > -	if (max_pixclk > 336000)
> > +	if (max_pixclk > 2 * 336000)
> >  		return 528000;
> > -	else if (max_pixclk > 168000)
> > +	else if (max_pixclk > 2 * 168000)
> >  		return 336000;
> >  	else
> >  		return 168000;
> > @@ -1752,9 +1752,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> >  	    crtc_state->has_audio &&
> >  	    crtc_state->port_clock >= 540000 &&
> >  	    crtc_state->lane_count == 4) {
> > -		if (IS_CANNONLAKE(dev_priv))
> > -			pixel_rate = max(316800, pixel_rate);
> > -		else if (IS_GEMINILAKE(dev_priv))
> > +		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
> >  			pixel_rate = max(2 * 316800, pixel_rate);
> >  		else
> >  			pixel_rate = max(432000, pixel_rate);
> > @@ -1766,7 +1764,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> >  	 * two pixels per clock.
> >  	 */
> >  	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
> > -		if (IS_GEMINILAKE(dev_priv))
> > +		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
> >  			pixel_rate = max(2 * 2 * 96000, pixel_rate);
> >  		else
> >  			pixel_rate = max(2 * 96000, pixel_rate);
> > @@ -1999,6 +1997,8 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
> >  {
> >  	int max_cdclk_freq = dev_priv->max_cdclk_freq;
> >  
> > +	if (IS_CANNONLAKE(dev_priv))
> > +		return 2 * max_cdclk_freq;
> >  	if (IS_GEMINILAKE(dev_priv))
> >  		/*
> >  		 * FIXME: Limiting to 99% as a temporary workaround. See
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ed662937ec3c..42f753df30cb 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3969,7 +3969,8 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
> >  	crtc_clock = crtc_state->adjusted_mode.crtc_clock;
> >  	dotclk = to_intel_atomic_state(state)->cdclk.logical.cdclk;
> >  
> > -	if (IS_GEMINILAKE(to_i915(intel_crtc->base.dev)))
> > +	if (IS_GEMINILAKE(to_i915(intel_crtc->base.dev)) ||
> > +	    IS_CANNONLAKE(to_i915(intel_crtc->base.dev)))
> >  		dotclk *= 2;
> >  
> >  	pipe_max_pixel_rate = div_round_up_u32_fixed16(dotclk, pipe_downscale);

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

Re: [PATCH 2/2] drm/i915: Introduce HAS_2PPC.

[Pandiyan, Dhinakaran]

On Wed, 2017-08-16 at 17:54 -0700, Rodrigo Vivi wrote:
> Let's make it easier to add platforms that supports 2 pixel per
> clock.
> 
> With spread checks per platform it was easy to miss one or
> another spot leading to loose some time on debug.
> 
> Hopefully this check would save some cases in the future.
> 
> No functional change.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    | 4 ++++
>  drivers/gpu/drm/i915/i915_pci.c    | 2 ++
>  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
>  drivers/gpu/drm/i915/intel_pm.c    | 3 +--
>  4 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6c25c8520c87..94f5e6522e5e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -748,6 +748,7 @@ struct intel_csr {
>  	func(is_lp); \
>  	func(is_alpha_support); \
>  	/* Keep has_* in alphabetical order */ \
> +	func(has_2ppc); \
>  	func(has_64bit_reloc); \
>  	func(has_aliasing_ppgtt); \
>  	func(has_csr); \
> @@ -3025,6 +3026,9 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
>  	(IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
>  
> +/* Supports 2 pixel per clock */
> +#define HAS_2PPC(dev_priv) ((dev_priv)->info.has_2ppc)
> +

How about
#define HAS_2PPC(dev_priv) (IS_GEMINILAKE(dev_priv) ||
INTEL_GEN(dev_priv) >= 10) ?

I am not clear on what qualifies for a place in device_info, but
defining it this way let's me go to the definition and quickly check
which platform has 2 pixels per clock.

But again, iirc we won't need this with Ville's changes merged.


>  /*
>   * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
>   * even when in MSI mode. This results in spurious interrupt warnings if the
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 09d97e0990b7..df84025579cf 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -405,6 +405,7 @@ static const struct intel_device_info intel_geminilake_info = {
>  	GEN9_LP_FEATURES,
>  	.platform = INTEL_GEMINILAKE,
>  	.ddb_size = 1024,
> +	.has_2ppc = 1,
>  	.color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 }
>  };
>  
> @@ -450,6 +451,7 @@ static const struct intel_device_info intel_cannonlake_info = {
>  	.gen = 10,
>  	.ddb_size = 1024,
>  	.has_csr = 1,
> +	.has_2ppc = 1,
>  	.color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 }
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 6b1d805fb755..edbccda40f0c 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1752,7 +1752,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
>  	    crtc_state->has_audio &&
>  	    crtc_state->port_clock >= 540000 &&
>  	    crtc_state->lane_count == 4) {
> -		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
> +		if (HAS_2PPC(dev_priv))
>  			pixel_rate = max(2 * 316800, pixel_rate);
>  		else
>  			pixel_rate = max(432000, pixel_rate);
> @@ -1764,7 +1764,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
>  	 * two pixels per clock.
>  	 */
>  	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
> -		if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
> +		if (HAS_2PPC(dev_priv))
>  			pixel_rate = max(2 * 2 * 96000, pixel_rate);
>  		else
>  			pixel_rate = max(2 * 96000, pixel_rate);
> @@ -1997,14 +1997,14 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
>  {
>  	int max_cdclk_freq = dev_priv->max_cdclk_freq;
>  
> -	if (IS_CANNONLAKE(dev_priv))
> -		return 2 * max_cdclk_freq;
>  	if (IS_GEMINILAKE(dev_priv))
>  		/*
>  		 * FIXME: Limiting to 99% as a temporary workaround. See
>  		 * glk_calc_cdclk() for details.
>  		 */
>  		return 2 * max_cdclk_freq * 99 / 100;
> +	else if (HAS_2PPC(dev_priv))
> +		return 2 * max_cdclk_freq;
>  	else if (INTEL_INFO(dev_priv)->gen >= 9 ||
>  		 IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		return max_cdclk_freq;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 42f753df30cb..c8da6ca4e8df 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3969,8 +3969,7 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>  	crtc_clock = crtc_state->adjusted_mode.crtc_clock;
>  	dotclk = to_intel_atomic_state(state)->cdclk.logical.cdclk;
>  
> -	if (IS_GEMINILAKE(to_i915(intel_crtc->base.dev)) ||
> -	    IS_CANNONLAKE(to_i915(intel_crtc->base.dev)))
> +	if (HAS_2PPC(to_i915(intel_crtc->base.dev)))
>  		dotclk *= 2;
>  
>  	pipe_max_pixel_rate = div_round_up_u32_fixed16(dotclk, pipe_downscale);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Introduce HAS_2PPC.

[Chris Wilson]

Quoting Pandiyan, Dhinakaran (2017-08-30 20:12:52)
> On Wed, 2017-08-16 at 17:54 -0700, Rodrigo Vivi wrote:
> > Let's make it easier to add platforms that supports 2 pixel per
> > clock.
> > 
> > With spread checks per platform it was easy to miss one or
> > another spot leading to loose some time on debug.
> > 
> > Hopefully this check would save some cases in the future.
> > 
> > No functional change.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h    | 4 ++++
> >  drivers/gpu/drm/i915/i915_pci.c    | 2 ++
> >  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
> >  drivers/gpu/drm/i915/intel_pm.c    | 3 +--
> >  4 files changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 6c25c8520c87..94f5e6522e5e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -748,6 +748,7 @@ struct intel_csr {
> >       func(is_lp); \
> >       func(is_alpha_support); \
> >       /* Keep has_* in alphabetical order */ \
> > +     func(has_2ppc); \
> >       func(has_64bit_reloc); \
> >       func(has_aliasing_ppgtt); \
> >       func(has_csr); \
> > @@ -3025,6 +3026,9 @@ intel_info(const struct drm_i915_private *dev_priv)
> >  #define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
> >       (IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
> >  
> > +/* Supports 2 pixel per clock */
> > +#define HAS_2PPC(dev_priv) ((dev_priv)->info.has_2ppc)
> > +
> 
> How about
> #define HAS_2PPC(dev_priv) (IS_GEMINILAKE(dev_priv) ||
> INTEL_GEN(dev_priv) >= 10) ?
> 
> I am not clear on what qualifies for a place in device_info, but
> defining it this way let's me go to the definition and quickly check
> which platform has 2 pixels per clock.

A couple of rules of thumb for starting with:

Use device_info if:

 - it fundamentally changes how the device operates, such that knowing
   about it in debug logs is a key means of triage
 
 - number of branches x callsites > 8
   (some estimate of the cost of inclusion inside device_info vs
   savings in object code, for a more realistic estimate a branch will
   ~12 bytes (depending on the phase of the moon) and cost for device
   info will be the addition of a few strings, and a couple of calls
   to use those string, so at a guess 100 bytes.)

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

Re: [PATCH 2/2] drm/i915: Introduce HAS_2PPC.

[Pandiyan, Dhinakaran]

On Wed, 2017-08-30 at 20:32 +0100, Chris Wilson wrote:
> Quoting Pandiyan, Dhinakaran (2017-08-30 20:12:52)
> > On Wed, 2017-08-16 at 17:54 -0700, Rodrigo Vivi wrote:
> > > Let's make it easier to add platforms that supports 2 pixel per
> > > clock.
> > > 
> > > With spread checks per platform it was easy to miss one or
> > > another spot leading to loose some time on debug.
> > > 
> > > Hopefully this check would save some cases in the future.
> > > 
> > > No functional change.
> > > 
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h    | 4 ++++
> > >  drivers/gpu/drm/i915/i915_pci.c    | 2 ++
> > >  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
> > >  drivers/gpu/drm/i915/intel_pm.c    | 3 +--
> > >  4 files changed, 11 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 6c25c8520c87..94f5e6522e5e 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -748,6 +748,7 @@ struct intel_csr {
> > >       func(is_lp); \
> > >       func(is_alpha_support); \
> > >       /* Keep has_* in alphabetical order */ \
> > > +     func(has_2ppc); \
> > >       func(has_64bit_reloc); \
> > >       func(has_aliasing_ppgtt); \
> > >       func(has_csr); \
> > > @@ -3025,6 +3026,9 @@ intel_info(const struct drm_i915_private *dev_priv)
> > >  #define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
> > >       (IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
> > >  
> > > +/* Supports 2 pixel per clock */
> > > +#define HAS_2PPC(dev_priv) ((dev_priv)->info.has_2ppc)
> > > +
> > 
> > How about
> > #define HAS_2PPC(dev_priv) (IS_GEMINILAKE(dev_priv) ||
> > INTEL_GEN(dev_priv) >= 10) ?
> > 
> > I am not clear on what qualifies for a place in device_info, but
> > defining it this way let's me go to the definition and quickly check
> > which platform has 2 pixels per clock.
> 
> A couple of rules of thumb for starting with:
> 
> Use device_info if:
> 
>  - it fundamentally changes how the device operates, such that knowing
>    about it in debug logs is a key means of triage
>  
>  - number of branches x callsites > 8
>    (some estimate of the cost of inclusion inside device_info vs
>    savings in object code, for a more realistic estimate a branch will
>    ~12 bytes (depending on the phase of the moon) and cost for device
>    info will be the addition of a few strings, and a couple of calls
>    to use those string, so at a guess 100 bytes.)

That's an interesting back-of-the-envelope calculation. I suppose the
compiler is also more likely to load device_info->gen into a register
than device_info->has_2ppc.


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

Re: [PATCH 2/2] drm/i915: Introduce HAS_2PPC.

[Rodrigo Vivi]

On Wed, Aug 30, 2017 at 7:58 PM, Pandiyan, Dhinakaran
<dhinakaran.pandiyan@intel.com> wrote:
> On Wed, 2017-08-30 at 20:32 +0100, Chris Wilson wrote:
>> Quoting Pandiyan, Dhinakaran (2017-08-30 20:12:52)
>> > On Wed, 2017-08-16 at 17:54 -0700, Rodrigo Vivi wrote:
>> > > Let's make it easier to add platforms that supports 2 pixel per
>> > > clock.
>> > >
>> > > With spread checks per platform it was easy to miss one or
>> > > another spot leading to loose some time on debug.
>> > >
>> > > Hopefully this check would save some cases in the future.
>> > >
>> > > No functional change.
>> > >
>> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/i915_drv.h    | 4 ++++
>> > >  drivers/gpu/drm/i915/i915_pci.c    | 2 ++
>> > >  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
>> > >  drivers/gpu/drm/i915/intel_pm.c    | 3 +--
>> > >  4 files changed, 11 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> > > index 6c25c8520c87..94f5e6522e5e 100644
>> > > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > > @@ -748,6 +748,7 @@ struct intel_csr {
>> > >       func(is_lp); \
>> > >       func(is_alpha_support); \
>> > >       /* Keep has_* in alphabetical order */ \
>> > > +     func(has_2ppc); \
>> > >       func(has_64bit_reloc); \
>> > >       func(has_aliasing_ppgtt); \
>> > >       func(has_csr); \
>> > > @@ -3025,6 +3026,9 @@ intel_info(const struct drm_i915_private *dev_priv)
>> > >  #define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
>> > >       (IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
>> > >
>> > > +/* Supports 2 pixel per clock */
>> > > +#define HAS_2PPC(dev_priv) ((dev_priv)->info.has_2ppc)
>> > > +
>> >
>> > How about
>> > #define HAS_2PPC(dev_priv) (IS_GEMINILAKE(dev_priv) ||
>> > INTEL_GEN(dev_priv) >= 10) ?
>> >
>> > I am not clear on what qualifies for a place in device_info, but
>> > defining it this way let's me go to the definition and quickly check
>> > which platform has 2 pixels per clock.

One thing I always wanted was to avoid 2 places to declare features &
capabilities.
Here or on the platform.

>>
>> A couple of rules of thumb for starting with:
>>
>> Use device_info if:
>>
>>  - it fundamentally changes how the device operates, such that knowing
>>    about it in debug logs is a key means of triage

I think on this one 2ppc qualifies

>>
>>  - number of branches x callsites > 8

2 * 5 > 8 in this case?
or I'm getting the number of "branches" incorrectly?

What I was trying to save as well is the addition of any next platform.
When adding the feature it would be easier to search for HAS_2PPC instead of
searching for glk or cnl and analising that individually to see if it
is 2pp before
 see if it applies to that platform.

But the reason that I put this patch on top is that I don't have
strong opinion here so
t would be fine for me to move on without ht.

Thanks,
Rodrigo.

>>    (some estimate of the cost of inclusion inside device_info vs
>>    savings in object code, for a more realistic estimate a branch will
>>    ~12 bytes (depending on the phase of the moon) and cost for device
>>    info will be the addition of a few strings, and a couple of calls
>>    to use those string, so at a guess 100 bytes.)
>
> That's an interesting back-of-the-envelope calculation. I suppose the
> compiler is also more likely to load device_info->gen into a register
> than device_info->has_2ppc.
>
>
>>
>> -Chris
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Introduce HAS_2PPC.

[Daniel Vetter]

On Wed, Aug 30, 2017 at 09:40:06PM -0700, Rodrigo Vivi wrote:
> On Wed, Aug 30, 2017 at 7:58 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
> > On Wed, 2017-08-30 at 20:32 +0100, Chris Wilson wrote:
> >> Quoting Pandiyan, Dhinakaran (2017-08-30 20:12:52)
> >> > On Wed, 2017-08-16 at 17:54 -0700, Rodrigo Vivi wrote:
> >> > > Let's make it easier to add platforms that supports 2 pixel per
> >> > > clock.
> >> > >
> >> > > With spread checks per platform it was easy to miss one or
> >> > > another spot leading to loose some time on debug.
> >> > >
> >> > > Hopefully this check would save some cases in the future.
> >> > >
> >> > > No functional change.
> >> > >
> >> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> > > ---
> >> > >  drivers/gpu/drm/i915/i915_drv.h    | 4 ++++
> >> > >  drivers/gpu/drm/i915/i915_pci.c    | 2 ++
> >> > >  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
> >> > >  drivers/gpu/drm/i915/intel_pm.c    | 3 +--
> >> > >  4 files changed, 11 insertions(+), 6 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> > > index 6c25c8520c87..94f5e6522e5e 100644
> >> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > > @@ -748,6 +748,7 @@ struct intel_csr {
> >> > >       func(is_lp); \
> >> > >       func(is_alpha_support); \
> >> > >       /* Keep has_* in alphabetical order */ \
> >> > > +     func(has_2ppc); \
> >> > >       func(has_64bit_reloc); \
> >> > >       func(has_aliasing_ppgtt); \
> >> > >       func(has_csr); \
> >> > > @@ -3025,6 +3026,9 @@ intel_info(const struct drm_i915_private *dev_priv)
> >> > >  #define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
> >> > >       (IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
> >> > >
> >> > > +/* Supports 2 pixel per clock */
> >> > > +#define HAS_2PPC(dev_priv) ((dev_priv)->info.has_2ppc)
> >> > > +
> >> >
> >> > How about
> >> > #define HAS_2PPC(dev_priv) (IS_GEMINILAKE(dev_priv) ||
> >> > INTEL_GEN(dev_priv) >= 10) ?
> >> >
> >> > I am not clear on what qualifies for a place in device_info, but
> >> > defining it this way let's me go to the definition and quickly check
> >> > which platform has 2 pixels per clock.
> 
> One thing I always wanted was to avoid 2 places to declare features &
> capabilities.
> Here or on the platform.

So should we do a bit of extraction to bring the two closer together? I.e.
h/c file with only the device info and feature macros, nothing else. We
still need the split, but well we need the structure in a header anyway.

If you actually want to bring them closer together, then you need to bring
them closer together. Not arbitrarily prefer one over the other (while
still having piles of other users around of the non-preferred version).
> 
> >>
> >> A couple of rules of thumb for starting with:
> >>
> >> Use device_info if:
> >>
> >>  - it fundamentally changes how the device operates, such that knowing
> >>    about it in debug logs is a key means of triage
> 
> I think on this one 2ppc qualifies

gen3 did double-clock too, just fyi :-) We survived just fine without a
feature flag on that.

Anyway, just my bikeshed.

> >>
> >>  - number of branches x callsites > 8
> 
> 2 * 5 > 8 in this case?
> or I'm getting the number of "branches" incorrectly?
> 
> What I was trying to save as well is the addition of any next platform.
> When adding the feature it would be easier to search for HAS_2PPC instead of
> searching for glk or cnl and analising that individually to see if it
> is 2pp before
>  see if it applies to that platform.
> 
> But the reason that I put this patch on top is that I don't have
> strong opinion here so
> t would be fine for me to move on without ht.

HAS_2PPC definitely makes sense, but I'm not sure it makes sense to stuff
everything into intel_device_info. Going that way some people might start
to think that this is the complete solution to describing everything, and
that definitely doesn't work.
-Daniel

> Thanks,
> Rodrigo.
> 
> >>    (some estimate of the cost of inclusion inside device_info vs
> >>    savings in object code, for a more realistic estimate a branch will
> >>    ~12 bytes (depending on the phase of the moon) and cost for device
> >>    info will be the addition of a few strings, and a couple of calls
> >>    to use those string, so at a guess 100 bytes.)
> >
> > That's an interesting back-of-the-envelope calculation. I suppose the
> > compiler is also more likely to load device_info->gen into a register
> > than device_info->has_2ppc.
> >
> >
> >>
> >> -Chris
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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