Re: [PATCH V2] drm/exynos: Add DECON driver
Hi Pankaj,
On Mon, Dec 1, 2014 at 1:36 PM, Pankaj Dubey pankaj.du...@samsung.com wrote:
Hi Ajay,
On 28 November 2014 at 16:45, Ajay Kumar ajaykumar...@samsung.com wrote:
This series is based on exynos-drm-next branch of Inki Dae's tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
DECON(Display and Enhancement Controller) is the new IP
in exynos7 SOC for generating video signals using pixel data.
DECON driver can be used to drive 2 different interfaces on Exynos7:
DECON-INT(video controller) and DECON-EXT(Mixer for HDMI)
The existing FIMD driver code was used as a template to create
DECON driver. Only DECON-INT is supported as of now, and
DECON-EXT support will be added later.
Signed-off-by: Akshu Agrawal aks...@gmail.com
Signed-off-by: Ajay Kumar ajaykumar...@samsung.com
---
Changes since V1:
-- Address comments from Pankaj and do few cleanups.
Thanks, but still I can see this needs modification, please see my comments:
.../devicetree/bindings/video/exynos7-decon.txt| 67 ++
drivers/gpu/drm/exynos/Kconfig | 13 +-
drivers/gpu/drm/exynos/Makefile|1 +
drivers/gpu/drm/exynos/exynos7_drm_decon.c | 1037
drivers/gpu/drm/exynos/exynos_drm_drv.c|4 +
drivers/gpu/drm/exynos/exynos_drm_drv.h|1 +
include/video/exynos7_decon.h | 346 +++
7 files changed, 1466 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/exynos7-decon.txt
create mode 100644 drivers/gpu/drm/exynos/exynos7_drm_decon.c
create mode 100644 include/video/exynos7_decon.h
[snip]
+static int decon_mgr_initialize(struct exynos_drm_manager *mgr,
+ struct drm_device *drm_dev)
+{
+ struct decon_context *ctx = mgr_to_decon(mgr);
+ struct exynos_drm_private *priv = drm_dev-dev_private;
+ int ret;
+
+ mgr-drm_dev = drm_dev;
+ mgr-pipe = ctx-pipe = priv-pipe++;
+
Do we really need 'pipe' in exynos_drm_manager and decon_context both?
Will fix this.
+ /* attach this sub driver to iommu mapping if supported. */
+ if (is_drm_iommu_supported(mgr-drm_dev)) {
[snip]
+static void decon_win_mode_set(struct exynos_drm_manager *mgr,
+ struct exynos_drm_overlay *overlay)
+{
+ struct decon_context *ctx = mgr_to_decon(mgr);
+ struct decon_win_data *win_data;
+ int win, padding;
+
+ if (!overlay) {
+ DRM_ERROR(overlay is NULL\n);
+ return;
+ }
+
+ win = overlay-zpos;
+ if (win == DEFAULT_ZPOS)
+ win = ctx-default_win;
+
+ if (win 0 || win = WINDOWS_NR)
+ return;
+
+
+ win_data = ctx-win_data[win];
+
As I mentioned in V1, since these 5 lines are getting repeating better
we move it in one static function. It will help in reducing code
footprint.
I tried this, the code readability is gone. I think its better to
leave this as it is.
[snip]
+
+/**
+ * shadow_protect_win() - disable updating values from shadow registers at
vsync
+ *
+ * @win: window to protect registers for
+ * @protect: 1 to protect (disable updates)
+ */
+static void decon_shadow_protect_win(struct decon_context *ctx,
+ int win, bool
protect)
+{
+ u32 reg, bits, val;
+
+ reg = SHADOWCON;
How about using SHADOWCON directly instead of using local variable?
Ok.
+ bits = SHADOWCON_WINx_PROTECT(win);
+
+ val = readl(ctx-regs + reg);
+ if (protect)
+ val |= bits;
+ else
+ val = ~bits;
+ writel(val, ctx-regs + reg);
+}
+
+static void decon_win_commit(struct exynos_drm_manager *mgr, int zpos)
+{
+ struct decon_context *ctx = mgr_to_decon(mgr);
+ struct decon_win_data *win_data;
+ int win = zpos;
+ unsigned long val, alpha, blendeq;
+ unsigned int last_x;
+ unsigned int last_y;
+
+ if (ctx-suspended)
+ return;
+
+ if (win == DEFAULT_ZPOS)
+ win = ctx-default_win;
+
+ if (win 0 || win = WINDOWS_NR)
+ return;
+
+ win_data = ctx-win_data[win];
+
+ /* If suspended, enable this on resume */
+ if (ctx-suspended) {
+ win_data-resume = true;
+ return;
+ }
+
+ /*
[snip]
+static int decon_probe(struct platform_device *pdev)
+{
+ struct device *dev = pdev-dev;
+ struct decon_context *ctx;
+ struct resource *res;
+ int ret = -EINVAL;
+
+ if (!dev-of_node)
+ return -ENODEV;
+
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx-manager.type = EXYNOS_DISPLAY_TYPE_LCD;
+
Re: [PATCH V2] drm/exynos: Add DECON driver
Hi Ajay,
On 28 November 2014 at 16:45, Ajay Kumar ajaykumar...@samsung.com wrote:
This series is based on exynos-drm-next branch of Inki Dae's tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
DECON(Display and Enhancement Controller) is the new IP
in exynos7 SOC for generating video signals using pixel data.
DECON driver can be used to drive 2 different interfaces on Exynos7:
DECON-INT(video controller) and DECON-EXT(Mixer for HDMI)
The existing FIMD driver code was used as a template to create
DECON driver. Only DECON-INT is supported as of now, and
DECON-EXT support will be added later.
Signed-off-by: Akshu Agrawal aks...@gmail.com
Signed-off-by: Ajay Kumar ajaykumar...@samsung.com
---
Changes since V1:
-- Address comments from Pankaj and do few cleanups.
Thanks, but still I can see this needs modification, please see my comments:
.../devicetree/bindings/video/exynos7-decon.txt| 67 ++
drivers/gpu/drm/exynos/Kconfig | 13 +-
drivers/gpu/drm/exynos/Makefile|1 +
drivers/gpu/drm/exynos/exynos7_drm_decon.c | 1037
drivers/gpu/drm/exynos/exynos_drm_drv.c|4 +
drivers/gpu/drm/exynos/exynos_drm_drv.h|1 +
include/video/exynos7_decon.h | 346 +++
7 files changed, 1466 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/exynos7-decon.txt
create mode 100644 drivers/gpu/drm/exynos/exynos7_drm_decon.c
create mode 100644 include/video/exynos7_decon.h
[snip]
+static int decon_mgr_initialize(struct exynos_drm_manager *mgr,
+ struct drm_device *drm_dev)
+{
+ struct decon_context *ctx = mgr_to_decon(mgr);
+ struct exynos_drm_private *priv = drm_dev-dev_private;
+ int ret;
+
+ mgr-drm_dev = drm_dev;
+ mgr-pipe = ctx-pipe = priv-pipe++;
+
Do we really need 'pipe' in exynos_drm_manager and decon_context both?
+ /* attach this sub driver to iommu mapping if supported. */
+ if (is_drm_iommu_supported(mgr-drm_dev)) {
[snip]
+static void decon_win_mode_set(struct exynos_drm_manager *mgr,
+ struct exynos_drm_overlay *overlay)
+{
+ struct decon_context *ctx = mgr_to_decon(mgr);
+ struct decon_win_data *win_data;
+ int win, padding;
+
+ if (!overlay) {
+ DRM_ERROR(overlay is NULL\n);
+ return;
+ }
+
+ win = overlay-zpos;
+ if (win == DEFAULT_ZPOS)
+ win = ctx-default_win;
+
+ if (win 0 || win = WINDOWS_NR)
+ return;
+
+
+ win_data = ctx-win_data[win];
+
As I mentioned in V1, since these 5 lines are getting repeating better
we move it in one static function. It will help in reducing code
footprint.
[snip]
+
+/**
+ * shadow_protect_win() - disable updating values from shadow registers at
vsync
+ *
+ * @win: window to protect registers for
+ * @protect: 1 to protect (disable updates)
+ */
+static void decon_shadow_protect_win(struct decon_context *ctx,
+ int win, bool protect)
+{
+ u32 reg, bits, val;
+
+ reg = SHADOWCON;
How about using SHADOWCON directly instead of using local variable?
+ bits = SHADOWCON_WINx_PROTECT(win);
+
+ val = readl(ctx-regs + reg);
+ if (protect)
+ val |= bits;
+ else
+ val = ~bits;
+ writel(val, ctx-regs + reg);
+}
+
+static void decon_win_commit(struct exynos_drm_manager *mgr, int zpos)
+{
+ struct decon_context *ctx = mgr_to_decon(mgr);
+ struct decon_win_data *win_data;
+ int win = zpos;
+ unsigned long val, alpha, blendeq;
+ unsigned int last_x;
+ unsigned int last_y;
+
+ if (ctx-suspended)
+ return;
+
+ if (win == DEFAULT_ZPOS)
+ win = ctx-default_win;
+
+ if (win 0 || win = WINDOWS_NR)
+ return;
+
+ win_data = ctx-win_data[win];
+
+ /* If suspended, enable this on resume */
+ if (ctx-suspended) {
+ win_data-resume = true;
+ return;
+ }
+
+ /*
[snip]
+static int decon_probe(struct platform_device *pdev)
+{
+ struct device *dev = pdev-dev;
+ struct decon_context *ctx;
+ struct resource *res;
+ int ret = -EINVAL;
+
+ if (!dev-of_node)
+ return -ENODEV;
+
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx-manager.type = EXYNOS_DISPLAY_TYPE_LCD;
+ ctx-manager.ops = decon_manager_ops;
+
+ ret = exynos_drm_component_add(dev, EXYNOS_DEVICE_TYPE_CRTC,
+ ctx-manager.type);
+ if (ret)
+ return
