Re: [Freedreno] [PATCH v3 12/13] drm/msm/dsi: Add support for DSC configuration

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

On 2022-03-22 22:46:50, Vinod Koul wrote:
> On 17-02-22, 16:11, Marijn Suijten wrote:
> > Hi Vinod,
> > 
> > Thanks for taking time to go through this review, please find some
> > clarifications below.
> > 
> > On 2022-02-17 16:44:04, Vinod Koul wrote:
> > > Hi Marijn,
> > > 
> > > On 11-12-21, 01:03, Marijn Suijten wrote:
> > > 
> > > > > +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> > > > > +				  int pic_width, int pic_height)
> > > > 
> > > > This function - adopted from downstream - does not seem to perform a
> > > > whole lot, especially without the modulo checks against the slice size.
> > > > Perhaps it can be inlined?
> > > 
> > > Most of the code here is :)
> > > 
> > > This was split from downstream code to check and update dimension. We
> > > can inline this, or should we leave that to compiler. I am not a very
> > > big fan of inlining...
> > 
> > It doesn't seem beneficial to code readability to have this function,
> > which is only called just once and also has the same struct members read
> > in a `DBG()` directly, abstracted away to a function.  Not really
> > concerned about generated code/performance FWIW.
> > 
> > Also note that the caller isn't checking the `-EINVAL` result...
> 
> I have made this void inline.

Perhaps there is a misunderstanding here: with inlining I am referring
to the process of transplanting the _function body_ to the only
call-site, not adding the `inline` keyword nor changing this to `void`.

The checks that make this function return `-EINVAL` seem valid, so the
caller should check it instead of removing the return?

> > > > 
> > > > > +{
> > > > > +	if (!dsc || !pic_width || !pic_height) {
> > > > > +		pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	dsc->drm->pic_width = pic_width;
> > > > > +	dsc->drm->pic_height = pic_height;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > > > >  {
> > > > >  	struct drm_display_mode *mode = msm_host->mode;
> > > > > @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > > > >  		hdisplay /= 2;
> > > > >  	}
> > > > >  
> > > > > +	if (msm_host->dsc) {
> > > > > +		struct msm_display_dsc_config *dsc = msm_host->dsc;
> > > > > +
> > > > > +		/* update dsc params with timing params */
> > > > > +		dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> > 
> > That is, the result code here should be checked (or function inlined).
> 
> This function return void, so no point in checking

It isn't returning `void` in the current patch series that my email is
reviewing, hence explicitly mentioning here that it may have been
overlooked.

Please only convert this to `void` if you are sure that the clause that
originally made `dsi_dsc_update_pic_dim()` return `-EINVAL` on invalid
input is unreachable (if, for example, you moved this check to another
location, say here in `dsi_timing_setup`).

Alas, it's pretty tricky to reason and pose assumptions about code that
I cannot see; we should probably continue this discussion in the next
patch revision depending on how it looks :)

> > [..]
> > Thanks.  I forgot to mention: there seem to be a lot of similarities
> > between the video and commandmode computations, can those possibly be
> > factored out of the if-else to save on duplication and accidental
> > mismatches like these?
> 
> Thanks, this was a good suggestion and am happy to report that I have
> incorporated this and indeed code looks better

Thank you for applying this and the other comments, glad to hear the
code is shaping up and looking forward to the next revision!

- Marijn