[RFC 2/3] drm/mediatek: add support for Mediatek SoC MT2701
On 18 May 2016 at 09:33, YT Shen wrote: >> > @@ -108,6 +108,10 @@ int mtk_drm_gem_dumb_create(struct drm_file >> > *file_priv, struct drm_device *dev, >> > int ret; >> > >> > args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); >> > + /* >> > + * align to 8 bytes since Mali requires it. >> > + */ >> > + args->pitch = ALIGN(args->pitch, 8); >> Are you sure we need this, based on the line just above ? > I think bpp stands for bits per pixel, so width * bpp / 8 simply transfer > from bits to bytes, which > cannot guarantee align to 8. > You're absolutely correct. Reading the comment made me loose my mind and completely misinterpret the division macro. > I will remove this align part from the patch, this constraint is not from > display controller. Thank you ! Regards Emil
[RFC 2/3] drm/mediatek: add support for Mediatek SoC MT2701
Hi Emil, On Tue, 2016-05-17 at 10:55 +0100, Emil Velikov wrote: > Hi YT Shen, > > On 12 May 2016 at 12:49, wrote: > > From: YT Shen > > > > This patch add support for the Mediatek MT2701 DISP subsystem. > > There is only one OVL engine in MT2701, and we have shadow > > register support here. > > > > Signed-off-by: YT Shen > > --- > > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 49 ++--- > > drivers/gpu/drm/mediatek/mtk_disp_rdma.c| 36 +-- > > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 78 +- > > drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 151 > > --- > > drivers/gpu/drm/mediatek/mtk_drm_ddp.h |2 + > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 63 +-- > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 15 +++ > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 72 +++-- > > drivers/gpu/drm/mediatek/mtk_drm_drv.h |9 ++ > > drivers/gpu/drm/mediatek/mtk_drm_gem.c |4 + > > 10 files changed, 373 insertions(+), 106 deletions(-) > > > This patch does a bit too many things at once imho > - Renames existing macros > - Factors out helper functions - mtk_crtc_ddp_config and alike. > - Introduces *driver_data for existing hardware and uses it > - and adds support for the different hardware. > > I'm no expert in the area, but it feels like you want to split things > roughly as per above. > A rather serious mali note and some "this should be const" follow > suggestions inline. Thanks for your suggestions. I will split this patch into several small patches. They are easier to understand, and easier to review. > > > > > > +static struct mtk_ddp_comp_driver_data mt2701_ovl_driver_data = { > > + .ovl = {0x0040, 1 << 12, 0} > > +}; > > + > > +static struct mtk_ddp_comp_driver_data mt8173_ovl_driver_data = { > > + .ovl = {0x0f40, 0, 1 << 12} > > +}; > > + > These two should be const right ? Yes, they should. > > > > > > +static struct mtk_ddp_comp_driver_data mt2701_rdma_driver_data = { > > + .rdma_fifo_pseudo_size = SZ_4K, > > +}; > > + > > +static struct mtk_ddp_comp_driver_data mt8173_rdma_driver_data = { > > + .rdma_fifo_pseudo_size = SZ_8K, > > +}; > > + > Same here. OK. > > > > > > -#define MUTEX_MOD_DISP_OVL0BIT(11) > > -#define MUTEX_MOD_DISP_OVL1BIT(12) > > -#define MUTEX_MOD_DISP_RDMA0 BIT(13) > > -#define MUTEX_MOD_DISP_RDMA1 BIT(14) > > -#define MUTEX_MOD_DISP_RDMA2 BIT(15) > > -#define MUTEX_MOD_DISP_WDMA0 BIT(16) > > -#define MUTEX_MOD_DISP_WDMA1 BIT(17) > > -#define MUTEX_MOD_DISP_COLOR0 BIT(18) > > -#define MUTEX_MOD_DISP_COLOR1 BIT(19) > > -#define MUTEX_MOD_DISP_AAL BIT(20) > > -#define MUTEX_MOD_DISP_GAMMA BIT(21) > > -#define MUTEX_MOD_DISP_UFOEBIT(22) > > -#define MUTEX_MOD_DISP_PWM0BIT(23) > > -#define MUTEX_MOD_DISP_PWM1BIT(24) > > -#define MUTEX_MOD_DISP_OD BIT(25) > > +#define MUTEX_MOD_MT8173_DISP_OVL0 BIT(11) > > +#define MUTEX_MOD_MT8173_DISP_OVL1 BIT(12) > > +#define MUTEX_MOD_MT8173_DISP_RDMA0BIT(13) > > +#define MUTEX_MOD_MT8173_DISP_RDMA1BIT(14) > > +#define MUTEX_MOD_MT8173_DISP_RDMA2BIT(15) > > +#define MUTEX_MOD_MT8173_DISP_WDMA0BIT(16) > > +#define MUTEX_MOD_MT8173_DISP_WDMA1BIT(17) > > +#define MUTEX_MOD_MT8173_DISP_COLOR0 BIT(18) > > +#define MUTEX_MOD_MT8173_DISP_COLOR1 BIT(19) > > +#define MUTEX_MOD_MT8173_DISP_AAL BIT(20) > > +#define MUTEX_MOD_MT8173_DISP_GAMMABIT(21) > > +#define MUTEX_MOD_MT8173_DISP_UFOE BIT(22) > > +#define MUTEX_MOD_MT8173_DISP_PWM0 BIT(23) > > +#define MUTEX_MOD_MT8173_DISP_PWM1 BIT(24) > > +#define MUTEX_MOD_MT8173_DISP_OD BIT(25) > > + > > +#define MUTEX_MOD_MT2701_DISP_OVL BIT(3) > > +#define MUTEX_MOD_MT2701_DISP_WDMA BIT(6) > > +#define MUTEX_MOD_MT2701_DISP_COLORBIT(7) > > +#define MUTEX_MOD_MT2701_DISP_BLS BIT(9) > > +#define MUTEX_MOD_MT2701_DISP_RDMA0BIT(10) > > +#define MUTEX_MOD_MT2701_DISP_RDMA1BIT(12) > > > Even though the driver not does use a unique prefix/namespace for > these macros (which it should imho), it's better to keep the hardware > name/part first. Ideally the rename will be a separate patch. OK, I will rename the macros and put it in a separate patch. > > > > @@ -131,6 +153,32 @@ static const struct mtk_ddp_comp_match > > mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = { > > [DDP_COMPONENT_WDMA1] = { MTK_DISP_WDMA, 1, NULL }, > > }; > > > > +static struct mtk_ddp_comp_driver_data mt2701_color_driver_data = { > > + .color_offset = 0x0f00, > > +}; > > + > > +static struct mtk_ddp_comp_driver_data mt8173_color_driver_data = { > > + .color_offset =
[RFC 2/3] drm/mediatek: add support for Mediatek SoC MT2701
Hi YT Shen, On 12 May 2016 at 12:49, wrote: > From: YT Shen > > This patch add support for the Mediatek MT2701 DISP subsystem. > There is only one OVL engine in MT2701, and we have shadow > register support here. > > Signed-off-by: YT Shen > --- > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 49 ++--- > drivers/gpu/drm/mediatek/mtk_disp_rdma.c| 36 +-- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 78 +- > drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 151 > --- > drivers/gpu/drm/mediatek/mtk_drm_ddp.h |2 + > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 63 +-- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 15 +++ > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 72 +++-- > drivers/gpu/drm/mediatek/mtk_drm_drv.h |9 ++ > drivers/gpu/drm/mediatek/mtk_drm_gem.c |4 + > 10 files changed, 373 insertions(+), 106 deletions(-) > This patch does a bit too many things at once imho - Renames existing macros - Factors out helper functions - mtk_crtc_ddp_config and alike. - Introduces *driver_data for existing hardware and uses it - and adds support for the different hardware. I'm no expert in the area, but it feels like you want to split things roughly as per above. A rather serious mali note and some "this should be const" follow suggestions inline. > > +static struct mtk_ddp_comp_driver_data mt2701_ovl_driver_data = { > + .ovl = {0x0040, 1 << 12, 0} > +}; > + > +static struct mtk_ddp_comp_driver_data mt8173_ovl_driver_data = { > + .ovl = {0x0f40, 0, 1 << 12} > +}; > + These two should be const right ? > > +static struct mtk_ddp_comp_driver_data mt2701_rdma_driver_data = { > + .rdma_fifo_pseudo_size = SZ_4K, > +}; > + > +static struct mtk_ddp_comp_driver_data mt8173_rdma_driver_data = { > + .rdma_fifo_pseudo_size = SZ_8K, > +}; > + Same here. > > -#define MUTEX_MOD_DISP_OVL0BIT(11) > -#define MUTEX_MOD_DISP_OVL1BIT(12) > -#define MUTEX_MOD_DISP_RDMA0 BIT(13) > -#define MUTEX_MOD_DISP_RDMA1 BIT(14) > -#define MUTEX_MOD_DISP_RDMA2 BIT(15) > -#define MUTEX_MOD_DISP_WDMA0 BIT(16) > -#define MUTEX_MOD_DISP_WDMA1 BIT(17) > -#define MUTEX_MOD_DISP_COLOR0 BIT(18) > -#define MUTEX_MOD_DISP_COLOR1 BIT(19) > -#define MUTEX_MOD_DISP_AAL BIT(20) > -#define MUTEX_MOD_DISP_GAMMA BIT(21) > -#define MUTEX_MOD_DISP_UFOEBIT(22) > -#define MUTEX_MOD_DISP_PWM0BIT(23) > -#define MUTEX_MOD_DISP_PWM1BIT(24) > -#define MUTEX_MOD_DISP_OD BIT(25) > +#define MUTEX_MOD_MT8173_DISP_OVL0 BIT(11) > +#define MUTEX_MOD_MT8173_DISP_OVL1 BIT(12) > +#define MUTEX_MOD_MT8173_DISP_RDMA0BIT(13) > +#define MUTEX_MOD_MT8173_DISP_RDMA1BIT(14) > +#define MUTEX_MOD_MT8173_DISP_RDMA2BIT(15) > +#define MUTEX_MOD_MT8173_DISP_WDMA0BIT(16) > +#define MUTEX_MOD_MT8173_DISP_WDMA1BIT(17) > +#define MUTEX_MOD_MT8173_DISP_COLOR0 BIT(18) > +#define MUTEX_MOD_MT8173_DISP_COLOR1 BIT(19) > +#define MUTEX_MOD_MT8173_DISP_AAL BIT(20) > +#define MUTEX_MOD_MT8173_DISP_GAMMABIT(21) > +#define MUTEX_MOD_MT8173_DISP_UFOE BIT(22) > +#define MUTEX_MOD_MT8173_DISP_PWM0 BIT(23) > +#define MUTEX_MOD_MT8173_DISP_PWM1 BIT(24) > +#define MUTEX_MOD_MT8173_DISP_OD BIT(25) > + > +#define MUTEX_MOD_MT2701_DISP_OVL BIT(3) > +#define MUTEX_MOD_MT2701_DISP_WDMA BIT(6) > +#define MUTEX_MOD_MT2701_DISP_COLORBIT(7) > +#define MUTEX_MOD_MT2701_DISP_BLS BIT(9) > +#define MUTEX_MOD_MT2701_DISP_RDMA0BIT(10) > +#define MUTEX_MOD_MT2701_DISP_RDMA1BIT(12) > Even though the driver not does use a unique prefix/namespace for these macros (which it should imho), it's better to keep the hardware name/part first. Ideally the rename will be a separate patch. > @@ -131,6 +153,32 @@ static const struct mtk_ddp_comp_match > mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = { > [DDP_COMPONENT_WDMA1] = { MTK_DISP_WDMA, 1, NULL }, > }; > > +static struct mtk_ddp_comp_driver_data mt2701_color_driver_data = { > + .color_offset = 0x0f00, > +}; > + > +static struct mtk_ddp_comp_driver_data mt8173_color_driver_data = { > + .color_offset = 0x0c00, > +}; > + const again ? You can even tweak the *_get_driver_data helpers, to return const struct foo*, and resolve the odd warning that the compiler will give you. > struct mtk_ddp_comp { > struct clk *clk; > void __iomem *regs; > @@ -82,6 +96,7 @@ struct mtk_ddp_comp { > struct device *larb_dev; > enum mtk_ddp_comp_id id; > const struct mtk_ddp_comp_funcs *funcs; > + struct mtk_ddp_comp_driver_data *data; const > +static struct mtk_mmsys_driver_data
[RFC 2/3] drm/mediatek: add support for Mediatek SoC MT2701
Hi CK, On Fri, 2016-05-13 at 11:59 +0800, CK Hu wrote: > Hi, YT: > > On Thu, 2016-05-12 at 19:49 +0800, yt.shen at mediatek.com wrote: > > From: YT Shen > > > > This patch add support for the Mediatek MT2701 DISP subsystem. > > There is only one OVL engine in MT2701, and we have shadow > > register support here. > > > > Signed-off-by: YT Shen > > --- > > > @@ -385,12 +422,16 @@ static void mtk_drm_crtc_atomic_begin(struct drm_crtc > > *crtc, > > mtk_crtc->event = state->base.event; > > state->base.event = NULL; > > } > > + > > + if (priv->data->shadow_register) > > + mtk_disp_mutex_acquire(mtk_crtc->mutex); > > } > > > > @@ -409,6 +450,11 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc > > *crtc, > > } > > if (pending_planes) > > mtk_crtc->pending_planes = true; > > + > > + if (priv->data->shadow_register) { > > + mtk_crtc_ddp_config(crtc); > > + mtk_disp_mutex_release(mtk_crtc->mutex); > > + } > > } > > > > I think it's better to call mtk_disp_mutex_acquire() and > mtk_disp_mutex_release() as near as possible to prevent mutex locked for > a long time. All HW register access of this atomic setting is in > mtk_crtc_ddp_config(), so it's better to move mtk_disp_mutex_acquire() > just before mtk_crtc_ddp_config(). > > Regards, > CK I'll do this in the next version. Thanks for the review. Regards, yt.shen
[RFC 2/3] drm/mediatek: add support for Mediatek SoC MT2701
Hi, YT: On Thu, 2016-05-12 at 19:49 +0800, yt.shen at mediatek.com wrote: > From: YT Shen > > This patch add support for the Mediatek MT2701 DISP subsystem. > There is only one OVL engine in MT2701, and we have shadow > register support here. > > Signed-off-by: YT Shen > --- > @@ -385,12 +422,16 @@ static void mtk_drm_crtc_atomic_begin(struct drm_crtc > *crtc, > mtk_crtc->event = state->base.event; > state->base.event = NULL; > } > + > + if (priv->data->shadow_register) > + mtk_disp_mutex_acquire(mtk_crtc->mutex); > } > > @@ -409,6 +450,11 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc > *crtc, > } > if (pending_planes) > mtk_crtc->pending_planes = true; > + > + if (priv->data->shadow_register) { > + mtk_crtc_ddp_config(crtc); > + mtk_disp_mutex_release(mtk_crtc->mutex); > + } > } > I think it's better to call mtk_disp_mutex_acquire() and mtk_disp_mutex_release() as near as possible to prevent mutex locked for a long time. All HW register access of this atomic setting is in mtk_crtc_ddp_config(), so it's better to move mtk_disp_mutex_acquire() just before mtk_crtc_ddp_config(). Regards, CK
[RFC 2/3] drm/mediatek: add support for Mediatek SoC MT2701
From: YT ShenThis patch add support for the Mediatek MT2701 DISP subsystem. There is only one OVL engine in MT2701, and we have shadow register support here. Signed-off-by: YT Shen --- drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 49 ++--- drivers/gpu/drm/mediatek/mtk_disp_rdma.c| 36 +-- drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 78 +- drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 151 --- drivers/gpu/drm/mediatek/mtk_drm_ddp.h |2 + drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 63 +-- drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 15 +++ drivers/gpu/drm/mediatek/mtk_drm_drv.c | 72 +++-- drivers/gpu/drm/mediatek/mtk_drm_drv.h |9 ++ drivers/gpu/drm/mediatek/mtk_drm_gem.c |4 + 10 files changed, 373 insertions(+), 106 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c index 8f62671f..e4a10bd6 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c @@ -40,8 +40,6 @@ #defineOVL_RDMA_MEM_GMC0x40402020 #define OVL_CON_BYTE_SWAP BIT(24) -#define OVL_CON_CLRFMT_RGB565 (0 << 12) -#define OVL_CON_CLRFMT_RGB888 (1 << 12) #define OVL_CON_CLRFMT_RGBA(2 << 12) #define OVL_CON_CLRFMT_ARGB(3 << 12) #defineOVL_CON_AEN BIT(8) @@ -136,18 +134,18 @@ static void mtk_ovl_layer_off(struct mtk_ddp_comp *comp, unsigned int idx) writel(0x0, comp->regs + DISP_REG_OVL_RDMA_CTRL(idx)); } -static unsigned int ovl_fmt_convert(unsigned int fmt) +static unsigned int ovl_fmt_convert(struct mtk_ddp_comp *comp, unsigned int fmt) { switch (fmt) { default: case DRM_FORMAT_RGB565: - return OVL_CON_CLRFMT_RGB565; + return comp->data->ovl.fmt_rgb565; case DRM_FORMAT_BGR565: - return OVL_CON_CLRFMT_RGB565 | OVL_CON_BYTE_SWAP; + return comp->data->ovl.fmt_rgb565 | OVL_CON_BYTE_SWAP; case DRM_FORMAT_RGB888: - return OVL_CON_CLRFMT_RGB888; + return comp->data->ovl.fmt_rgb888; case DRM_FORMAT_BGR888: - return OVL_CON_CLRFMT_RGB888 | OVL_CON_BYTE_SWAP; + return comp->data->ovl.fmt_rgb888 | OVL_CON_BYTE_SWAP; case DRM_FORMAT_RGBX: case DRM_FORMAT_RGBA: return OVL_CON_CLRFMT_ARGB; @@ -177,7 +175,7 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx, if (!pending->enable) mtk_ovl_layer_off(comp, idx); - con = ovl_fmt_convert(fmt); + con = ovl_fmt_convert(comp, fmt); if (idx != 0) con |= OVL_CON_AEN | OVL_CON_ALPHA; @@ -185,7 +183,7 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx, writel_relaxed(pitch, comp->regs + DISP_REG_OVL_PITCH(idx)); writel_relaxed(src_size, comp->regs + DISP_REG_OVL_SRC_SIZE(idx)); writel_relaxed(offset, comp->regs + DISP_REG_OVL_OFFSET(idx)); - writel_relaxed(addr, comp->regs + DISP_REG_OVL_ADDR(idx)); + writel_relaxed(addr, comp->regs + (comp->data->ovl.addr_offset + idx * 0x20)); if (pending->enable) mtk_ovl_layer_on(comp, idx); @@ -233,6 +231,32 @@ static const struct component_ops mtk_disp_ovl_component_ops = { .unbind = mtk_disp_ovl_unbind, }; +static struct mtk_ddp_comp_driver_data mt2701_ovl_driver_data = { + .ovl = {0x0040, 1 << 12, 0} +}; + +static struct mtk_ddp_comp_driver_data mt8173_ovl_driver_data = { + .ovl = {0x0f40, 0, 1 << 12} +}; + +static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = { + { .compatible = "mediatek,mt2701-disp-ovl", + .data = _ovl_driver_data}, + { .compatible = "mediatek,mt8173-disp-ovl", + .data = _ovl_driver_data}, + {}, +}; +MODULE_DEVICE_TABLE(of, mtk_disp_ovl_driver_dt_match); + +static inline struct mtk_ddp_comp_driver_data *mtk_ovl_get_driver_data( + struct platform_device *pdev) +{ + const struct of_device_id *of_id = + of_match_device(mtk_disp_ovl_driver_dt_match, >dev); + + return (struct mtk_ddp_comp_driver_data *)of_id->data; +} + static int mtk_disp_ovl_probe(struct platform_device *pdev) { struct device *dev = >dev; @@ -269,6 +293,8 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev) return ret; } + priv->ddp_comp.data = mtk_ovl_get_driver_data(pdev); + platform_set_drvdata(pdev, priv); ret = component_add(dev, _disp_ovl_component_ops); @@ -285,11 +311,6 @@ static int mtk_disp_ovl_remove(struct platform_device *pdev) return 0; } -static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = { - { .compatible = "mediatek,mt8173-disp-ovl", }, - {}, -}; -MODULE_DEVICE_TABLE(of,