Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing
On 02-05-22, 10:43, Marijn Suijten wrote: > On 2022-05-02 01:44:20, Dmitry Baryshkov wrote: > that require DSC for the screen to work. I've been told the series > didn't result in positive screen output way back in its infancy, but I would be intrested to hear about that. I have only pixel3 at my disposal so tested on that. I would be willing to help with more testing efforts. Also, to get DSC to work, the panel needs to be set as well... > I'll re-evaluate and send fixes or improvements if/when necessary. That would be nice -- ~Vinod
Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing
On 30-04-22, 20:55, 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] Reviewed-by: Vinod Koul Tested on pixel3: Tested-by: Vinod Koul -- ~Vinod
Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing
On 2022-05-02 13:02:09, Dmitry Baryshkov wrote: > [snip] > > How would you represent this in XML? I was hoping for a method that > > allows to construct the value in a generic way, without register names, > > and then simply have a "register macro" that moves (and perhaps masks) > > the preconstructed value into the right place. A bit like how `enum`s > > are currently set up in XML, but with bit ranges for the values and > > macros to set a value. > > > > I think I've _partially_ found what I was looking for: a ``. > > However, I don't know if we can utilize this multiple times within a > > single `reg32`, once with an offset for stream1. Alas, it's just > > bikeshedding at this point. > > Unfortunately the following naïve patch doesn't work, stream1 bits are > still defined in the 0:15 bit space. One would have to modify rnn tool > to make sure that it takes into account the low/high parts of the > bitfield when generating offsets/masks. > > diff --git a/src/freedreno/registers/dsi/dsi.xml > b/src/freedreno/registers/dsi/dsi.xml > index f2eef4ff41ae..b0166628ad0a 100644 > --- a/src/freedreno/registers/dsi/dsi.xml > +++ b/src/freedreno/registers/dsi/dsi.xml > @@ -361,22 +361,19 @@ > xsi:schemaLocation="http://nouveau.freedesktop.org/ rules-ng.xsd"> > > > > - > - > + > > type="uint"/> > type="uint"/> > > + > + > + > +type="COMPRESSION_MODE_CTRL"/> > > > -type="uint"/> > -type="uint"/> > -type="uint"/> > - > -type="uint"/> > -type="uint"/> > -type="uint"/> > - > +type="COMPRESSION_MODE_CTRL"/> > +type="COMPRESSION_MODE_CTRL"/> > > > type="uint"/> This is approximately what I was aiming for. `inline="true"` does "inline" all the individual bitfields so that they're prefixed with the reg32+bitfield name again, right? That's what I wanted to avoid :) - Marijn
Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing
On 02/05/2022 11:34, Marijn Suijten wrote:
On 2022-05-01 16:56:45, Abhinav Kumar wrote:
[snip]
Wouln't this macro already make sure that 'reg' doesnt have anything in
the top 16 bits? Its doing a & with 0x3f00
Like I said, it is unlikely that this happens, only if someone starts
changing the code that assigns to `reg` which is unlikely to pass review
anyway.
[snip]
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;
}
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.
How would you represent this in XML? I was hoping for a method that
allows to construct the value in a generic way, without register names,
and then simply have a "register macro" that moves (and perhaps masks)
the preconstructed value into the right place. A bit like how `enum`s
are currently set up in XML, but with bit ranges for the values and
macros to set a value.
I think I've _partially_ found what I was looking for: a ``.
However, I don't know if we can utilize this multiple times within a
single `reg32`, once with an offset for stream1. Alas, it's just
bikeshedding at this point.
Unfortunately the following naïve patch doesn't work, stream1 bits are
still defined in the 0:15 bit space. One would have to modify rnn tool
to make sure that it takes into account the low/high parts of the
bitfield when generating offsets/masks.
diff --git a/src/freedreno/registers/dsi/dsi.xml
b/src/freedreno/registers/dsi/dsi.xml
index f2eef4ff41ae..b0166628ad0a 100644
--- a/src/freedreno/registers/dsi/dsi.xml
+++ b/src/freedreno/registers/dsi/dsi.xml
@@ -361,22 +361,19 @@
xsi:schemaLocation="http://nouveau.freedesktop.org/ rules-ng.xsd">
-
-
+
type="uint"/>
type="uint"/>
+
+
+
+ type="COMPRESSION_MODE_CTRL"/>
- type="uint"/>
- type="uint"/>
- type="uint"/>
-
- type="uint"/>
- type="uint"/>
- type="uint"/>
-
+ type="COMPRESSION_MODE_CTRL"/>
+ type="COMPRESSION_MODE_CTRL"/>
type="uint"/>
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing
On 2022-05-01 16:56:45, Abhinav Kumar wrote:
> [snip]
> Wouln't this macro already make sure that 'reg' doesnt have anything in
> the top 16 bits? Its doing a & with 0x3f00
Like I said, it is unlikely that this happens, only if someone starts
changing the code that assigns to `reg` which is unlikely to pass review
anyway.
> [snip]
> 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;
> }
>
> 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.
How would you represent this in XML? I was hoping for a method that
allows to construct the value in a generic way, without register names,
and then simply have a "register macro" that moves (and perhaps masks)
the preconstructed value into the right place. A bit like how `enum`s
are currently set up in XML, but with bit ranges for the values and
macros to set a value.
I think I've _partially_ found what I was looking for: a ``.
However, I don't know if we can utilize this multiple times within a
single `reg32`, once with an offset for stream1. Alas, it's just
bikeshedding at this point.
- Marijn
Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing
On Mon, 2 May 2022 at 02:56, Abhinav Kumar 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
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Dmitry Baryshkov
> >>>
> >>> I pointed this out in review multiple times, so you'll obviously get my:
> >>>
> >>> Reviewed-by: Marijn Suijten
> >>>
> >>> (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 0x)
> >>
> >> 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 0x3f00
>
> #define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK
> 0x3f00
> #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 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 &= ~0x;
> 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);
>
> -
Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing
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
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Dmitry Baryshkov
I pointed this out in review multiple times, so you'll obviously get my:
Reviewed-by: Marijn Suijten
(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 0x)
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 0x3f00
#define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK
0x3f00
#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;
}
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 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 &= ~0x;
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
Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing
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
> >> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> >> Signed-off-by: Dmitry Baryshkov
> >
> > I pointed this out in review multiple times, so you'll obviously get my:
> >
> > Reviewed-by: Marijn Suijten
> >
> > (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 0x)
>
> 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?
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.
[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 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 &= ~0x;
> >>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
> >>
Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing
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
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Dmitry Baryshkov
I pointed this out in review multiple times, so you'll obviously get my:
Reviewed-by: Marijn Suijten
(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 0x)
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.
However, this seems to indicate that the DSC patch series has been
approved and merged somehow??
---
Changes since v1:
- Fix c 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 &= ~0x;
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
