| Message ID | 1407495226-12620-2-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 08, 2014 at 04:23:40PM +0530, sonika.jindal@intel.com wrote: > From: Sonika Jindal <sonika.jindal@intel.com> > > Adding new defines, older one will be removed in the last patch in the series. > This is to rename the defines to have levels instead of values for vswing and > pre-emph levels as the values may differ in other scenarios like low vswing of > eDP1.4 where the values are different. > > Done using following cocci patch for each define: > @@ > @@ > > # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) > + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) > > ... > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
On Fri, Aug 08, 2014 at 04:23:40PM +0530, sonika.jindal@intel.com wrote: > From: Sonika Jindal <sonika.jindal@intel.com> > > Adding new defines, older one will be removed in the last patch in the series. > This is to rename the defines to have levels instead of values for vswing and > pre-emph levels as the values may differ in other scenarios like low vswing of > eDP1.4 where the values are different. > > Done using following cocci patch for each define: > @@ > @@ > > # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) > + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) Could this perhaps be simply: #define DP_TRAIN_VOLTAGE_SWING(x) ((x) << 0) As it is, there's no information about the value within the symbolic name anyway, so _LEVEL_* really isn't that useful and keeping several macros for each value seems isn't either. An alternative would be to provide a second set of defines for eDP 1.4 where the name implies the meaning and then use them as appropriate. Thierry
On 8/26/2014 4:58 PM, Thierry Reding wrote: > On Fri, Aug 08, 2014 at 04:23:40PM +0530, sonika.jindal@intel.com wrote: >> From: Sonika Jindal <sonika.jindal@intel.com> >> >> Adding new defines, older one will be removed in the last patch in the series. >> This is to rename the defines to have levels instead of values for vswing and >> pre-emph levels as the values may differ in other scenarios like low vswing of >> eDP1.4 where the values are different. >> >> Done using following cocci patch for each define: >> @@ >> @@ >> >> # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) >> + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) > > Could this perhaps be simply: > > #define DP_TRAIN_VOLTAGE_SWING(x) ((x) << 0) > > As it is, there's no information about the value within the symbolic > name anyway, so _LEVEL_* really isn't that useful and keeping several > macros for each value seems isn't either. > I feel _LEVEL_* makes it more readable and since there are only 4 values possible, it is ok to have 4 different macros for readability purpose. What do you think? > An alternative would be to provide a second set of defines for eDP 1.4 > where the name implies the meaning and then use them as appropriate. > > Thierry >
On Tue, Aug 26, 2014 at 01:28:19PM +0200, Thierry Reding wrote: > On Fri, Aug 08, 2014 at 04:23:40PM +0530, sonika.jindal@intel.com wrote: > > From: Sonika Jindal <sonika.jindal@intel.com> > > > > Adding new defines, older one will be removed in the last patch in the series. > > This is to rename the defines to have levels instead of values for vswing and > > pre-emph levels as the values may differ in other scenarios like low vswing of > > eDP1.4 where the values are different. > > > > Done using following cocci patch for each define: > > @@ > > @@ > > > > # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) > > + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) > > Could this perhaps be simply: > > #define DP_TRAIN_VOLTAGE_SWING(x) ((x) << 0) > > As it is, there's no information about the value within the symbolic > name anyway, so _LEVEL_* really isn't that useful and keeping several > macros for each value seems isn't either. The _LEVEL_ part is quite important IMHO, that's what changes between those different defines, controlling a level shifter, somewhere. So we're left with #define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) Vs #define DP_TRAIN_VOLTAGE_SWING_LEVEL(x) ((x) << 0) The second variant doesn't really bring much more clarity? Can we just go with the first? > An alternative would be to provide a second set of defines for eDP 1.4 > where the name implies the meaning and then use them as appropriate. We went through the idea as well and: I actually think the nominal voltage swing and pre-emph values are quite misleading. The hw is free to implement a wildly different set of voltage swing/pre-emph values. eDP 1.4 changes those nominal values as described in the cover letter, but there again, the actual hw implementation can choose fairly different values than the nominal ones. Also, the DP 1.2 spec documents this field as (see address 103h): TRAINING_LANE0_SET : Link Training Control_Lane0 Bits 1:0 = VOLTAGE SWING SET 00 –Voltage swing level 0 01 –Voltage swing level 1 10 –Voltage swing level 2 11 –Voltage swing level 3 So, in that sense, we're closer to the latest spec with those LEVEL_X defines. HTH,
On Wed, Aug 27, 2014 at 08:47:54AM +0100, Damien Lespiau wrote: > > An alternative would be to provide a second set of defines for eDP 1.4 > > where the name implies the meaning and then use them as appropriate. > > We went through the idea as well and: > > I actually think the nominal voltage swing and pre-emph values are quite > misleading. The hw is free to implement a wildly different set of voltage > swing/pre-emph values. > > eDP 1.4 changes those nominal values as described in the cover letter, > but there again, the actual hw implementation can choose fairly > different values than the nominal ones. > > Also, the DP 1.2 spec documents this field as (see address 103h): > > TRAINING_LANE0_SET : Link Training Control_Lane0 > Bits 1:0 = VOLTAGE SWING SET > 00 –Voltage swing level 0 > 01 –Voltage swing level 1 > 10 –Voltage swing level 2 > 11 –Voltage swing level 3 > > So, in that sense, we're closer to the latest spec with those LEVEL_X > defines. I forgot to mention here that if we have separate defines for eDP 1.4, then we lose the possibility to share training code with big DP and eDP 1.3, not something desirable.
On Wed, Aug 27, 2014 at 08:51:35AM +0100, Damien Lespiau wrote: > On Wed, Aug 27, 2014 at 08:47:54AM +0100, Damien Lespiau wrote: > > > An alternative would be to provide a second set of defines for eDP 1.4 > > > where the name implies the meaning and then use them as appropriate. > > > > We went through the idea as well and: > > > > I actually think the nominal voltage swing and pre-emph values are quite > > misleading. The hw is free to implement a wildly different set of voltage > > swing/pre-emph values. > > > > eDP 1.4 changes those nominal values as described in the cover letter, > > but there again, the actual hw implementation can choose fairly > > different values than the nominal ones. > > > > Also, the DP 1.2 spec documents this field as (see address 103h): > > > > TRAINING_LANE0_SET : Link Training Control_Lane0 > > Bits 1:0 = VOLTAGE SWING SET > > 00 –Voltage swing level 0 > > 01 –Voltage swing level 1 > > 10 –Voltage swing level 2 > > 11 –Voltage swing level 3 > > > > So, in that sense, we're closer to the latest spec with those LEVEL_X > > defines. > > I forgot to mention here that if we have separate defines for eDP 1.4, > then we lose the possibility to share training code with big DP and eDP > 1.3, not something desirable. Yeah, I'd like to see the training sequences extracted into common helpers. Thierry
On Wed, Aug 27, 2014 at 08:47:54AM +0100, Damien Lespiau wrote: > On Tue, Aug 26, 2014 at 01:28:19PM +0200, Thierry Reding wrote: > > On Fri, Aug 08, 2014 at 04:23:40PM +0530, sonika.jindal@intel.com wrote: > > > From: Sonika Jindal <sonika.jindal@intel.com> > > > > > > Adding new defines, older one will be removed in the last patch in the series. > > > This is to rename the defines to have levels instead of values for vswing and > > > pre-emph levels as the values may differ in other scenarios like low vswing of > > > eDP1.4 where the values are different. > > > > > > Done using following cocci patch for each define: > > > @@ > > > @@ > > > > > > # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) > > > + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) > > > > Could this perhaps be simply: > > > > #define DP_TRAIN_VOLTAGE_SWING(x) ((x) << 0) > > > > As it is, there's no information about the value within the symbolic > > name anyway, so _LEVEL_* really isn't that useful and keeping several > > macros for each value seems isn't either. > > The _LEVEL_ part is quite important IMHO, that's what changes between those > different defines, controlling a level shifter, somewhere. > > So we're left with > > #define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) > > Vs > > #define DP_TRAIN_VOLTAGE_SWING_LEVEL(x) ((x) << 0) > > The second variant doesn't really bring much more clarity? Can we just > go with the first? I think the parameterized version is more convenient, especially if you want to use that during training sequences and iterate over the levels. But I don't feel too strongly about it, so either way is fine with me. Thierry
On Wednesday, August 27, 2014 1:31 PM, Jindal, Sonika wrote: > On 8/26/2014 4:58 PM, Thierry Reding wrote: > > On Fri, Aug 08, 2014 at 04:23:40PM +0530, sonika.jindal@intel.com wrote: > >> From: Sonika Jindal <sonika.jindal@intel.com> > >> > >> Adding new defines, older one will be removed in the last patch in the series. > >> This is to rename the defines to have levels instead of values for vswing and > >> pre-emph levels as the values may differ in other scenarios like low vswing of > >> eDP1.4 where the values are different. > >> > >> Done using following cocci patch for each define: > >> @@ > >> @@ > >> > >> # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) > >> + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) > > > > Could this perhaps be simply: > > > > #define DP_TRAIN_VOLTAGE_SWING(x) ((x) << 0) > > > > As it is, there's no information about the value within the symbolic > > name anyway, so _LEVEL_* really isn't that useful and keeping several > > macros for each value seems isn't either. > > > I feel _LEVEL_* makes it more readable and since there are only 4 values > possible, it is ok to have 4 different macros for readability purpose. > What do you think? (+cc Damien Lespiau) Personally, I also think that LEVEL_* looks more readable. Best regards, Jingoo Han > > An alternative would be to provide a second set of defines for eDP 1.4 > > where the name implies the meaning and then use them as appropriate. > > > > Thierry > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Aug 27, 2014 at 03:11:08PM +0200, Thierry Reding wrote: > > So we're left with > > > > #define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) > > > > Vs > > > > #define DP_TRAIN_VOLTAGE_SWING_LEVEL(x) ((x) << 0) > > > > The second variant doesn't really bring much more clarity? Can we just > > go with the first? > > I think the parameterized version is more convenient, especially if you > want to use that during training sequences and iterate over the levels. That's a fair point, but today's code manages to do without that nicety. I think these kind of refinements could go in series with code actually using them on top. > But I don't feel too strongly about it, so either way is fine with me. Thanks, taking some of your time to provide feedback is always appreciated!
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index a21568b..3840a05 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -191,15 +191,23 @@ # define DP_TRAIN_VOLTAGE_SWING_SHIFT 0 # define DP_TRAIN_MAX_SWING_REACHED (1 << 2) # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) # define DP_TRAIN_VOLTAGE_SWING_600 (1 << 0) +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_1 (1 << 0) # define DP_TRAIN_VOLTAGE_SWING_800 (2 << 0) +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_2 (2 << 0) # define DP_TRAIN_VOLTAGE_SWING_1200 (3 << 0) +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_3 (3 << 0) # define DP_TRAIN_PRE_EMPHASIS_MASK (3 << 3) # define DP_TRAIN_PRE_EMPHASIS_0 (0 << 3) +# define DP_TRAIN_PRE_EMPH_LEVEL_0 (0 << 3) # define DP_TRAIN_PRE_EMPHASIS_3_5 (1 << 3) +# define DP_TRAIN_PRE_EMPH_LEVEL_1 (1 << 3) # define DP_TRAIN_PRE_EMPHASIS_6 (2 << 3) +# define DP_TRAIN_PRE_EMPH_LEVEL_2 (2 << 3) # define DP_TRAIN_PRE_EMPHASIS_9_5 (3 << 3) +# define DP_TRAIN_PRE_EMPH_LEVEL_3 (3 << 3) # define DP_TRAIN_PRE_EMPHASIS_SHIFT 3 # define DP_TRAIN_MAX_PRE_EMPHASIS_REACHED (1 << 5)