| Message ID | 20180611222655.5696-1-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Seg, 2018-06-11 às 22:35 +0000, Patchwork escreveu: > == Series Details == > > Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP > rates for Icelake > URL : https://patchwork.freedesktop.org/series/44595/ > State : warning > > == Summary == > > $ dim checkpatch origin/drm-tip > e6e6b2f7af58 drm/i915/icl: Add allowed DP rates for Icelake > 3fe43cb729fe drm/i915/dp: Add support for HBR3 and TPS4 during link > training > -:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > #26: FILE: drivers/gpu/drm/i915/i915_reg.h:8694: > +#define DP_TP_CTL_LINK_TRAIN_PAT4 (5<<8) Dear maintainers, I get this type of error way too often. What's the most desirable thing here? 1 - Make it "(5 << 8)" so checkpatch doesn't complain, which will leave the coding style inconsistent with the surrounding lines. 2 - Drive-by fix all the bits around it so everybody in the same definition has nice spaces, 2.a: in the same patch, 2.b: in a separate patch. 3 - Just ignore the checkpatch message, push code as-is. 4 - Blacklist this check from checkpatch. 5 - Submit a separate patch fixing all the spacing errors on i915_reg.h once and for all. Live happily ever after. 6 - Submit a separate patch converting everything to BIT() on i915_reg.h. Thanks, Paulo > ^ > > total: 0 errors, 0 warnings, 1 checks, 127 lines checked >
On Mon, 11 Jun 2018, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: > Em Seg, 2018-06-11 às 22:35 +0000, Patchwork escreveu: >> == Series Details == >> >> Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP >> rates for Icelake >> URL : https://patchwork.freedesktop.org/series/44595/ >> State : warning >> >> == Summary == >> >> $ dim checkpatch origin/drm-tip >> e6e6b2f7af58 drm/i915/icl: Add allowed DP rates for Icelake >> 3fe43cb729fe drm/i915/dp: Add support for HBR3 and TPS4 during link >> training >> -:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) >> #26: FILE: drivers/gpu/drm/i915/i915_reg.h:8694: >> +#define DP_TP_CTL_LINK_TRAIN_PAT4 (5<<8) > > Dear maintainers, > > I get this type of error way too often. What's the most desirable thing > here? > > 1 - Make it "(5 << 8)" so checkpatch doesn't complain, which will leave > the coding style inconsistent with the surrounding lines. I don't like the inconsistency. > 2 - Drive-by fix all the bits around it so everybody in the same > definition has nice spaces, 2.a: in the same patch, 2.b: in a separate > patch. Fine by me. Both a and b. I was kind of hoping this would have happened more. > 3 - Just ignore the checkpatch message, push code as-is. Also fine by me. > 4 - Blacklist this check from checkpatch. Unfortunately the SPACING class in checkpatch would silence much, much more than just this specific thing, so it would be a net negative. > 5 - Submit a separate patch fixing all the spacing errors on i915_reg.h > once and for all. Live happily ever after. It would be annoying for a while with conflicts, but I'd be fine. Not sure if it would be better to do it in some arbitrary chunks rather than mass change. > 6 - Submit a separate patch converting everything to BIT() on > i915_reg.h. Same as above. BR, Jani.
On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote: > From: Manasi Navare <manasi.d.navare@intel.com> > > For ICL, on Combo PHY the allowed max rates are: > - HBR3 8.1 eDP (DDIA) > - HBR2 5.4 DisplayPort (DDIB) > and for MG PHY/TC DDI Ports allowed DP rates are: > - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT, > - DP on legacy connector - DDIC/D/E/F) > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Reviewed-by: James Ausmus <james.ausmus@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > Signed-off-by: James Ausmus <james.ausmus@intel.com> > [Paulo: bikeshed to keep future platforms on "else".] > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 37b9f62aeb6e..8371159cc192 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct intel_dp *intel_dp) > return 810000; > } > > +static int icl_max_source_rate(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > + enum port port = dig_port->base.port; > + > + /* On Combo PHY port A max speed is HBR3 for all Vccio voltages > + * and on Combo PHY Port B the maximum supported is HBR2. > + */ And what about the other ports? If port B is the only exception why are we even discussing port A specifically here? > + if (port == PORT_B) > + return 540000; > + > + return 810000; > +} > + > static void > intel_dp_set_source_rates(struct intel_dp *intel_dp) > { > @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > /* This should only be done once */ > WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates); > > - if (IS_CANNONLAKE(dev_priv)) { > + if (INTEL_GEN(dev_priv) >= 10) { > source_rates = cnl_rates; > size = ARRAY_SIZE(cnl_rates); > - max_rate = cnl_max_source_rate(intel_dp); > + if (INTEL_GEN(dev_priv) == 10) > + max_rate = cnl_max_source_rate(intel_dp); > + else > + max_rate = icl_max_source_rate(intel_dp); > } else if (IS_GEN9_LP(dev_priv)) { > source_rates = bxt_rates; > size = ARRAY_SIZE(bxt_rates); > -- > 2.14.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jun 12, 2018 at 03:15:53PM +0300, Ville Syrjälä wrote: > On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote: > > From: Manasi Navare <manasi.d.navare@intel.com> > > > > For ICL, on Combo PHY the allowed max rates are: > > - HBR3 8.1 eDP (DDIA) > > - HBR2 5.4 DisplayPort (DDIB) > > and for MG PHY/TC DDI Ports allowed DP rates are: > > - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT, > > - DP on legacy connector - DDIC/D/E/F) > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Reviewed-by: James Ausmus <james.ausmus@intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > Signed-off-by: James Ausmus <james.ausmus@intel.com> > > [Paulo: bikeshed to keep future platforms on "else".] > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++-- > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 37b9f62aeb6e..8371159cc192 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct intel_dp *intel_dp) > > return 810000; > > } > > > > +static int icl_max_source_rate(struct intel_dp *intel_dp) > > +{ > > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > + enum port port = dig_port->base.port; > > + > > + /* On Combo PHY port A max speed is HBR3 for all Vccio voltages > > + * and on Combo PHY Port B the maximum supported is HBR2. > > + */ > > And what about the other ports? If port B is the only > exception why are we even discussing port A specifically > here? All the MG PHY ports (C/D/E/F) support a max of HBR3 that is 810000 but for Combo PHY ports which is Port A or B, HBR3 only supported for Port A but for Port B it is max of HBR2 which is 540000 hence the comment for Combo PHY ports and if port B then just return HBR2 Manasi > > > + if (port == PORT_B) > > + return 540000; > > + > > + return 810000; > > +} > > + > > static void > > intel_dp_set_source_rates(struct intel_dp *intel_dp) > > { > > @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > > /* This should only be done once */ > > WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates); > > > > - if (IS_CANNONLAKE(dev_priv)) { > > + if (INTEL_GEN(dev_priv) >= 10) { > > source_rates = cnl_rates; > > size = ARRAY_SIZE(cnl_rates); > > - max_rate = cnl_max_source_rate(intel_dp); > > + if (INTEL_GEN(dev_priv) == 10) > > + max_rate = cnl_max_source_rate(intel_dp); > > + else > > + max_rate = icl_max_source_rate(intel_dp); > > } else if (IS_GEN9_LP(dev_priv)) { > > source_rates = bxt_rates; > > size = ARRAY_SIZE(bxt_rates); > > -- > > 2.14.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jun 12, 2018 at 11:46:08AM +0300, Jani Nikula wrote: > On Mon, 11 Jun 2018, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: > > Em Seg, 2018-06-11 às 22:35 +0000, Patchwork escreveu: > >> == Series Details == > >> > >> Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP > >> rates for Icelake > >> URL : https://patchwork.freedesktop.org/series/44595/ > >> State : warning > >> > >> == Summary == > >> > >> $ dim checkpatch origin/drm-tip > >> e6e6b2f7af58 drm/i915/icl: Add allowed DP rates for Icelake > >> 3fe43cb729fe drm/i915/dp: Add support for HBR3 and TPS4 during link > >> training > >> -:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > >> #26: FILE: drivers/gpu/drm/i915/i915_reg.h:8694: > >> +#define DP_TP_CTL_LINK_TRAIN_PAT4 (5<<8) > > > > Dear maintainers, > > > > I get this type of error way too often. What's the most desirable thing > > here? > > > > 1 - Make it "(5 << 8)" so checkpatch doesn't complain, which will leave > > the coding style inconsistent with the surrounding lines. > > I don't like the inconsistency. me neither... > > > 2 - Drive-by fix all the bits around it so everybody in the same > > definition has nice spaces, 2.a: in the same patch, 2.b: in a separate > > patch. > > Fine by me. Both a and b. I was kind of hoping this would have happened > more. > > > 3 - Just ignore the checkpatch message, push code as-is. > > Also fine by me. what I'm currently doing... > > > 4 - Blacklist this check from checkpatch. > > Unfortunately the SPACING class in checkpatch would silence much, much > more than just this specific thing, so it would be a net negative. Let's keep the style we want there even if this cause warnings while we haven't finished the standardization. > > > 5 - Submit a separate patch fixing all the spacing errors on i915_reg.h > > once and for all. Live happily ever after. > > It would be annoying for a while with conflicts, but I'd be fine. Not > sure if it would be better to do it in some arbitrary chunks rather than > mass change. I believe I prefer one mass commit. So we convert once for all and cause rebase conflict on internal branch only once. So we solve all at one and be happy... > > > 6 - Submit a separate patch converting everything to BIT() on > > i915_reg.h. > > Same as above. Do we really want BIT everywhere?! Thanks, Rodrigo. > > BR, > Jani. > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 12 Jun 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Do we really want BIT everywhere?!
I think I'd go for everywhere except part of a register field value:
#define SINGLE_BIT_OKAY BIT(25)
#define FIELD_SHIFT 20
#define FIELD_MASK (0xf << 20)
#define FIELD_FOO_PLEASE_NO BIT(20) /* Don't do this */
#define FIELD_FOO (1 << 20) /* This is consistent */
#define FIELD_BAR (2 << 20)
#define FIELD_BAZ (3 << 20)
BR,
Jani.
Em Qua, 2018-06-13 às 11:07 +0300, Jani Nikula escreveu: > On Tue, 12 Jun 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > Do we really want BIT everywhere?! > > I think I'd go for everywhere except part of a register field value: > While I completely agree with your reasoning, this means we'll kinda always want to blacklist the BIT_MACRO checkpath type because checkpatch won't know about these exceptions, which means we won't actually need to convert everything to BIT() since no false negative emails anyway. Anyway, I submitted a patch to fix the spacing issues, I'd love to have some comments from the maintainers on it. Thanks, Paulo > #define SINGLE_BIT_OKAY BIT(25) > #define FIELD_SHIFT 20 > #define FIELD_MASK (0xf << 20) > #define FIELD_FOO_PLEASE_NO BIT(20) /* Don't do > this */ > #define FIELD_FOO (1 << 20) /* This is > consistent */ > #define FIELD_BAR (2 << 20) > #define FIELD_BAZ (3 << 20) > > > BR, > Jani. >
Em Ter, 2018-06-12 às 11:37 -0700, Manasi Navare escreveu: > On Tue, Jun 12, 2018 at 03:15:53PM +0300, Ville Syrjälä wrote: > > On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote: > > > From: Manasi Navare <manasi.d.navare@intel.com> > > > > > > For ICL, on Combo PHY the allowed max rates are: > > > - HBR3 8.1 eDP (DDIA) > > > - HBR2 5.4 DisplayPort (DDIB) > > > and for MG PHY/TC DDI Ports allowed DP rates are: > > > - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT, > > > - DP on legacy connector - DDIC/D/E/F) > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Reviewed-by: James Ausmus <james.ausmus@intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > Signed-off-by: James Ausmus <james.ausmus@intel.com> > > > [Paulo: bikeshed to keep future platforms on "else".] > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++-- > > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 37b9f62aeb6e..8371159cc192 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct > > > intel_dp *intel_dp) > > > return 810000; > > > } > > > > > > +static int icl_max_source_rate(struct intel_dp *intel_dp) > > > +{ > > > + struct intel_digital_port *dig_port = > > > dp_to_dig_port(intel_dp); > > > + enum port port = dig_port->base.port; > > > + > > > + /* On Combo PHY port A max speed is HBR3 for all Vccio > > > voltages > > > + * and on Combo PHY Port B the maximum supported is > > > HBR2. > > > + */ > > > > And what about the other ports? If port B is the only > > exception why are we even discussing port A specifically > > here? > > All the MG PHY ports (C/D/E/F) support a max of HBR3 that is 810000 > but for > Combo PHY ports which is Port A or B, HBR3 only supported for Port A > but for Port B it is max of HBR2 which is 540000 hence the comment > for Combo PHY > ports and if port B then just return HBR2 I think Ville's point was that having a comment that only discusses ports A and B on code that handles all ports gives the impression that perhaps the code is "forgetting" to consider the other ports, or something like that. Which makes sense to me. Perhaps to address this issue we could either reword the comment to include C-F or simply just remove it, since the commit message should be enough and the comment only says what the code says. > > Manasi > > > > > > + if (port == PORT_B) > > > + return 540000; > > > + > > > + return 810000; > > > +} > > > + > > > static void > > > intel_dp_set_source_rates(struct intel_dp *intel_dp) > > > { > > > @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp > > > *intel_dp) > > > /* This should only be done once */ > > > WARN_ON(intel_dp->source_rates || intel_dp- > > > >num_source_rates); > > > > > > - if (IS_CANNONLAKE(dev_priv)) { > > > + if (INTEL_GEN(dev_priv) >= 10) { > > > source_rates = cnl_rates; > > > size = ARRAY_SIZE(cnl_rates); > > > - max_rate = cnl_max_source_rate(intel_dp); > > > + if (INTEL_GEN(dev_priv) == 10) > > > + max_rate = > > > cnl_max_source_rate(intel_dp); > > > + else > > > + max_rate = > > > icl_max_source_rate(intel_dp); > > > } else if (IS_GEN9_LP(dev_priv)) { > > > source_rates = bxt_rates; > > > size = ARRAY_SIZE(bxt_rates); > > > -- > > > 2.14.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > 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
On Wed, Jun 13, 2018 at 12:42:20PM -0700, Paulo Zanoni wrote: > Em Ter, 2018-06-12 às 11:37 -0700, Manasi Navare escreveu: > > On Tue, Jun 12, 2018 at 03:15:53PM +0300, Ville Syrjälä wrote: > > > On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote: > > > > From: Manasi Navare <manasi.d.navare@intel.com> > > > > > > > > For ICL, on Combo PHY the allowed max rates are: > > > > - HBR3 8.1 eDP (DDIA) > > > > - HBR2 5.4 DisplayPort (DDIB) > > > > and for MG PHY/TC DDI Ports allowed DP rates are: > > > > - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT, > > > > - DP on legacy connector - DDIC/D/E/F) > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > Reviewed-by: James Ausmus <james.ausmus@intel.com> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > Signed-off-by: James Ausmus <james.ausmus@intel.com> > > > > [Paulo: bikeshed to keep future platforms on "else".] > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++-- > > > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index 37b9f62aeb6e..8371159cc192 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct > > > > intel_dp *intel_dp) > > > > return 810000; > > > > } > > > > > > > > +static int icl_max_source_rate(struct intel_dp *intel_dp) > > > > +{ > > > > + struct intel_digital_port *dig_port = > > > > dp_to_dig_port(intel_dp); > > > > + enum port port = dig_port->base.port; > > > > + > > > > + /* On Combo PHY port A max speed is HBR3 for all Vccio > > > > voltages > > > > + * and on Combo PHY Port B the maximum supported is > > > > HBR2. > > > > + */ > > > > > > And what about the other ports? If port B is the only > > > exception why are we even discussing port A specifically > > > here? > > > > All the MG PHY ports (C/D/E/F) support a max of HBR3 that is 810000 > > but for > > Combo PHY ports which is Port A or B, HBR3 only supported for Port A > > but for Port B it is max of HBR2 which is 540000 hence the comment > > for Combo PHY > > ports and if port B then just return HBR2 > > I think Ville's point was that having a comment that only discusses > ports A and B on code that handles all ports gives the impression that > perhaps the code is "forgetting" to consider the other ports, or > something like that. Which makes sense to me. > > Perhaps to address this issue we could either reword the comment to > include C-F or simply just remove it, since the commit message should > be enough and the comment only says what the code says. > Yes I agree. Lets remove the comment in the code since the commit message covers that part. Should I send a new revision for this with comment removed? Manasi > > > > Manasi > > > > > > > > > + if (port == PORT_B) > > > > + return 540000; > > > > + > > > > + return 810000; > > > > +} > > > > + > > > > static void > > > > intel_dp_set_source_rates(struct intel_dp *intel_dp) > > > > { > > > > @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp > > > > *intel_dp) > > > > /* This should only be done once */ > > > > WARN_ON(intel_dp->source_rates || intel_dp- > > > > >num_source_rates); > > > > > > > > - if (IS_CANNONLAKE(dev_priv)) { > > > > + if (INTEL_GEN(dev_priv) >= 10) { > > > > source_rates = cnl_rates; > > > > size = ARRAY_SIZE(cnl_rates); > > > > - max_rate = cnl_max_source_rate(intel_dp); > > > > + if (INTEL_GEN(dev_priv) == 10) > > > > + max_rate = > > > > cnl_max_source_rate(intel_dp); > > > > + else > > > > + max_rate = > > > > icl_max_source_rate(intel_dp); > > > > } else if (IS_GEN9_LP(dev_priv)) { > > > > source_rates = bxt_rates; > > > > size = ARRAY_SIZE(bxt_rates); > > > > -- > > > > 2.14.4 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Ville Syrjälä > > > Intel > > > _______________________________________________ > > > 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
Em Qua, 2018-06-13 às 13:15 -0700, Manasi Navare escreveu: > On Wed, Jun 13, 2018 at 12:42:20PM -0700, Paulo Zanoni wrote: > > Em Ter, 2018-06-12 às 11:37 -0700, Manasi Navare escreveu: > > > On Tue, Jun 12, 2018 at 03:15:53PM +0300, Ville Syrjälä wrote: > > > > On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote: > > > > > From: Manasi Navare <manasi.d.navare@intel.com> > > > > > > > > > > For ICL, on Combo PHY the allowed max rates are: > > > > > - HBR3 8.1 eDP (DDIA) > > > > > - HBR2 5.4 DisplayPort (DDIB) > > > > > and for MG PHY/TC DDI Ports allowed DP rates are: > > > > > - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT, > > > > > - DP on legacy connector - DDIC/D/E/F) > > > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > > Reviewed-by: James Ausmus <james.ausmus@intel.com> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > > Signed-off-by: James Ausmus <james.ausmus@intel.com> > > > > > [Paulo: bikeshed to keep future platforms on "else".] > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++-- > > > > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > index 37b9f62aeb6e..8371159cc192 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct > > > > > intel_dp *intel_dp) > > > > > return 810000; > > > > > } > > > > > > > > > > +static int icl_max_source_rate(struct intel_dp *intel_dp) > > > > > +{ > > > > > + struct intel_digital_port *dig_port = > > > > > dp_to_dig_port(intel_dp); > > > > > + enum port port = dig_port->base.port; > > > > > + > > > > > + /* On Combo PHY port A max speed is HBR3 for all > > > > > Vccio > > > > > voltages > > > > > + * and on Combo PHY Port B the maximum supported is > > > > > HBR2. > > > > > + */ > > > > > > > > And what about the other ports? If port B is the only > > > > exception why are we even discussing port A specifically > > > > here? > > > > > > All the MG PHY ports (C/D/E/F) support a max of HBR3 that is > > > 810000 > > > but for > > > Combo PHY ports which is Port A or B, HBR3 only supported for > > > Port A > > > but for Port B it is max of HBR2 which is 540000 hence the > > > comment > > > for Combo PHY > > > ports and if port B then just return HBR2 > > > > I think Ville's point was that having a comment that only discusses > > ports A and B on code that handles all ports gives the impression > > that > > perhaps the code is "forgetting" to consider the other ports, or > > something like that. Which makes sense to me. > > > > Perhaps to address this issue we could either reword the comment to > > include C-F or simply just remove it, since the commit message > > should > > be enough and the comment only says what the code says. > > > > Yes I agree. Lets remove the comment in the code since the commit > message > covers that part. > Should I send a new revision for this with comment removed? Feel free to do it, otherwise I can do it while applying since it's a trivial change. Thanks, Paulo > > Manasi > > > > > > > Manasi > > > > > > > > > > > > + if (port == PORT_B) > > > > > + return 540000; > > > > > + > > > > > + return 810000; > > > > > +} > > > > > + > > > > > static void > > > > > intel_dp_set_source_rates(struct intel_dp *intel_dp) > > > > > { > > > > > @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct > > > > > intel_dp > > > > > *intel_dp) > > > > > /* This should only be done once */ > > > > > WARN_ON(intel_dp->source_rates || intel_dp- > > > > > > num_source_rates); > > > > > > > > > > > > > > > - if (IS_CANNONLAKE(dev_priv)) { > > > > > + if (INTEL_GEN(dev_priv) >= 10) { > > > > > source_rates = cnl_rates; > > > > > size = ARRAY_SIZE(cnl_rates); > > > > > - max_rate = cnl_max_source_rate(intel_dp); > > > > > + if (INTEL_GEN(dev_priv) == 10) > > > > > + max_rate = > > > > > cnl_max_source_rate(intel_dp); > > > > > + else > > > > > + max_rate = > > > > > icl_max_source_rate(intel_dp); > > > > > } else if (IS_GEN9_LP(dev_priv)) { > > > > > source_rates = bxt_rates; > > > > > size = ARRAY_SIZE(bxt_rates); > > > > > -- > > > > > 2.14.4 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > _______________________________________________ > > > > 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
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 37b9f62aeb6e..8371159cc192 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct intel_dp *intel_dp) return 810000; } +static int icl_max_source_rate(struct intel_dp *intel_dp) +{ + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); + enum port port = dig_port->base.port; + + /* On Combo PHY port A max speed is HBR3 for all Vccio voltages + * and on Combo PHY Port B the maximum supported is HBR2. + */ + if (port == PORT_B) + return 540000; + + return 810000; +} + static void intel_dp_set_source_rates(struct intel_dp *intel_dp) { @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) /* This should only be done once */ WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates); - if (IS_CANNONLAKE(dev_priv)) { + if (INTEL_GEN(dev_priv) >= 10) { source_rates = cnl_rates; size = ARRAY_SIZE(cnl_rates); - max_rate = cnl_max_source_rate(intel_dp); + if (INTEL_GEN(dev_priv) == 10) + max_rate = cnl_max_source_rate(intel_dp); + else + max_rate = icl_max_source_rate(intel_dp); } else if (IS_GEN9_LP(dev_priv)) { source_rates = bxt_rates; size = ARRAY_SIZE(bxt_rates);