On 01/05/2022 23:41, Marijn Suijten wrote:
On 2022-04-30 22:28:42, Dmitry Baryshkov wrote:
On 30/04/2022 21:58, Marijn Suijten wrote:
On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
The downstream uses read-modify-write for updating command mode
compression registers. Let's follow this approach. This also fixes the
following warning:
drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
Reported-by: kernel test robot <lkp@xxxxxxxxx>
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
I pointed this out in review multiple times, so you'll obviously get my:
I think I might have also pointed this out once (and then forgot to
check that the issue was fixed by Vinod).
Reviewed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
(But are you sure there's nothing else to clear in the 1st CTRL
register, only the lowest 16 bits? That should mean `reg` never
contains anything in 0xffff0000)
Judging from the downstream the upper half conains the same fields, but
used for other virtual channel. I didn't research what's the difference
yet. All the dtsi files that I have here at hand use
'qcom,mdss-dsi-virtual-channel-id = <0>;'
As replied to Abhinav I'm simply asking whether we should be strict
and add `reg & 0xffff` to prevent accidentally writing the top 16 bits,
which are stream 1. It doesn't seem like the current code can hit that
though, with all the macros using masks internally already; but it's
still a little scary since we're assuming the registers for VIDEO are
identical to CMD (as mentioned in the reply to Abhinav: I wonder if it's
possible to declare a a pair of bitfields as a single layout in the XML,
and reuse that across CMD's stream 0/1 and the VIDEO register).
However, this seems to indicate that the DSC patch series has been
approved and merged somehow??
Pending inclusion, yes. If Vinod missed or ignored any other review
points, please excuse Abhinav and me not noticing that.
Vinod replied to most of the comments so I'll double-check if they were
applied in the way requested. Note that I don't always post a full
review up-front if it gets too noisy: I'll simply start out with the
most glaring issues and go in more detail in further revisions to
prevent drowning everyone in comments.
Can you please take a look at the latest revision posted, if there are
any other missing points. Let's decide if there are grave issues or we
can work them through.
Thanks, I'll queue that up this week. One of my thus-far-unaddressed
issues with the patches which can't be addressed in hindsight is the
relatively lackluster commit messages: most happen to be repeating the
title in a slightly different way without any additional clarification,
which doesn't fit the upstream spirit at all.
I understand that the reference manuals can't be quoted nor am I asking
to, but a little more insight in the process and details of each patch
goes a very long way. Explain how certain calculations work or came to
be, link to public sources detailing the protocol, explain design
decisions or document how to use/test the feature and describe possible
limitations.
I usually link contributors to [1], but it's a bit of an odd read at
times.
[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
In any case, given that you've already sent this patch and another three
patches [2] fixing/cleaning up the series tells me it's far from ready.
Most of this should just be handled - or have been handled - in review
and amended?
During the review time we agreed that [2] would come as a separate
change It is an API change that would make using panel-bridge easier,
but isn't otherwise required.
I have been working towards more logical drm_bridge/drm_bridge_connector
chains employing panel-bridge and display-connector where required, [2]
is a part of that effort (as well as few other patches that hit
dri-devel in the last few days).
[2]: https://lore.kernel.org/linux-arm-msm/20220501151220.3999164-1-dmitry.baryshkov@xxxxxxxxxx/T/#t
I'll look through v14 again this week and let you know.
- Marijn
---
Changes since v1:
- Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
(Abhinav)
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c983698d1384..a95d5df52653 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
+ reg_ctrl &= ~0xffff;
reg_ctrl |= reg;
+
+ reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
- dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
+ dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
} else {
dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
--
2.35.1
--
With best wishes
Dmitry
--
With best wishes
Dmitry
