| 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: <e594fed4-fc01-477b-1e7c-d1d58e1a0dda@quicinc.com>
Date: Thu, 22 Jun 2023 16:04:30 -0700
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Marijn Suijten <marijn.suijten@...ainline.org>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Jessica Zhang <quic_jesszhan@...cinc.com>,
Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
<linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
<freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] drm/msm/dsi: Enable DATABUS_WIDEN for DSI command
mode
On 6/14/2023 3:03 AM, Marijn Suijten wrote:
> On 2023-06-14 10:49:31, Dmitry Baryshkov wrote:
>> On 14/06/2023 04:57, Jessica Zhang wrote:
>>> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
>>> 48 bits of compressed data per pclk instead of 24.
>>>
>>> For all chipsets that support this mode, enable it whenever DSC is
>>> enabled as recommend by the hardware programming guide.
>>>
>>> Only enable this for command mode as we are currently unable to validate
>>> it for video mode.
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
>>> ---
>>>
>>> Note: The dsi.xml.h changes were generated using the headergen2 script in
>>> envytools [1], but the changes to the copyright and rules-ng-ng source file
>>> paths were dropped.
>>>
>>> [1] https://github.com/freedreno/envytools/
>>>
>>> drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
>>> drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
>>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>> index a4a154601114..2a7d980e12c3 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>> @@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
>>> return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
>>> }
>>> #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE 0x00010000
>>> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN 0x00100000
>>>
>>> #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL 0x000001b8
>>> #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK 0x0000003f
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 5d7b4409e4e9..1da5238e7105 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>> u32 hdisplay = mode->hdisplay;
>>> u32 wc;
>>> int ret;
>>> + bool widebus_supported = msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
>>> + msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
>>> +
>>>
>>> DBG("");
>>>
>>> @@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>> *
>>> * hdisplay will be divided by 3 here to account for the fact
>>> * that DPU sends 3 bytes per pclk cycle to DSI.
>>> + *
>>> + * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
>>> + * instead of 3
>>
>> This is useless, it is already obvious from the code below. Instead
>> there should be something like "wide bus extends that to 6 bytes per
>> pclk cycle"
>
> Yes please. In general, don't paraphrase the code, but explain _why_ it
> is doing what it does. Saying that the widebus feature doubles the
> bandwidth per pclk tick is much more clear than "divide by 6 instead of
> 3" - we can read that from the code.
>
> Overall comments have been very good so far (such as the original "to
> account for the fact that DPU sends 3 bytes per pclk cycle"), though!
>
>>> */
>>> - hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>> + if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) && widebus_supported)
>>> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
>>> + else
>>> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>> +
>>> h_total += hdisplay;
>>> ha_end = ha_start + hdisplay;
>>> }
>>> @@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
>>> DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
>>> DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
>>> +
>>> + if (msm_host->dsc && widebus_supported) {
>>> + u32 mdp_ctrl2 = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
>>> +
>>> + mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
>>> + dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
>>
>> Is widebus applicable only to the CMD mode, or video mode can employ it too?
>
> The patch description states that it was not tested on video-mode yet,
> so I assume it will. But this should also be highlighted with a comment
> (e.g. /* XXX: Allow for video-mode once tested/fixed */), _especially_
> on the check for MIPI_DSI_MODE_VIDEO above.
>
Sure, we can leave a comment.
> If I understand this correctly DSC is not working for video mode at all
> on these setups, right? Or no-one was able to test it? I'm inclined to
> request dropping these artifical guards to have as little friction as
> possible when someone starts enabling and testing this - and less
> patches removing artificial bounds in the future.
>
Noone was able to test it. Like I have said before, we dont have or have
not brought up any DSI DSC panel with video mode. DP will be the first
to validate the video mode path for DSC so even that time we cannot test
DSI with DSC on video mode.
I think we should find a panel which supports cmd and video mode ( I
believe one of the HDKs does have that ) and bring that one up first to
validate this.
I believe we should keep this checks with the comment you have
suggested. If someone tests it and then removes it, I am comfortable
with that.
Till then, I would rather guard that configuration.
> - Marijn
Powered by blists - more mailing lists