Re: [PATCH v3 2/2] drm/mediatek: set mt8173 dithering function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
> > ---
> >  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)
> 
> 


_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux