diff mbox series

[v3,07/10] drm/msm/dsi: Disallow 8 BPC DSC configuration for alternative BPC values

Message ID 20221009185150.461323-1-marijn.suijten@somainline.org (mailing list archive)
State New, archived
Headers show
Series drm/msm: Fix math issues in MSM DSC implementation | expand

Commit Message

Marijn Suijten Oct. 9, 2022, 6:51 p.m. UTC 
According to the `/* bpc 8 */` comment below only values for a
bits_per_component of 8 are currently hardcoded in place.  This is
further confirmed by downstream sources [1] containing different
constants for other BPC values (and different initial_offset too,
with an extra dependency on bits_per_pixel).  Prevent future mishaps by
explicitly disallowing any other bits_per_component value until the
right parameters are put in place and tested.

[1]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde_dsc_helper.c#L110-139

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Dmitry Baryshkov Oct. 9, 2022, 6:57 p.m. UTC | #1 
On 09/10/2022 21:51, Marijn Suijten wrote:
> According to the `/* bpc 8 */` comment below only values for a
> bits_per_component of 8 are currently hardcoded in place.  This is
> further confirmed by downstream sources [1] containing different
> constants for other BPC values (and different initial_offset too,
> with an extra dependency on bits_per_pixel).  Prevent future mishaps by
> explicitly disallowing any other bits_per_component value until the
> right parameters are put in place and tested.
> 
> [1]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde_dsc_helper.c#L110-139
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
kernel test robot Oct. 9, 2022, 11:53 p.m. UTC | #2 
Hi Marijn,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on next-20221007]
[cannot apply to drm-misc/drm-misc-next v6.0]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Marijn-Suijten/drm-msm-Fix-math-issues-in-MSM-DSC-implementation/20221010-025519
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a6afa4199d3d038fbfdff5511f7523b0e30cb774
config: s390-allyesconfig
compiler: s390-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/919ed8d5cca6928a77890ea464052a0ff6017c34
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Marijn-Suijten/drm-msm-Fix-math-issues-in-MSM-DSC-implementation/20221010-025519
        git checkout 919ed8d5cca6928a77890ea464052a0ff6017c34
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/drm/drm_mm.h:51,
                    from include/drm/drm_vma_manager.h:26,
                    from include/drm/drm_gem.h:40,
                    from drivers/gpu/drm/msm/msm_drv.h:34,
                    from drivers/gpu/drm/msm/dsi/dsi.h:16,
                    from drivers/gpu/drm/msm/dsi/dsi_host.c:27:
   drivers/gpu/drm/msm/dsi/dsi_host.c: In function 'dsi_populate_dsc_params':
>> drivers/gpu/drm/msm/dsi/dsi_host.c:1756:32: error: 'msm_host' undeclared (first use in this function)
    1756 |                 DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n");
         |                                ^~~~~~~~
   include/drm/drm_print.h:371:24: note: in definition of macro 'DRM_DEV_ERROR'
     371 |         drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
         |                        ^~~
   drivers/gpu/drm/msm/dsi/dsi_host.c:1756:32: note: each undeclared identifier is reported only once for each function it appears in
    1756 |                 DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n");
         |                                ^~~~~~~~
   include/drm/drm_print.h:371:24: note: in definition of macro 'DRM_DEV_ERROR'
     371 |         drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
         |                        ^~~


vim +/msm_host +1756 drivers/gpu/drm/msm/dsi/dsi_host.c

  1750	
  1751	static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
  1752	{
  1753		int i;
  1754	
  1755		if (dsc->bits_per_component != 8) {
> 1756			DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n");
  1757			return -EOPNOTSUPP;
  1758		}
  1759	
  1760		dsc->rc_model_size = 8192;
  1761		dsc->first_line_bpg_offset = 12;
  1762		dsc->rc_edge_factor = 6;
  1763		dsc->rc_tgt_offset_high = 3;
  1764		dsc->rc_tgt_offset_low = 3;
  1765		dsc->simple_422 = 0;
  1766		dsc->convert_rgb = 1;
  1767		dsc->vbr_enable = 0;
  1768	
  1769		/* handle only bpp = bpc = 8 */
  1770		for (i = 0; i < DSC_NUM_BUF_RANGES - 1 ; i++)
  1771			dsc->rc_buf_thresh[i] = dsi_dsc_rc_buf_thresh[i];
  1772	
  1773		for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
  1774			dsc->rc_range_params[i].range_min_qp = min_qp[i];
  1775			dsc->rc_range_params[i].range_max_qp = max_qp[i];
  1776			dsc->rc_range_params[i].range_bpg_offset = bpg_offset[i];
  1777		}
  1778	
  1779		dsc->initial_offset = 6144; /* Not bpp 12 */
  1780		if (dsc->bits_per_pixel != 8)
  1781			dsc->initial_offset = 2048;	/* bpp = 12 */
  1782	
  1783		if (dsc->bits_per_component <= 10)
  1784			dsc->mux_word_size = DSC_MUX_WORD_SIZE_8_10_BPC;
  1785		else
  1786			dsc->mux_word_size = DSC_MUX_WORD_SIZE_12_BPC;
  1787	
  1788		dsc->initial_xmit_delay = 512;
  1789		dsc->initial_scale_value = 32;
  1790		dsc->first_line_bpg_offset = 12;
  1791		dsc->line_buf_depth = dsc->bits_per_component + 1;
  1792	
  1793		/* bpc 8 */
  1794		dsc->flatness_min_qp = 3;
  1795		dsc->flatness_max_qp = 12;
  1796		dsc->rc_quant_incr_limit0 = 11;
  1797		dsc->rc_quant_incr_limit1 = 11;
  1798	
  1799		return drm_dsc_compute_rc_parameters(dsc);
  1800	}
  1801
Abhinav Kumar Oct. 12, 2022, 11:08 p.m. UTC | #3 
On 10/9/2022 11:51 AM, Marijn Suijten wrote:
> According to the `/* bpc 8 */` comment below only values for a
> bits_per_component of 8 are currently hardcoded in place.  This is
> further confirmed by downstream sources [1] containing different
> constants for other BPC values (and different initial_offset too,
> with an extra dependency on bits_per_pixel).  Prevent future mishaps by
> explicitly disallowing any other bits_per_component value until the
> right parameters are put in place and tested.
> 
> [1]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde_dsc_helper.c#L110-139
> 

Seems like a valid kbot error.

https://patchwork.freedesktop.org/patch/506359/#comment_912830

> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 68c39debc22f..7e6b7e506ae8 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1774,6 +1774,11 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>   {
>   	int i;
>   
> +	if (dsc->bits_per_component != 8) {
> +		DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n");
> +		return -EOPNOTSUPP;
> +	}
> +
>   	dsc->rc_model_size = 8192;
>   	dsc->first_line_bpg_offset = 12;
>   	dsc->rc_edge_factor = 6;
Marijn Suijten Oct. 13, 2022, 9:27 a.m. UTC | #4 
On 2022-10-12 16:08:07, Abhinav Kumar wrote:
> 
> 
> On 10/9/2022 11:51 AM, Marijn Suijten wrote:
> > According to the `/* bpc 8 */` comment below only values for a
> > bits_per_component of 8 are currently hardcoded in place.  This is
> > further confirmed by downstream sources [1] containing different
> > constants for other BPC values (and different initial_offset too,
> > with an extra dependency on bits_per_pixel).  Prevent future mishaps by
> > explicitly disallowing any other bits_per_component value until the
> > right parameters are put in place and tested.
> > 
> > [1]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde_dsc_helper.c#L110-139
> > 
> 
> Seems like a valid kbot error.
> 
> https://patchwork.freedesktop.org/patch/506359/#comment_912830

It is correct, and I suggested in [1] to either reorder this patch 7/10
after 8/10, or pull back the msm_host pointer argument into this patch.

[1]: https://lore.kernel.org/linux-arm-msm/20221011075119.tvn5j5jm6aqnhqv2@SoMainline.org/

- Marijn

> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 68c39debc22f..7e6b7e506ae8 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -1774,6 +1774,11 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
> >   {
> >   	int i;
> >   
> > +	if (dsc->bits_per_component != 8) {
> > +		DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> >   	dsc->rc_model_size = 8192;
> >   	dsc->first_line_bpg_offset = 12;
> >   	dsc->rc_edge_factor = 6;
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 68c39debc22f..7e6b7e506ae8 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1774,6 +1774,11 @@  static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
 {
 	int i;
 
+	if (dsc->bits_per_component != 8) {
+		DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n");
+		return -EOPNOTSUPP;
+	}
+
 	dsc->rc_model_size = 8192;
 	dsc->first_line_bpg_offset = 12;
 	dsc->rc_edge_factor = 6;