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

[Marijn Suijten]

On 2023-05-15 13:29:21, Jessica Zhang wrote:
<snip>
> > Const, as requested elsewhere.  But this function is not used anywhere
> > in any of the series (because we replaced the usages with more sensible
> > member accesses like slice_chunk_size).
> 
> Acked.
> 
> I would prefer to keep this helper so that we have a way to easily get 
> BPP information from the DRM DSC config in the future, but if you'd 
> prefer we add this helper then, I'm also ok with that.

The inverse helper is available ad DSC_BPP in drm_dsc_helper.c.  Perhaps
we can move the two variants to the header and define them uniformly?
This isn't MSM-specific it seems (i.e. the format supports fractional
bpp but no RC parameters appear to be defined for such a format yet).

<snip>
> >> + * @dsc: Pointer to drm dsc config struct
> >> + * Returns: Integer value representing pclk per interface
> >> + *
> >> + * Note: This value will then be passed along to DSI and DP for some more
> >> + * calculations. This is because DSI and DP divide the pclk_per_intf value
> >> + * by different values depending on if widebus is enabled.
> > 
> > Can you elaborate what this "note" is trying to tell users of this
> > function?  That they should not use bytes_per_line raw?  That it doesn't
> > actually represent bytes_per_line if the extra calculations mentioned
> > here are not applied?
> 
> The latter -- this method is used for calculating the pclk for DSI and 
> DP. While it does get the raw bytes_per_line, there are some extra 
> calculations needed before it can be set as the pclk_rate. These "extra 
> calculations" are different between DP and DSI.
> 
> For more context, please refer to the earlier revisions of this patch 
> and the usage of the helper in dsi_host.c

Note that I'm not just asking to explain it to me, but to explain it in
the documentation.  The function is named bytes_per_line() but then
Returns: says it returns the pclk per interface, then the note says that
it actually doesn't unless extra calculations are applied.

We can explain the same much more concisely by rewriting Returns and
dropping Note:

    Returns: Integer value representing bytes per line.  DSI and DP need
    to perform further processing/calculations to turn this into
    pclk_per_intf, such as dividing by different values depending on
    if widebus is enabled.

And so we have written the same, just more concisely.  Feel free to
reword it slightly, such as dropping the word "processing".

<snip>
> > Not sure that this helper is useful though: it is
> > only used where msm_dsc_get_slice_per_intf() was already called, so it
> > makes more sense to the reader to just multiply slice_per_intf by
> > slice_chunk_size than to defer to an opaque helper.
> 
> I would prefer to keep this as a helper as this math is common between 
> DP and DSI, and I believe the name of the helper accurately reflects 
> what is being calculated.
> 
> If there's any confusion with the name of the method, I am open to 
> suggestions.

The name is good, I'm just not too keen on it hiding the multiplication
with msm_dsc_get_slice_per_intf() which is already also computed and
available in DSI, and I assume DP too?

- Marijn