Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
On Fri, 2018-04-06 at 11:20 -0700, Mark Janes wrote: > Emil Velikov writes: > > > On 5 April 2018 at 20:33, Mark Janes > > wrote: > > > Emil Velikov writes: > > > > > > > On 4 April 2018 at 22:50, Mark Janes > > > > wrote: > > > > > Leo Liu writes: > > > > > > > > > > > On 04/04/2018 12:40 PM, Mark Janes wrote: > > > > > > > Leo Liu writes: > > > > > > > > > > > > > > > On the CI family, firmware requires the destory command > > > > > > > > have to be the > > > > > > > > last command in the IB, moving feedback command after > > > > > > > > destroy is causing > > > > > > > > issues on CI cards, so we have to keep the previous > > > > > > > > logic that moves > > > > > > > > destroy back to the last command. > > > > > > > > > > > > > > > > But as the original issue fixed previously, with the > > > > > > > > newer family like Vega10, > > > > > > > > feedback command have to be included inside of the task > > > > > > > > info command along > > > > > > > > with destroy command. > > > > > > > > > > > > > > > > Fixes: 6d74cb25("radeon/vce: move destroy command > > > > > > > > before feedback command") > > > > > > > > > > > > > > > > Signed-off-by: Leo Liu > > > > > > > > Cc: mesa-sta...@lists.freedesktop.org > > > > > > > > > > > > > > These tags seem ambiguous to me. If this commit fixes a > > > > > > > specific > > > > > > > commit, then the patch should be applied only to stable > > > > > > > branches which > > > > > > > contain that commit. > > > > > > > > > > > > > > However, the mesa-stable CC caused this patch to be > > > > > > > applied to 17.3, > > > > > > > which does *not* contain the broken patch. > > > > > > > > > > > > > > Leo: did you intend for the mesa-stable CC to cause this > > > > > > > patch to be > > > > > > > applied to older stable branches? > > > > > > > > > > > > I would like to have this patch apply to branches "17.2", > > > > > > "17.3", > > > > > > "18.0", which got patch titled "radeon/vce: move destroy > > > > > > command before > > > > > > feedback command" > > > > > > > > > > Ok, I understand now. You cc'd a buggy patch to stable, and > > > > > the bug was > > > > > shipped in 17.3.1. > > > > > > > > > > > > > May I suggest phrasing things less personally. Mistakes happen, > > > > so > > > > let's work in providing suggestions for improvement as opposed > > > > to "you > > > > did X/Y". > > > > > > Thank you for the feedback. I was trying to state the facts, but > > > I > > > understand how this could be read as a criticism. > > > > > > > Does that mean you tested radeon/vce and observed the breakage? > > > > > > > As you say, mistakes happen -- and when they happen on the stable > > > branches, there is very little to protect the end users. Could > > > we > > > enhance automation to prevent this situation? For example: > > > > > > > Consistent testing/reporting is needed. I believe I've mentioned if > > before: > > > > You are the only one who consistently provides feedback about the > > state. > > There have been individuals to report, while I'm very grateful > > these > > reports are very rare and far between. > > > > Approx 4 years ago Carl suggested another alternative. Roughly put: > > > > Driver specific patches are _omitted_ unless team member has > > explicitly tested them. > > Needless to say plan did not go forward - see the whole thread [1]. > > > > One thing that it had in common with recent discussion is a > > tester/frontman/maintainer/etc for each team. > > > > Having such a person alongside the actual testing is optional, yet > > _highly_ recommended. > > > > As you know Intel's team is the largest one, perhaps as big as all > > the > > others combined. So expecting the same amount of manpower and time > > dedication is impossible. > > I agree with you, however our release process still has a gap. We > (Intel) test commits on master, and file bugs when we find them in > i965 > or other components. > > If those commits already have a stable tag in the commit message, > they > will be shipped at a later date directly to customers, with no > testing. > There is no way to blacklist broken patches in our Mesa's release > automation. > Another option would be replying to the stable mailing list about the commit generating a regression. Every time I pick a stable commit, I search it in the stable mailing list to see if there is any important feedback, like please do not cherry-pick as it is causing a regression. This also adds an opportunity for people informing/providing a fix for the regression. If it is communicated in the same thread, I will just realize that I need to include such patch within the stable. Otherwise, I know I just need to add it to .cherry-ignore. > > HTH > > Emil > > > > [1] https://lists.freedesktop.org/archives/mesa-dev/2014-July/06299 > > 2.html > > ___ > > mesa-stable mailing list > > mesa-sta...@lists.freedesktop.org > > https://lists.freedesktop.org/
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
On 04/04/2018 11:30 AM, Mark Janes wrote: > Emil Velikov writes: > >> On 4 April 2018 at 17:40, Mark Janes wrote: >>> Leo Liu writes: >>> On the CI family, firmware requires the destory command have to be the last command in the IB, moving feedback command after destroy is causing issues on CI cards, so we have to keep the previous logic that moves destroy back to the last command. But as the original issue fixed previously, with the newer family like Vega10, feedback command have to be included inside of the task info command along with destroy command. Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command") Signed-off-by: Leo Liu Cc: mesa-sta...@lists.freedesktop.org >>> >>> These tags seem ambiguous to me. If this commit fixes a specific >>> commit, then the patch should be applied only to stable branches which >>> contain that commit. >>> >>> However, the mesa-stable CC caused this patch to be applied to 17.3, >>> which does *not* contain the broken patch. >>> >>> Leo: did you intend for the mesa-stable CC to cause this patch to be >>> applied to older stable branches? >>> >>> Release managers: is there a protocol for how this specification should >>> be parsed, when considering a patch for stable? >>> >> If the Fixes tag, reference a commit that is missing in specific >> stable branch then obviously the fix is not suitable. >> Hence the stable piece than be ignored + alongside a reply to the >> patch that it will not be in $stable_branch because $reason. > > OK, we have violated this policy at least a couple times in the previous > release, based on my audits: > > 2f67c9b17518cf0d2fe946e39e5b8ff5ec2797c5 > i965/vec4: Fix null destination register in 3-source instructions I thought: NOTE: I have sent a couple tests to the piglit list that reproduce this bug *without* the commit mentioned below. This commit fixes those tests. made it pretty clear that this is a pre-existing bug. The commit mentioned in Fixes: just made it happen more. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
Emil Velikov writes: > (Dropping Leo, since it doesn't affect him. He's already subscribed to > the list.) > > On 6 April 2018 at 19:20, Mark Janes wrote: > >> I agree with you, however our release process still has a gap. We >> (Intel) test commits on master, and file bugs when we find them in i965 >> or other components. >> >> If those commits already have a stable tag in the commit message, they >> will be shipped at a later date directly to customers, with no testing. >> There is no way to blacklist broken patches in our Mesa's release >> automation. >> > That's why I mentioned that the process cannot be fully automated ;-) > > Let me try to explain slightly differently. Amongst others you want: > > a) 24h (ish) buffer (getting closer to 0, as we reach the pre-release > announcement) before landing fix in the stable branch. > > We had broken _badly_ a few multiple times, a balance between the two > is essential. > > Looking at it from Jenkins POV: > You don't want to test/bisect that master is broken, only to apply > same patch and run Jenkins on the same broken patch. > > - when issues to happen for example: fdo#103626 currently there's two > ways to handle it > > 1) add the commit to bin/.cherry-ignore. latter of which means that > you miss the patch when it's actually fixed up. > See a094314340387ef2463ed8b4ddc9317bc539832b for context. You are right. We can just add the commit to .cherry-ignore files in affected branches when the bug bisects to something with a stable tag. > 2) carefully/manually git cherry-pick > Doing this allowed me to add the regression to the tracker, as > otherwise we would have missed it for 18.0.0 ;-) > > Yet we could introduce on-hold list to cherry-ignore. It's fairly trivial. > > > Hope that makes things a bit clearer. > > -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
(Dropping Leo, since it doesn't affect him. He's already subscribed to the list.) On 6 April 2018 at 19:20, Mark Janes wrote: > I agree with you, however our release process still has a gap. We > (Intel) test commits on master, and file bugs when we find them in i965 > or other components. > > If those commits already have a stable tag in the commit message, they > will be shipped at a later date directly to customers, with no testing. > There is no way to blacklist broken patches in our Mesa's release > automation. > That's why I mentioned that the process cannot be fully automated ;-) Let me try to explain slightly differently. Amongst others you want: a) 24h (ish) buffer (getting closer to 0, as we reach the pre-release announcement) before landing fix in the stable branch. We had broken _badly_ a few multiple times, a balance between the two is essential. Looking at it from Jenkins POV: You don't want to test/bisect that master is broken, only to apply same patch and run Jenkins on the same broken patch. - when issues to happen for example: fdo#103626 currently there's two ways to handle it 1) add the commit to bin/.cherry-ignore. latter of which means that you miss the patch when it's actually fixed up. See a094314340387ef2463ed8b4ddc9317bc539832b for context. 2) carefully/manually git cherry-pick Doing this allowed me to add the regression to the tracker, as otherwise we would have missed it for 18.0.0 ;-) Yet we could introduce on-hold list to cherry-ignore. It's fairly trivial. Hope that makes things a bit clearer. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
Emil Velikov writes: > On 5 April 2018 at 20:33, Mark Janes wrote: >> Emil Velikov writes: >> >>> On 4 April 2018 at 22:50, Mark Janes wrote: Leo Liu writes: > On 04/04/2018 12:40 PM, Mark Janes wrote: >> Leo Liu writes: >> >>> On the CI family, firmware requires the destory command have to be the >>> last command in the IB, moving feedback command after destroy is causing >>> issues on CI cards, so we have to keep the previous logic that moves >>> destroy back to the last command. >>> >>> But as the original issue fixed previously, with the newer family like >>> Vega10, >>> feedback command have to be included inside of the task info command >>> along >>> with destroy command. >>> >>> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback >>> command") >>> >>> Signed-off-by: Leo Liu >>> Cc: mesa-sta...@lists.freedesktop.org >> These tags seem ambiguous to me. If this commit fixes a specific >> commit, then the patch should be applied only to stable branches which >> contain that commit. >> >> However, the mesa-stable CC caused this patch to be applied to 17.3, >> which does *not* contain the broken patch. >> >> Leo: did you intend for the mesa-stable CC to cause this patch to be >> applied to older stable branches? > I would like to have this patch apply to branches "17.2", "17.3", > "18.0", which got patch titled "radeon/vce: move destroy command before > feedback command" Ok, I understand now. You cc'd a buggy patch to stable, and the bug was shipped in 17.3.1. >>> May I suggest phrasing things less personally. Mistakes happen, so >>> let's work in providing suggestions for improvement as opposed to "you >>> did X/Y". >> >> Thank you for the feedback. I was trying to state the facts, but I >> understand how this could be read as a criticism. >> > Does that mean you tested radeon/vce and observed the breakage? > > >> As you say, mistakes happen -- and when they happen on the stable >> branches, there is very little to protect the end users. Could we >> enhance automation to prevent this situation? For example: >> > Consistent testing/reporting is needed. I believe I've mentioned if before: > > You are the only one who consistently provides feedback about the state. > There have been individuals to report, while I'm very grateful these > reports are very rare and far between. > > Approx 4 years ago Carl suggested another alternative. Roughly put: > > Driver specific patches are _omitted_ unless team member has > explicitly tested them. > Needless to say plan did not go forward - see the whole thread [1]. > > One thing that it had in common with recent discussion is a > tester/frontman/maintainer/etc for each team. > > Having such a person alongside the actual testing is optional, yet > _highly_ recommended. > > As you know Intel's team is the largest one, perhaps as big as all the > others combined. So expecting the same amount of manpower and time > dedication is impossible. I agree with you, however our release process still has a gap. We (Intel) test commits on master, and file bugs when we find them in i965 or other components. If those commits already have a stable tag in the commit message, they will be shipped at a later date directly to customers, with no testing. There is no way to blacklist broken patches in our Mesa's release automation. > HTH > Emil > > [1] https://lists.freedesktop.org/archives/mesa-dev/2014-July/062992.html > ___ > mesa-stable mailing list > mesa-sta...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-stable ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
On 5 April 2018 at 20:33, Mark Janes wrote: > Emil Velikov writes: > >> On 4 April 2018 at 22:50, Mark Janes wrote: >>> Leo Liu writes: >>> On 04/04/2018 12:40 PM, Mark Janes wrote: > Leo Liu writes: > >> On the CI family, firmware requires the destory command have to be the >> last command in the IB, moving feedback command after destroy is causing >> issues on CI cards, so we have to keep the previous logic that moves >> destroy back to the last command. >> >> But as the original issue fixed previously, with the newer family like >> Vega10, >> feedback command have to be included inside of the task info command >> along >> with destroy command. >> >> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback >> command") >> >> Signed-off-by: Leo Liu >> Cc: mesa-sta...@lists.freedesktop.org > These tags seem ambiguous to me. If this commit fixes a specific > commit, then the patch should be applied only to stable branches which > contain that commit. > > However, the mesa-stable CC caused this patch to be applied to 17.3, > which does *not* contain the broken patch. > > Leo: did you intend for the mesa-stable CC to cause this patch to be > applied to older stable branches? I would like to have this patch apply to branches "17.2", "17.3", "18.0", which got patch titled "radeon/vce: move destroy command before feedback command" >>> >>> Ok, I understand now. You cc'd a buggy patch to stable, and the bug was >>> shipped in 17.3.1. >>> >> May I suggest phrasing things less personally. Mistakes happen, so >> let's work in providing suggestions for improvement as opposed to "you >> did X/Y". > > Thank you for the feedback. I was trying to state the facts, but I > understand how this could be read as a criticism. > Does that mean you tested radeon/vce and observed the breakage? > As you say, mistakes happen -- and when they happen on the stable > branches, there is very little to protect the end users. Could we > enhance automation to prevent this situation? For example: > Consistent testing/reporting is needed. I believe I've mentioned if before: You are the only one who consistently provides feedback about the state. There have been individuals to report, while I'm very grateful these reports are very rare and far between. Approx 4 years ago Carl suggested another alternative. Roughly put: Driver specific patches are _omitted_ unless team member has explicitly tested them. Needless to say plan did not go forward - see the whole thread [1]. One thing that it had in common with recent discussion is a tester/frontman/maintainer/etc for each team. Having such a person alongside the actual testing is optional, yet _highly_ recommended. As you know Intel's team is the largest one, perhaps as big as all the others combined. So expecting the same amount of manpower and time dedication is impossible. HTH Emil [1] https://lists.freedesktop.org/archives/mesa-dev/2014-July/062992.html ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
Emil Velikov writes: > On 4 April 2018 at 22:50, Mark Janes wrote: >> Leo Liu writes: >> >>> On 04/04/2018 12:40 PM, Mark Janes wrote: Leo Liu writes: > On the CI family, firmware requires the destory command have to be the > last command in the IB, moving feedback command after destroy is causing > issues on CI cards, so we have to keep the previous logic that moves > destroy back to the last command. > > But as the original issue fixed previously, with the newer family like > Vega10, > feedback command have to be included inside of the task info command along > with destroy command. > > Fixes: 6d74cb25("radeon/vce: move destroy command before feedback > command") > > Signed-off-by: Leo Liu > Cc: mesa-sta...@lists.freedesktop.org These tags seem ambiguous to me. If this commit fixes a specific commit, then the patch should be applied only to stable branches which contain that commit. However, the mesa-stable CC caused this patch to be applied to 17.3, which does *not* contain the broken patch. Leo: did you intend for the mesa-stable CC to cause this patch to be applied to older stable branches? >>> I would like to have this patch apply to branches "17.2", "17.3", >>> "18.0", which got patch titled "radeon/vce: move destroy command before >>> feedback command" >> >> Ok, I understand now. You cc'd a buggy patch to stable, and the bug was >> shipped in 17.3.1. >> > May I suggest phrasing things less personally. Mistakes happen, so > let's work in providing suggestions for improvement as opposed to "you > did X/Y". Thank you for the feedback. I was trying to state the facts, but I understand how this could be read as a criticism. As you say, mistakes happen -- and when they happen on the stable branches, there is very little to protect the end users. Could we enhance automation to prevent this situation? For example: - bin/.cherry-blacklist lists commits that should never be shipped on stable. - bisected bugs -> update the blacklist I can't think of a way to automate that, but it would have highlighted one of the instance where we shipped a regression in a 17.3 point release. > Aside from the normal stable/fixes tag, people can nominate patches by > sending them to the list [1]. > We had patch authors, other developers and even 'random' members of > the public to use the last method. > > HTH > Emil > > https://www.mesa3d.org/submittingpatches.html#nominations > ___ > mesa-stable mailing list > mesa-sta...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-stable ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
On 4 April 2018 at 22:50, Mark Janes wrote: > Leo Liu writes: > >> On 04/04/2018 12:40 PM, Mark Janes wrote: >>> Leo Liu writes: >>> On the CI family, firmware requires the destory command have to be the last command in the IB, moving feedback command after destroy is causing issues on CI cards, so we have to keep the previous logic that moves destroy back to the last command. But as the original issue fixed previously, with the newer family like Vega10, feedback command have to be included inside of the task info command along with destroy command. Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command") Signed-off-by: Leo Liu Cc: mesa-sta...@lists.freedesktop.org >>> These tags seem ambiguous to me. If this commit fixes a specific >>> commit, then the patch should be applied only to stable branches which >>> contain that commit. >>> >>> However, the mesa-stable CC caused this patch to be applied to 17.3, >>> which does *not* contain the broken patch. >>> >>> Leo: did you intend for the mesa-stable CC to cause this patch to be >>> applied to older stable branches? >> I would like to have this patch apply to branches "17.2", "17.3", >> "18.0", which got patch titled "radeon/vce: move destroy command before >> feedback command" > > Ok, I understand now. You cc'd a buggy patch to stable, and the bug was > shipped in 17.3.1. > May I suggest phrasing things less personally. Mistakes happen, so let's work in providing suggestions for improvement as opposed to "you did X/Y". Aside from the normal stable/fixes tag, people can nominate patches by sending them to the list [1]. We had patch authors, other developers and even 'random' members of the public to use the last method. HTH Emil https://www.mesa3d.org/submittingpatches.html#nominations ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
Leo Liu writes: > On 04/04/2018 12:40 PM, Mark Janes wrote: >> Leo Liu writes: >> >>> On the CI family, firmware requires the destory command have to be the >>> last command in the IB, moving feedback command after destroy is causing >>> issues on CI cards, so we have to keep the previous logic that moves >>> destroy back to the last command. >>> >>> But as the original issue fixed previously, with the newer family like >>> Vega10, >>> feedback command have to be included inside of the task info command along >>> with destroy command. >>> >>> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command") >>> >>> Signed-off-by: Leo Liu >>> Cc: mesa-sta...@lists.freedesktop.org >> These tags seem ambiguous to me. If this commit fixes a specific >> commit, then the patch should be applied only to stable branches which >> contain that commit. >> >> However, the mesa-stable CC caused this patch to be applied to 17.3, >> which does *not* contain the broken patch. >> >> Leo: did you intend for the mesa-stable CC to cause this patch to be >> applied to older stable branches? > I would like to have this patch apply to branches "17.2", "17.3", > "18.0", which got patch titled "radeon/vce: move destroy command before > feedback command" Ok, I understand now. You cc'd a buggy patch to stable, and the bug was shipped in 17.3.1. > And this Cc-ed patch is to fix "radeon/vce: move destroy command before > feedback command" > > Thanks, > Leo > > >> >> Release managers: is there a protocol for how this specification should >> be parsed, when considering a patch for stable? >> >>> --- >>> src/gallium/drivers/radeon/radeon_vce.c| 1 - >>> src/gallium/drivers/radeon/radeon_vce_40_2_2.c | 2 ++ >>> src/gallium/drivers/radeon/radeon_vce_52.c | 18 ++ >>> 3 files changed, 12 insertions(+), 9 deletions(-) >>> >>> diff --git a/src/gallium/drivers/radeon/radeon_vce.c >>> b/src/gallium/drivers/radeon/radeon_vce.c >>> index 427bf01ed8..c84103e0ac 100644 >>> --- a/src/gallium/drivers/radeon/radeon_vce.c >>> +++ b/src/gallium/drivers/radeon/radeon_vce.c >>> @@ -247,7 +247,6 @@ static void rvce_destroy(struct pipe_video_codec >>> *encoder) >>> enc->fb = &fb; >>> enc->session(enc); >>> enc->destroy(enc); >>> - enc->feedback(enc); >>> flush(enc); >>> si_vid_destroy_buffer(&fb); >>> } >>> diff --git a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c >>> b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c >>> index f1db47d4bd..04e9d7f5e1 100644 >>> --- a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c >>> +++ b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c >>> @@ -421,6 +421,8 @@ static void destroy(struct rvce_encoder *enc) >>> { >>> enc->task_info(enc, 0x0001, 0, 0, 0); >>> >>> + feedback(enc); >>> + >>> RVCE_BEGIN(0x0201); // destroy >>> RVCE_END(); >>> } >>> diff --git a/src/gallium/drivers/radeon/radeon_vce_52.c >>> b/src/gallium/drivers/radeon/radeon_vce_52.c >>> index a941c476f6..421539c4bd 100644 >>> --- a/src/gallium/drivers/radeon/radeon_vce_52.c >>> +++ b/src/gallium/drivers/radeon/radeon_vce_52.c >>> @@ -458,14 +458,6 @@ static void config_extension(struct rvce_encoder *enc) >>> RVCE_END(); >>> } >>> >>> -static void destroy(struct rvce_encoder *enc) >>> -{ >>> - enc->task_info(enc, 0x0001, 0, 0, 0); >>> - >>> - RVCE_BEGIN(0x0201); // destroy >>> - RVCE_END(); >>> -} >>> - >>> static void feedback(struct rvce_encoder *enc) >>> { >>> RVCE_BEGIN(0x0505); // feedback buffer >>> @@ -474,6 +466,16 @@ static void feedback(struct rvce_encoder *enc) >>> RVCE_END(); >>> } >>> >>> +static void destroy(struct rvce_encoder *enc) >>> +{ >>> + enc->task_info(enc, 0x0001, 0, 0, 0); >>> + >>> + feedback(enc); >>> + >>> + RVCE_BEGIN(0x0201); // destroy >>> + RVCE_END(); >>> +} >>> + >>> static void motion_estimation(struct rvce_encoder *enc) >>> { >>> RVCE_BEGIN(0x0407); // motion estimation >>> -- >>> 2.14.1 >>> >>> ___ >>> mesa-stable mailing list >>> mesa-sta...@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/mesa-stable ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
On 04/04/2018 12:40 PM, Mark Janes wrote: Leo Liu writes: On the CI family, firmware requires the destory command have to be the last command in the IB, moving feedback command after destroy is causing issues on CI cards, so we have to keep the previous logic that moves destroy back to the last command. But as the original issue fixed previously, with the newer family like Vega10, feedback command have to be included inside of the task info command along with destroy command. Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command") Signed-off-by: Leo Liu Cc: mesa-sta...@lists.freedesktop.org These tags seem ambiguous to me. If this commit fixes a specific commit, then the patch should be applied only to stable branches which contain that commit. However, the mesa-stable CC caused this patch to be applied to 17.3, which does *not* contain the broken patch. Leo: did you intend for the mesa-stable CC to cause this patch to be applied to older stable branches? I would like to have this patch apply to branches "17.2", "17.3", "18.0", which got patch titled "radeon/vce: move destroy command before feedback command" And this Cc-ed patch is to fix "radeon/vce: move destroy command before feedback command" Thanks, Leo Release managers: is there a protocol for how this specification should be parsed, when considering a patch for stable? --- src/gallium/drivers/radeon/radeon_vce.c| 1 - src/gallium/drivers/radeon/radeon_vce_40_2_2.c | 2 ++ src/gallium/drivers/radeon/radeon_vce_52.c | 18 ++ 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/gallium/drivers/radeon/radeon_vce.c b/src/gallium/drivers/radeon/radeon_vce.c index 427bf01ed8..c84103e0ac 100644 --- a/src/gallium/drivers/radeon/radeon_vce.c +++ b/src/gallium/drivers/radeon/radeon_vce.c @@ -247,7 +247,6 @@ static void rvce_destroy(struct pipe_video_codec *encoder) enc->fb = &fb; enc->session(enc); enc->destroy(enc); - enc->feedback(enc); flush(enc); si_vid_destroy_buffer(&fb); } diff --git a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c index f1db47d4bd..04e9d7f5e1 100644 --- a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c +++ b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c @@ -421,6 +421,8 @@ static void destroy(struct rvce_encoder *enc) { enc->task_info(enc, 0x0001, 0, 0, 0); + feedback(enc); + RVCE_BEGIN(0x0201); // destroy RVCE_END(); } diff --git a/src/gallium/drivers/radeon/radeon_vce_52.c b/src/gallium/drivers/radeon/radeon_vce_52.c index a941c476f6..421539c4bd 100644 --- a/src/gallium/drivers/radeon/radeon_vce_52.c +++ b/src/gallium/drivers/radeon/radeon_vce_52.c @@ -458,14 +458,6 @@ static void config_extension(struct rvce_encoder *enc) RVCE_END(); } -static void destroy(struct rvce_encoder *enc) -{ - enc->task_info(enc, 0x0001, 0, 0, 0); - - RVCE_BEGIN(0x0201); // destroy - RVCE_END(); -} - static void feedback(struct rvce_encoder *enc) { RVCE_BEGIN(0x0505); // feedback buffer @@ -474,6 +466,16 @@ static void feedback(struct rvce_encoder *enc) RVCE_END(); } +static void destroy(struct rvce_encoder *enc) +{ + enc->task_info(enc, 0x0001, 0, 0, 0); + + feedback(enc); + + RVCE_BEGIN(0x0201); // destroy + RVCE_END(); +} + static void motion_estimation(struct rvce_encoder *enc) { RVCE_BEGIN(0x0407); // motion estimation -- 2.14.1 ___ mesa-stable mailing list mesa-sta...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-stable ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
Emil Velikov writes: > On 4 April 2018 at 17:40, Mark Janes wrote: >> Leo Liu writes: >> >>> On the CI family, firmware requires the destory command have to be the >>> last command in the IB, moving feedback command after destroy is causing >>> issues on CI cards, so we have to keep the previous logic that moves >>> destroy back to the last command. >>> >>> But as the original issue fixed previously, with the newer family like >>> Vega10, >>> feedback command have to be included inside of the task info command along >>> with destroy command. >>> >>> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command") >>> >>> Signed-off-by: Leo Liu >>> Cc: mesa-sta...@lists.freedesktop.org >> >> These tags seem ambiguous to me. If this commit fixes a specific >> commit, then the patch should be applied only to stable branches which >> contain that commit. >> >> However, the mesa-stable CC caused this patch to be applied to 17.3, >> which does *not* contain the broken patch. >> >> Leo: did you intend for the mesa-stable CC to cause this patch to be >> applied to older stable branches? >> >> Release managers: is there a protocol for how this specification should >> be parsed, when considering a patch for stable? >> > If the Fixes tag, reference a commit that is missing in specific > stable branch then obviously the fix is not suitable. > Hence the stable piece than be ignored + alongside a reply to the > patch that it will not be in $stable_branch because $reason. OK, we have violated this policy at least a couple times in the previous release, based on my audits: 2f67c9b17518cf0d2fe946e39e5b8ff5ec2797c5 i965/vec4: Fix null destination register in 3-source instructions 6f69b63896ce2352aaa25f89287133f7f2ba1fab radeon/vce: move feedback command inside of destroy function > Even if offending commit is not part of $stable_branch, getting into > the habit and cross-referencing is very beneficial. > Some customers may use non-stable branch, hence having track of which > fixes they need is a smart move. > > HTH > Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
On 4 April 2018 at 17:40, Mark Janes wrote: > Leo Liu writes: > >> On the CI family, firmware requires the destory command have to be the >> last command in the IB, moving feedback command after destroy is causing >> issues on CI cards, so we have to keep the previous logic that moves >> destroy back to the last command. >> >> But as the original issue fixed previously, with the newer family like >> Vega10, >> feedback command have to be included inside of the task info command along >> with destroy command. >> >> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command") >> >> Signed-off-by: Leo Liu >> Cc: mesa-sta...@lists.freedesktop.org > > These tags seem ambiguous to me. If this commit fixes a specific > commit, then the patch should be applied only to stable branches which > contain that commit. > > However, the mesa-stable CC caused this patch to be applied to 17.3, > which does *not* contain the broken patch. > > Leo: did you intend for the mesa-stable CC to cause this patch to be > applied to older stable branches? > > Release managers: is there a protocol for how this specification should > be parsed, when considering a patch for stable? > If the Fixes tag, reference a commit that is missing in specific stable branch then obviously the fix is not suitable. Hence the stable piece than be ignored + alongside a reply to the patch that it will not be in $stable_branch because $reason. Even if offending commit is not part of $stable_branch, getting into the habit and cross-referencing is very beneficial. Some customers may use non-stable branch, hence having track of which fixes they need is a smart move. HTH Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
On Wed, 2018-04-04 at 09:40 -0700, Mark Janes wrote: > Leo Liu writes: > > > On the CI family, firmware requires the destory command have to be the > > last command in the IB, moving feedback command after destroy is causing > > issues on CI cards, so we have to keep the previous logic that moves > > destroy back to the last command. > > > > But as the original issue fixed previously, with the newer family like > > Vega10, > > feedback command have to be included inside of the task info command along > > with destroy command. > > > > Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command") > > > > Signed-off-by: Leo Liu > > Cc: mesa-sta...@lists.freedesktop.org > > These tags seem ambiguous to me. If this commit fixes a specific > commit, then the patch should be applied only to stable branches which > contain that commit. > > However, the mesa-stable CC caused this patch to be applied to 17.3, > which does *not* contain the broken patch. > > Leo: did you intend for the mesa-stable CC to cause this patch to be > applied to older stable branches? > > Release managers: is there a protocol for how this specification should > be parsed, when considering a patch for stable? > In my case, if a patch is nominated, then it is applied to the proper stable branch. I understand that while the patch probably fixes a commit, it makes sense per se as a valid independent patch. But I think we should modify our script to warn us in this situation and just clarify with the author about the intention. J.A. > > --- > > src/gallium/drivers/radeon/radeon_vce.c| 1 - > > src/gallium/drivers/radeon/radeon_vce_40_2_2.c | 2 ++ > > src/gallium/drivers/radeon/radeon_vce_52.c | 18 ++ > > 3 files changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/src/gallium/drivers/radeon/radeon_vce.c > > b/src/gallium/drivers/radeon/radeon_vce.c > > index 427bf01ed8..c84103e0ac 100644 > > --- a/src/gallium/drivers/radeon/radeon_vce.c > > +++ b/src/gallium/drivers/radeon/radeon_vce.c > > @@ -247,7 +247,6 @@ static void rvce_destroy(struct pipe_video_codec > > *encoder) > > enc->fb = &fb; > > enc->session(enc); > > enc->destroy(enc); > > - enc->feedback(enc); > > flush(enc); > > si_vid_destroy_buffer(&fb); > > } > > diff --git a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c > > b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c > > index f1db47d4bd..04e9d7f5e1 100644 > > --- a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c > > +++ b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c > > @@ -421,6 +421,8 @@ static void destroy(struct rvce_encoder *enc) > > { > > enc->task_info(enc, 0x0001, 0, 0, 0); > > > > + feedback(enc); > > + > > RVCE_BEGIN(0x0201); // destroy > > RVCE_END(); > > } > > diff --git a/src/gallium/drivers/radeon/radeon_vce_52.c > > b/src/gallium/drivers/radeon/radeon_vce_52.c > > index a941c476f6..421539c4bd 100644 > > --- a/src/gallium/drivers/radeon/radeon_vce_52.c > > +++ b/src/gallium/drivers/radeon/radeon_vce_52.c > > @@ -458,14 +458,6 @@ static void config_extension(struct rvce_encoder *enc) > > RVCE_END(); > > } > > > > -static void destroy(struct rvce_encoder *enc) > > -{ > > - enc->task_info(enc, 0x0001, 0, 0, 0); > > - > > - RVCE_BEGIN(0x0201); // destroy > > - RVCE_END(); > > -} > > - > > static void feedback(struct rvce_encoder *enc) > > { > > RVCE_BEGIN(0x0505); // feedback buffer > > @@ -474,6 +466,16 @@ static void feedback(struct rvce_encoder *enc) > > RVCE_END(); > > } > > > > +static void destroy(struct rvce_encoder *enc) > > +{ > > + enc->task_info(enc, 0x0001, 0, 0, 0); > > + > > + feedback(enc); > > + > > + RVCE_BEGIN(0x0201); // destroy > > + RVCE_END(); > > +} > > + > > static void motion_estimation(struct rvce_encoder *enc) > > { > > RVCE_BEGIN(0x0407); // motion estimation > > -- > > 2.14.1 > > > > ___ > > mesa-stable mailing list > > mesa-sta...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-stable > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function
Leo Liu writes: > On the CI family, firmware requires the destory command have to be the > last command in the IB, moving feedback command after destroy is causing > issues on CI cards, so we have to keep the previous logic that moves > destroy back to the last command. > > But as the original issue fixed previously, with the newer family like Vega10, > feedback command have to be included inside of the task info command along > with destroy command. > > Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command") > > Signed-off-by: Leo Liu > Cc: mesa-sta...@lists.freedesktop.org These tags seem ambiguous to me. If this commit fixes a specific commit, then the patch should be applied only to stable branches which contain that commit. However, the mesa-stable CC caused this patch to be applied to 17.3, which does *not* contain the broken patch. Leo: did you intend for the mesa-stable CC to cause this patch to be applied to older stable branches? Release managers: is there a protocol for how this specification should be parsed, when considering a patch for stable? > --- > src/gallium/drivers/radeon/radeon_vce.c| 1 - > src/gallium/drivers/radeon/radeon_vce_40_2_2.c | 2 ++ > src/gallium/drivers/radeon/radeon_vce_52.c | 18 ++ > 3 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/src/gallium/drivers/radeon/radeon_vce.c > b/src/gallium/drivers/radeon/radeon_vce.c > index 427bf01ed8..c84103e0ac 100644 > --- a/src/gallium/drivers/radeon/radeon_vce.c > +++ b/src/gallium/drivers/radeon/radeon_vce.c > @@ -247,7 +247,6 @@ static void rvce_destroy(struct pipe_video_codec *encoder) > enc->fb = &fb; > enc->session(enc); > enc->destroy(enc); > - enc->feedback(enc); > flush(enc); > si_vid_destroy_buffer(&fb); > } > diff --git a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c > b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c > index f1db47d4bd..04e9d7f5e1 100644 > --- a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c > +++ b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c > @@ -421,6 +421,8 @@ static void destroy(struct rvce_encoder *enc) > { > enc->task_info(enc, 0x0001, 0, 0, 0); > > + feedback(enc); > + > RVCE_BEGIN(0x0201); // destroy > RVCE_END(); > } > diff --git a/src/gallium/drivers/radeon/radeon_vce_52.c > b/src/gallium/drivers/radeon/radeon_vce_52.c > index a941c476f6..421539c4bd 100644 > --- a/src/gallium/drivers/radeon/radeon_vce_52.c > +++ b/src/gallium/drivers/radeon/radeon_vce_52.c > @@ -458,14 +458,6 @@ static void config_extension(struct rvce_encoder *enc) > RVCE_END(); > } > > -static void destroy(struct rvce_encoder *enc) > -{ > - enc->task_info(enc, 0x0001, 0, 0, 0); > - > - RVCE_BEGIN(0x0201); // destroy > - RVCE_END(); > -} > - > static void feedback(struct rvce_encoder *enc) > { > RVCE_BEGIN(0x0505); // feedback buffer > @@ -474,6 +466,16 @@ static void feedback(struct rvce_encoder *enc) > RVCE_END(); > } > > +static void destroy(struct rvce_encoder *enc) > +{ > + enc->task_info(enc, 0x0001, 0, 0, 0); > + > + feedback(enc); > + > + RVCE_BEGIN(0x0201); // destroy > + RVCE_END(); > +} > + > static void motion_estimation(struct rvce_encoder *enc) > { > RVCE_BEGIN(0x0407); // motion estimation > -- > 2.14.1 > > ___ > mesa-stable mailing list > mesa-sta...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-stable ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev