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

[Jessica Zhang]

On 5/15/2023 3:01 PM, Marijn Suijten wrote:
> 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".

Sure, sounds good.

> 
> <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?

Got it, I see why you want to make that change now. DP only uses 
get_slice_per_intf() to get eol_byte_num similarly to DSI, so I can just 
do that then.

Thanks,

Jessica Zhang

> 
> - Marijn