[Freedreno] [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods

[Marijn Suijten]

On 2023-05-11 13:05:15, Abhinav Kumar wrote:
<snip>
> > Don't think the DP series alone should point that out, as it heavily
> > depends on the relation between slice size and bpp parameters, and
> > whether those end up with a fractional part or not (are you able to test
> > and confirm all those combinations?).  Regardless it seems more natural
> > to use slice_chunk_size which is used in various other ways and sent in
> > the PPS: it wouldn't make sense to adhere to a different slice byte
> > count elsewhere.
> > 
> 
> They are not totally different.
> 
> I guess what Jessica is trying to say here is we will have to do more 
> validation with slice/bpp combinations for DP to conclude whether there 
> is a difference in math. We can go with two approaches:
> 
> 1) Either go with slice_chunk_size for now and re-validate with the 
> monitors we have and drop this particular helper
> 2) Keep this particular helper for now and upon more validation if we 
> see it's same across more combinations, drop this later.

I would argue that if we need a different rounding strategy on the
number of bytes within a slice, shouldn't the DRM helper calculating
slice_chunk_size be updated as well?  This sounds/feels like a thing
that is specified in the DSC docs, as all the parameters involved are
part of the DSC spec (but I am not currently in a position to review
that within the coming few days).

> I just have a slight preference for (2), but looks like your preference 
> is for (1) but this is just more validation work for us which is fine I 
> guess this time since we will revalidate DP again anyway.

Thanks, that's appreciated, but do make sure to test the cases where the
rounding method actually makes a difference in the resulting value.

> >> I also want to note that this math has stayed the same throughout all 7
> >> revisions. In the interest of making review more efficient, I think it
> >> would be helpful to point out important details like this early on in
> >> the process. That way we can address major concerns early on and keep
> >> the number of revisions per series low.
> > 
> > I've already expressed to Abhinav and you that the revisions for all
> > these series were coming in way too hot, leaving no time for review and
> > discussions before the next revision hits the list.  If you want to keep
> > the number of revisions low, v8 shouln't have already been sent.
> > 
> 
> You had the concern for DSC 1.2 DPU series from kuogee. So to respect 
> that we held that back the entire end of last week and early this week 
> (~6 days) and posted today.
> 
> Now, you have the same concern with this series from jessica. Sorry, as 
> much as i would like to get your feedback on every series, I cannot hold 
> back every QC display developer to wait for your reviews on patch rev 1 
> before posting patch rev 2. They all work mostly independently. So if 
> one reviewer say dmitry has reviewed one revision and we agreed on the 
> comments, in the absence of another comment from another reviewer, the 
> developer is going to post the next revision for more reviews as its 
> more efficient to manage their time as they are in the same context. 
> This is nothing unusual from normal upstream development.

I am not saying that it is unusual to send many revisions (though in
"quick succession", which is only loosely defined).  Just that it is
unusual to "demand" reviews (which are seemingly perceived as requesting
a lot, even though I don't think I'm asking anything outrageous here) to
stop when the revisions reach a certain number, or to have happened
early on in the series.  I understand it's annoying from a developer
perspective, but sometimes it is what it is.

This whole process would be better if there's a more explicit list of
"these specific people have to test, review and sign off on every patch"
before this series is ready to be merged, but I understand that people
do not want to commit to that from both sides.

> If by the time you start reviewing its going to be revision 7 or 8, then 
> start it as a fresh review which it is for you, folks are obviously 
> going to question why you didnt review revisions 1-7 so far. So I cannot 
> take sides on this.

Fine to question it, and my answer is: besides having a hard time
finding enough free time to look at this, I also want to test it by
piecing together all the various series, and besides regression-testing
on sdm845 even managed to use these series in a very positive way to
finally (albeit with some additional fixes) get working DSC and panels
on my SM8150 and SM8250 boards.  Meaning I have even more hardware and
configurations to test, valudate, **and provide feedback** on, taking
away from the review time initially.

> I wish I could help more to accommodate your schedules but its hard to 
> control who pushes when with distributed development and tell each of 
> them to hold back.
> 
> > I desire to review *and test* all these patches (which I believe is in
> > everyone's best interest) and have not initially been able to do so as
> > all these efforts come out of my free time, which is sometimes limited
> > of has been used to finalize the INTF TE series (which is a dependency
> > of these series).
> > 
> > It should be pretty obvious to see that this patch has not had a reply
> > from me on any of the previous revisions, and there are more patches
> > that I have not been able to comment on yet.
> > 
> > The alternative to that is of course not seeing enough value in my
> > review and testing (or ""late"" comments...) and merging it regardless
> > at some point, but that's not up to me to decide.
> > 
> > - Marijn
> 
> Thats true even for mine or other's reviews :) that if i am not able to 
> get to some patches on time and other reviewers are happy with it they 
> do get landed in the tree. Has happened many times and I have not 
> complained. I just fix the issues on top of them (which again happens a 
> lot) and move on.

That is totally fine, but in this case the series is not merged yet (nor
seems to happen within the next couple of hours) so I rather share
findings rather than keeping them on a private list to send
cleanups/fixes days after it lands.  Especially in the event that this
takes a few more revisions.  In other words, I understand that my review
gets rightfully ignored/dropped if the maintainer has already picked or
is about to pick it.

> So there doesnt seem to be any rule as such today that wait for this 
> particular developer to review and validate.

Exactly.  As mentioned briefly above this process would involve
committing to a fixed list of reviewers to at least have r-b'd or
replied to every patch, but (as also said above) I can see from both
sides that might not be desirable to commit to.

Thanks for the patience and cooperation thus far!  I'll try to give this
another review- and testing round over the weekend.

- Marijn

<snip>