| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <14809708-df0b-1003-3f31-4f856b10d600@linaro.org>
Date: Thu, 6 Oct 2022 00:06:39 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Marijn Suijten <marijn.suijten@...ainline.org>,
phone-devel@...r.kernel.org, Rob Clark <robdclark@...il.com>,
Vinod Koul <vkoul@...nel.org>,
~postmarketos/upstreaming@...ts.sr.ht,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...ainline.org>,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
Martin Botka <martin.botka@...ainline.org>,
Jami Kettunen <jami.kettunen@...ainline.org>,
Daniel Vetter <daniel@...ll.ch>,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Sean Paul <sean@...rly.run>,
Thomas Zimmermann <tzimmermann@...e.de>,
Javier Martinez Canillas <javierm@...hat.com>,
Alex Deucher <alexander.deucher@....com>,
Douglas Anderson <dianders@...omium.org>,
Vladimir Lypak <vladimir.lypak@...il.com>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org,
David Airlie <airlied@...ux.ie>
Subject: Re: [PATCH v2 5/7] drm/msm/dsi: Account for DSC's bits_per_pixel
having 4 fractional bits
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@...ainline.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@...ainline.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
Powered by blists - more mailing lists