Re: [PATCH v2 5/7] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 05/10/2022 23:58, Marijn Suijten wrote:
> On 2022-10-05 22:31:43, Dmitry Baryshkov wrote:
>> On Wed, 5 Oct 2022 at 21:17, Marijn Suijten
>> <marijn.suijten@somainline.org> wrote:
>>>
>>> drm_dsc_config's bits_per_pixel field holds a fractional value with 4
>>> bits, which all panel drivers should adhere to for
>>> drm_dsc_pps_payload_pack() to generate a valid payload.  All code in the
>>> DSI driver here seems to assume that this field doesn't contain any
>>> fractional bits, hence resulting in the wrong values being computed.
>>> Since none of the calculations leave any room for fractional bits or
>>> seem to indicate any possible area of support, disallow such values
>>> altogether.
>>>
>>> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 25 +++++++++++++++++--------
>>>   1 file changed, 17 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index f42794cdd4c1..4717d49d76be 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -33,7 +33,7 @@
>>>
>>>   #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>
>>> -static int dsi_populate_dsc_params(struct drm_dsc_config *dsc);
>>> +static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc_config *dsc);
>>>
>>>   static int dsi_get_version(const void __iomem *base, u32 *major, u32 *minor)
>>>   {
>>> @@ -908,6 +908,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>          u32 va_end = va_start + mode->vdisplay;
>>>          u32 hdisplay = mode->hdisplay;
>>>          u32 wc;
>>> +       int ret;
>>>
>>>          DBG("");
>>>
>>> @@ -943,7 +944,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>                  /* we do the calculations for dsc parameters here so that
>>>                   * panel can use these parameters
>>>                   */
>>> -               dsi_populate_dsc_params(dsc);
>>> +               ret = dsi_populate_dsc_params(msm_host, dsc);
>>> +               if (ret)
>>> +                       return;
>>>
>>>                  /* Divide the display by 3 but keep back/font porch and
>>>                   * pulse width same
>>> @@ -1769,7 +1772,7 @@ static char bpg_offset[DSC_NUM_BUF_RANGES] = {
>>>          2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12
>>>   };
>>>
>>> -static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>>> +static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc_config *dsc)
>>>   {
>>>          int mux_words_size;
>>>          int groups_per_line, groups_total;
>>> @@ -1780,6 +1783,12 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>>>          int data;
>>>          int final_value, final_scale;
>>>          int i;
>>> +       u16 bpp = dsc->bits_per_pixel >> 4;
>>> +
>>> +       if (dsc->bits_per_pixel & 0xf) {
>>> +               DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support fractional bits_per_pixel\n");
>>> +               return -EINVAL;
>>> +       }
>>>
>>>          dsc->rc_model_size = 8192;
>>>          dsc->first_line_bpg_offset = 12;
>>> @@ -1801,7 +1810,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>>>          }
>>>
>>>          dsc->initial_offset = 6144; /* Not bpp 12 */
>>> -       if (dsc->bits_per_pixel != 8)
>>> +       if (bpp != 8)
>>>                  dsc->initial_offset = 2048;     /* bpp = 12 */
>>>
>>>          mux_words_size = 48;            /* bpc == 8/10 */
>>> @@ -1824,14 +1833,14 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>>>           * params are calculated
>>>           */
>>>          groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
>>> -       dsc->slice_chunk_size = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
>>> +       dsc->slice_chunk_size = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
>>
>> I'd still prefer if we can get closer to drm_dsc_compute_rc_parameters().
>> The mentioned function has the following code:
>>
>> vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width *
>>
>> vdsc_cfg->bits_per_pixel,
>>                                                            (8 * 16));
> 
> Fwiw this discussion happened in dsi_update_dsc_timing() where a similar
> calculation was the sole user of bits_per_pixel.  This function,
> dsi_populate_dsc_params(), has more uses of bits_per_pixel so it made
> more sense to compute and document this "discrepancy" up front.
> drm_dsc_compute_rc_parameters() doesn't document where this "16" comes
> from, unfortunately (requiring knowledge of the contents of the struct).
> 
>> In fact, could you please take a look if we can switch to using this
>> function and drop our code?
> 
> There's alread a:
> 
> 	/* FIXME: need to call drm_dsc_compute_rc_parameters() so that rest of
> 	 * params are calculated
> 	 */
> 
> And it was trivial to replace everything below that comment with this
> function call; I have not checked the math in detail but it assigns
> _every_ field that was also assigned here, and the resulting values
> provide an identical DCS PPS (which I happened to be printing to compare
> to downstream, leading to find all the issues solved in this series) and
> working phone screen.
> 
> Makes me wonder why this wasn't caught in review and replaced from the
> get-go...

Good question. Partially it was because everybody wanted to get DSC 
support in to unblock other features. Thus DSC supporting code received 
several bumps afterwards.

> Do you want me to do this in a v3, before applying this fractional-bits
> fix?  At that point this becomes the only user of bits_per_pixel:

Yes, please. This sounds like a perfect solution.

> 
> 	dsc->initial_offset = 6144; /* Not bpp 12 */
> 	if (bpp != 8)
> 		dsc->initial_offset = 2048;	/* bpp = 12 */
> 
> Note that intel_vdsc.c has the exact same code right where they fill
> vdsc_cfg->initial_offset:
> 
> 	int bpp = vdsc_cfg->bits_per_pixel >> 4;
> 
> I'm inclined to leave this as it is.
> 
> - Marijn

-- 
With best wishes
Dmitry