| Message ID | 20180323172419.24911-5-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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 --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) {