Re: [PATCH v3 4/9] drm/mediatek: Add gamma support different lut_size for other SoC

Re: [PATCH v3 4/9] drm/mediatek: Add gamma support different lut_size for other SoC

[kernel test robot]

Hi Jason-JH.Lin",

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20220909]
[also build test WARNING on v6.0-rc4]
[cannot apply to drm-misc/drm-misc-next robh/for-next drm/drm-next linus/master v6.0-rc4 v6.0-rc3 v6.0-rc2]
[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/Jason-JH-Lin/Add-gamma-lut-support-for-mt8195/20220911-233949
base:    9a82ccda91ed2b40619cb3c10d446ae1f97bab6e
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20220912/202209120412.a3tFZuO9-lkp@intel.com/config)
compiler: arm-linux-gnueabi-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/1d5b909c49fd62f1e6055fd3d077b9c5fae53e00
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jason-JH-Lin/Add-gamma-lut-support-for-mt8195/20220911-233949
        git checkout 1d5b909c49fd62f1e6055fd3d077b9c5fae53e00
        # 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=arm SHELL=/bin/bash

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

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mediatek/mtk_disp_gamma.c:59:14: warning: no previous prototype for 'mtk_gamma_get_size' [-Wmissing-prototypes]
      59 | unsigned int mtk_gamma_get_size(struct device *dev)
         |              ^~~~~~~~~~~~~~~~~~


vim +/mtk_gamma_get_size +59 drivers/gpu/drm/mediatek/mtk_disp_gamma.c

    58	
  > 59	unsigned int mtk_gamma_get_size(struct device *dev)
    60	{
    61		struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
    62		unsigned int lut_size = LUT_SIZE_DEFAULT;
    63	
    64		if (gamma && gamma->data)
    65			lut_size = gamma->data->lut_size;
    66	
    67		return lut_size;
    68	}
    69	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 3/9] drm/mediatek: Adjust mtk_drm_gamma_set_common parameters

[AngeloGioacchino Del Regno]

Il 11/09/22 17:37, Jason-JH.Lin ha scritto:
> Adjust the parameters in mtk_drm_gamma_set_common()
>    - add (struct device *dev) to get lut_diff from gamma's driver data
>    - remove (bool lut_diff) and use false as default value in the function
> 
> Fixes: 051524cbe62d ("FROMGIT: drm/mediatek: Add lut diff flag for new gamma hardware support")

This Fixes tag contains a commit from a downstream kernel, which is useless
fo upstream. Please fix it.

Regards,
Angelo

Re: [PATCH v3 4/9] drm/mediatek: Add gamma support different lut_size for other SoC

[AngeloGioacchino Del Regno]

Il 11/09/22 17:37, Jason-JH.Lin ha scritto:
> 1. Add mtk_drm_gamma_get_lut_size() and remove MTK_LUT_SIZE macro.
> 2. Add lut_size to gamma driver data for different SoC.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  1 +
>   drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 22 +++++++++++++++++++--
>   drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  4 ++--
>   drivers/gpu/drm/mediatek/mtk_drm_crtc.h     |  1 -
>   drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  9 +++++++++
>   5 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index a83e5fbc8724..6a05bb56e693 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -51,6 +51,7 @@ void mtk_gamma_clk_disable(struct device *dev);
>   void mtk_gamma_config(struct device *dev, unsigned int w,
>   		      unsigned int h, unsigned int vrefresh,
>   		      unsigned int bpc, struct cmdq_pkt *cmdq_pkt);
> +unsigned int mtk_gamma_get_lut_size(struct device *dev);
>   void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state);
>   void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crtc_state *state);
>   void mtk_gamma_start(struct device *dev);
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> index f54a6a618348..e69d0b205b9a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -24,10 +24,12 @@
>   #define DISP_GAMMA_LUT				0x0700
>   
>   #define LUT_10BIT_MASK				0x03ff
> +#define LUT_SIZE_DEFAULT			512 /* for setting gamma lut from AAL */
>   
>   struct mtk_disp_gamma_data {
>   	bool has_dither;
>   	bool lut_diff;
> +	u16 lut_size;
>   };
>   
>   /*
> @@ -54,18 +56,32 @@ void mtk_gamma_clk_disable(struct device *dev)
>   	clk_disable_unprepare(gamma->clk);
>   }
>   
> +unsigned int mtk_gamma_get_size(struct device *dev)
> +{
> +	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> +	unsigned int lut_size = LUT_SIZE_DEFAULT;
> +
> +	if (gamma && gamma->data)
> +		lut_size = gamma->data->lut_size;
> +
> +	return lut_size;
> +}
> +
>   void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crtc_state *state)
>   {
>   	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
>   	bool lut_diff = false;
> +	u16 lut_size = LUT_SIZE_DEFAULT;

This makes us get a double assignment in case gamma->data is populated.

>   	unsigned int i, reg;
>   	struct drm_color_lut *lut;
>   	void __iomem *lut_base;
>   	u32 word;
>   	u32 diff[3] = {0};
>   
> -	if (gamma && gamma->data)
> +	if (gamma && gamma->data) {
>   		lut_diff = gamma->data->lut_diff;
> +		lut_size = gamma->data->lut_size;
> +	}

...you can avoid it like that:

} else {
	lut_size = LUT_SIZE_DEFAULT;
}


Regards,
Angelo

Re: [PATCH v3 5/9] drm/mediatek: Add gamma support different lut_bits for other SoC

[AngeloGioacchino Del Regno]

Il 11/09/22 17:37, Jason-JH.Lin ha scritto:
> Add lut_bits in gamma driver data for each SoC and adjust the usage
> of lut_bits in mtk_drm_gamma_set_common().
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 36 ++++++++++++++++-------
>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> index e69d0b205b9a..155fb5d94d79 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -25,11 +25,14 @@
>   
>   #define LUT_10BIT_MASK				0x03ff
>   #define LUT_SIZE_DEFAULT			512 /* for setting gamma lut from AAL */
> +#define LUT_BITS_DEFAULT			10
> +#define LUT_INPUT_BITS				16 /* input lut bit from application */
>   
>   struct mtk_disp_gamma_data {
>   	bool has_dither;
>   	bool lut_diff;
>   	u16 lut_size;
> +	u8 lut_bits;
>   };
>   
>   /*
> @@ -72,17 +75,23 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
>   	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
>   	bool lut_diff = false;
>   	u16 lut_size = LUT_SIZE_DEFAULT;
> +	u8 lut_bits = LUT_BITS_DEFAULT;

Please fix this double assignment like shown in the comment for patch [4/9].

Thanks,
Angelo

Re: [PATCH v3 8/9] drm/mediatek: Add clear RELAY_MODE bit to set gamma

[AngeloGioacchino Del Regno]

Il 11/09/22 17:37, Jason-JH.Lin ha scritto:
> Since the bootlaoder may set the RELAY_MODE to gamma be for the kerenl,
> we have to clear the RELAY_MODE bit to make sure that the gamma is
> enabled correctly.
> 
> Fixes: b10023b03082 ("FROMGIT: drm/mediatek: Separate gamma module")

This Fixes tag is invalid. Please fix.

Regards,
Angelo

Re: [PATCH v3 3/9] drm/mediatek: Adjust mtk_drm_gamma_set_common parameters

[Jason-JH Lin]

Hi Angelo,

Thanks for the reviews.

On Mon, 2022-09-12 at 12:12 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/09/22 17:37, Jason-JH.Lin ha scritto:
> > Adjust the parameters in mtk_drm_gamma_set_common()
> >    - add (struct device *dev) to get lut_diff from gamma's driver
> > data
> >    - remove (bool lut_diff) and use false as default value in the
> > function
> > 
> > Fixes: 051524cbe62d ("FROMGIT: drm/mediatek: Add lut diff flag for
> > new gamma hardware support")
> 
> This Fixes tag contains a commit from a downstream kernel, which is
> useless
> fo upstream. Please fix it.
> 
> Regards,
> Angelo

OK, I'll fix this.

Regards,
Jason-JH.Lin

-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>

Re: [PATCH v3 4/9] drm/mediatek: Add gamma support different lut_size for other SoC

[Jason-JH Lin]

On Mon, 2022-09-12 at 12:24 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/09/22 17:37, Jason-JH.Lin ha scritto:
> > 1. Add mtk_drm_gamma_get_lut_size() and remove MTK_LUT_SIZE macro.
> > 2. Add lut_size to gamma driver data for different SoC.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >   drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  1 +
> >   drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 22
> > +++++++++++++++++++--
> >   drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  4 ++--
> >   drivers/gpu/drm/mediatek/mtk_drm_crtc.h     |  1 -
> >   drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  9 +++++++++
> >   5 files changed, 32 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > index a83e5fbc8724..6a05bb56e693 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > @@ -51,6 +51,7 @@ void mtk_gamma_clk_disable(struct device *dev);
> >   void mtk_gamma_config(struct device *dev, unsigned int w,
> >   		      unsigned int h, unsigned int vrefresh,
> >   		      unsigned int bpc, struct cmdq_pkt *cmdq_pkt);
> > +unsigned int mtk_gamma_get_lut_size(struct device *dev);
> >   void mtk_gamma_set(struct device *dev, struct drm_crtc_state
> > *state);
> >   void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
> > struct drm_crtc_state *state);
> >   void mtk_gamma_start(struct device *dev);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > index f54a6a618348..e69d0b205b9a 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > @@ -24,10 +24,12 @@
> >   #define DISP_GAMMA_LUT				0x0700
> >   
> >   #define LUT_10BIT_MASK				0x03ff
> > +#define LUT_SIZE_DEFAULT			512 /* for setting
> > gamma lut from AAL */
> >   
> >   struct mtk_disp_gamma_data {
> >   	bool has_dither;
> >   	bool lut_diff;
> > +	u16 lut_size;
> >   };
> >   
> >   /*
> > @@ -54,18 +56,32 @@ void mtk_gamma_clk_disable(struct device *dev)
> >   	clk_disable_unprepare(gamma->clk);
> >   }
> >   
> > +unsigned int mtk_gamma_get_size(struct device *dev)
> > +{
> > +	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> > +	unsigned int lut_size = LUT_SIZE_DEFAULT;
> > +
> > +	if (gamma && gamma->data)
> > +		lut_size = gamma->data->lut_size;
> > +
> > +	return lut_size;
> > +}
> > +
> >   void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
> > struct drm_crtc_state *state)
> >   {
> >   	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> >   	bool lut_diff = false;
> > +	u16 lut_size = LUT_SIZE_DEFAULT;
> 
> This makes us get a double assignment in case gamma->data is
> populated.
> 
> >   	unsigned int i, reg;
> >   	struct drm_color_lut *lut;
> >   	void __iomem *lut_base;
> >   	u32 word;
> >   	u32 diff[3] = {0};
> >   
> > -	if (gamma && gamma->data)
> > +	if (gamma && gamma->data) {
> >   		lut_diff = gamma->data->lut_diff;
> > +		lut_size = gamma->data->lut_size;
> > +	}
> 
> ...you can avoid it like that:
> 
> } else {
> 	lut_size = LUT_SIZE_DEFAULT;
> }
> 
> 
> Regards,
> Angelo
> 
> 
-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>

Re: [PATCH v3 4/9] drm/mediatek: Add gamma support different lut_size for other SoC

[Jason-JH Lin]

Hi Angelo,

Thanks for the reviews.

On Mon, 2022-09-12 at 12:24 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/09/22 17:37, Jason-JH.Lin ha scritto:
> > 1. Add mtk_drm_gamma_get_lut_size() and remove MTK_LUT_SIZE macro.
> > 2. Add lut_size to gamma driver data for different SoC.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >   drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  1 +
> >   drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 22
> > +++++++++++++++++++--
> >   drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  4 ++--
> >   drivers/gpu/drm/mediatek/mtk_drm_crtc.h     |  1 -
> >   drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  9 +++++++++
> >   5 files changed, 32 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > index a83e5fbc8724..6a05bb56e693 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > @@ -51,6 +51,7 @@ void mtk_gamma_clk_disable(struct device *dev);
> >   void mtk_gamma_config(struct device *dev, unsigned int w,
> >   		      unsigned int h, unsigned int vrefresh,
> >   		      unsigned int bpc, struct cmdq_pkt *cmdq_pkt);
> > +unsigned int mtk_gamma_get_lut_size(struct device *dev);
> >   void mtk_gamma_set(struct device *dev, struct drm_crtc_state
> > *state);
> >   void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
> > struct drm_crtc_state *state);
> >   void mtk_gamma_start(struct device *dev);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > index f54a6a618348..e69d0b205b9a 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > @@ -24,10 +24,12 @@
> >   #define DISP_GAMMA_LUT				0x0700
> >   
> >   #define LUT_10BIT_MASK				0x03ff
> > +#define LUT_SIZE_DEFAULT			512 /* for setting
> > gamma lut from AAL */
> >   
> >   struct mtk_disp_gamma_data {
> >   	bool has_dither;
> >   	bool lut_diff;
> > +	u16 lut_size;
> >   };
> >   
> >   /*
> > @@ -54,18 +56,32 @@ void mtk_gamma_clk_disable(struct device *dev)
> >   	clk_disable_unprepare(gamma->clk);
> >   }
> >   
> > +unsigned int mtk_gamma_get_size(struct device *dev)
> > +{
> > +	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> > +	unsigned int lut_size = LUT_SIZE_DEFAULT;
> > +
> > +	if (gamma && gamma->data)
> > +		lut_size = gamma->data->lut_size;
> > +
> > +	return lut_size;
> > +}
> > +
> >   void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
> > struct drm_crtc_state *state)
> >   {
> >   	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> >   	bool lut_diff = false;
> > +	u16 lut_size = LUT_SIZE_DEFAULT;
> 
> This makes us get a double assignment in case gamma->data is
> populated.
> 
> >   	unsigned int i, reg;
> >   	struct drm_color_lut *lut;
> >   	void __iomem *lut_base;
> >   	u32 word;
> >   	u32 diff[3] = {0};
> >   
> > -	if (gamma && gamma->data)
> > +	if (gamma && gamma->data) {
> >   		lut_diff = gamma->data->lut_diff;
> > +		lut_size = gamma->data->lut_size;
> > +	}
> 
> ...you can avoid it like that:
> 
> } else {
> 	lut_size = LUT_SIZE_DEFAULT;
> }
> 
> 
> Regards,
> Angelo
> 
OK, I'll fix it, but I'll move the gamma priv data handling to the
mtk_gamma_set.

Regards,
Jason-JH.Lin

> 
-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>

Re: [PATCH v3 5/9] drm/mediatek: Add gamma support different lut_bits for other SoC

[Jason-JH Lin]

Hi Angelo,

Thanks for the reviews.

On Mon, 2022-09-12 at 12:25 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/09/22 17:37, Jason-JH.Lin ha scritto:
> > Add lut_bits in gamma driver data for each SoC and adjust the usage
> > of lut_bits in mtk_drm_gamma_set_common().
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >   drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 36 ++++++++++++++++
> > -------
> >   1 file changed, 25 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > index e69d0b205b9a..155fb5d94d79 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > @@ -25,11 +25,14 @@
> >   
> >   #define LUT_10BIT_MASK				0x03ff
> >   #define LUT_SIZE_DEFAULT			512 /* for setting
> > gamma lut from AAL */
> > +#define LUT_BITS_DEFAULT			10
> > +#define LUT_INPUT_BITS				16 /* input lut
> > bit from application */
> >   
> >   struct mtk_disp_gamma_data {
> >   	bool has_dither;
> >   	bool lut_diff;
> >   	u16 lut_size;
> > +	u8 lut_bits;
> >   };
> >   
> >   /*
> > @@ -72,17 +75,23 @@ void mtk_gamma_set_common(struct device *dev,
> > void __iomem *regs, struct drm_crt
> >   	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> >   	bool lut_diff = false;
> >   	u16 lut_size = LUT_SIZE_DEFAULT;
> > +	u8 lut_bits = LUT_BITS_DEFAULT;
> 
> Please fix this double assignment like shown in the comment for patch
> [4/9].
> 
> Thanks,
> Angelo
> 
OK, I'll fix this.
Regards,
Jason-JH.Lin

-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>

Re: [PATCH v3 8/9] drm/mediatek: Add clear RELAY_MODE bit to set gamma

[Jason-JH Lin]

Hi Angelo

Thanks for the reviews.

On Mon, 2022-09-12 at 12:26 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/09/22 17:37, Jason-JH.Lin ha scritto:
> > Since the bootlaoder may set the RELAY_MODE to gamma be for the
> > kerenl,
> > we have to clear the RELAY_MODE bit to make sure that the gamma is
> > enabled correctly.
> > 
> > Fixes: b10023b03082 ("FROMGIT: drm/mediatek: Separate gamma
> > module")
> 
> This Fixes tag is invalid. Please fix.
> 
> Regards,
> Angelo
> 
OK, I,ll fix it.

Regards,
Jason-JH.Lin
-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>