| Message ID | 20220812095346.45379-1-janusz.krzysztofik@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t,1/3] tests/gem_exec_fence: Fix wrong engine checked for store_dword capability | expand |
Hi Janusz, On Fri, 12 Aug 2022 11:53:44 +0200 Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: It seems that there is a numeration issue on this series, as the patches on it are: [PATCH i-g-t 1/3] tests/gem_exec_fence: Fix wrong engine checked for store_dword capability [PATCH i-g-t v2 2/3] tests/gem_exec_fence: Exclude 0 from use in store batches [PATCH i-g-t v3 3/3] tests/gem_exec_fence: Restore pre-hang checks in *await-hang scenarios Maybe some broken script? It is also missing a cover letter. > Commit ed7c8cfb67e3 ("tests/i915/gem_exec_fence: Add > __for_each_physical_engine to utilize all engines."), while replacing > depraciated for_each_physical_engine(e, fd) loop with > __for_each_physical_engine(fd, e2) and gem_can_store_dword() inside that > loop with gem_class_can_store_dword(), didn't switch to e2 consequently -- > eb_ring(e) argument of gem_can_store_dword() was replaced with e->class > passed to gem_class_can_store_dword() instead of e2->class. As a result, > a batch that stores dword is now submitted to all engines, also those that > don't support that operation. Fix it. > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com> LGTM. Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> > --- > tests/i915/gem_exec_fence.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c > index 7ff7614dad..048870053a 100644 > --- a/tests/i915/gem_exec_fence.c > +++ b/tests/i915/gem_exec_fence.c > @@ -331,7 +331,7 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx, > > i = 0; > for_each_ctx_engine(fd, ctx, e2) { > - if (!gem_class_can_store_dword(fd, e->class)) > + if (!gem_class_can_store_dword(fd, e2->class)) > continue; > > if (flags & NONBLOCK) {
Hi Mauro, Thanks for reviewing this series, I've just pushed it. On Wednesday, 17 August 2022 14:53:48 CEST Mauro Carvalho Chehab wrote: > Hi Janusz, > > On Fri, 12 Aug 2022 11:53:44 +0200 > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: > > It seems that there is a numeration issue on this series, as the patches > on it are: > > [PATCH i-g-t 1/3] tests/gem_exec_fence: Fix wrong engine checked for store_dword capability > [PATCH i-g-t v2 2/3] tests/gem_exec_fence: Exclude 0 from use in store batches > [PATCH i-g-t v3 3/3] tests/gem_exec_fence: Restore pre-hang checks in *await-hang scenarios > > Maybe some broken script? It is also missing a cover letter. That was not a script, I provided version numbers of individual patches manually, and not provided any cover letter. First patch was a small fix, not directly related to the two others. Second patch was a small enhancement, also not directly related to the third one. However, the third one depended on the two for clean apply, and that was the only reason for me sending them in series. That said, let me ask, based on your huge upstream experience, what are your preferences on patch version tagging if one is going to submit a series with new versions of some patches while still including some other that don't need to be changed? Should all be marked as new (and the same) versions? Thanks, Janusz
On Thu, 18 Aug 2022 17:27:26 +0200 Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: > Hi Mauro, > > Thanks for reviewing this series, I've just pushed it. > > On Wednesday, 17 August 2022 14:53:48 CEST Mauro Carvalho Chehab wrote: > > Hi Janusz, > > > > On Fri, 12 Aug 2022 11:53:44 +0200 > > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: > > > > It seems that there is a numeration issue on this series, as the patches > > on it are: > > > > [PATCH i-g-t 1/3] tests/gem_exec_fence: Fix wrong engine checked for store_dword capability > > [PATCH i-g-t v2 2/3] tests/gem_exec_fence: Exclude 0 from use in store batches > > [PATCH i-g-t v3 3/3] tests/gem_exec_fence: Restore pre-hang checks in *await-hang scenarios > > > > Maybe some broken script? It is also missing a cover letter. > > That was not a script, I provided version numbers of individual patches > manually, and not provided any cover letter. First patch was a small fix, not > directly related to the two others. Second patch was a small enhancement, > also not directly related to the third one. However, the third one depended > on the two for clean apply, and that was the only reason for me sending them > in series. > > That said, let me ask, based on your huge upstream experience, what are your > preferences on patch version tagging if one is going to submit a series with > new versions of some patches while still including some other that don't need > to be changed? Should all be marked as new (and the same) versions? I guess you started without a cover letter because it was originally a single patch. Then, you realized that this was not enough, so you needed extra stuff. When I submit single fixes upstream, I usually don't add cover letters, as the patch description is usually enough, but, at the moment it becomes multiple (dependent) ones, I add it. Yet, sometimes even for simple patch submissions, I add cover letters, when I feel the need to add some explanation to help reviewers to analyze it. Basically: - the cover letter provides temporary information meant to help reviewers and maintainers to understand the series. It includes, for instance: - a short summary of the patches in the tree; - on what tree/branch the patch applies, if not on upstream (like someone's else tree, next-20220819 branch, etc). - any special instructions for the maintainers when applying it, like if the patch should be merged after some other series; - patch versions; - etc. - the patch title and the body describes: why, what and how. Those should be providing enough understanding for anyone that would later look at the git logs to understand the changes applied there. Patch version log doesn't belong here. Yet, as patch reviews could be interesting even after things get merged, you can include a link to lore, in the form of: Link: https://lore.kernel.org/$mailing_list/$msg_id (instead of $mailing list, you could just use "all") So, for instance, if I want to place a pointer to the last e-mail on this thread, I would add: Link: https://lore.kernel.org/intel-gfx/6809017.18pcnM708K@jkrzyszt-mobl1.ger.corp.intel.com or: Link: https://lore.kernel.org/all/6809017.18pcnM708K@jkrzyszt-mobl1.ger.corp.intel.com Btw, on patches generated on git, the msg_id is there after format-email. So, you can even add links to the patch you're sending. - In cases like the one you described, I would be adding a cover letter with something like: [PATCH i-g-t v3 0/3] Add some fixes to tests/gem_exec_fence some description about the series --- v3: - Added a patch to restore pre-hang checks; - patches 1 and 2 unchanged. v2: - Added a fix to exclude 0 in store batches; - patch 1 unchanged. If you need a v4, add it before v3, and so on. At the series itself, version numbers are incremented on all patches. This makes clear for reviewers that already checked your series about what changed and what remains the same. I hope that helps. Regards, Mauro
diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c index 7ff7614dad..048870053a 100644 --- a/tests/i915/gem_exec_fence.c +++ b/tests/i915/gem_exec_fence.c @@ -331,7 +331,7 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx, i = 0; for_each_ctx_engine(fd, ctx, e2) { - if (!gem_class_can_store_dword(fd, e->class)) + if (!gem_class_can_store_dword(fd, e2->class)) continue; if (flags & NONBLOCK) {
Commit ed7c8cfb67e3 ("tests/i915/gem_exec_fence: Add __for_each_physical_engine to utilize all engines."), while replacing depraciated for_each_physical_engine(e, fd) loop with __for_each_physical_engine(fd, e2) and gem_can_store_dword() inside that loop with gem_class_can_store_dword(), didn't switch to e2 consequently -- eb_ring(e) argument of gem_can_store_dword() was replaced with e->class passed to gem_class_can_store_dword() instead of e2->class. As a result, a batch that stores dword is now submitted to all engines, also those that don't support that operation. Fix it. Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com> --- tests/i915/gem_exec_fence.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)