Hi, CK

I'm appreciate your comments.

On Fri, 2016-07-15 at 17:11 +0800, CK Hu wrote:

Hi, Bibby:

Some comments inline.

On Thu, 2016-07-07 at 15:37 +0800, Bibby Hsieh wrote:

...

Apply gamma function to correct brightness values. It applies arbitrary mapping curve to compensate the incorrect transfer function of the panel.

Signed-off-by: Bibby Hsieh bibby.hsieh@mediatek.com

drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 8 +++ drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 1 + drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 89 ++++++++++++++++++++++++++- drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 10 +++ 4 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index 24aa3ba..ee219bb 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -409,6 +409,10 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc, } if (pending_planes) mtk_crtc->pending_planes = true;

• if (crtc->state->color_mgmt_changed)

• for (i = 0; i < mtk_crtc->ddp_comp_nr; i++)
  

• 	mtk_ddp_gamma_set(mtk_crtc->ddp_comp[i], crtc->state);
  

}

static const struct drm_crtc_funcs mtk_crtc_funcs = { @@ -418,6 +422,7 @@ static const struct drm_crtc_funcs mtk_crtc_funcs = { .reset = mtk_drm_crtc_reset, .atomic_duplicate_state = mtk_drm_crtc_duplicate_state, .atomic_destroy_state = mtk_drm_crtc_destroy_state,

• .gamma_set = drm_atomic_helper_legacy_gamma_set,

};

static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = { @@ -569,6 +574,9 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, if (ret < 0) goto unprepare;

• drm_mode_crtc_set_gamma_size(&mtk_crtc->base, MTK_LUT_SIZE);
• drm_helper_crtc_enable_color_mgmt(&mtk_crtc->base, MTK_LUT_SIZE,

• 			  MTK_LUT_SIZE);
  

  priv->crtc[pipe] = &mtk_crtc->base; priv->num_pipes++;

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h index 81e5566..d332564 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h @@ -19,6 +19,7 @@ #include "mtk_drm_plane.h"

#define OVL_LAYER_NR 4 +#define MTK_LUT_SIZE 512

int mtk_drm_crtc_enable_vblank(struct drm_device *drm, unsigned int pipe); void mtk_drm_crtc_disable_vblank(struct drm_device *drm, unsigned int pipe); diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c index 3970fcf..56c5894 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c @@ -24,6 +24,7 @@ #include "mtk_drm_drv.h" #include "mtk_drm_plane.h" #include "mtk_drm_ddp_comp.h" +#include "mtk_drm_crtc.h"

#define DISP_OD_EN 0x0000 #define DISP_OD_INTEN 0x0008 @@ -38,6 +39,21 @@ #define DISP_COLOR_WIDTH 0x0c50 #define DISP_COLOR_HEIGHT 0x0c54

+#define DISP_AAL_EN 0x000 +#define DISP_AAL_SIZE 0x030

+#define DISP_GAMMA_EN 0x000 +#define DISP_GAMMA_CFG 0x020 +#define DISP_GAMMA_SIZE 0x030 +#define DISP_GAMMA_LUT 0x700

It's better that the digits of register address of OD, COLOR, AAL, and GAMMA are the same. Maybe you can align all to 4 digits.

Ok, will fix.

...

+#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) @@ -76,6 +92,61 @@ static void mtk_ufoe_start(struct mtk_ddp_comp *comp) writel(UFO_BYPASS, comp->regs + DISP_REG_UFO_START); }

+static void mtk_aal_config(struct mtk_ddp_comp *comp, unsigned int w,

• 	   unsigned int h, unsigned int vrefresh)
  

+{

• writel(h << 16 | w, comp->regs + DISP_AAL_SIZE);

+}

+static void mtk_aal_start(struct mtk_ddp_comp *comp) +{

• writel(AAL_EN, comp->regs + DISP_AAL_EN);

+}

+static void mtk_aal_stop(struct mtk_ddp_comp *comp) +{

• writel_relaxed(0x0, comp->regs + DISP_AAL_EN);

+}

I think AAL is somewhat different from GAMMA and this patch include 3 modifications:

1. AAL basic config
2. GAMMA basic config
3. Add gamma function of AAL and GAMMA

So you should split this patch into 3 patches.

Ok, I will split that.

...

+static void mtk_gamma_config(struct mtk_ddp_comp *comp, unsigned int w,

• 	     unsigned int h, unsigned int vrefresh)
  

+{

• writel(h << 16 | w, comp->regs + DISP_GAMMA_SIZE);

+}

+static void mtk_gamma_start(struct mtk_ddp_comp *comp) +{

• writel(GAMMA_EN, comp->regs + DISP_GAMMA_EN);

+}

+static void mtk_gamma_stop(struct mtk_ddp_comp *comp) +{

• writel_relaxed(0x0, comp->regs + DISP_GAMMA_EN);

+}

+static void mtk_gamma_set(struct mtk_ddp_comp *comp,

• 	  struct drm_crtc_state *state)
  

+{

• unsigned int i, reg;
• struct drm_color_lut *lut;
• void __iomem *lut_base;
• u32 word;
• if (state->gamma_lut) {

• reg = readl(comp->regs + DISP_GAMMA_CFG);
  

• reg = reg | GAMMA_LUT_EN;
  

• writel(reg, comp->regs + DISP_GAMMA_CFG);
  

• lut_base = comp->regs + DISP_GAMMA_LUT;
  

• lut = (struct drm_color_lut *)state->gamma_lut->data;
  

• for (i = 0; i < MTK_LUT_SIZE; i++) {
  

• 	word = (((lut[i].red >> 6) & LUT_10BIT_MASK) << 20) +
  

• 	      (((lut[i].green >> 6) & LUT_10BIT_MASK) << 10) +
  

• 	      ((lut[i].blue >> 6) & LUT_10BIT_MASK);
  

• 	writel(word, (lut_base + i * 4));
  

• }
  

• }

+}

static const struct mtk_ddp_comp_funcs ddp_color = { .config = mtk_color_config, .start = mtk_color_start, @@ -90,6 +161,20 @@ static const struct mtk_ddp_comp_funcs ddp_ufoe = { .start = mtk_ufoe_start, };

+static const struct mtk_ddp_comp_funcs ddp_aal = {

• .gamma_set = mtk_gamma_set,
• .config = mtk_aal_config,
• .start = mtk_aal_start,
• .stop = mtk_aal_stop,

+};

+static const struct mtk_ddp_comp_funcs ddp_gamma = {

• .gamma_set = mtk_gamma_set,
• .config = mtk_gamma_config,
• .start = mtk_gamma_start,
• .stop = mtk_gamma_stop,

+};

static const char * const mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX] = { [MTK_DISP_OVL] = "ovl", [MTK_DISP_RDMA] = "rdma", @@ -112,13 +197,13 @@ struct mtk_ddp_comp_match { };

static const struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = {

• [DDP_COMPONENT_AAL] = { MTK_DISP_AAL, 0, NULL },

• [DDP_COMPONENT_AAL] = { MTK_DISP_AAL, 0, &ddp_aal }, [DDP_COMPONENT_COLOR0] = { MTK_DISP_COLOR, 0, &ddp_color }, [DDP_COMPONENT_COLOR1] = { MTK_DISP_COLOR, 1, &ddp_color }, [DDP_COMPONENT_DPI0] = { MTK_DPI, 0, NULL }, [DDP_COMPONENT_DSI0] = { MTK_DSI, 0, NULL }, [DDP_COMPONENT_DSI1] = { MTK_DSI, 1, NULL },

• [DDP_COMPONENT_GAMMA] = { MTK_DISP_GAMMA, 0, NULL },

• [DDP_COMPONENT_GAMMA] = { MTK_DISP_GAMMA, 0, &ddp_gamma }, [DDP_COMPONENT_OD] = { MTK_DISP_OD, 0, &ddp_od }, [DDP_COMPONENT_OVL0] = { MTK_DISP_OVL, 0, NULL }, [DDP_COMPONENT_OVL1] = { MTK_DISP_OVL, 1, NULL },

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h index 6b13ba9..07e17fe 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h @@ -20,6 +20,7 @@ struct device; struct device_node; struct drm_crtc; struct drm_device; +struct drm_crtc_state; struct mtk_plane_state;

enum mtk_ddp_comp_type { @@ -73,6 +74,8 @@ struct mtk_ddp_comp_funcs { void (*layer_off)(struct mtk_ddp_comp *comp, unsigned int idx); void (*layer_config)(struct mtk_ddp_comp *comp, unsigned int idx, struct mtk_plane_state *state);

• void (*gamma_set)(struct mtk_ddp_comp *comp,

• 	  struct drm_crtc_state *state);
  

};

struct mtk_ddp_comp { @@ -139,6 +142,13 @@ static inline void mtk_ddp_comp_layer_config(struct mtk_ddp_comp *comp, comp->funcs->layer_config(comp, idx, state); }

+static inline void mtk_ddp_gamma_set(struct mtk_ddp_comp *comp,

• 		     struct drm_crtc_state *state)
  

+{

• if (comp->funcs && comp->funcs->gamma_set)

• comp->funcs->gamma_set(comp, state);
  

+}

int mtk_ddp_comp_get_id(struct device_node *node, enum mtk_ddp_comp_type comp_type); int mtk_ddp_comp_init(struct device *dev, struct device_node *comp_node,

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@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.

• 	    (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.

• 		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.

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

• 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.

}

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)

Hi, Bibby:

On Thu, 2016-07-21 at 11:21 +0800, Bibby Hsieh wrote:

Hi, CK

I'm appreciate your comments.

[snip...]

...

...

@@ -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.

Using 0 as 'don't care' looks tricky. Please add comments to describe this.

...

...

state->pending_config = false;

}

Regards, CK