[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 <emil.l.velikov at gmail.com> writes:
> 
> > On 5 April 2018 at 20:33, Mark Janes <mark.a.janes at intel.com>
> > wrote:
> > > Emil Velikov <emil.l.velikov at gmail.com> writes:
> > > 
> > > > On 4 April 2018 at 22:50, Mark Janes <mark.a.janes at intel.com>
> > > > wrote:
> > > > > Leo Liu <leo.liu at amd.com> writes:
> > > > > 
> > > > > > On 04/04/2018 12:40 PM, Mark Janes wrote:
> > > > > > > Leo Liu <leo.liu at amd.com> 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 <leo.liu at amd.com>
> > > > > > > > Cc: mesa-stable at 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?
> > </cheeky>
> > 
> > > 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-stable at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-stable
> 
>