diff mbox

[4/7] drm/i915/icl: Add Voltage swing table for MG PHY DDI Buffer

Message ID 20180323172419.24911-5-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R March 23, 2018, 5:24 p.m. UTC
From: Manasi Navare <manasi.d.navare@intel.com>

This table is used for voltage swing programming sequence during DDI
Buffer initialization for MG PHY DDI Buffers on Icelake.

v2 (from Paulo):
* Fix white space issues.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Chris Wilson April 10, 2018, 8:51 a.m. UTC | #1
Quoting Paulo Zanoni (2018-03-23 17:24:16)
> From: Manasi Navare <manasi.d.navare@intel.com>
> 
> This table is used for voltage swing programming sequence during DDI
> Buffer initialization for MG PHY DDI Buffers on Icelake.

Except it is not used at all...

drivers/gpu/drm/i915/intel_ddi.c:601:46: error: ‘icl_mg_phy_ddi_translations’ defined but not used [-Werror=unused-const-variable=]
 static const struct icl_mg_phy_ddi_buf_trans icl_mg_phy_ddi_translations[] = {
                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/intel_ddi.c:581:49: error: ‘icl_combo_phy_ddi_translations_edp_1_05V’ defined but not used [-Werror=unused-const-variable=]
 static const struct icl_combo_phy_ddi_buf_trans icl_combo_phy_ddi_translations_edp_1_05V[] = {
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/intel_ddi.c:565:49: error: ‘icl_combo_phy_ddi_translations_dp_hdmi_1_05V’ defined but not used [-Werror=unused-const-variable=]
 static const struct icl_combo_phy_ddi_buf_trans icl_combo_phy_ddi_translations_dp_hdmi_1_05V[] = {
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/intel_ddi.c:550:49: error: ‘icl_combo_phy_ddi_translations_edp_0_95V’ defined but not used [-Werror=unused-const-variable=]
 static const struct icl_combo_phy_ddi_buf_trans icl_combo_phy_ddi_translations_edp_0_95V[] = {
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/intel_ddi.c:534:49: error: ‘icl_combo_phy_ddi_translations_dp_hdmi_0_95V’ defined but not used [-Werror=unused-const-variable=]
 static const struct icl_combo_phy_ddi_buf_trans icl_combo_phy_ddi_translations_dp_hdmi_0_95V[] = {
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/intel_ddi.c:519:49: error: ‘icl_combo_phy_ddi_translations_edp_0_85V’ defined but not used [-Werror=unused-const-variable=]
 static const struct icl_combo_phy_ddi_buf_trans icl_combo_phy_ddi_translations_edp_0_85V[] = {
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/intel_ddi.c:503:49: error: ‘icl_combo_phy_ddi_translations_dp_hdmi_0_85V’ defined but not used [-Werror=unused-const-variable=]
 static const struct icl_combo_phy_ddi_buf_trans icl_combo_phy_ddi_translations_dp_hdmi_0_85V[] = {
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


> v2 (from Paulo):
> * Fix white space issues.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 229b9d5250c4..359acbfec4b1 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -592,6 +592,26 @@ static const struct icl_combo_phy_ddi_buf_trans icl_combo_phy_ddi_translations_e
>         { 0x0, 0x00, 0x00 },    /* 350         0.0   */
>  };
>  
> +struct icl_mg_phy_ddi_buf_trans {
> +       u32 cri_txdeemph_override_5_0;
> +       u32 cri_txdeemph_override_11_6;
> +       u32 cri_txdeemph_override_17_12;
> +};
> +
> +static const struct icl_mg_phy_ddi_buf_trans icl_mg_phy_ddi_translations[] = {
> +                               /* Voltage swing  pre-emphasis */
> +       { 0x0, 0x1B, 0x00 },    /* 0              0   */
> +       { 0x0, 0x23, 0x08 },    /* 0              1   */
> +       { 0x0, 0x2D, 0x12 },    /* 0              2   */
> +       { 0x0, 0x00, 0x00 },    /* 0              3   */
> +       { 0x0, 0x23, 0x00 },    /* 1              0   */
> +       { 0x0, 0x2B, 0x09 },    /* 1              1   */
> +       { 0x0, 0x2E, 0x11 },    /* 1              2   */
> +       { 0x0, 0x2F, 0x00 },    /* 2              0   */
> +       { 0x0, 0x33, 0x0C },    /* 2              1   */
> +       { 0x0, 0x00, 0x00 },    /* 3              0   */
> +};
> +
>  static const struct ddi_buf_trans *
>  bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
>  {
> -- 
> 2.14.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Zanoni, Paulo R April 10, 2018, 8:39 p.m. UTC | #2
Em Ter, 2018-04-10 às 09:51 +0100, Chris Wilson escreveu:
> Quoting Paulo Zanoni (2018-03-23 17:24:16)
> > From: Manasi Navare <manasi.d.navare@intel.com>
> > 
> > This table is used for voltage swing programming sequence during
> > DDI
> > Buffer initialization for MG PHY DDI Buffers on Icelake.
> 
> Except it is not used at all...

It's going to be used later in the series.

> 
> drivers/gpu/drm/i915/intel_ddi.c:601:46: error:
> ‘icl_mg_phy_ddi_translations’ defined but not used [-Werror=unused-
> const-variable=]

I have all of the I915 debugging options enabled, including
CONFIG_DRM_I915_WERROR=y, and I don't get this error. I'm using Fedora
27's gcc. Would it be the case that you have somehow enabled the
unused-const-variable warning through some non-traditional way that's
not upstream?

If that's the case, and if we decide that we want Werror=unused-const-
variable to block patches from being merged, then I think we should put
this error/warning under the i915 debugging .config options and force
CI to also use them and tell us about them.

Because I'm pretty sure if we start enabling random gcc warning/error
flags we'll be able to block a huge number of patches from being
upstreamed. I just don't think this is something we should do.


>  static const struct icl_mg_phy_ddi_buf_trans
> icl_mg_phy_ddi_translations[] = {
>                                               ^~~~~~~~~~~~~~~~~~~~~~~
> ~~~~
> drivers/gpu/drm/i915/intel_ddi.c:581:49: error:
> ‘icl_combo_phy_ddi_translations_edp_1_05V’ defined but not used [-
> Werror=unused-const-variable=]
>  static const struct icl_combo_phy_ddi_buf_trans
> icl_combo_phy_ddi_translations_edp_1_05V[] = {
>                                                  ^~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/intel_ddi.c:565:49: error:
> ‘icl_combo_phy_ddi_translations_dp_hdmi_1_05V’ defined but not used
> [-Werror=unused-const-variable=]
>  static const struct icl_combo_phy_ddi_buf_trans
> icl_combo_phy_ddi_translations_dp_hdmi_1_05V[] = {
>                                                  ^~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/intel_ddi.c:550:49: error:
> ‘icl_combo_phy_ddi_translations_edp_0_95V’ defined but not used [-
> Werror=unused-const-variable=]
>  static const struct icl_combo_phy_ddi_buf_trans
> icl_combo_phy_ddi_translations_edp_0_95V[] = {
>                                                  ^~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/intel_ddi.c:534:49: error:
> ‘icl_combo_phy_ddi_translations_dp_hdmi_0_95V’ defined but not used
> [-Werror=unused-const-variable=]
>  static const struct icl_combo_phy_ddi_buf_trans
> icl_combo_phy_ddi_translations_dp_hdmi_0_95V[] = {
>                                                  ^~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/intel_ddi.c:519:49: error:
> ‘icl_combo_phy_ddi_translations_edp_0_85V’ defined but not used [-
> Werror=unused-const-variable=]
>  static const struct icl_combo_phy_ddi_buf_trans
> icl_combo_phy_ddi_translations_edp_0_85V[] = {
>                                                  ^~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/intel_ddi.c:503:49: error:
> ‘icl_combo_phy_ddi_translations_dp_hdmi_0_85V’ defined but not used
> [-Werror=unused-const-variable=]
>  static const struct icl_combo_phy_ddi_buf_trans
> icl_combo_phy_ddi_translations_dp_hdmi_0_85V[] = {
>                                                  ^~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> 
> > v2 (from Paulo):
> > * Fix white space issues.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 229b9d5250c4..359acbfec4b1 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -592,6 +592,26 @@ static const struct
> > icl_combo_phy_ddi_buf_trans icl_combo_phy_ddi_translations_e
> >         { 0x0, 0x00, 0x00 },    /* 350         0.0   */
> >  };
> >  
> > +struct icl_mg_phy_ddi_buf_trans {
> > +       u32 cri_txdeemph_override_5_0;
> > +       u32 cri_txdeemph_override_11_6;
> > +       u32 cri_txdeemph_override_17_12;
> > +};
> > +
> > +static const struct icl_mg_phy_ddi_buf_trans
> > icl_mg_phy_ddi_translations[] = {
> > +                               /* Voltage swing  pre-emphasis */
> > +       { 0x0, 0x1B, 0x00 },    /* 0              0   */
> > +       { 0x0, 0x23, 0x08 },    /* 0              1   */
> > +       { 0x0, 0x2D, 0x12 },    /* 0              2   */
> > +       { 0x0, 0x00, 0x00 },    /* 0              3   */
> > +       { 0x0, 0x23, 0x00 },    /* 1              0   */
> > +       { 0x0, 0x2B, 0x09 },    /* 1              1   */
> > +       { 0x0, 0x2E, 0x11 },    /* 1              2   */
> > +       { 0x0, 0x2F, 0x00 },    /* 2              0   */
> > +       { 0x0, 0x33, 0x0C },    /* 2              1   */
> > +       { 0x0, 0x00, 0x00 },    /* 3              0   */
> > +};
> > +
> >  static const struct ddi_buf_trans *
> >  bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int
> > *n_entries)
> >  {
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson April 10, 2018, 9:01 p.m. UTC | #3
Quoting Paulo Zanoni (2018-04-10 21:39:31)
> Em Ter, 2018-04-10 às 09:51 +0100, Chris Wilson escreveu:
> > Quoting Paulo Zanoni (2018-03-23 17:24:16)
> > > From: Manasi Navare <manasi.d.navare@intel.com>
> > > 
> > > This table is used for voltage swing programming sequence during
> > > DDI
> > > Buffer initialization for MG PHY DDI Buffers on Icelake.
> > 
> > Except it is not used at all...
> 
> It's going to be used later in the series.
> 
> > 
> > drivers/gpu/drm/i915/intel_ddi.c:601:46: error:
> > ‘icl_mg_phy_ddi_translations’ defined but not used [-Werror=unused-
> > const-variable=]
> 
> I have all of the I915 debugging options enabled, including
> CONFIG_DRM_I915_WERROR=y, and I don't get this error. I'm using Fedora
> 27's gcc. Would it be the case that you have somehow enabled the
> unused-const-variable warning through some non-traditional way that's
> not upstream?

Try make W=1 (which includes kerneldoc checks!). Or clang (don't try
clang unless you are a masochist, especially not right now as -Wvla
upsets it).
 
> If that's the case, and if we decide that we want Werror=unused-const-
> variable to block patches from being merged, then I think we should put
> this error/warning under the i915 debugging .config options and force
> CI to also use them and tell us about them.

I'm trying to get W=1 as part of the pre-merge warning set, at the same
level of severity as ignoring checkpatch.
 
> Because I'm pretty sure if we start enabling random gcc warning/error
> flags we'll be able to block a huge number of patches from being
> upstreamed. I just don't think this is something we should do.

We do. Our code is clean at W=1 except for the odd mistake. Let's keep
it that way. As new gcc warnings are developed, we will squash petty
nuisances and sometimes outright bugs from the code (it has happened
before and will happen again).
-Chris
Chris Wilson April 10, 2018, 9:07 p.m. UTC | #4
Quoting Chris Wilson (2018-04-10 22:01:33)
> Quoting Paulo Zanoni (2018-04-10 21:39:31)
> > Em Ter, 2018-04-10 às 09:51 +0100, Chris Wilson escreveu:
> > > Quoting Paulo Zanoni (2018-03-23 17:24:16)
> > > > From: Manasi Navare <manasi.d.navare@intel.com>
> > > > 
> > > > This table is used for voltage swing programming sequence during
> > > > DDI
> > > > Buffer initialization for MG PHY DDI Buffers on Icelake.
> > > 
> > > Except it is not used at all...
> > 
> > It's going to be used later in the series.

In the context of this patch in merged series, it means that we have
added unexecuted, untested code. Please justify that. (Unused data
tables have in the past remain unused, leaving the code using old
tables; a bug that would have been caught by the compiler.)

We know our test coverage is lacking, this makes it worse. Yes, it's
icl, yes we won't even have sim coverage for the entire set of freq
tables, so what's stopping us from writing unittests to double check the
code at least conforms to our expectations and is exercised?
-Chris
Zanoni, Paulo R April 10, 2018, 9:17 p.m. UTC | #5
Em Ter, 2018-04-10 às 22:07 +0100, Chris Wilson escreveu:
> Quoting Chris Wilson (2018-04-10 22:01:33)
> > Quoting Paulo Zanoni (2018-04-10 21:39:31)
> > > Em Ter, 2018-04-10 às 09:51 +0100, Chris Wilson escreveu:
> > > > Quoting Paulo Zanoni (2018-03-23 17:24:16)
> > > > > From: Manasi Navare <manasi.d.navare@intel.com>
> > > > > 
> > > > > This table is used for voltage swing programming sequence
> > > > > during
> > > > > DDI
> > > > > Buffer initialization for MG PHY DDI Buffers on Icelake.
> > > > 
> > > > Except it is not used at all...
> > > 
> > > It's going to be used later in the series.
> 
> In the context of this patch in merged series, it means that we have
> added unexecuted, untested code. Please justify that. (Unused data
> tables have in the past remain unused, leaving the code using old
> tables; a bug that would have been caught by the compiler.)
> 
> We know our test coverage is lacking, this makes it worse. Yes, it's
> icl, yes we won't even have sim coverage for the entire set of freq
> tables, so what's stopping us from writing unittests to double check
> the
> code at least conforms to our expectations and is exercised?

We have properly tested this code and verified it works. It's just that
we didn't upstream the whole series yet since it's too big. Almost 100
ICL patches already made it to the list already, and about 60 were
merged, but the specific patch that uses that table is not here yet.
I'll take the blame for missing that patch from this series.

> -Chris
Zanoni, Paulo R April 10, 2018, 9:29 p.m. UTC | #6
Em Ter, 2018-04-10 às 22:01 +0100, Chris Wilson escreveu:
> Quoting Paulo Zanoni (2018-04-10 21:39:31)
> > Em Ter, 2018-04-10 às 09:51 +0100, Chris Wilson escreveu:
> > > Quoting Paulo Zanoni (2018-03-23 17:24:16)
> > > > From: Manasi Navare <manasi.d.navare@intel.com>
> > > > 
> > > > This table is used for voltage swing programming sequence
> > > > during
> > > > DDI
> > > > Buffer initialization for MG PHY DDI Buffers on Icelake.
> > > 
> > > Except it is not used at all...
> > 
> > It's going to be used later in the series.
> > 
> > > 
> > > drivers/gpu/drm/i915/intel_ddi.c:601:46: error:
> > > ‘icl_mg_phy_ddi_translations’ defined but not used [-
> > > Werror=unused-
> > > const-variable=]
> > 
> > I have all of the I915 debugging options enabled, including
> > CONFIG_DRM_I915_WERROR=y, and I don't get this error. I'm using
> > Fedora
> > 27's gcc. Would it be the case that you have somehow enabled the
> > unused-const-variable warning through some non-traditional way
> > that's
> > not upstream?
> 
> Try make W=1 (which includes kerneldoc checks!). Or clang (don't try
> clang unless you are a masochist, especially not right now as -Wvla
> upsets it).
>  
> > If that's the case, and if we decide that we want Werror=unused-
> > const-
> > variable to block patches from being merged, then I think we should
> > put
> > this error/warning under the i915 debugging .config options and
> > force
> > CI to also use them and tell us about them.
> 
> I'm trying to get W=1 as part of the pre-merge warning set, at the
> same
> level of severity as ignoring checkpatch.

Good. Perhaps some CONFIG_I915_DEBUG_SOMETHING option could also force
W=1 for us so developers get it for free?

>  
> > Because I'm pretty sure if we start enabling random gcc
> > warning/error
> > flags we'll be able to block a huge number of patches from being
> > upstreamed. I just don't think this is something we should do.
> 
> We do. Our code is clean at W=1 except for the odd mistake.

Ok, I'm convinced. I'll add W=1 to my compilation script. If you want
we can revert this patch and I'll squash it to the patch that actually
uses the table.


>  Let's keep
> it that way. As new gcc warnings are developed, we will squash petty
> nuisances and sometimes outright bugs from the code (it has happened
> before and will happen again).
> -Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 229b9d5250c4..359acbfec4b1 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -592,6 +592,26 @@  static const struct icl_combo_phy_ddi_buf_trans icl_combo_phy_ddi_translations_e
 	{ 0x0, 0x00, 0x00 },	/* 350         0.0   */
 };
 
+struct icl_mg_phy_ddi_buf_trans {
+	u32 cri_txdeemph_override_5_0;
+	u32 cri_txdeemph_override_11_6;
+	u32 cri_txdeemph_override_17_12;
+};
+
+static const struct icl_mg_phy_ddi_buf_trans icl_mg_phy_ddi_translations[] = {
+				/* Voltage swing  pre-emphasis */
+	{ 0x0, 0x1B, 0x00 },	/* 0              0   */
+	{ 0x0, 0x23, 0x08 },	/* 0              1   */
+	{ 0x0, 0x2D, 0x12 },	/* 0              2   */
+	{ 0x0, 0x00, 0x00 },	/* 0              3   */
+	{ 0x0, 0x23, 0x00 },	/* 1              0   */
+	{ 0x0, 0x2B, 0x09 },	/* 1              1   */
+	{ 0x0, 0x2E, 0x11 },	/* 1              2   */
+	{ 0x0, 0x2F, 0x00 },	/* 2              0   */
+	{ 0x0, 0x33, 0x0C },	/* 2              1   */
+	{ 0x0, 0x00, 0x00 },	/* 3              0   */
+};
+
 static const struct ddi_buf_trans *
 bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
 {