Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1636403 > unrolled thread

[PATCH 0/5] GPU-DRM-STI: Fine-tuning for some function implementations

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] GPU-DRM-STI: Fine-tuning for some function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-05 16:00 +0200
    [PATCH 1/5] drm/sti: Reduce function calls for sequence output at  five places SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-05 16:00 +0200
    Re: [PATCH 0/5] GPU-DRM-STI: Fine-tuning for some function  implementations Sean Paul <seanpaul@chromium.org> - 2017-05-05 16:10 +0200
      Re: GPU-DRM-STI: Fine-tuning for some function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-05 17:10 +0200
        Re: GPU-DRM-STI: Fine-tuning for some function implementations Sean Paul <seanpaul@chromium.org> - 2017-05-06 14:40 +0200
          Re: GPU-DRM-STI: Fine-tuning for some function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-06 16:00 +0200
            Re: GPU-DRM-STI: Fine-tuning for some function implementations Sean Paul <seanpaul@chromium.org> - 2017-05-06 17:30 +0200
              Re: GPU-DRM-STI: Fine-tuning for some function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-06 19:10 +0200
                Re: GPU-DRM-STI: Fine-tuning for some function implementations Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2017-05-09 10:10 +0200
                  Re: GPU-DRM-STI: Fine-tuning for some function implementations Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2017-05-09 12:10 +0200

#1636403 — [PATCH 0/5] GPU-DRM-STI: Fine-tuning for some function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 15:45:45 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Reduce function calls for sequence output at five places
  Replace 17 seq_puts() calls by seq_putc()
  Fix a typo in a comment line
  Fix typos in a comment line
  Adjust two checks for null pointers in sti_hqvdp_probe()

 drivers/gpu/drm/sti/sti_cursor.c |  5 ++---
 drivers/gpu/drm/sti/sti_dvo.c    |  3 +--
 drivers/gpu/drm/sti/sti_gdp.c    |  3 +--
 drivers/gpu/drm/sti/sti_hda.c    |  9 +++------
 drivers/gpu/drm/sti/sti_hdmi.c   | 23 ++++++++++-------------
 drivers/gpu/drm/sti/sti_hqvdp.c  |  7 +++----
 drivers/gpu/drm/sti/sti_mixer.c  |  3 +--
 drivers/gpu/drm/sti/sti_tvout.c  |  7 +++----
 drivers/gpu/drm/sti/sti_vid.c    |  5 ++---
 9 files changed, 26 insertions(+), 39 deletions(-)

-- 
2.12.2

[toc] | [next] | [standalone]

#1636406 — [PATCH 1/5] drm/sti: Reduce function calls for sequence output at five places

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 14:54:52 +0200

Some data were put into a sequence by separate function calls.
Print the same data by five single function calls instead.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/sti/sti_gdp.c  | 3 +--
 drivers/gpu/drm/sti/sti_hda.c  | 6 ++----
 drivers/gpu/drm/sti/sti_hdmi.c | 6 ++----
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
index 88f16cdf6a4b..3caced5f2e86 100644
--- a/drivers/gpu/drm/sti/sti_gdp.c
+++ b/drivers/gpu/drm/sti/sti_gdp.c
@@ -266,8 +266,7 @@ static void gdp_node_dump_node(struct seq_file *s, struct sti_gdp_node *node)
 	seq_printf(s, "\n\tKEY2 0x%08X", node->gam_gdp_key2);
 	seq_printf(s, "\n\tPPT  0x%08X", node->gam_gdp_ppt);
 	gdp_dbg_ppt(s, node->gam_gdp_ppt);
-	seq_printf(s, "\n\tCML  0x%08X", node->gam_gdp_cml);
-	seq_puts(s, "\n");
+	seq_printf(s, "\n\tCML  0x%08X\n", node->gam_gdp_cml);
 }
 
 static int gdp_node_dbg_show(struct seq_file *s, void *arg)
diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 0c0a75bc8bc3..e3475a17eaeb 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -320,8 +320,7 @@ static void hda_dbg_awg_microcode(struct seq_file *s, void __iomem *reg)
 {
 	unsigned int i;
 
-	seq_puts(s, "\n\n");
-	seq_puts(s, "  HDA AWG microcode:");
+	seq_puts(s, "\n\n  HDA AWG microcode:");
 	for (i = 0; i < AWG_MAX_INST; i++) {
 		if (i % 8 == 0)
 			seq_printf(s, "\n  %04X:", i);
@@ -333,8 +332,7 @@ static void hda_dbg_video_dacs_ctrl(struct seq_file *s, void __iomem *reg)
 {
 	u32 val = readl(reg);
 
-	seq_puts(s, "\n");
-	seq_printf(s, "\n  %-25s 0x%08X", "VIDEO_DACS_CONTROL", val);
+	seq_printf(s, "\n\n  %-25s 0x%08X", "VIDEO_DACS_CONTROL", val);
 	seq_puts(s, "\tHD DACs ");
 	seq_puts(s, val & DAC_CFG_HD_HZUVW_OFF_MASK ? "disabled" : "enabled");
 }
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 243905b6ae59..52cdff651c0d 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -692,8 +692,7 @@ static int hdmi_dbg_show(struct seq_file *s, void *data)
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD4, HDMI_IFRAME_SLOT_AVI);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD5, HDMI_IFRAME_SLOT_AVI);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD6, HDMI_IFRAME_SLOT_AVI);
-	seq_puts(s, "\n");
-	seq_printf(s, "\n AUDIO Infoframe (Data Island slot N=%d):",
+	seq_printf(s, "\n\n AUDIO Infoframe (Data Island slot N=%d):",
 		   HDMI_IFRAME_SLOT_AUDIO);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_HEAD_WORD, HDMI_IFRAME_SLOT_AUDIO);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD0, HDMI_IFRAME_SLOT_AUDIO);
@@ -703,8 +702,7 @@ static int hdmi_dbg_show(struct seq_file *s, void *data)
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD4, HDMI_IFRAME_SLOT_AUDIO);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD5, HDMI_IFRAME_SLOT_AUDIO);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD6, HDMI_IFRAME_SLOT_AUDIO);
-	seq_puts(s, "\n");
-	seq_printf(s, "\n VENDOR SPECIFIC Infoframe (Data Island slot N=%d):",
+	seq_printf(s, "\n\n VENDOR SPECIFIC Infoframe (Data Island slot N=%d):",
 		   HDMI_IFRAME_SLOT_VENDOR);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_HEAD_WORD, HDMI_IFRAME_SLOT_VENDOR);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD0, HDMI_IFRAME_SLOT_VENDOR);
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1636409

On Fri, May 05, 2017 at 03:50:49PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 5 May 2017 15:45:45 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 

Hi Markus,
It seems like you're back to submitting cocci patches again :)

We left off in September asking you to contact the list before sending these
types of patches, and to ensure they were properly tested [1]. I don't see any
evidence that either of these things have been done.

I don't want to waste your time by ignoring your patches, so please ensure that
your patches provide value and that they are tested.

Sean


[1]- http://www.spinics.net/lists/linux-kernel-janitors/msg28134.html

> Markus Elfring (5):
>   Reduce function calls for sequence output at five places
>   Replace 17 seq_puts() calls by seq_putc()
>   Fix a typo in a comment line
>   Fix typos in a comment line
>   Adjust two checks for null pointers in sti_hqvdp_probe()
> 
>  drivers/gpu/drm/sti/sti_cursor.c |  5 ++---
>  drivers/gpu/drm/sti/sti_dvo.c    |  3 +--
>  drivers/gpu/drm/sti/sti_gdp.c    |  3 +--
>  drivers/gpu/drm/sti/sti_hda.c    |  9 +++------
>  drivers/gpu/drm/sti/sti_hdmi.c   | 23 ++++++++++-------------
>  drivers/gpu/drm/sti/sti_hqvdp.c  |  7 +++----
>  drivers/gpu/drm/sti/sti_mixer.c  |  3 +--
>  drivers/gpu/drm/sti/sti_tvout.c  |  7 +++----
>  drivers/gpu/drm/sti/sti_vid.c    |  5 ++---
>  9 files changed, 26 insertions(+), 39 deletions(-)
> 
> -- 
> 2.12.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[toc] | [prev] | [next] | [standalone]

#1636436 — Re: GPU-DRM-STI: Fine-tuning for some function implementations

> It seems like you're back to submitting cocci patches again :)

My contribution activities are varying also for Linux software over time.   ;-)

The corresponding source code search patterns get different popularity.


> I don't want to waste your time by ignoring your patches, so please ensure that
> your patches provide value and that they are tested.

Which benchmarks and system tests would you find representative for this patch series?

How do you think generally about the proposed change possibilities?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1636894 — Re: GPU-DRM-STI: Fine-tuning for some function implementations

On Fri, May 05, 2017 at 05:04:40PM +0200, SF Markus Elfring wrote:
> > It seems like you're back to submitting cocci patches again :)
> 
> My contribution activities are varying also for Linux software over time.   ;-)
> 
> The corresponding source code search patterns get different popularity.
> 
> 
> > I don't want to waste your time by ignoring your patches, so please ensure that
> > your patches provide value and that they are tested.
> 
> Which benchmarks and system tests would you find representative for this patch series?
> 

Given your history of submitting changes which break working code, I want
assurance that you've actually run the code and verified that it does what you
want it to do.

> How do you think generally about the proposed change possibilities?

Generally speaking, I don't care about checkpatch/cocci changes that aren't
tested. They clutter the log and don't provide enough value to justify the risk
of breaking stuff. 

IMO, the only time this would be acceptable is if a new contributor wanted to
wet their feet with a couple cleanup patches before diving in to actual
functional changes. In that case, I wouldn't mind dealing with breakage since
we'll benefit from their contributions in the future. With your changes, we
don't have this upside.

Sean


> 
> Regards,
> Markus

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[toc] | [prev] | [next] | [standalone]

#1636910 — Re: GPU-DRM-STI: Fine-tuning for some function implementations

> Generally speaking, I don't care about checkpatch/cocci changes that aren't tested.

I find this view interesting only to some degree.

1. I suggest to combine a few functions into fewer ones.
   * Do you spot any programming mistakes in these concrete cases?
   * Can such code reduction result into desired effects?

2. I propose to use the function “seq_putc” at more source code places.
   * Do you really find any previous system test approaches insufficient around
     such a Linux feature?
   * Does the programming interface “seq_puts” provide any properties
     that you prefer over the other one for the sequence output of single characters?
     http://elixir.free-electrons.com/linux/v4.11/source/fs/seq_file.c#L664


> With your changes, we don't have this upside.

How do you think about to pick spelling corrections up for two comment lines?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1636921 — Re: GPU-DRM-STI: Fine-tuning for some function implementations

On Sat, May 06, 2017 at 03:54:51PM +0200, SF Markus Elfring wrote:
> > Generally speaking, I don't care about checkpatch/cocci changes that aren't tested.
> 
> I find this view interesting only to some degree.

We're bordering on becoming unproductive here, but I'll try once more.

> 
> 1. I suggest to combine a few functions into fewer ones.
>    * Do you spot any programming mistakes in these concrete cases?

Not in the patches I skimmed. However, your history of breaking code tells me that
there have been mistakes missed in the past. As such, I'm not willing to take
untested code from you that does not change functionality at the risk of
breaking something that is currently working. This is non-negotiable.

>    * Can such code reduction result into desired effects?
> 
> 2. I propose to use the function “seq_putc” at more source code places.
>    * Do you really find any previous system test approaches insufficient around
>      such a Linux feature?
>    * Does the programming interface “seq_puts” provide any properties
>      that you prefer over the other one for the sequence output of single characters?
>      http://elixir.free-electrons.com/linux/v4.11/source/fs/seq_file.c#L664
> 

As I said before, if you test it, I'll consider it. If you are unwilling to test
your changes, I'm unwilling to apply them. I'm not interested in double checking
all of your work, and fixing your bugs for no functional benefit.

> 
> > With your changes, we don't have this upside.
> 
> How do you think about to pick spelling corrections up for two comment lines?

Well, it wouldn't break anything, so that's positive. As I said in my last
email, these types of changes are perfect for new contributors to get started
with kernel development. I find less value in these patches if they're from
someone seemingly trying to rack up patch count.

Sean


> 
> Regards,
> Markus

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[toc] | [prev] | [next] | [standalone]

#1636954 — Re: GPU-DRM-STI: Fine-tuning for some function implementations

>> 1. I suggest to combine a few functions into fewer ones.
>>    * Do you spot any programming mistakes in these concrete cases?
> 
> Not in the patches I skimmed.

Thanks for such feedback.


> However, your history of breaking code tells me that there have been mistakes
> missed in the past.

I admit that I had my own share of software development hiccups. I would also
like to reduce them. But a probability remains that I will stumble on
various glitches as usual.


> As such, I'm not willing to take untested code from you that does not change
> functionality at the risk of breaking something that is currently working.

I imagine that the shown software refactoring will improve the affected
sequence outputs in useful ways, won't it?


> This is non-negotiable.

It seems that we have got different views around the ways to get to acceptable
final system test results.


> As I said before, if you test it, I'll consider it.

I got a few doubts for this information. If you find my software development
reputation so questionable, I assume that you would not trust any tests
that I would try out on my own.


> If you are unwilling to test your changes, I'm unwilling to apply them.

I guess that the desired willingness will depend on a test environment
which will be trusted by all involved parties. Other incentives might
also matter.


> I'm not interested in double checking all of your work, and fixing your bugs
> for no functional benefit.

Do you care for improvements in the implementation of logging functions?


> I find less value in these patches if they're from someone seemingly
> trying to rack up patch count.

I am picking special source code search patterns up.
The evolving development tools can point then hundreds of source files
out which contain similar update candidates.
I found also a few spelling weaknesses while I was looking around
in affected source code. These tools can also increase the awareness
for such change possibilities, can't they?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1637962 — Re: GPU-DRM-STI: Fine-tuning for some function implementations

2017-05-06 19:00 GMT+02:00 SF Markus Elfring <elfring@users.sourceforge.net>:
>>> 1. I suggest to combine a few functions into fewer ones.
>>>    * Do you spot any programming mistakes in these concrete cases?
>>
>> Not in the patches I skimmed.
>
> Thanks for such feedback.
>
>
>> However, your history of breaking code tells me that there have been mistakes
>> missed in the past.
>
> I admit that I had my own share of software development hiccups. I would also
> like to reduce them. But a probability remains that I will stumble on
> various glitches as usual.
>
>
>> As such, I'm not willing to take untested code from you that does not change
>> functionality at the risk of breaking something that is currently working.
>
> I imagine that the shown software refactoring will improve the affected
> sequence outputs in useful ways, won't it?
>
>
>> This is non-negotiable.
>
> It seems that we have got different views around the ways to get to acceptable
> final system test results.
>
>
>> As I said before, if you test it, I'll consider it.

As sti driver maintainer I will test those patches.
If their are ok and get some other reviewed/ack I will use them
for myself training on how push patches in drm-misc.

Benjamin

>
> I got a few doubts for this information. If you find my software development
> reputation so questionable, I assume that you would not trust any tests
> that I would try out on my own.
>
>
>> If you are unwilling to test your changes, I'm unwilling to apply them.
>
> I guess that the desired willingness will depend on a test environment
> which will be trusted by all involved parties. Other incentives might
> also matter.
>
>
>> I'm not interested in double checking all of your work, and fixing your bugs
>> for no functional benefit.
>
> Do you care for improvements in the implementation of logging functions?
>
>
>> I find less value in these patches if they're from someone seemingly
>> trying to rack up patch count.
>
> I am picking special source code search patterns up.
> The evolving development tools can point then hundreds of source files
> out which contain similar update candidates.
> I found also a few spelling weaknesses while I was looking around
> in affected source code. These tools can also increase the awareness
> for such change possibilities, can't they?
>
> Regards,
> Markus

[toc] | [prev] | [next] | [standalone]

#1638024 — Re: GPU-DRM-STI: Fine-tuning for some function implementations

2017-05-09 10:03 GMT+02:00 Benjamin Gaignard <benjamin.gaignard@linaro.org>:
> 2017-05-06 19:00 GMT+02:00 SF Markus Elfring <elfring@users.sourceforge.net>:
>>>> 1. I suggest to combine a few functions into fewer ones.
>>>>    * Do you spot any programming mistakes in these concrete cases?
>>>
>>> Not in the patches I skimmed.
>>
>> Thanks for such feedback.
>>
>>
>>> However, your history of breaking code tells me that there have been mistakes
>>> missed in the past.
>>
>> I admit that I had my own share of software development hiccups. I would also
>> like to reduce them. But a probability remains that I will stumble on
>> various glitches as usual.
>>
>>
>>> As such, I'm not willing to take untested code from you that does not change
>>> functionality at the risk of breaking something that is currently working.
>>
>> I imagine that the shown software refactoring will improve the affected
>> sequence outputs in useful ways, won't it?
>>
>>
>>> This is non-negotiable.
>>
>> It seems that we have got different views around the ways to get to acceptable
>> final system test results.
>>
>>
>>> As I said before, if you test it, I'll consider it.
>
> As sti driver maintainer I will test those patches.
> If their are ok and get some other reviewed/ack I will use them
> for myself training on how push patches in drm-misc.
>
> Benjamin

After testing:
Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

>
>>
>> I got a few doubts for this information. If you find my software development
>> reputation so questionable, I assume that you would not trust any tests
>> that I would try out on my own.
>>
>>
>>> If you are unwilling to test your changes, I'm unwilling to apply them.
>>
>> I guess that the desired willingness will depend on a test environment
>> which will be trusted by all involved parties. Other incentives might
>> also matter.
>>
>>
>>> I'm not interested in double checking all of your work, and fixing your bugs
>>> for no functional benefit.
>>
>> Do you care for improvements in the implementation of logging functions?
>>
>>
>>> I find less value in these patches if they're from someone seemingly
>>> trying to rack up patch count.
>>
>> I am picking special source code search patterns up.
>> The evolving development tools can point then hundreds of source files
>> out which contain similar update candidates.
>> I found also a few spelling weaknesses while I was looking around
>> in affected source code. These tools can also increase the awareness
>> for such change possibilities, can't they?
>>
>> Regards,
>> Markus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web