[RFC 2/3] drm/mediatek: add support for Mediatek SoC MT2701

2016-05-18 Thread Emil Velikov
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

2016-05-18 Thread YT Shen
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

2016-05-17 Thread Emil Velikov
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

2016-05-16 Thread YT Shen
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

2016-05-13 Thread CK Hu
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

2016-05-12 Thread yt.s...@mediatek.com
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(-)

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,