Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

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

On Mon, 2 May 2022 at 02:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/1/2022 1:06 PM, Marijn Suijten wrote:
> > On 2022-04-30 12:25:57, Abhinav Kumar wrote:
> >>
> >>
> >> On 4/30/2022 11:58 AM, 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@intel.com>
> >>>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>
> >>> I pointed this out in review multiple times, so you'll obviously get my:
> >>>
> >>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> >>>
> >>> (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)
> >>
> >> The top 16 bits contain information for stream 1.
> >>
> >> Stream 1 is unused. And whatever is the reset value we should retain that.
> >>
> >> So this patch is correct.
> >
> > I was not debating the correctness of this patch, quite the contrary:
> > it's already much better than it was before.
> >
> > I'm simply asking whether we should prevent `reg` (not `reg_ctrl`!)
> > from having anything in the top 16 bits, which would overwrite the reset
> > value for stream 1 which you correctly say should be retained as it is.
> > It seems unlikely that this happens, but better be safe than sorry?
>
> Wouln't this macro already make sure that 'reg' doesnt have anything in
> the top 16 bits? Its doing a & with 0x00003f00
>
> #define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK
> 0x00003f00
> #define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT   8
> static inline uint32_t
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
> {
>      return ((val) <<
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) &
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
> }
>
> Did you have anything else in mind? If so, please suggest.
>
> >
> > Looking at the DSI register definition for DSC [1] again, I wonder if
> > there's support for defining a common bitfield layout and reusing it
> > thrice: the two channels for CMD mode and a single use for VIDEO.  Don't
> > think that'd make the kernel code better though if even possible at all.
> >
>
> We can have a common bitfield layout for the two channels for command mode.
>
> So we can do something like below for common fields:
>
> -static inline uint32_t
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
> +static inline uint32_t
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM_DATATYPE(uint32_t val, uint32_t
> stream_id)
>   {
> -       return ((val) <<
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) &
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
> +       return ((val) <<
> (DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT + (stream_id
> * 16)) & DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
>   }

NAK. Please express this in the xml source. This file is autogenerated.

>
> Video mode can also use all of these except for WC as that field is not
> present for cmd modes.
>
> This can go as a separate change .
>
> I can push this and perhaps get a Tested-by from Vinod as I dont have a
> setup to re-validate this.
>
> Thanks
>
> Abhinav
> > [1]: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14967/diffs
> >
> > - Marijn
> >
> >>>
> >>> However, this seems to indicate that the DSC patch series has been
> >>> approved and merged somehow??
> >>>
> >>>> ---
> >>>>
> >>>> 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