| Message ID | 20230518104857.124265-7-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MediaTek DDP GAMMA - 12-bit LUT support | expand |
Hi, Angelo: On Thu, 2023-05-18 at 12:48 +0200, AngeloGioacchino Del Regno wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Make the code more robust and improve readability by using bitfield > macros instead of open coding bit operations. > While at it, also add a definition for LUT_BITS_DEFAULT. > > Signed-off-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 41 ++++++++++++++------- > -- > 1 file changed, 26 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c > b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c > index 1592614b6de7..ed2aa1fb0171 100644 > --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c > +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2021 MediaTek Inc. > */ > > +#include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/component.h> > #include <linux/module.h> > @@ -22,9 +23,16 @@ > #define GAMMA_LUT_EN BIT(1) > #define > GAMMA_DITHERING BIT(2) > #define DISP_GAMMA_SIZE 0x0030 > +#define DISP_GAMMA_SIZE_HSIZE GENMASK(28, > 16) > +#define DISP_GAMMA_SIZE_VSIZE GENMASK(12, > 0) > #define DISP_GAMMA_LUT 0x0700 > > +#define DISP_GAMMA_LUT_10BIT_R GENMASK(29, 20) > +#define DISP_GAMMA_LUT_10BIT_G GENMASK(19, 10) > +#define DISP_GAMMA_LUT_10BIT_B GENMASK(9, 0) > + > #define LUT_10BIT_MASK 0x03ff > +#define LUT_BITS_DEFAULT 10 This is used only for AAL after patch "drm/mediatek: gamma: Support specifying number of bits per LUT component", so I would like move AAL definition to AAL driver and pass it to gamma driver. Regards, CK > #define LUT_SIZE_DEFAULT 512 /* for setting > gamma lut from AAL */ > > struct mtk_disp_gamma_data { > @@ -96,33 +104,33 @@ void mtk_gamma_set_common(struct device *dev, > void __iomem *regs, struct drm_crt > for (i = 0; i < lut_size; i++) { > struct drm_color_lut diff, hwlut; > > - hwlut.red = drm_color_lut_extract(lut[i].red, 10); > - hwlut.green = drm_color_lut_extract(lut[i].green, > 10); > - hwlut.blue = drm_color_lut_extract(lut[i].blue, 10); > + hwlut.red = drm_color_lut_extract(lut[i].red, > LUT_BITS_DEFAULT); > + hwlut.green = drm_color_lut_extract(lut[i].green, > LUT_BITS_DEFAULT); > + hwlut.blue = drm_color_lut_extract(lut[i].blue, > LUT_BITS_DEFAULT); > > if (!lut_diff || (i % 2 == 0)) { > - word = hwlut.red << 20 + > - hwlut.green << 10 + > - hwlut.red; > + word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, > hwlut.red); > + word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, > hwlut.green); > + word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, > hwlut.blue); > } else { > diff.red = lut[i].red - lut[i - 1].red; > - diff.red = drm_color_lut_extract(diff.red, > 10); > + diff.red = drm_color_lut_extract(diff.red, > LUT_BITS_DEFAULT); > > diff.green = lut[i].green - lut[i - 1].green; > - diff.green = > drm_color_lut_extract(diff.green, 10); > + diff.green = > drm_color_lut_extract(diff.green, LUT_BITS_DEFAULT); > > diff.blue = lut[i].blue - lut[i - 1].blue; > - diff.blue = drm_color_lut_extract(diff.blue, > 10); > + diff.blue = drm_color_lut_extract(diff.blue, > LUT_BITS_DEFAULT); > > - word = diff.blue << 20 + > - diff.green << 10 + > - diff.red; > + word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, > diff.red); > + word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, > diff.green); > + word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, > diff.blue); > } > writel(word, (lut_base + i * 4)); > } > > /* Enable the gamma table */ > - cfg_val = cfg_val | GAMMA_LUT_EN; > + cfg_val |= FIELD_PREP(GAMMA_LUT_EN, 1); > > writel(cfg_val, regs + DISP_GAMMA_CFG); > } > @@ -139,9 +147,12 @@ void mtk_gamma_config(struct device *dev, > unsigned int w, > unsigned int bpc, struct cmdq_pkt *cmdq_pkt) > { > struct mtk_disp_gamma *gamma = dev_get_drvdata(dev); > + u32 sz; > + > + sz = FIELD_PREP(DISP_GAMMA_SIZE_HSIZE, w); > + sz |= FIELD_PREP(DISP_GAMMA_SIZE_VSIZE, h); > > - mtk_ddp_write(cmdq_pkt, h << 16 | w, &gamma->cmdq_reg, gamma- > >regs, > - DISP_GAMMA_SIZE); > + mtk_ddp_write(cmdq_pkt, sz, &gamma->cmdq_reg, gamma->regs, > DISP_GAMMA_SIZE); > if (gamma->data && gamma->data->has_dither) > mtk_dither_set_common(gamma->regs, &gamma->cmdq_reg, > bpc, > DISP_GAMMA_CFG, > GAMMA_DITHERING, cmdq_pkt); > -- > 2.40.1 >
Il 26/05/23 07:32, CK Hu (胡俊光) ha scritto: > Hi, Angelo: > > On Thu, 2023-05-18 at 12:48 +0200, AngeloGioacchino Del Regno wrote: >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> >> >> Make the code more robust and improve readability by using bitfield >> macros instead of open coding bit operations. >> While at it, also add a definition for LUT_BITS_DEFAULT. >> >> Signed-off-by: AngeloGioacchino Del Regno < >> angelogioacchino.delregno@collabora.com> >> Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> >> --- >> drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 41 ++++++++++++++------- >> -- >> 1 file changed, 26 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c >> b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c >> index 1592614b6de7..ed2aa1fb0171 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c >> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c >> @@ -3,6 +3,7 @@ >> * Copyright (c) 2021 MediaTek Inc. >> */ >> >> +#include <linux/bitfield.h> >> #include <linux/clk.h> >> #include <linux/component.h> >> #include <linux/module.h> >> @@ -22,9 +23,16 @@ >> #define GAMMA_LUT_EN BIT(1) >> #define >> GAMMA_DITHERING BIT(2) >> #define DISP_GAMMA_SIZE 0x0030 >> +#define DISP_GAMMA_SIZE_HSIZE GENMASK(28, >> 16) >> +#define DISP_GAMMA_SIZE_VSIZE GENMASK(12, >> 0) >> #define DISP_GAMMA_LUT 0x0700 >> >> +#define DISP_GAMMA_LUT_10BIT_R GENMASK(29, 20) >> +#define DISP_GAMMA_LUT_10BIT_G GENMASK(19, 10) >> +#define DISP_GAMMA_LUT_10BIT_B GENMASK(9, 0) >> + >> #define LUT_10BIT_MASK 0x03ff >> +#define LUT_BITS_DEFAULT 10 > > This is used only for AAL after patch "drm/mediatek: gamma: Support > specifying number of bits per LUT component", so I would like move AAL > definition to AAL driver and pass it to gamma driver. > Like LUT_SIZE_DEFAULT, this definition is not only for AAL but also for the older gamma lut register layout. In any case, I'll check if there's any clean way to pass AAL's gamma size to this driver... it's a different "component", so this may get complicated. Let's see what I can come up with in v5... Thanks, Angelo
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c index 1592614b6de7..ed2aa1fb0171 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c @@ -3,6 +3,7 @@ * Copyright (c) 2021 MediaTek Inc. */ +#include <linux/bitfield.h> #include <linux/clk.h> #include <linux/component.h> #include <linux/module.h> @@ -22,9 +23,16 @@ #define GAMMA_LUT_EN BIT(1) #define GAMMA_DITHERING BIT(2) #define DISP_GAMMA_SIZE 0x0030 +#define DISP_GAMMA_SIZE_HSIZE GENMASK(28, 16) +#define DISP_GAMMA_SIZE_VSIZE GENMASK(12, 0) #define DISP_GAMMA_LUT 0x0700 +#define DISP_GAMMA_LUT_10BIT_R GENMASK(29, 20) +#define DISP_GAMMA_LUT_10BIT_G GENMASK(19, 10) +#define DISP_GAMMA_LUT_10BIT_B GENMASK(9, 0) + #define LUT_10BIT_MASK 0x03ff +#define LUT_BITS_DEFAULT 10 #define LUT_SIZE_DEFAULT 512 /* for setting gamma lut from AAL */ struct mtk_disp_gamma_data { @@ -96,33 +104,33 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt for (i = 0; i < lut_size; i++) { struct drm_color_lut diff, hwlut; - hwlut.red = drm_color_lut_extract(lut[i].red, 10); - hwlut.green = drm_color_lut_extract(lut[i].green, 10); - hwlut.blue = drm_color_lut_extract(lut[i].blue, 10); + hwlut.red = drm_color_lut_extract(lut[i].red, LUT_BITS_DEFAULT); + hwlut.green = drm_color_lut_extract(lut[i].green, LUT_BITS_DEFAULT); + hwlut.blue = drm_color_lut_extract(lut[i].blue, LUT_BITS_DEFAULT); if (!lut_diff || (i % 2 == 0)) { - word = hwlut.red << 20 + - hwlut.green << 10 + - hwlut.red; + word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red); + word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green); + word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue); } else { diff.red = lut[i].red - lut[i - 1].red; - diff.red = drm_color_lut_extract(diff.red, 10); + diff.red = drm_color_lut_extract(diff.red, LUT_BITS_DEFAULT); diff.green = lut[i].green - lut[i - 1].green; - diff.green = drm_color_lut_extract(diff.green, 10); + diff.green = drm_color_lut_extract(diff.green, LUT_BITS_DEFAULT); diff.blue = lut[i].blue - lut[i - 1].blue; - diff.blue = drm_color_lut_extract(diff.blue, 10); + diff.blue = drm_color_lut_extract(diff.blue, LUT_BITS_DEFAULT); - word = diff.blue << 20 + - diff.green << 10 + - diff.red; + word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red); + word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green); + word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue); } writel(word, (lut_base + i * 4)); } /* Enable the gamma table */ - cfg_val = cfg_val | GAMMA_LUT_EN; + cfg_val |= FIELD_PREP(GAMMA_LUT_EN, 1); writel(cfg_val, regs + DISP_GAMMA_CFG); } @@ -139,9 +147,12 @@ void mtk_gamma_config(struct device *dev, unsigned int w, unsigned int bpc, struct cmdq_pkt *cmdq_pkt) { struct mtk_disp_gamma *gamma = dev_get_drvdata(dev); + u32 sz; + + sz = FIELD_PREP(DISP_GAMMA_SIZE_HSIZE, w); + sz |= FIELD_PREP(DISP_GAMMA_SIZE_VSIZE, h); - mtk_ddp_write(cmdq_pkt, h << 16 | w, &gamma->cmdq_reg, gamma->regs, - DISP_GAMMA_SIZE); + mtk_ddp_write(cmdq_pkt, sz, &gamma->cmdq_reg, gamma->regs, DISP_GAMMA_SIZE); if (gamma->data && gamma->data->has_dither) mtk_dither_set_common(gamma->regs, &gamma->cmdq_reg, bpc, DISP_GAMMA_CFG, GAMMA_DITHERING, cmdq_pkt);