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; }
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
- 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);-- 2.35.1