Hi, CK
I'm appreciate your comments.
On Mon, 2016-07-18 at 10:33 +0800, CK Hu wrote:
> Hi, Bibby:
>
> Some comments inline.
>
> On Thu, 2016-07-07 at 15:37 +0800, Bibby Hsieh wrote:
> > Some panels only accept bpc (bit per color) 6-bit.
> > But, the default bpc in mt8173 display data path is 8-bit.
> > If we didn't enable dithering function to convert bpc,
> > display cannot show the smooth grayscale image.
> >
> > In mt8173, the dithering function in OD (OverDrive) and
> > GAMMA module, we have to config them with
> > connector->display_mode.bpc when CRTC initial.
> >
> > 1. Clear the default value at *_DITHER_5 and *_DITHER_7 register.
> > 2. Calculate the LSB_ERR_SHIFT bits and ADD_LSHIFT bits two values.
> > i.e. Input bpc of OD is 10 bits, we assume the bpc of panel is 6-bit,
> > so, we need to set 4-bit to LSB_ERR_SHIFT and ADD_LSHIFT bits respectively.
> > 3. Then, set the OD or GAMMA to dithering mode depends on path-1 or path-2.
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh at mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 3 +-
> > drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 3 +-
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 21 ++++++--
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 77
> > +++++++++++++++++++++++----
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 6 +--
> > 5 files changed, 92 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > index 8f62671f..019b7ca 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > @@ -103,7 +103,8 @@ static void mtk_ovl_stop(struct mtk_ddp_comp *comp)
> > }
> >
> > static void mtk_ovl_config(struct mtk_ddp_comp *comp, unsigned int w,
> > - unsigned int h, unsigned int vrefresh)
> > + unsigned int h, unsigned int vrefresh,
> > + unsigned int bpc)
> > {
> > if (w != 0 && h != 0)
> > writel_relaxed(h << 16 | w, comp->regs + DISP_REG_OVL_ROI_SIZE);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > index 5fb80cb..0df05f9 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > @@ -106,7 +106,8 @@ static void mtk_rdma_stop(struct mtk_ddp_comp *comp)
> > }
> >
> > static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
> > - unsigned int height, unsigned int vrefresh)
> > + unsigned int height, unsigned int vrefresh,
> > + unsigned int bpc)
> > {
> > unsigned int threshold;
> > unsigned int reg;
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index ee219bb..18c9d0d 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -222,7 +222,9 @@ static void mtk_crtc_ddp_clk_disable(struct
> > mtk_drm_crtc *mtk_crtc)
> > static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > {
> > struct drm_crtc *crtc = &mtk_crtc->base;
> > - unsigned int width, height, vrefresh;
> > + struct drm_connector *connector;
> > + struct drm_encoder *encoder;
> > + unsigned int width, height, vrefresh, bpc = 0;
> > int ret;
> > int i;
> >
> > @@ -234,6 +236,19 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc
> > *mtk_crtc)
> > height = crtc->state->adjusted_mode.vdisplay;
> > vrefresh = crtc->state->adjusted_mode.vrefresh;
> >
> > + drm_for_each_encoder(encoder, crtc->dev) {
> > + if (encoder->crtc != crtc)
> > + continue;
> > +
> > + drm_for_each_connector(connector, crtc->dev) {
> > + if (connector->encoder != encoder)
> > + continue;
> > + if (connector->display_info.bpc >= 3 &&
>
> I think you should left this HW constrain in mtk_od_config() and
> mtk_gamma_config(). Here just calculate the expected bpc.
>
Ok, I will do that.
> > + (bpc == 0 || bpc > connector->display_info.bpc))
>
> I think bpc should be initialized as HW default bpc and you can remove
> this condiction 'bpc == 0' because we should set bpc to HW default bpc
> while all connnector bpc is greater than HW default bpc.
>
Ok, will do.
> > + bpc = connector->display_info.bpc;
> > + }
> > + }
> > +
> > ret = pm_runtime_get_sync(crtc->dev->dev);
> > if (ret < 0) {
> > DRM_ERROR("Failed to enable power domain: %d\n", ret);
> > @@ -266,7 +281,7 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc
> > *mtk_crtc)
> > for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> > struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
> >
> > - mtk_ddp_comp_config(comp, width, height, vrefresh);
> > + mtk_ddp_comp_config(comp, width, height, vrefresh, bpc);
> > mtk_ddp_comp_start(comp);
> > }
> >
> > @@ -469,7 +484,7 @@ void mtk_crtc_ddp_irq(struct drm_crtc *crtc, struct
> > mtk_ddp_comp *ovl)
> > if (state->pending_config) {
> > mtk_ddp_comp_config(ovl, state->pending_width,
> > state->pending_height,
> > - state->pending_vrefresh);
> > + state->pending_vrefresh, 0);
>
> Why set bpc as 0 here? Maybe you have a assumption that OVL don't care
> the bpc parameter. If one day OVL care it and we do not review here, the
> bugs happen.
>
Pass 0 means don't care, I will modify mtk_od_config() and
mtk_gamma_config() to filter this condition.
> >
> > state->pending_config = false;
> > }
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > index 56c5894..c10faac 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > @@ -30,7 +30,26 @@
> > #define DISP_OD_INTEN 0x0008
> > #define DISP_OD_INTSTA 0x000c
> > #define DISP_OD_CFG 0x0020
> > +#define OD_RELAYMODE BIT(0)
> > +#define OD_DITHERING BIT(2)
> > #define DISP_OD_SIZE 0x0030
> > +#define DISP_OD_DITHER_5 0x0114
> > +#define DISP_OD_DITHER_7 0x011c
> > +#define DISP_OD_DITHER_15 0x013c
> > +#define DITHER_LSB_ERR_SHIFT_R(x) (((x) & 0x7) << 28)
> > +#define DITHER_OVFLW_BIT_R(x) (((x) & 0x7) << 24)
> > +#define DITHER_ADD_LSHIFT_R(x) (((x) & 0x7) << 20)
> > +#define DITHER_ADD_RSHIFT_R(x) (((x) & 0x7) << 16)
> > +#define DITHER_NEW_BIT_MODE BIT(0)
> > +#define DISP_OD_DITHER_16 0x140
> > +#define DITHER_LSB_ERR_SHIFT_B(x) (((x) & 0x7) << 28)
> > +#define DITHER_OVFLW_BIT_B(x) (((x) & 0x7) << 24)
> > +#define DITHER_ADD_LSHIFT_B(x) (((x) & 0x7) << 20)
> > +#define DITHER_ADD_RSHIFT_B(x) (((x) & 0x7) << 16)
> > +#define DITHER_LSB_ERR_SHIFT_G(x) (((x) & 0x7) << 12)
> > +#define DITHER_OVFLW_BIT_G(x) (((x) & 0x7) << 8)
> > +#define DITHER_ADD_LSHIFT_G(x) (((x) & 0x7) << 4)
> > +#define DITHER_ADD_RSHIFT_G(x) (((x) & 0x7) << 0)
> >
> > #define DISP_REG_UFO_START 0x0000
> >
> > @@ -44,25 +63,28 @@
> >
> > #define DISP_GAMMA_EN 0x000
> > #define DISP_GAMMA_CFG 0x020
> > +#define GAMMA_EN BIT(0)
> > +#define GAMMA_LUT_EN BIT(1)
> > +#define GAMMA_DITHERING BIT(2)
> > #define DISP_GAMMA_SIZE 0x030
> > #define DISP_GAMMA_LUT 0x700
> > +#define DISP_GAMMA_DITHER_5 0x0114
> > +#define DISP_GAMMA_DITHER_7 0x011c
> > +#define DISP_GAMMA_DITHER_15 0x013c
> > +#define DISP_GAMMA_DITHER_16 0x0140
> >
> > #define LUT_10BIT_MASK 0x3ff
> >
> > #define AAL_EN BIT(0)
> >
> > -#define GAMMA_EN BIT(0)
> > -#define GAMMA_LUT_EN BIT(1)
> > -
> > -#define OD_RELAY_MODE BIT(0)
> > -
> > #define UFO_BYPASS BIT(2)
> >
> > #define COLOR_BYPASS_ALL BIT(7)
> > #define COLOR_SEQ_SEL BIT(13)
> >
> > static void mtk_color_config(struct mtk_ddp_comp *comp, unsigned int w,
> > - unsigned int h, unsigned int vrefresh)
> > + unsigned int h, unsigned int vrefresh,
> > + unsigned int bpc)
> > {
> > writel(w, comp->regs + DISP_COLOR_WIDTH);
> > writel(h, comp->regs + DISP_COLOR_HEIGHT);
> > @@ -76,14 +98,33 @@ static void mtk_color_start(struct mtk_ddp_comp *comp)
> > }
> >
> > static void mtk_od_config(struct mtk_ddp_comp *comp, unsigned int w,
> > - unsigned int h, unsigned int vrefresh)
> > + unsigned int h, unsigned int vrefresh,
> > + unsigned int bpc)
> > {
> > writel(w << 16 | h, comp->regs + DISP_OD_SIZE);
> > + if (bpc >= 3 && bpc < 10) {
>
> Use symbol to replace magic number can improve the readability. For
> example.
>
> #define DITHER_MIN_BITS 3
> #define DITHER_MAX_BITS 8
>
Ok, I will use definition instead of magic number.
> > + writel(0, comp->regs + DISP_OD_DITHER_5);
> > + writel(0, comp->regs + DISP_OD_DITHER_7);
> > + writel(DITHER_LSB_ERR_SHIFT_R(10 - bpc) |
> > + DITHER_ADD_LSHIFT_R(10 - bpc) |
> > + DITHER_NEW_BIT_MODE,
> > + comp->regs + DISP_OD_DITHER_15);
> > + writel(DITHER_LSB_ERR_SHIFT_B(10 - bpc) |
> > + DITHER_ADD_LSHIFT_B(10 - bpc) |
> > + DITHER_LSB_ERR_SHIFT_G(10 - bpc) |
> > + DITHER_ADD_LSHIFT_G(10 - bpc),
> > + comp->regs + DISP_OD_DITHER_16);
> > + writel(OD_DITHERING,
> > + comp->regs + DISP_OD_CFG);
> > + } else {
> > + writel(OD_RELAYMODE,
> > + comp->regs + DISP_OD_CFG);
> > + }
> > +
> > }
> >
> > static void mtk_od_start(struct mtk_ddp_comp *comp)
> > {
> > - writel(OD_RELAY_MODE, comp->regs + DISP_OD_CFG);
> > writel(1, comp->regs + DISP_OD_EN);
> > }
> >
> > @@ -93,7 +134,8 @@ static void mtk_ufoe_start(struct mtk_ddp_comp *comp)
> > }
> >
> > static void mtk_aal_config(struct mtk_ddp_comp *comp, unsigned int w,
> > - unsigned int h, unsigned int vrefresh)
> > + unsigned int h, unsigned int vrefresh,
> > + unsigned int bpc)
> > {
> > writel(h << 16 | w, comp->regs + DISP_AAL_SIZE);
> > }
> > @@ -109,9 +151,24 @@ static void mtk_aal_stop(struct mtk_ddp_comp *comp)
> > }
> >
> > static void mtk_gamma_config(struct mtk_ddp_comp *comp, unsigned int w,
> > - unsigned int h, unsigned int vrefresh)
> > + unsigned int h, unsigned int vrefresh,
> > + unsigned int bpc)
> > {
> > writel(h << 16 | w, comp->regs + DISP_GAMMA_SIZE);
> > + if (bpc >= 3 && bpc < 10) {
> > + writel(0, comp->regs + DISP_GAMMA_DITHER_5);
> > + writel(0, comp->regs + DISP_GAMMA_DITHER_7);
> > + writel(DITHER_LSB_ERR_SHIFT_R(10 - bpc) |
> > + DITHER_ADD_LSHIFT_R(10 - bpc) |
> > + DITHER_NEW_BIT_MODE,
> > + comp->regs + DISP_GAMMA_DITHER_15);
> > + writel(DITHER_LSB_ERR_SHIFT_B(10 - bpc) |
> > + DITHER_ADD_LSHIFT_B(10 - bpc) |
> > + DITHER_LSB_ERR_SHIFT_G(10 - bpc) |
> > + DITHER_ADD_LSHIFT_G(10 - bpc),
> > + comp->regs + DISP_GAMMA_DITHER_16);
> > + writel(GAMMA_DITHERING, comp->regs + DISP_GAMMA_CFG);
> > + }
>
> This part is the same as mtk_od_config(), so it's better to have a
> function 'mtk_dither_config()' and both mtk_od_config() and
> mtk_gamma_config() call this function.
>
Ok, will do.
> > }
> >
> > static void mtk_gamma_start(struct mtk_ddp_comp *comp)
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > index 07e17fe..c7c2e92 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > @@ -65,7 +65,7 @@ struct mtk_ddp_comp;
> >
> > struct mtk_ddp_comp_funcs {
> > void (*config)(struct mtk_ddp_comp *comp, unsigned int w,
> > - unsigned int h, unsigned int vrefresh);
> > + unsigned int h, unsigned int vrefresh, unsigned int bpc);
> > void (*start)(struct mtk_ddp_comp *comp);
> > void (*stop)(struct mtk_ddp_comp *comp);
> > void (*enable_vblank)(struct mtk_ddp_comp *comp, struct drm_crtc *crtc);
> > @@ -89,10 +89,10 @@ struct mtk_ddp_comp {
> >
> > static inline void mtk_ddp_comp_config(struct mtk_ddp_comp *comp,
> > unsigned int w, unsigned int h,
> > - unsigned int vrefresh)
> > + unsigned int vrefresh, unsigned int bpc)
> > {
> > if (comp->funcs && comp->funcs->config)
> > - comp->funcs->config(comp, w, h, vrefresh);
> > + comp->funcs->config(comp, w, h, vrefresh, bpc);
> > }
> >
> > static inline void mtk_ddp_comp_start(struct mtk_ddp_comp *comp)
>
>
