Re: [RFC 2/3] drm/mediatek: add support for Mediatek SoC MT2701
From: Emil Velikov
Date: Tue May 17 2016 - 05:55:52 EST
Hi YT Shen,
On 12 May 2016 at 12:49, <yt.shen@xxxxxxxxxxxx> wrote:
> From: YT Shen <yt.shen@xxxxxxxxxxxx>
>
> 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 <yt.shen@xxxxxxxxxxxx>
> ---
> 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_OVL0 BIT(11)
> -#define MUTEX_MOD_DISP_OVL1 BIT(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_UFOE BIT(22)
> -#define MUTEX_MOD_DISP_PWM0 BIT(23)
> -#define MUTEX_MOD_DISP_PWM1 BIT(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_RDMA0 BIT(13)
> +#define MUTEX_MOD_MT8173_DISP_RDMA1 BIT(14)
> +#define MUTEX_MOD_MT8173_DISP_RDMA2 BIT(15)
> +#define MUTEX_MOD_MT8173_DISP_WDMA0 BIT(16)
> +#define MUTEX_MOD_MT8173_DISP_WDMA1 BIT(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_GAMMA BIT(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_COLOR BIT(7)
> +#define MUTEX_MOD_MT2701_DISP_BLS BIT(9)
> +#define MUTEX_MOD_MT2701_DISP_RDMA0 BIT(10)
> +#define MUTEX_MOD_MT2701_DISP_RDMA1 BIT(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 mt2701_mmsys_driver_data = {
> + .main_path = mtk_ddp_main_2701,
> + .main_len = ARRAY_SIZE(mtk_ddp_main_2701),
> + .ext_path = mtk_ddp_ext_2701,
> + .ext_len = ARRAY_SIZE(mtk_ddp_ext_2701),
> + .shadow_register = true,
> +};
> +
> +static struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
> + .main_path = mtk_ddp_main_8173,
> + .main_len = ARRAY_SIZE(mtk_ddp_main_8173),
> + .ext_path = mtk_ddp_ext_8173,
> + .ext_len = ARRAY_SIZE(mtk_ddp_ext_8173),
> + .shadow_register = false,
> +};
> +
const
> @@ -40,6 +48,7 @@ struct mtk_drm_private {
> void __iomem *config_regs;
> struct device_node *comp_node[DDP_COMPONENT_ID_MAX];
> struct mtk_ddp_comp *ddp_comp[DDP_COMPONENT_ID_MAX];
> + struct mtk_mmsys_driver_data *data;
const ?
> @@ -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 ?
Iirc we had a chat earlier that upstream kernel code should not be
tailoured for the needs to closed source userspace... seems like the
comment got removed but the code remained. Philipp Zabel I believe you
were the person who did the original upstreaming - can we remove this
hack/workaround and keep it downstream until we have an open-source
user that requires it ?
Can we have a MAINTAINERS entry for this driver ?
Thanks
Emil