| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGS+omAa_SLg8WKfXXvZ-UT6-Mdep28FQVJuuwfskNqP2x50eg@mail.gmail.com>
Date: Fri, 18 Nov 2016 12:56:47 +0800
From: Daniel Kurtz <djkurtz@...omium.org>
To: YT Shen <yt.shen@...iatek.com>
Cc: dri-devel <dri-devel@...ts.freedesktop.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
David Airlie <airlied@...ux.ie>,
Matthias Brugger <matthias.bgg@...il.com>,
Mao Huang <littlecvr@...omium.org>, CK Hu <ck.hu@...iatek.com>,
Bibby Hsieh <bibby.hsieh@...iatek.com>,
Daniel Vetter <daniel.vetter@...ll.ch>,
Thierry Reding <thierry.reding@...il.com>,
Jie Qiu <jie.qiu@...iatek.com>,
Maxime Ripard <maxime.ripard@...e-electrons.com>,
Chris Wilson <chris@...is-wilson.co.uk>,
shaoming chen <shaoming.chen@...iatek.com>,
Jitao Shi <jitao.shi@...iatek.com>,
Boris Brezillon <boris.brezillon@...e-electrons.com>,
Dan Carpenter <dan.carpenter@...cle.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
srv_heupstream <srv_heupstream@...iatek.com>,
Sascha Hauer <kernel@...gutronix.de>,
Yingjoe Chen (陳英洲)
<yingjoe.chen@...iatek.com>,
Emil Velikov <emil.l.velikov@...il.com>
Subject: Re: [PATCH v9 02/10] drm/mediatek: add *driver_data for different
hardware settings
Hi YT,
I don't see a reason to handle device_data in such a generic way at
the generic mtk_ddp_comp layer.
The device data is very component specific, so just define different
structs for different comp types, ie:
struct mtk_disp_ovl_driver_data {
unsigned int reg_ovl_addr;
unsigned int fmt_rgb565;
unsigned int fmt_rgb888;
};
struct mtk_disp_rdma_driver_data {
unsigned int fifo_pseudo_size;
};
struct mtk_disp_color_driver_data {
unsigned int color_offset;
};
Then add typed pointers to the local structs that use them, for example:
struct mtk_disp_ovl {
struct mtk_ddp_comp ddp_comp;
struct drm_crtc *crtc;
const struct mtk_disp_ovl_driver_data *data;
};
And fetch the device specific driver data directly in .probe, as you
are already doing:
static int mtk_disp_ovl_probe(struct platform_device *pdev) {
...
priv->data = of_device_get_match_data(dev);
...
}
More comments in-line...
On Fri, Nov 11, 2016 at 7:55 PM, YT Shen <yt.shen@...iatek.com> wrote:
> There are some hardware settings changed, between MT8173 & MT2701:
> DISP_OVL address offset changed, color format definition changed.
> DISP_RDMA fifo size changed.
> DISP_COLOR offset changed.
> MIPI_TX pll setting changed.
> And add prefix for mtk_ddp_main & mtk_ddp_ext & mutex_mod.
Nit: I think it would make sense to combine this patch with
drm/mediatek: rename macros, add chip prefix
>
> Signed-off-by: YT Shen <yt.shen@...iatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 27 ++++++++++++++++-----------
> drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 11 +++++++++--
> drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 11 +++++++----
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 27 +++++++++++++++++++++------
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 13 +++++++++++++
> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 25 ++++++++++++++++++-------
> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 8 ++++++++
> drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 24 +++++++++++++++++++++++-
> 8 files changed, 115 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 019b7ca..1139834 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -35,13 +35,10 @@
> #define DISP_REG_OVL_PITCH(n) (0x0044 + 0x20 * (n))
> #define DISP_REG_OVL_RDMA_CTRL(n) (0x00c0 + 0x20 * (n))
> #define DISP_REG_OVL_RDMA_GMC(n) (0x00c8 + 0x20 * (n))
> -#define DISP_REG_OVL_ADDR(n) (0x0f40 + 0x20 * (n))
Also, I would still use the "#define macros", for example
"DISP_REG_OVL_ADDR offsets, and use the named constant in the
driver_data:
#define DISP_REG_OVL_ADDR_MT8173 0x0f40
(and in a later patch:
#define DISP_REG_OVL_ADDR_MT2701 0x0040
)
Also, I would still use the macro rather than open coding the "0x20 *
(n)", and just pass 'ovl' to the overlay macros that depend on
hardware type.
Something like the following:
#define DISP_REG_OVL_ADDR(ovl, n) ((ovl)->data->ovl_addr + 0x20 * (n))
>
> #define OVL_RDMA_MEM_GMC 0x40402020
>
> #define OVL_CON_BYTE_SWAP BIT(24)
> -#define OVL_CON_CLRFMT_RGB565 (0 << 12)
> -#define OVL_CON_CLRFMT_RGB888 (1 << 12)
This seems like a really random and unnecessary hardware change.
Why chip designers, why!!?!?
For this one, it seems the polarity is either one way or the other, so
we can just use a bool to distinguish:
bool fmt_rgb565_is_0;
> +static const struct mtk_ddp_comp_driver_data mt8173_ovl_driver_data = {
> + .ovl = { DISP_REG_OVL_ADDR_MT8173, .fmt_rgb565_is_0 = true }
> +};
For use at runtime, the defines could become:
#define OVL_CON_CLRFMT_RGB565(ovl) ((ovl)->data->fmt_rgb565_is_0 ? 0
: OVL_CON_CLRFMT_RGB888)
#define OVL_CON_CLRFMT_RGB888(ovl) ((ovl)->data->fmt_rgb565_is_0 ?
OVL_CON_CLRFMT_RGB888 : 0)
> #define OVL_CON_CLRFMT_RGBA8888 (2 << 12)
> #define OVL_CON_CLRFMT_ARGB8888 (3 << 12)
> #define OVL_CON_AEN BIT(8)
> @@ -137,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;
It will be nice to define a helper function for converting from the
generic 'mtk_ddp_comp' to the specific 'mtk_disp_ovl':
static inline struct mtk_disp_ovl *comp_to_ovl(struct mtk_ddp_comp *comp) {
return container_of(comp, struct mtk_disp_ovl, ddp_comp);
}
Then these could become:
return OVL_CON_CLRFMT_RGB565(comp_to_ovl(comp));
Or maybe cleaner, do the conversion once at the top of the function:
struct mtk_disp_ovl *ovl = comp_to_ovl(comp);
And then just:
return OVL_CON_CLRFMT_RGB565(ovl);
> 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_RGBX8888:
> case DRM_FORMAT_RGBA8888:
> return OVL_CON_CLRFMT_ARGB8888;
[snip]
> diff --git a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> index 1c366f8..935a8ef 100644
> --- a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> +++ b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> @@ -16,6 +16,7 @@
> #include <linux/delay.h>
> #include <linux/io.h>
> #include <linux/module.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/phy/phy.h>
>
> @@ -87,6 +88,9 @@
>
> #define MIPITX_DSI_PLL_CON2 0x58
>
> +#define MIPITX_DSI_PLL_TOP 0x64
> +#define RG_DSI_MPPLL_PRESERVE (0xff << 8)
> +
> #define MIPITX_DSI_PLL_PWR 0x68
> #define RG_DSI_MPPLL_SDM_PWR_ON BIT(0)
> #define RG_DSI_MPPLL_SDM_ISO_EN BIT(1)
> @@ -123,10 +127,15 @@
> #define SW_LNT2_HSTX_PRE_OE BIT(24)
> #define SW_LNT2_HSTX_OE BIT(25)
>
> +struct mtk_mipitx_data {
> + const u32 data;
Use a better name, like "mppll_preserve".
Ok, that's it for now.
Actually, the patch set in general looks pretty good.
-Dan
Powered by blists - more mailing lists