[Intel-gfx] [PATCH] drm/i915: simplify switch to if-elseif
clang with W=1 reports
drivers/gpu/drm/i915/display/intel_display.c:6012:3: error: unannotated
fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case I915_FORMAT_MOD_X_TILED:
^
Only one case and the default does anything in this switch, so it should
be changed to an if-elseif.
Signed-off-by: Tom Rix
---
drivers/gpu/drm/i915/display/intel_display.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 0490c6412ab5..1f852e49fc20 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5994,8 +5994,7 @@ static int intel_async_flip_check_hw(struct
intel_atomic_state *state, struct in
* Need to verify this for all gen9 platforms to enable
* this selectively if required.
*/
- switch (new_plane_state->hw.fb->modifier) {
- case DRM_FORMAT_MOD_LINEAR:
+ if (new_plane_state->hw.fb->modifier == DRM_FORMAT_MOD_LINEAR) {
/*
* FIXME: Async on Linear buffer is supported on ICL as
* but with additional alignment and fbc restrictions
@@ -6008,13 +6007,10 @@ static int intel_async_flip_check_hw(struct
intel_atomic_state *state, struct in
plane->base.base.id,
plane->base.name);
return -EINVAL;
}
-
- case I915_FORMAT_MOD_X_TILED:
- case I915_FORMAT_MOD_Y_TILED:
- case I915_FORMAT_MOD_Yf_TILED:
- case I915_FORMAT_MOD_4_TILED:
- break;
- default:
+ } else if (!(new_plane_state->hw.fb->modifier ==
I915_FORMAT_MOD_X_TILED ||
+new_plane_state->hw.fb->modifier ==
I915_FORMAT_MOD_Y_TILED ||
+new_plane_state->hw.fb->modifier ==
I915_FORMAT_MOD_Yf_TILED ||
+new_plane_state->hw.fb->modifier ==
I915_FORMAT_MOD_4_TILED)) {
drm_dbg_kms(>drm,
"[PLANE:%d:%s] Modifier does not support
async flips\n",
plane->base.base.id, plane->base.name);
--
2.27.0
[Intel-gfx] [PATCH] drm/i915/display: clean up comments
spelling changes
resoluition -> resolution
dont-> don't
commmit -> commit
Invalidade -> Invalidate
Signed-off-by: Tom Rix
---
drivers/gpu/drm/i915/display/intel_psr.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
b/drivers/gpu/drm/i915/display/intel_psr.c
index 7d61c55184e5..e6a870641cd2 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -555,7 +555,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
/*
* TODO: 7 lines of IO_BUFFER_WAKE and FAST_WAKE are default
* values from BSpec. In order to setting an optimal power
-* consumption, lower than 4k resoluition mode needs to decrese
+* consumption, lower than 4k resolution mode needs to decrease
* IO_BUFFER_WAKE and FAST_WAKE. And higher than 4K resolution
* mode needs to increase IO_BUFFER_WAKE and FAST_WAKE.
*/
@@ -959,7 +959,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
int psr_setup_time;
/*
-* Current PSR panels dont work reliably with VRR enabled
+* Current PSR panels don't work reliably with VRR enabled
* So if VRR is enabled, do not enable PSR.
*/
if (crtc_state->vrr.enable)
@@ -1664,7 +1664,7 @@ static void intel_psr2_sel_fetch_pipe_alignment(const
struct intel_crtc_state *c
*
* Plane scaling and rotation is not supported by selective fetch and both
* properties can change without a modeset, so need to be check at every
- * atomic commmit.
+ * atomic commit.
*/
static bool psr2_sel_fetch_plane_state_supported(const struct
intel_plane_state *plane_state)
{
@@ -2203,7 +2203,7 @@ static void _psr_invalidate_handle(struct intel_dp
*intel_dp)
}
/**
- * intel_psr_invalidate - Invalidade PSR
+ * intel_psr_invalidate - Invalidate PSR
* @dev_priv: i915 device
* @frontbuffer_bits: frontbuffer plane tracking bits
* @origin: which operation caused the invalidate
--
2.27.0
[Intel-gfx] [PATCH v2] drm/i915: change node clearing from memset to initialization
In insert_mappable_node(), the parameter node is
cleared late in node's use with memset.
insert_mappable_node() is a singleton, called only
from i915_gem_gtt_prepare() which itself is only
called by i915_gem_gtt_pread() and
i915_gem_gtt_pwrite_fast() where the definition of
node originates.
Instead of using memset, initialize node to 0 at its
definitions.
Signed-off-by: Tom Rix
---
v2: restore clearing of flags
drivers/gpu/drm/i915/i915_gem.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2e10187cd0a0..268b1f66b873 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -69,7 +69,6 @@ insert_mappable_node(struct i915_ggtt *ggtt, struct
drm_mm_node *node, u32 size)
if (err)
return err;
- memset(node, 0, sizeof(*node));
err = drm_mm_insert_node_in_range(>vm.mm, node,
size, 0, I915_COLOR_UNEVICTABLE,
0, ggtt->mappable_end,
@@ -381,7 +380,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
intel_wakeref_t wakeref;
- struct drm_mm_node node;
+ struct drm_mm_node node = {};
void __user *user_data;
struct i915_vma *vma;
u64 remain, offset;
@@ -538,7 +537,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
struct intel_runtime_pm *rpm = >runtime_pm;
intel_wakeref_t wakeref;
- struct drm_mm_node node;
+ struct drm_mm_node node = {};
struct i915_vma *vma;
u64 remain, offset;
void __user *user_data;
--
2.27.0
Re: [Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization
On 4/16/22 2:04 PM, Joe Perches wrote:
On Sat, 2022-04-16 at 13:48 -0700, Tom Rix wrote:
On 4/16/22 11:33 AM, Joe Perches wrote:
On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
In insert_mappable_node(), the parameter node is
cleared late in node's use with memset.
insert_mappable_node() is a singleton, called only
from i915_gem_gtt_prepare() which itself is only
called by i915_gem_gtt_pread() and
i915_gem_gtt_pwrite_fast() where the definition of
node originates.
Instead of using memset, initialize node to 0 at it's
definitions.
trivia: /it's/its/
Only reason _not_ to do this is memset is guaranteed to
zero any padding that might go to userspace.
But it doesn't seem there is any padding anyway nor is
the struct available to userspace.
So this seems fine though it might increase overall code
size a tiny bit.
I do have a caveat: see below:
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
[]
@@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
drm_i915_gem_object *obj,
goto err_ww;
} else if (!IS_ERR(vma)) {
node->start = i915_ggtt_offset(vma);
- node->flags = 0;
Why is this unneeded?
node = {} initializes all of node's elements to 0, including this one.
true, but could the call to insert_mappable_node combined with the
retry goto in i915_gem_gtt_prepare set this to non-zero?
Yikes!
I'll add that back.
Thanks for pointing it out.
Tom
Re: [Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization
On 4/16/22 11:33 AM, Joe Perches wrote:
On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
In insert_mappable_node(), the parameter node is
cleared late in node's use with memset.
insert_mappable_node() is a singleton, called only
from i915_gem_gtt_prepare() which itself is only
called by i915_gem_gtt_pread() and
i915_gem_gtt_pwrite_fast() where the definition of
node originates.
Instead of using memset, initialize node to 0 at it's
definitions.
trivia: /it's/its/
Only reason _not_ to do this is memset is guaranteed to
zero any padding that might go to userspace.
But it doesn't seem there is any padding anyway nor is
the struct available to userspace.
So this seems fine though it might increase overall code
size a tiny bit.
I do have a caveat: see below:
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
[]
@@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
drm_i915_gem_object *obj,
goto err_ww;
} else if (!IS_ERR(vma)) {
node->start = i915_ggtt_offset(vma);
- node->flags = 0;
Why is this unneeded?
node = {} initializes all of node's elements to 0, including this one.
Tom
from: drm_mm_insert_node_in_range which can set node->flags
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, >flags);
[Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization
In insert_mappable_node(), the parameter node is
cleared late in node's use with memset.
insert_mappable_node() is a singleton, called only
from i915_gem_gtt_prepare() which itself is only
called by i915_gem_gtt_pread() and
i915_gem_gtt_pwrite_fast() where the definition of
node originates.
Instead of using memset, initialize node to 0 at it's
definitions. And remove unneeded clearing of the flags
element.
Signed-off-by: Tom Rix
---
drivers/gpu/drm/i915/i915_gem.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2e10187cd0a0..7dbd0b325c43 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -69,7 +69,6 @@ insert_mappable_node(struct i915_ggtt *ggtt, struct
drm_mm_node *node, u32 size)
if (err)
return err;
- memset(node, 0, sizeof(*node));
err = drm_mm_insert_node_in_range(>vm.mm, node,
size, 0, I915_COLOR_UNEVICTABLE,
0, ggtt->mappable_end,
@@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
drm_i915_gem_object *obj,
goto err_ww;
} else if (!IS_ERR(vma)) {
node->start = i915_ggtt_offset(vma);
- node->flags = 0;
} else {
ret = insert_mappable_node(ggtt, node, PAGE_SIZE);
if (ret)
@@ -381,7 +379,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
intel_wakeref_t wakeref;
- struct drm_mm_node node;
+ struct drm_mm_node node = {};
void __user *user_data;
struct i915_vma *vma;
u64 remain, offset;
@@ -538,7 +536,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
struct intel_runtime_pm *rpm = >runtime_pm;
intel_wakeref_t wakeref;
- struct drm_mm_node node;
+ struct drm_mm_node node = {};
struct i915_vma *vma;
u64 remain, offset;
void __user *user_data;
--
2.27.0
Re: [Intel-gfx] [PATCH] drm/i915: remove h from printk format specifier
On 12/15/20 10:13 AM, Chris Wilson wrote:
> Quoting t...@redhat.com (2020-12-15 14:41:01)
>> From: Tom Rix
>>
>> See Documentation/core-api/printk-formats.rst.
>> h should no longer be used in the format specifier for printk.
> It's understood by format_decode().
> * 'h', 'l', or 'L' for integer fields
>
> At least reference commit cbacb5ab0aa0 ("docs: printk-formats: Stop
> encouraging use of unnecessary %h[xudi] and %hh[xudi]") as to why the
> printk-formats.rst was altered so we know the code is merely in bad
> taste and not using undefined behaviour of printk.
Ok, i will fix this after the first run of patches.
Tom
> -Chris
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] MAINTAINERS tag for cleanup robot
On 11/22/20 10:22 AM, Joe Perches wrote: > On Sun, 2020-11-22 at 08:33 -0800, Tom Rix wrote: >> On 11/21/20 9:10 AM, Joe Perches wrote: >>> On Sat, 2020-11-21 at 08:50 -0800, t...@redhat.com wrote: >>>> A difficult part of automating commits is composing the subsystem >>>> preamble in the commit log. For the ongoing effort of a fixer producing >>>> one or two fixes a release the use of 'treewide:' does not seem >>>> appropriate. >>>> >>>> It would be better if the normal prefix was used. Unfortunately normal is >>>> not consistent across the tree. >>>> >>>> So I am looking for comments for adding a new tag to the MAINTAINERS file >>>> >>>>D: Commit subsystem prefix >>>> >>>> ex/ for FPGA DFL DRIVERS >>>> >>>>D: fpga: dfl: >>> I'm all for it. Good luck with the effort. It's not completely trivial. >>> >>> From a decade ago: >>> >>> https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/ >>> >>> (and that thread started with extra semicolon patches too) >> Reading the history, how about this. >> >> get_maintainer.pl outputs a single prefix, if multiple files have the >> same prefix it works, if they don't its an error. >> >> Another script 'commit_one_file.sh' does the call to get_mainainter.pl >> to get the prefix and be called by run-clang-tools.py to get the fixer >> specific message. > It's not whether the script used is get_maintainer or any other script, > the question is really if the MAINTAINERS file is the appropriate place > to store per-subsystem patch specific prefixes. > > It is. > > Then the question should be how are the forms described and what is the > inheritance priority. My preference would be to have a default of > inherit the parent base and add basename(subsystem dirname). > > Commit history seems to have standardized on using colons as the separator > between the commit prefix and the subject. > > A good mechanism to explore how various subsystems have uses prefixes in > the past might be something like: > > $ git log --no-merges --pretty='%s' - | \ > perl -n -e 'print substr($_, 0, rindex($_, ":") + 1) . "\n";' | \ > sort | uniq -c | sort -rn Thanks, I have shamelessly stolen this line and limited the commits to the maintainer. I will post something once the generation of the prefixes is done. Tom ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] MAINTAINERS tag for cleanup robot
On 11/21/20 9:10 AM, Joe Perches wrote: > On Sat, 2020-11-21 at 08:50 -0800, t...@redhat.com wrote: >> A difficult part of automating commits is composing the subsystem >> preamble in the commit log. For the ongoing effort of a fixer producing >> one or two fixes a release the use of 'treewide:' does not seem appropriate. >> >> It would be better if the normal prefix was used. Unfortunately normal is >> not consistent across the tree. >> >> So I am looking for comments for adding a new tag to the MAINTAINERS file >> >> D: Commit subsystem prefix >> >> ex/ for FPGA DFL DRIVERS >> >> D: fpga: dfl: > I'm all for it. Good luck with the effort. It's not completely trivial. > > From a decade ago: > > https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/ > > (and that thread started with extra semicolon patches too) Reading the history, how about this. get_mataintainer.pl outputs a single prefix, if multiple files have the same prefix it works, if they don't its an error. Another script 'commit_one_file.sh' does the call to get_mainainter.pl to get the prefix and be called by run-clang-tools.py to get the fixer specific message. Defer minimizing the commits by combining similar subsystems till later. In a steady state case, this should be uncommon. > >> Continuing with cleaning up clang's -Wextra-semi-stmt >> diff --git a/Makefile b/Makefile > [] >> @@ -1567,20 +1567,21 @@ help: >> echo '' >> @echo 'Static analysers:' >> @echo ' checkstack - Generate a list of stack hogs' >> @echo ' versioncheck- Sanity check on version.h usage' >> @echo ' includecheck- Check for duplicate included header files' >> @echo ' export_report - List the usages of all exported symbols' >> @echo ' headerdep - Detect inclusion cycles in headers' >> @echo ' coccicheck - Check with Coccinelle' >> @echo ' clang-analyzer - Check with clang static analyzer' >> @echo ' clang-tidy - Check with clang-tidy' >> +@echo ' clang-tidy-fix - Check and fix with clang-tidy' > A pity the ordering of the code below isn't the same as the above. Taken care thanks! Tom ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] MAINTAINERS tag for cleanup robot
On 11/22/20 6:56 AM, Matthew Wilcox wrote: > On Sun, Nov 22, 2020 at 06:46:46AM -0800, Tom Rix wrote: >> On 11/21/20 7:23 PM, Matthew Wilcox wrote: >>> On Sat, Nov 21, 2020 at 08:50:58AM -0800, t...@redhat.com wrote: >>>> The fixer review is >>>> https://reviews.llvm.org/D91789 >>>> >>>> A run over allyesconfig for x86_64 finds 62 issues, 5 are false positives. >>>> The false positives are caused by macros passed to other macros and by >>>> some macro expansions that did not have an extra semicolon. >>>> >>>> This cleans up about 1,000 of the current 10,000 -Wextra-semi-stmt >>>> warnings in linux-next. >>> Are any of them not false-positives? It's all very well to enable >>> stricter warnings, but if they don't fix any bugs, they're just churn. >>> >> While enabling additional warnings may be a side effect of this effort >> >> the primary goal is to set up a cleaning robot. After that a refactoring >> robot. > Why do we need such a thing? Again, it sounds like more churn. > It's really annoying when I'm working on something important that gets > derailed by pointless churn. Churn also makes it harder to backport > patches to earlier kernels. > A refactoring example on moving to treewide, consistent use of a new api may help. Consider 2efc459d06f1630001e3984854848a5647086232 sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output A new api for printing in the sysfs. How do we use it treewide ? Done manually, it would be a heroic effort requiring high level maintainers pushing and likely only get partially done. If a refactoring programatic fixit is done and validated on a one subsystem, it can run on all the subsystems. The effort is a couple of weeks to write and validate the fixer, hours to run over the tree. It won't be perfect but will be better than doing it manually. Tom ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] MAINTAINERS tag for cleanup robot
On 11/21/20 7:23 PM, Matthew Wilcox wrote: > On Sat, Nov 21, 2020 at 08:50:58AM -0800, t...@redhat.com wrote: >> The fixer review is >> https://reviews.llvm.org/D91789 >> >> A run over allyesconfig for x86_64 finds 62 issues, 5 are false positives. >> The false positives are caused by macros passed to other macros and by >> some macro expansions that did not have an extra semicolon. >> >> This cleans up about 1,000 of the current 10,000 -Wextra-semi-stmt >> warnings in linux-next. > Are any of them not false-positives? It's all very well to enable > stricter warnings, but if they don't fix any bugs, they're just churn. > While enabling additional warnings may be a side effect of this effort the primary goal is to set up a cleaning robot. After that a refactoring robot. Tom ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 00/17] Drop uses of pci_read_config_*() return value
On 8/1/20 5:56 AM, Borislav Petkov wrote:
> On Sat, Aug 01, 2020 at 01:24:29PM +0200, Saheed O. Bolarinwa wrote:
>> The return value of pci_read_config_*() may not indicate a device error.
>> However, the value read by these functions is more likely to indicate
>> this kind of error. This presents two overlapping ways of reporting
>> errors and complicates error checking.
> So why isn't the *value check done in the pci_read_config_* functions
> instead of touching gazillion callers?
>
> For example, pci_conf{1,2}_read() could check whether the u32 *value it
> just read depending on the access method, whether that value is ~0 and
> return proper PCIBIOS_ error in that case.
>
> The check you're replicating
>
> if (val32 == (u32)~0)
>
> everywhere, instead, is just ugly and tests a naked value ~0 which
> doesn't mean anything...
>
I agree, if there is a change, it should be in the pci_read_* functions.
Anything returning void should not fail and likely future users of the proposed
change will not do the extra checks.
Tom
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
