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