Re: [PATCH v1 10/14] drm/msm/disp/dpu: add supports of DSC encoder v1.2 engine

[Marijn Suijten <marijn.suijten@somainline.org>]

On 2023-01-24 15:52:46, Kuogee Hsieh wrote:

<snip>

If only replying to a small chunk somewhere in the middle of a diff
and/or large review, please cut out unnecessary bits to make your reply
easier to find :)

> >> +	data = (dsc->flatness_min_qp & 0x1f);
> >> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
> >> +	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;
> > dpu_hw_dsc.c computes this on the fly.  After removing that, and
> > using initial_lines from the function parameters, only
> > dsc_info->num_active_ss_per_enc remains.  Do you really need that
> > msm_display_dsc_info struct here, do you need it at all?
> 
> I ported these code from our down stream code base.
> 
> I make it work first, then clean it up will follow.
> 
> I submit it for review since it looks like you guy like to have code sooner.

Correct, I was looking forward to these patches albeit complete with the
promised DSI support from Jessica, which still seems to be pending.

When sending patches to that extent, with the intent of getting quick
turnaround but knowing that they are not ready for prime time yet (or
were they, based on your "submit it for review" mention? Don't you mean
testing?), please annotate the series with an RFC tag accompanied with a
description what still needs to be done and why.  That would have saved
a great deal of comments and review.

> yes, eliminate msm_display_dsc_info is my next target and hope it can be 
> done.

Thank you.  Again, if that was the intent from the get-go, that's
perfect material to put in an RFC series' cover letter.

- Marijn