| Message ID | 20220917004404.414981-7-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce struct cdclk_step | expand |
On Fri, Sep 16, 2022 at 05:44:04PM -0700, Anusha Srivatsa wrote: > Add a helper function to get stringify values of the > desired cdclk action and dump it with rest of the > cdclk config values > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> Please add Suggested-by: field to give proper credits as per our discussion. This applies to other patches as well to add proper credits to other folks who suggested design changes/ fixes. This needs to be followed as per the upstream patch review methodology. > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index bc627daade3e..12f5e4d23245 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -1688,6 +1688,19 @@ static u32 cdclk_squash_waveform(struct drm_i915_private *dev_priv, > > return 0xffff; > } Missing newline causing checkpatch error Manasi > +static const char *cdclk_sequence_to_string(enum cdclk_sequence cdclk_sequence) > +{ > + switch (cdclk_sequence) { > + case CDCLK_SQUASH_ONLY: > + return "Squash only"; > + case CDCLK_CRAWL_ONLY: > + return "Crawl only"; > + case CDCLK_LEGACY: > + return "Legacy method"; > + default: > + return "Not a valid cdclk sequence"; > + } > +} > > static void dg2_prog_squash_ctl(struct drm_i915_private *i915, u16 waveform) > { > @@ -2083,10 +2096,11 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915, > const struct intel_cdclk_config *cdclk_config, > const char *context) > { > - drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d\n", > + drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d, %s action\n", > context, cdclk_config->cdclk, cdclk_config->vco, > cdclk_config->ref, cdclk_config->bypass, > - cdclk_config->voltage_level); > + cdclk_config->voltage_level, > + cdclk_sequence_to_string(cdclk_config->steps->action)); > } > > /** > -- > 2.25.1 >
On Mon, Sep 19, 2022 at 12:46:45PM -0700, Navare, Manasi wrote: > On Fri, Sep 16, 2022 at 05:44:04PM -0700, Anusha Srivatsa wrote: > > Add a helper function to get stringify values of the > > desired cdclk action and dump it with rest of the > > cdclk config values > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Please add Suggested-by: field to give proper credits as per our > discussion. This applies to other patches as well to add proper credits > to other folks who suggested design changes/ fixes. > This needs to be followed as per the upstream patch review methodology. Having the suggested by is a good way to give the proper credits when the whole big idea and design was co authored, or started by the other developer. We really need to mind about it. However it is not a very good thing for all of the patches we have. Many ideas come out of conversation with many folks and we cannot simply start adding the suggested by list with all the names involved. There's always a threshold there that we should mind. And the official rule as reference is this one: "A Suggested-by: tag indicates that the patch idea is suggested by the person named and ensures credit to the person for the idea. Please note that this tag should not be added without the reporter’s permission, especially if the idea was not posted in a public forum." [1] As a reference we don't keep adding official suggested-by tags for any and all of the suggestions we receive during reviews. The simple name version in the commit message is what we traditionally uses for small things. Looking to the history of this series here I see that this patch looks like an evolution of the previous series with small increment and suggestions from multiple folks. Not sure if it is really worthwhile to have to add the official tag and start to ping everyone to get the ack if it is okay or not to add it. [1] - https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index bc627daade3e..12f5e4d23245 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -1688,6 +1688,19 @@ static u32 cdclk_squash_waveform(struct drm_i915_private *dev_priv, > > > > return 0xffff; > > } > > Missing newline causing checkpatch error > > Manasi > > > +static const char *cdclk_sequence_to_string(enum cdclk_sequence cdclk_sequence) > > +{ > > + switch (cdclk_sequence) { > > + case CDCLK_SQUASH_ONLY: > > + return "Squash only"; > > + case CDCLK_CRAWL_ONLY: > > + return "Crawl only"; > > + case CDCLK_LEGACY: > > + return "Legacy method"; > > + default: > > + return "Not a valid cdclk sequence"; > > + } > > +} > > > > static void dg2_prog_squash_ctl(struct drm_i915_private *i915, u16 waveform) > > { > > @@ -2083,10 +2096,11 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915, > > const struct intel_cdclk_config *cdclk_config, > > const char *context) > > { > > - drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d\n", > > + drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d, %s action\n", > > context, cdclk_config->cdclk, cdclk_config->vco, > > cdclk_config->ref, cdclk_config->bypass, > > - cdclk_config->voltage_level); > > + cdclk_config->voltage_level, > > + cdclk_sequence_to_string(cdclk_config->steps->action)); > > } > > > > /** > > -- > > 2.25.1 > >
On Mon, Sep 19, 2022 at 05:10:47PM -0400, Rodrigo Vivi wrote: > On Mon, Sep 19, 2022 at 12:46:45PM -0700, Navare, Manasi wrote: > > On Fri, Sep 16, 2022 at 05:44:04PM -0700, Anusha Srivatsa wrote: > > > Add a helper function to get stringify values of the > > > desired cdclk action and dump it with rest of the > > > cdclk config values > > > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > Please add Suggested-by: field to give proper credits as per our > > discussion. This applies to other patches as well to add proper credits > > to other folks who suggested design changes/ fixes. > > This needs to be followed as per the upstream patch review methodology. > > Having the suggested by is a good way to give the proper credits when the whole > big idea and design was co authored, or started by the other developer. > > We really need to mind about it. However it is not a very good thing for all > of the patches we have. Many ideas come out of conversation with many folks > and we cannot simply start adding the suggested by list with all the names > involved. > > There's always a threshold there that we should mind. And the official rule > as reference is this one: > > "A Suggested-by: tag indicates that the patch idea is suggested by the person > named and ensures credit to the person for the idea. Please note that this tag > should not be added without the reporter’s permission, especially if the idea > was not posted in a public forum." [1] > > As a reference we don't keep adding official suggested-by tags for any and all > of the suggestions we receive during reviews. The simple name version in the > commit message is what we traditionally uses for small things. Thanks Rodrigo for the inputs. I agree with you in terms of not having this for all patches. But particularly this patch was a suggestion that came up through our offline discussion. And in cases like this it is a good idea to be mindful about and give respectful credits. For internal folks its not a big deal, but Just want to make sure we dont lose the credibility in the community if this is missed for external folks. People do spend time reviewing and suggesting and improving codebase so it is good to be respectful and add the tags for patches that are entirely someone else's suggestion. Manasi > > Looking to the history of this series here I see that this patch looks like > an evolution of the previous series with small increment and suggestions from > multiple folks. Not sure if it is really worthwhile to have to add the official > tag and start to ping everyone to get the ack if it is okay or not to add it. > > [1] - https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > > > > > --- > > > drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++-- > > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > > > index bc627daade3e..12f5e4d23245 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > > @@ -1688,6 +1688,19 @@ static u32 cdclk_squash_waveform(struct drm_i915_private *dev_priv, > > > > > > return 0xffff; > > > } > > > > Missing newline causing checkpatch error > > > > Manasi > > > > > +static const char *cdclk_sequence_to_string(enum cdclk_sequence cdclk_sequence) > > > +{ > > > + switch (cdclk_sequence) { > > > + case CDCLK_SQUASH_ONLY: > > > + return "Squash only"; > > > + case CDCLK_CRAWL_ONLY: > > > + return "Crawl only"; > > > + case CDCLK_LEGACY: > > > + return "Legacy method"; > > > + default: > > > + return "Not a valid cdclk sequence"; > > > + } > > > +} > > > > > > static void dg2_prog_squash_ctl(struct drm_i915_private *i915, u16 waveform) > > > { > > > @@ -2083,10 +2096,11 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915, > > > const struct intel_cdclk_config *cdclk_config, > > > const char *context) > > > { > > > - drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d\n", > > > + drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d, %s action\n", > > > context, cdclk_config->cdclk, cdclk_config->vco, > > > cdclk_config->ref, cdclk_config->bypass, > > > - cdclk_config->voltage_level); > > > + cdclk_config->voltage_level, > > > + cdclk_sequence_to_string(cdclk_config->steps->action)); > > > } > > > > > > /** > > > -- > > > 2.25.1 > > >
On Fri, 16 Sep 2022, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote: > Add a helper function to get stringify values of the > desired cdclk action and dump it with rest of the > cdclk config values > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index bc627daade3e..12f5e4d23245 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -1688,6 +1688,19 @@ static u32 cdclk_squash_waveform(struct drm_i915_private *dev_priv, > > return 0xffff; > } > +static const char *cdclk_sequence_to_string(enum cdclk_sequence cdclk_sequence) For any enum foobar, this would be customarily called foobar_name(), i.e. cdclk_sequence_name(). (Though the enum should probably be renamed as mentioned earlier.) > +{ > + switch (cdclk_sequence) { > + case CDCLK_SQUASH_ONLY: > + return "Squash only"; > + case CDCLK_CRAWL_ONLY: > + return "Crawl only"; > + case CDCLK_LEGACY: > + return "Legacy method"; > + default: > + return "Not a valid cdclk sequence"; > + } > +} > > static void dg2_prog_squash_ctl(struct drm_i915_private *i915, u16 waveform) > { > @@ -2083,10 +2096,11 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915, > const struct intel_cdclk_config *cdclk_config, > const char *context) > { > - drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d\n", > + drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d, %s action\n", "%s action" leads to: Squash only action Crawl only action Legacy method action Not a valid cdclk sequence action A bit odd perhaps. *shrug* BR, Jani. > context, cdclk_config->cdclk, cdclk_config->vco, > cdclk_config->ref, cdclk_config->bypass, > - cdclk_config->voltage_level); > + cdclk_config->voltage_level, > + cdclk_sequence_to_string(cdclk_config->steps->action)); > } > > /**
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Tuesday, September 20, 2022 12:27 AM > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 6/6] drm/i915/display: Dump the new cdclk > config values > > On Fri, 16 Sep 2022, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote: > > Add a helper function to get stringify values of the desired cdclk > > action and dump it with rest of the cdclk config values > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > > b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index bc627daade3e..12f5e4d23245 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -1688,6 +1688,19 @@ static u32 cdclk_squash_waveform(struct > > drm_i915_private *dev_priv, > > > > return 0xffff; > > } > > +static const char *cdclk_sequence_to_string(enum cdclk_sequence > > +cdclk_sequence) > > For any enum foobar, this would be customarily called foobar_name(), i.e. > cdclk_sequence_name(). (Though the enum should probably be renamed as > mentioned earlier.) Will do. The initial change and then change here. > > +{ > > + switch (cdclk_sequence) { > > + case CDCLK_SQUASH_ONLY: > > + return "Squash only"; > > + case CDCLK_CRAWL_ONLY: > > + return "Crawl only"; > > + case CDCLK_LEGACY: > > + return "Legacy method"; > > + default: > > + return "Not a valid cdclk sequence"; > > + } > > +} > > > > static void dg2_prog_squash_ctl(struct drm_i915_private *i915, u16 > > waveform) { @@ -2083,10 +2096,11 @@ void > > intel_cdclk_dump_config(struct drm_i915_private *i915, > > const struct intel_cdclk_config *cdclk_config, > > const char *context) > > { > > - drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, > bypass %d kHz, voltage level %d\n", > > + drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, > bypass > > +%d kHz, voltage level %d, %s action\n", > > "%s action" leads to: > > Squash only action > Crawl only action > Legacy method action > Not a valid cdclk sequence action > > A bit odd perhaps. *shrug* Yeah now I see it. Will thin over what can be a better replacement. Anusha > BR, > Jani. > > > context, cdclk_config->cdclk, cdclk_config->vco, > > cdclk_config->ref, cdclk_config->bypass, > > - cdclk_config->voltage_level); > > + cdclk_config->voltage_level, > > + cdclk_sequence_to_string(cdclk_config->steps->action)); > > } > > > > /** > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index bc627daade3e..12f5e4d23245 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -1688,6 +1688,19 @@ static u32 cdclk_squash_waveform(struct drm_i915_private *dev_priv, return 0xffff; } +static const char *cdclk_sequence_to_string(enum cdclk_sequence cdclk_sequence) +{ + switch (cdclk_sequence) { + case CDCLK_SQUASH_ONLY: + return "Squash only"; + case CDCLK_CRAWL_ONLY: + return "Crawl only"; + case CDCLK_LEGACY: + return "Legacy method"; + default: + return "Not a valid cdclk sequence"; + } +} static void dg2_prog_squash_ctl(struct drm_i915_private *i915, u16 waveform) { @@ -2083,10 +2096,11 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915, const struct intel_cdclk_config *cdclk_config, const char *context) { - drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d\n", + drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d, %s action\n", context, cdclk_config->cdclk, cdclk_config->vco, cdclk_config->ref, cdclk_config->bypass, - cdclk_config->voltage_level); + cdclk_config->voltage_level, + cdclk_sequence_to_string(cdclk_config->steps->action)); } /**
Add a helper function to get stringify values of the desired cdclk action and dump it with rest of the cdclk config values Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)