Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function

[Juan A. Suarez Romero]

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
> 

Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function

[Ian Romanick]

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

[Mark Janes]

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

[Emil Velikov]

(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

[Mark Janes]

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

[Emil Velikov]

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

[Mark Janes]

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

[Emil Velikov]

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

[Mark Janes]

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 = 
>>> enc->session(enc);
>>> enc->destroy(enc);
>>> -   enc->feedback(enc);
>>> flush(enc);
>>> si_vid_destroy_buffer();
>>> }
>>> 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]

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 = 
enc->session(enc);
enc->destroy(enc);
-   enc->feedback(enc);
flush(enc);
si_vid_destroy_buffer();
}
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

[Mark Janes]

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

[Emil Velikov]

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

[Juan A. Suarez Romero]

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 = 
> > enc->session(enc);
> > enc->destroy(enc);
> > -   enc->feedback(enc);
> > flush(enc);
> > si_vid_destroy_buffer();
> > }
> > 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

[Mark Janes]

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 = 
>   enc->session(enc);
>   enc->destroy(enc);
> - enc->feedback(enc);
>   flush(enc);
>   si_vid_destroy_buffer();
>   }
> 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