| Message ID | 20210715065203.709914-9-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: Add Display Stream Compression Support | expand |
Hi Vinod,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v5.13]
[cannot apply to linus/master v5.14-rc1 next-20210716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Vinod-Koul/drm-msm-Add-Display-Stream-Compression-Support/20210715-145540
base: 62fb9874f5da54fdb243003b386128037319b219
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/e61efb569c28d8036eb18f53763c195c16d8a396
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Vinod-Koul/drm-msm-Add-Display-Stream-Compression-Support/20210715-145540
git checkout e61efb569c28d8036eb18f53763c195c16d8a396
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c: In function 'dpu_encoder_prep_dsc':
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:1911:17: warning: variable 'pic_height' set but not used [-Wunused-but-set-variable]
1911 | int pic_width, pic_height;
| ^~~~~~~~~~
vim +/pic_height +1911 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
1900
1901 static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
1902 struct msm_display_dsc_config *dsc)
1903 {
1904 /* coding only for 2LM, 2enc, 1 dsc config */
1905 struct dpu_encoder_phys *enc_master = dpu_enc->cur_master;
1906 struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];
1907 struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
1908 int this_frame_slices;
1909 int intf_ip_w, enc_ip_w;
1910 int dsc_common_mode;
> 1911 int pic_width, pic_height;
1912 int i;
1913
1914 for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
1915 hw_pp[i] = dpu_enc->hw_pp[i];
1916 hw_dsc[i] = dpu_enc->hw_dsc[i];
1917
1918 if (!hw_pp[i] || !hw_dsc[i]) {
1919 DPU_ERROR_ENC(dpu_enc, "invalid params for DSC\n");
1920 return;
1921 }
1922 }
1923
1924 dsc_common_mode = 0;
1925 pic_width = dsc->drm->pic_width;
1926 pic_height = dsc->drm->pic_height;
1927
1928 dsc_common_mode = DSC_MODE_MULTIPLEX | DSC_MODE_SPLIT_PANEL;
1929 if (enc_master->intf_mode == INTF_MODE_VIDEO)
1930 dsc_common_mode |= DSC_MODE_VIDEO;
1931
1932 this_frame_slices = pic_width / dsc->drm->slice_width;
1933 intf_ip_w = this_frame_slices * dsc->drm->slice_width;
1934
1935 dpu_encoder_dsc_pclk_param_calc(dsc, intf_ip_w);
1936
1937 /*
1938 * dsc merge case: when using 2 encoders for the same stream,
1939 * no. of slices need to be same on both the encoders.
1940 */
1941 enc_ip_w = intf_ip_w / 2;
1942 dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w);
1943
1944 for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
1945 dpu_encoder_dsc_pipe_cfg(hw_dsc[i], hw_pp[i], dsc, dsc_common_mode);
1946 }
1947
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 15/07/2021 09:52, Vinod Koul wrote: > When DSC is enabled in DT, we need to configure the encoder for DSC > configuration, calculate DSC parameters for the given timing. > > This patch adds that support by adding dpu_encoder_prep_dsc() which is > invoked when DSC is enabled in DT > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 142 +++++++++++++++++++- > 1 file changed, 141 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 8d942052db8a..41140b781e66 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -21,12 +21,17 @@ > #include "dpu_hw_intf.h" > #include "dpu_hw_ctl.h" > #include "dpu_hw_dspp.h" > +#include "dpu_hw_dsc.h" > #include "dpu_formats.h" > #include "dpu_encoder_phys.h" > #include "dpu_crtc.h" > #include "dpu_trace.h" > #include "dpu_core_irq.h" > > +#define DSC_MODE_SPLIT_PANEL BIT(0) > +#define DSC_MODE_MULTIPLEX BIT(1) > +#define DSC_MODE_VIDEO BIT(2) This should go into dpu_hw_dsc.h. Ah. They are already defined there and just redefined there. Remove the defines here. It might be cleaner to add bool flags to struct msm_display_dsc_config and then calculate common mode in the dpu_hw_dsc_config(). > + > #define DPU_DEBUG_ENC(e, fmt, ...) DPU_DEBUG("enc%d " fmt,\ > (e) ? (e)->base.base.id : -1, ##__VA_ARGS__) > > @@ -135,6 +140,7 @@ enum dpu_enc_rc_states { > * @cur_slave: As above but for the slave encoder. > * @hw_pp: Handle to the pingpong blocks used for the display. No. > * pingpong blocks can be different than num_phys_encs. > + * @hw_dsc Handle to the DSC blocks used for the display. > * @intfs_swapped: Whether or not the phys_enc interfaces have been swapped > * for partial update right-only cases, such as pingpong > * split where virtual pingpong does not generate IRQs > @@ -180,6 +186,7 @@ struct dpu_encoder_virt { > struct dpu_encoder_phys *cur_master; > struct dpu_encoder_phys *cur_slave; > struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC]; > + struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC]; > > bool intfs_swapped; > > @@ -1008,7 +1015,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, > struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; > - int num_lm, num_ctl, num_pp; > + struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; > + int num_lm, num_ctl, num_pp, num_dsc; > int i, j; > > if (!drm_enc) { > @@ -1061,11 +1069,16 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, > dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, > ARRAY_SIZE(hw_dspp)); > + num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > + drm_enc->base.id, DPU_HW_BLK_DSC, hw_dsc, ARRAY_SIZE(hw_dsc)); > > for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i]) > : NULL; > > + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > + dpu_enc->hw_dsc[i] = i < num_dsc ? to_dpu_hw_dsc(hw_dsc[i]) : NULL; > + > cstate = to_dpu_crtc_state(drm_crtc->state); > > for (i = 0; i < num_lm; i++) { > @@ -1810,10 +1823,133 @@ static void dpu_encoder_vsync_event_work_handler(struct kthread_work *work) > nsecs_to_jiffies(ktime_to_ns(wakeup_time))); > } > > +static void > +dpu_encoder_dsc_pclk_param_calc(struct msm_display_dsc_config *dsc, u32 width) > +{ > + int slice_count, slice_per_intf; > + int bytes_in_slice, total_bytes_per_intf; > + > + if (!dsc || !dsc->drm->slice_width || !dsc->drm->slice_count) { > + DPU_ERROR("Invalid DSC/slices\n"); > + return; > + } > + > + slice_count = dsc->drm->slice_count; > + slice_per_intf = DIV_ROUND_UP(width, dsc->drm->slice_width); > + > + /* > + * If slice_count is greater than slice_per_intf then default to 1. > + * This can happen during partial update. > + */ > + if (slice_count > slice_per_intf) > + slice_count = 1; > + > + bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * > + dsc->drm->bits_per_pixel, 8); > + total_bytes_per_intf = bytes_in_slice * slice_per_intf; > + > + dsc->eol_byte_num = total_bytes_per_intf % 3; > + dsc->pclk_per_line = DIV_ROUND_UP(total_bytes_per_intf, 3); > + dsc->bytes_in_slice = bytes_in_slice; > + dsc->bytes_per_pkt = bytes_in_slice * slice_count; > + dsc->pkt_per_line = slice_per_intf / slice_count; > +} > + > +static void > +dpu_encoder_dsc_initial_line_calc(struct msm_display_dsc_config *dsc, > + u32 enc_ip_width) > +{ > + int ssm_delay, total_pixels, soft_slice_per_enc; > + > + soft_slice_per_enc = enc_ip_width / dsc->drm->slice_width; > + > + /* > + * minimum number of initial line pixels is a sum of: > + * 1. sub-stream multiplexer delay (83 groups for 8bpc, > + * 91 for 10 bpc) * 3 > + * 2. for two soft slice cases, add extra sub-stream multiplexer * 3 > + * 3. the initial xmit delay > + * 4. total pipeline delay through the "lock step" of encoder (47) > + * 5. 6 additional pixels as the output of the rate buffer is > + * 48 bits wide > + */ > + ssm_delay = ((dsc->drm->bits_per_component < 10) ? 84 : 92); > + total_pixels = ssm_delay * 3 + dsc->drm->initial_xmit_delay + 47; > + if (soft_slice_per_enc > 1) > + total_pixels += (ssm_delay * 3); > + dsc->initial_lines = DIV_ROUND_UP(total_pixels, dsc->drm->slice_width); > +} > + > +static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, > + struct dpu_hw_pingpong *hw_pp, > + struct msm_display_dsc_config *dsc, > + u32 common_mode) > +{ > + if (hw_dsc->ops.dsc_config) > + hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode); > + > + if (hw_dsc->ops.dsc_config_thresh) > + hw_dsc->ops.dsc_config_thresh(hw_dsc, dsc); > + > + if (hw_pp->ops.setup_dsc) > + hw_pp->ops.setup_dsc(hw_pp); > + > + if (hw_pp->ops.enable_dsc) > + hw_pp->ops.enable_dsc(hw_pp); I think, we do not need to split these operations, I'd suggest having just hw_dsc->ops.dsc_config() and hw_pp->ops.enable_dsc(), merging dsc_config_thres() and setup_dsc() into respective methods. > +} > + > +static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, > + struct msm_display_dsc_config *dsc) > +{ > + /* coding only for 2LM, 2enc, 1 dsc config */ > + struct dpu_encoder_phys *enc_master = dpu_enc->cur_master; > + struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC]; > + struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC]; > + int this_frame_slices; > + int intf_ip_w, enc_ip_w; > + int dsc_common_mode; > + int pic_width, pic_height; > + int i; > + > + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) { > + hw_pp[i] = dpu_enc->hw_pp[i]; > + hw_dsc[i] = dpu_enc->hw_dsc[i]; > + > + if (!hw_pp[i] || !hw_dsc[i]) { > + DPU_ERROR_ENC(dpu_enc, "invalid params for DSC\n"); > + return; > + } > + } > + > + dsc_common_mode = 0; > + pic_width = dsc->drm->pic_width; > + pic_height = dsc->drm->pic_height; > + > + dsc_common_mode = DSC_MODE_MULTIPLEX | DSC_MODE_SPLIT_PANEL; > + if (enc_master->intf_mode == INTF_MODE_VIDEO) > + dsc_common_mode |= DSC_MODE_VIDEO; > + > + this_frame_slices = pic_width / dsc->drm->slice_width; > + intf_ip_w = this_frame_slices * dsc->drm->slice_width; > + > + dpu_encoder_dsc_pclk_param_calc(dsc, intf_ip_w); > + > + /* > + * dsc merge case: when using 2 encoders for the same stream, > + * no. of slices need to be same on both the encoders. > + */ > + enc_ip_w = intf_ip_w / 2; > + dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w); > + > + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > + dpu_encoder_dsc_pipe_cfg(hw_dsc[i], hw_pp[i], dsc, dsc_common_mode); > +} > + > void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) > { > struct dpu_encoder_virt *dpu_enc; > struct dpu_encoder_phys *phys; > + struct msm_drm_private *priv; > bool needs_hw_reset = false; > unsigned int i; > > @@ -1841,6 +1977,10 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) > dpu_encoder_helper_hw_reset(dpu_enc->phys_encs[i]); > } > } > + > + priv = drm_enc->dev->dev_private; > + if (priv->dsc) > + dpu_encoder_prep_dsc(dpu_enc, priv->dsc); Not quite. This makes dsc config global, while we can have several encoders enabled at once (think of DSI + DP). So the dsc should be a per-encoder setting rather than global. > } > > void dpu_encoder_kickoff(struct drm_encoder *drm_enc) >
On 2021-07-14 23:52, Vinod Koul wrote: > When DSC is enabled in DT, we need to configure the encoder for DSC > configuration, calculate DSC parameters for the given timing. > > This patch adds that support by adding dpu_encoder_prep_dsc() which is > invoked when DSC is enabled in DT correct me if wrong but this commit text is not valid anymore in my opinion. are there any params you are getting from DT now? I thought its all coming from the panel driver directly. > > Signed-off-by: Vinod Koul <vkoul@kernel.org> agree with dmitry's comment's https://patchwork.freedesktop.org/patch/444078/?series=90413&rev=2 instead of dsc being part of priv->dsc it should be per encoder. On top of his comment, I also think that like on the newer chipsets, moving the dsc related encoder configuration to a dpu_encoder_dce.c will help for future expansion of other topologies and also for other compression algorithms. > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 142 +++++++++++++++++++- > 1 file changed, 141 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 8d942052db8a..41140b781e66 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -21,12 +21,17 @@ > #include "dpu_hw_intf.h" > #include "dpu_hw_ctl.h" > #include "dpu_hw_dspp.h" > +#include "dpu_hw_dsc.h" > #include "dpu_formats.h" > #include "dpu_encoder_phys.h" > #include "dpu_crtc.h" > #include "dpu_trace.h" > #include "dpu_core_irq.h" > > +#define DSC_MODE_SPLIT_PANEL BIT(0) > +#define DSC_MODE_MULTIPLEX BIT(1) > +#define DSC_MODE_VIDEO BIT(2) > + > #define DPU_DEBUG_ENC(e, fmt, ...) DPU_DEBUG("enc%d " fmt,\ > (e) ? (e)->base.base.id : -1, ##__VA_ARGS__) > > @@ -135,6 +140,7 @@ enum dpu_enc_rc_states { > * @cur_slave: As above but for the slave encoder. > * @hw_pp: Handle to the pingpong blocks used for the display. No. > * pingpong blocks can be different than num_phys_encs. > + * @hw_dsc Handle to the DSC blocks used for the display. > * @intfs_swapped: Whether or not the phys_enc interfaces have been > swapped > * for partial update right-only cases, such as pingpong > * split where virtual pingpong does not generate IRQs > @@ -180,6 +186,7 @@ struct dpu_encoder_virt { > struct dpu_encoder_phys *cur_master; > struct dpu_encoder_phys *cur_slave; > struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC]; > + struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC]; > > bool intfs_swapped; > > @@ -1008,7 +1015,8 @@ static void dpu_encoder_virt_mode_set(struct > drm_encoder *drm_enc, > struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; > - int num_lm, num_ctl, num_pp; > + struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; > + int num_lm, num_ctl, num_pp, num_dsc; > int i, j; > > if (!drm_enc) { > @@ -1061,11 +1069,16 @@ static void dpu_encoder_virt_mode_set(struct > drm_encoder *drm_enc, > dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, > ARRAY_SIZE(hw_dspp)); > + num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > + drm_enc->base.id, DPU_HW_BLK_DSC, hw_dsc, ARRAY_SIZE(hw_dsc)); > > for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i]) > : NULL; > > + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > + dpu_enc->hw_dsc[i] = i < num_dsc ? to_dpu_hw_dsc(hw_dsc[i]) : NULL; > + > cstate = to_dpu_crtc_state(drm_crtc->state); > > for (i = 0; i < num_lm; i++) { > @@ -1810,10 +1823,133 @@ static void > dpu_encoder_vsync_event_work_handler(struct kthread_work *work) > nsecs_to_jiffies(ktime_to_ns(wakeup_time))); > } > > +static void > +dpu_encoder_dsc_pclk_param_calc(struct msm_display_dsc_config *dsc, > u32 width) > +{ > + int slice_count, slice_per_intf; > + int bytes_in_slice, total_bytes_per_intf; > + > + if (!dsc || !dsc->drm->slice_width || !dsc->drm->slice_count) { > + DPU_ERROR("Invalid DSC/slices\n"); > + return; > + } > + > + slice_count = dsc->drm->slice_count; > + slice_per_intf = DIV_ROUND_UP(width, dsc->drm->slice_width); > + > + /* > + * If slice_count is greater than slice_per_intf then default to 1. > + * This can happen during partial update. > + */ > + if (slice_count > slice_per_intf) > + slice_count = 1; > + > + bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * > + dsc->drm->bits_per_pixel, 8); > + total_bytes_per_intf = bytes_in_slice * slice_per_intf; > + > + dsc->eol_byte_num = total_bytes_per_intf % 3; > + dsc->pclk_per_line = DIV_ROUND_UP(total_bytes_per_intf, 3); > + dsc->bytes_in_slice = bytes_in_slice; > + dsc->bytes_per_pkt = bytes_in_slice * slice_count; > + dsc->pkt_per_line = slice_per_intf / slice_count; > +} > + > +static void > +dpu_encoder_dsc_initial_line_calc(struct msm_display_dsc_config *dsc, > + u32 enc_ip_width) > +{ > + int ssm_delay, total_pixels, soft_slice_per_enc; > + > + soft_slice_per_enc = enc_ip_width / dsc->drm->slice_width; > + > + /* > + * minimum number of initial line pixels is a sum of: > + * 1. sub-stream multiplexer delay (83 groups for 8bpc, > + * 91 for 10 bpc) * 3 > + * 2. for two soft slice cases, add extra sub-stream multiplexer * 3 > + * 3. the initial xmit delay > + * 4. total pipeline delay through the "lock step" of encoder (47) > + * 5. 6 additional pixels as the output of the rate buffer is > + * 48 bits wide > + */ > + ssm_delay = ((dsc->drm->bits_per_component < 10) ? 84 : 92); > + total_pixels = ssm_delay * 3 + dsc->drm->initial_xmit_delay + 47; > + if (soft_slice_per_enc > 1) > + total_pixels += (ssm_delay * 3); > + dsc->initial_lines = DIV_ROUND_UP(total_pixels, > dsc->drm->slice_width); > +} > + > +static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, > + struct dpu_hw_pingpong *hw_pp, > + struct msm_display_dsc_config *dsc, > + u32 common_mode) > +{ > + if (hw_dsc->ops.dsc_config) > + hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode); > + > + if (hw_dsc->ops.dsc_config_thresh) > + hw_dsc->ops.dsc_config_thresh(hw_dsc, dsc); > + > + if (hw_pp->ops.setup_dsc) > + hw_pp->ops.setup_dsc(hw_pp); > + > + if (hw_pp->ops.enable_dsc) > + hw_pp->ops.enable_dsc(hw_pp); > +} > + > +static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, > + struct msm_display_dsc_config *dsc) > +{ > + /* coding only for 2LM, 2enc, 1 dsc config */ > + struct dpu_encoder_phys *enc_master = dpu_enc->cur_master; > + struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC]; > + struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC]; > + int this_frame_slices; > + int intf_ip_w, enc_ip_w; > + int dsc_common_mode; > + int pic_width, pic_height; > + int i; > + > + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) { > + hw_pp[i] = dpu_enc->hw_pp[i]; > + hw_dsc[i] = dpu_enc->hw_dsc[i]; > + > + if (!hw_pp[i] || !hw_dsc[i]) { > + DPU_ERROR_ENC(dpu_enc, "invalid params for DSC\n"); > + return; > + } > + } > + > + dsc_common_mode = 0; > + pic_width = dsc->drm->pic_width; > + pic_height = dsc->drm->pic_height; > + > + dsc_common_mode = DSC_MODE_MULTIPLEX | DSC_MODE_SPLIT_PANEL; > + if (enc_master->intf_mode == INTF_MODE_VIDEO) > + dsc_common_mode |= DSC_MODE_VIDEO; > + > + this_frame_slices = pic_width / dsc->drm->slice_width; > + intf_ip_w = this_frame_slices * dsc->drm->slice_width; > + > + dpu_encoder_dsc_pclk_param_calc(dsc, intf_ip_w); > + > + /* > + * dsc merge case: when using 2 encoders for the same stream, > + * no. of slices need to be same on both the encoders. > + */ > + enc_ip_w = intf_ip_w / 2; > + dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w); > + > + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > + dpu_encoder_dsc_pipe_cfg(hw_dsc[i], hw_pp[i], dsc, dsc_common_mode); > +} > + > void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) > { > struct dpu_encoder_virt *dpu_enc; > struct dpu_encoder_phys *phys; > + struct msm_drm_private *priv; > bool needs_hw_reset = false; > unsigned int i; > > @@ -1841,6 +1977,10 @@ void dpu_encoder_prepare_for_kickoff(struct > drm_encoder *drm_enc) > dpu_encoder_helper_hw_reset(dpu_enc->phys_encs[i]); > } > } > + > + priv = drm_enc->dev->dev_private; > + if (priv->dsc) > + dpu_encoder_prep_dsc(dpu_enc, priv->dsc); > } > > void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
On 29-07-21, 23:54, Dmitry Baryshkov wrote: > On 15/07/2021 09:52, Vinod Koul wrote: > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index 8d942052db8a..41140b781e66 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -21,12 +21,17 @@ > > #include "dpu_hw_intf.h" > > #include "dpu_hw_ctl.h" > > #include "dpu_hw_dspp.h" > > +#include "dpu_hw_dsc.h" > > #include "dpu_formats.h" > > #include "dpu_encoder_phys.h" > > #include "dpu_crtc.h" > > #include "dpu_trace.h" > > #include "dpu_core_irq.h" > > +#define DSC_MODE_SPLIT_PANEL BIT(0) > > +#define DSC_MODE_MULTIPLEX BIT(1) > > +#define DSC_MODE_VIDEO BIT(2) > > This should go into dpu_hw_dsc.h. Ah. They are already defined there and > just redefined there. Remove the defines here. Sure, updated > It might be cleaner to add bool flags to struct msm_display_dsc_config and > then calculate common mode in the dpu_hw_dsc_config(). How would that be better than calculating here? I dont see much of an advantage. > > +static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, > > + struct dpu_hw_pingpong *hw_pp, > > + struct msm_display_dsc_config *dsc, > > + u32 common_mode) > > +{ > > + if (hw_dsc->ops.dsc_config) > > + hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode); > > + > > + if (hw_dsc->ops.dsc_config_thresh) > > + hw_dsc->ops.dsc_config_thresh(hw_dsc, dsc); > > + > > + if (hw_pp->ops.setup_dsc) > > + hw_pp->ops.setup_dsc(hw_pp); > > + > > + if (hw_pp->ops.enable_dsc) > > + hw_pp->ops.enable_dsc(hw_pp); > > I think, we do not need to split these operations, I'd suggest having just > hw_dsc->ops.dsc_config() and hw_pp->ops.enable_dsc(), merging > dsc_config_thres() and setup_dsc() into respective methods. Merging hw_dsc->ops.dsc_config() and hw_dsc->ops.dsc_config_thresh() would make it from L to XL size, so lets keep them split. We could merge the small hw_pp->ops.setup_dsc() and hw_pp->ops.enable_dsc() though. > > void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) > > { > > struct dpu_encoder_virt *dpu_enc; > > struct dpu_encoder_phys *phys; > > + struct msm_drm_private *priv; > > bool needs_hw_reset = false; > > unsigned int i; > > @@ -1841,6 +1977,10 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) > > dpu_encoder_helper_hw_reset(dpu_enc->phys_encs[i]); > > } > > } > > + > > + priv = drm_enc->dev->dev_private; > > + if (priv->dsc) > > + dpu_encoder_prep_dsc(dpu_enc, priv->dsc); > > Not quite. This makes dsc config global, while we can have several encoders > enabled at once (think of DSI + DP). So the dsc should be a per-encoder > setting rather than global. I agree it would make sense to have per-encoder. The DP part needs to be comprehended for DSC and would need more changes. I think updating this for DP then and making it generic as required for DP would be better, right? In that case I will skip moving to encoder for now.
On 02-08-21, 17:57, abhinavk@codeaurora.org wrote: > On 2021-07-14 23:52, Vinod Koul wrote: > > When DSC is enabled in DT, we need to configure the encoder for DSC > > configuration, calculate DSC parameters for the given timing. > > > > This patch adds that support by adding dpu_encoder_prep_dsc() which is > > invoked when DSC is enabled in DT > correct me if wrong but this commit text is not valid anymore in my opinion. > are there any params you are getting from DT now? I thought its all coming > from the panel > driver directly. Yes thanks for spotting this, updated! > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > agree with dmitry's comment's > https://patchwork.freedesktop.org/patch/444078/?series=90413&rev=2 > > instead of dsc being part of priv->dsc it should be per encoder. > > On top of his comment, I also think that like on the newer chipsets, moving > the dsc related > encoder configuration to a dpu_encoder_dce.c will help for future expansion > of other topologies > and also for other compression algorithms. As replied to Dimitry, the DP and other topology support needs to be comprehended so this should be done when we know how DP, other compression algorithms and other topologies would be modeled here :)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 8d942052db8a..41140b781e66 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -21,12 +21,17 @@ #include "dpu_hw_intf.h" #include "dpu_hw_ctl.h" #include "dpu_hw_dspp.h" +#include "dpu_hw_dsc.h" #include "dpu_formats.h" #include "dpu_encoder_phys.h" #include "dpu_crtc.h" #include "dpu_trace.h" #include "dpu_core_irq.h" +#define DSC_MODE_SPLIT_PANEL BIT(0) +#define DSC_MODE_MULTIPLEX BIT(1) +#define DSC_MODE_VIDEO BIT(2) + #define DPU_DEBUG_ENC(e, fmt, ...) DPU_DEBUG("enc%d " fmt,\ (e) ? (e)->base.base.id : -1, ##__VA_ARGS__) @@ -135,6 +140,7 @@ enum dpu_enc_rc_states { * @cur_slave: As above but for the slave encoder. * @hw_pp: Handle to the pingpong blocks used for the display. No. * pingpong blocks can be different than num_phys_encs. + * @hw_dsc Handle to the DSC blocks used for the display. * @intfs_swapped: Whether or not the phys_enc interfaces have been swapped * for partial update right-only cases, such as pingpong * split where virtual pingpong does not generate IRQs @@ -180,6 +186,7 @@ struct dpu_encoder_virt { struct dpu_encoder_phys *cur_master; struct dpu_encoder_phys *cur_slave; struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC]; + struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC]; bool intfs_swapped; @@ -1008,7 +1015,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; - int num_lm, num_ctl, num_pp; + struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; + int num_lm, num_ctl, num_pp, num_dsc; int i, j; if (!drm_enc) { @@ -1061,11 +1069,16 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, ARRAY_SIZE(hw_dspp)); + num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, + drm_enc->base.id, DPU_HW_BLK_DSC, hw_dsc, ARRAY_SIZE(hw_dsc)); for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i]) : NULL; + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) + dpu_enc->hw_dsc[i] = i < num_dsc ? to_dpu_hw_dsc(hw_dsc[i]) : NULL; + cstate = to_dpu_crtc_state(drm_crtc->state); for (i = 0; i < num_lm; i++) { @@ -1810,10 +1823,133 @@ static void dpu_encoder_vsync_event_work_handler(struct kthread_work *work) nsecs_to_jiffies(ktime_to_ns(wakeup_time))); } +static void +dpu_encoder_dsc_pclk_param_calc(struct msm_display_dsc_config *dsc, u32 width) +{ + int slice_count, slice_per_intf; + int bytes_in_slice, total_bytes_per_intf; + + if (!dsc || !dsc->drm->slice_width || !dsc->drm->slice_count) { + DPU_ERROR("Invalid DSC/slices\n"); + return; + } + + slice_count = dsc->drm->slice_count; + slice_per_intf = DIV_ROUND_UP(width, dsc->drm->slice_width); + + /* + * If slice_count is greater than slice_per_intf then default to 1. + * This can happen during partial update. + */ + if (slice_count > slice_per_intf) + slice_count = 1; + + bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * + dsc->drm->bits_per_pixel, 8); + total_bytes_per_intf = bytes_in_slice * slice_per_intf; + + dsc->eol_byte_num = total_bytes_per_intf % 3; + dsc->pclk_per_line = DIV_ROUND_UP(total_bytes_per_intf, 3); + dsc->bytes_in_slice = bytes_in_slice; + dsc->bytes_per_pkt = bytes_in_slice * slice_count; + dsc->pkt_per_line = slice_per_intf / slice_count; +} + +static void +dpu_encoder_dsc_initial_line_calc(struct msm_display_dsc_config *dsc, + u32 enc_ip_width) +{ + int ssm_delay, total_pixels, soft_slice_per_enc; + + soft_slice_per_enc = enc_ip_width / dsc->drm->slice_width; + + /* + * minimum number of initial line pixels is a sum of: + * 1. sub-stream multiplexer delay (83 groups for 8bpc, + * 91 for 10 bpc) * 3 + * 2. for two soft slice cases, add extra sub-stream multiplexer * 3 + * 3. the initial xmit delay + * 4. total pipeline delay through the "lock step" of encoder (47) + * 5. 6 additional pixels as the output of the rate buffer is + * 48 bits wide + */ + ssm_delay = ((dsc->drm->bits_per_component < 10) ? 84 : 92); + total_pixels = ssm_delay * 3 + dsc->drm->initial_xmit_delay + 47; + if (soft_slice_per_enc > 1) + total_pixels += (ssm_delay * 3); + dsc->initial_lines = DIV_ROUND_UP(total_pixels, dsc->drm->slice_width); +} + +static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, + struct dpu_hw_pingpong *hw_pp, + struct msm_display_dsc_config *dsc, + u32 common_mode) +{ + if (hw_dsc->ops.dsc_config) + hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode); + + if (hw_dsc->ops.dsc_config_thresh) + hw_dsc->ops.dsc_config_thresh(hw_dsc, dsc); + + if (hw_pp->ops.setup_dsc) + hw_pp->ops.setup_dsc(hw_pp); + + if (hw_pp->ops.enable_dsc) + hw_pp->ops.enable_dsc(hw_pp); +} + +static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, + struct msm_display_dsc_config *dsc) +{ + /* coding only for 2LM, 2enc, 1 dsc config */ + struct dpu_encoder_phys *enc_master = dpu_enc->cur_master; + struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC]; + struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC]; + int this_frame_slices; + int intf_ip_w, enc_ip_w; + int dsc_common_mode; + int pic_width, pic_height; + int i; + + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) { + hw_pp[i] = dpu_enc->hw_pp[i]; + hw_dsc[i] = dpu_enc->hw_dsc[i]; + + if (!hw_pp[i] || !hw_dsc[i]) { + DPU_ERROR_ENC(dpu_enc, "invalid params for DSC\n"); + return; + } + } + + dsc_common_mode = 0; + pic_width = dsc->drm->pic_width; + pic_height = dsc->drm->pic_height; + + dsc_common_mode = DSC_MODE_MULTIPLEX | DSC_MODE_SPLIT_PANEL; + if (enc_master->intf_mode == INTF_MODE_VIDEO) + dsc_common_mode |= DSC_MODE_VIDEO; + + this_frame_slices = pic_width / dsc->drm->slice_width; + intf_ip_w = this_frame_slices * dsc->drm->slice_width; + + dpu_encoder_dsc_pclk_param_calc(dsc, intf_ip_w); + + /* + * dsc merge case: when using 2 encoders for the same stream, + * no. of slices need to be same on both the encoders. + */ + enc_ip_w = intf_ip_w / 2; + dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w); + + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) + dpu_encoder_dsc_pipe_cfg(hw_dsc[i], hw_pp[i], dsc, dsc_common_mode); +} + void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) { struct dpu_encoder_virt *dpu_enc; struct dpu_encoder_phys *phys; + struct msm_drm_private *priv; bool needs_hw_reset = false; unsigned int i; @@ -1841,6 +1977,10 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) dpu_encoder_helper_hw_reset(dpu_enc->phys_encs[i]); } } + + priv = drm_enc->dev->dev_private; + if (priv->dsc) + dpu_encoder_prep_dsc(dpu_enc, priv->dsc); } void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
When DSC is enabled in DT, we need to configure the encoder for DSC configuration, calculate DSC parameters for the given timing. This patch adds that support by adding dpu_encoder_prep_dsc() which is invoked when DSC is enabled in DT Signed-off-by: Vinod Koul <vkoul@kernel.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 142 +++++++++++++++++++- 1 file changed, 141 insertions(+), 1 deletion(-)