From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Hi Inki,
In this series I've removed some level of indirection from the exynos_drm_code. There two moves in these patches, first we remove all exynos_drm_crtc_plane_*() wrappers and call the manager specific functions directly. The other change is the removal of struct exynos_drm_overlay(). In my understanding the overlay struct was just storing plane data in a 1:1 relationship so it made sense to merge its fields in struct exynos_drm_plane and remove another abstraction from the driver.
Next steps:
During our initial investigation on the Exynos DRM driver we've spoted a few abstractions that could be removed to get a more clean and less abstract code. - struct exynos_drm_manager: this is just a helper of struct exynos_drm_crtc, I suggest we could merge them both. - struct *_win_data: Most of the share common fields and could be merged int struct exynos_drm_plane. - some more function wrapper can be removed as well.
After these changes intead of looking to manager and win_data we will look into crtc and planes. The new names give us more clue about what a piece of code is doing since they are already defined and used by the whole DRM ecossytem.
What your thoughts on this? I've seen that you pushed some patches to remove static usage of managers so I would like to check with you which direction are you planning to go with this. I've done some code[0] around this but now it needs a rebase against you exynos-drm-next.
[0] https://git.kernel.org/cgit/linux/kernel/git/padovan/drm-exynos.git/log/?h=c...
Gustavo Padovan (4): drm/exynos: move to_exynos_crtc() macro to main header drm/exynos: expose struct exynos_drm_crtc drm/exynos: remove exynos_drm_crtc_plane_* wrappers drm/exynos: remove struct exynos_drm_overlay
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 66 --------------------- drivers/gpu/drm/exynos/exynos_drm_crtc.h | 2 +- drivers/gpu/drm/exynos/exynos_drm_drv.h | 43 +++++++++++++- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 44 +++++++------- drivers/gpu/drm/exynos/exynos_drm_plane.c | 96 +++++++++++++++---------------- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 38 ++++++------ drivers/gpu/drm/exynos/exynos_mixer.c | 50 ++++++++-------- 7 files changed, 156 insertions(+), 183 deletions(-)
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
With this change we allow other pieces of the code to use this macro.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 --- drivers/gpu/drm/exynos/exynos_drm_drv.h | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 45026e6..c8a3169 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -20,9 +20,6 @@ #include "exynos_drm_encoder.h" #include "exynos_drm_plane.h"
-#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ - drm_crtc) - enum exynos_crtc_mode { CRTC_MODE_NORMAL, /* normal mode */ CRTC_MODE_BLANK, /* The private plane of crtc is blank */ diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 2e4e91b..9e14ae6f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -23,6 +23,9 @@ #define MAX_FB_BUFFER 4 #define DEFAULT_ZPOS -1
+#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ + drm_crtc) + /* This enumerates device type. */ enum exynos_drm_device_type { EXYNOS_DEVICE_TYPE_NONE,
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Let other pieces of the driver access struct exynos_drm_crtc as well.
struct exynos_drm_manager will be merged into struct exynos_drm_crtc, in the sense we will move all its members to exynos_drm_crtc, so to start this conversion exynos_drm_crtc need to be exposed as well.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 30 ------------------------------ drivers/gpu/drm/exynos/exynos_drm_drv.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index c8a3169..e74b6fe 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -20,36 +20,6 @@ #include "exynos_drm_encoder.h" #include "exynos_drm_plane.h"
-enum exynos_crtc_mode { - CRTC_MODE_NORMAL, /* normal mode */ - CRTC_MODE_BLANK, /* The private plane of crtc is blank */ -}; - -/* - * Exynos specific crtc structure. - * - * @drm_crtc: crtc object. - * @manager: the manager associated with this crtc - * @pipe: a crtc index created at load() with a new crtc object creation - * and the crtc object would be set to private->crtc array - * to get a crtc object corresponding to this pipe from private->crtc - * array when irq interrupt occurred. the reason of using this pipe is that - * drm framework doesn't support multiple irq yet. - * we can refer to the crtc to current hardware interrupt occurred through - * this pipe value. - * @dpms: store the crtc dpms value - * @mode: store the crtc mode value - */ -struct exynos_drm_crtc { - struct drm_crtc drm_crtc; - struct exynos_drm_manager *manager; - unsigned int pipe; - unsigned int dpms; - enum exynos_crtc_mode mode; - wait_queue_head_t pending_flip_queue; - atomic_t pending_flip; -}; - static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode) { struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 9e14ae6f..ffef077 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -215,6 +215,36 @@ struct exynos_drm_manager { struct exynos_drm_manager_ops *ops; };
+enum exynos_crtc_mode { + CRTC_MODE_NORMAL, /* normal mode */ + CRTC_MODE_BLANK, /* The private plane of crtc is blank */ +}; + +/* + * Exynos specific crtc structure. + * + * @drm_crtc: crtc object. + * @manager: the manager associated with this crtc + * @pipe: a crtc index created at load() with a new crtc object creation + * and the crtc object would be set to private->crtc array + * to get a crtc object corresponding to this pipe from private->crtc + * array when irq interrupt occurred. the reason of using this pipe is that + * drm framework doesn't support multiple irq yet. + * we can refer to the crtc to current hardware interrupt occurred through + * this pipe value. + * @dpms: store the crtc dpms value + * @mode: store the crtc mode value + */ +struct exynos_drm_crtc { + struct drm_crtc drm_crtc; + struct exynos_drm_manager *manager; + unsigned int pipe; + unsigned int dpms; + enum exynos_crtc_mode mode; + wait_queue_head_t pending_flip_queue; + atomic_t pending_flip; +}; + struct exynos_drm_g2d_private { struct device *dev; struct list_head inuse_cmdlist;
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
This functions were doing nothing but calling a manager op function, so remove them and call the manager directly.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 33 ------------------------------- drivers/gpu/drm/exynos/exynos_drm_plane.c | 19 ++++++++++++++---- 2 files changed, 15 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index e74b6fe..13c7ba5 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -410,39 +410,6 @@ void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int pipe) spin_unlock_irqrestore(&dev->event_lock, flags); }
-void exynos_drm_crtc_plane_mode_set(struct drm_crtc *crtc, - struct exynos_drm_overlay *overlay) -{ - struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager; - - if (manager->ops->win_mode_set) - manager->ops->win_mode_set(manager, overlay); -} - -void exynos_drm_crtc_plane_commit(struct drm_crtc *crtc, int zpos) -{ - struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager; - - if (manager->ops->win_commit) - manager->ops->win_commit(manager, zpos); -} - -void exynos_drm_crtc_plane_enable(struct drm_crtc *crtc, int zpos) -{ - struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager; - - if (manager->ops->win_enable) - manager->ops->win_enable(manager, zpos); -} - -void exynos_drm_crtc_plane_disable(struct drm_crtc *crtc, int zpos) -{ - struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager; - - if (manager->ops->win_disable) - manager->ops->win_disable(manager, zpos); -} - void exynos_drm_crtc_complete_scanout(struct drm_framebuffer *fb) { struct exynos_drm_manager *manager; diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index c7045a66..7d76861 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -76,6 +76,7 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, uint32_t src_w, uint32_t src_h) { struct exynos_plane *exynos_plane = to_exynos_plane(plane); + struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager; struct exynos_drm_overlay *overlay = &exynos_plane->overlay; unsigned int actual_w; unsigned int actual_h; @@ -141,7 +142,8 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
plane->crtc = crtc;
- exynos_drm_crtc_plane_mode_set(crtc, overlay); + if (manager->ops->win_mode_set) + manager->ops->win_mode_set(manager, overlay);
return 0; } @@ -150,26 +152,35 @@ void exynos_plane_commit(struct drm_plane *plane) { struct exynos_plane *exynos_plane = to_exynos_plane(plane); struct exynos_drm_overlay *overlay = &exynos_plane->overlay; + struct exynos_drm_manager *manager = to_exynos_crtc(plane->crtc)->manager;
- exynos_drm_crtc_plane_commit(plane->crtc, overlay->zpos); + if (manager->ops->win_commit) + manager->ops->win_commit(manager, overlay->zpos); }
void exynos_plane_dpms(struct drm_plane *plane, int mode) { struct exynos_plane *exynos_plane = to_exynos_plane(plane); struct exynos_drm_overlay *overlay = &exynos_plane->overlay; + struct exynos_drm_manager *manager;
if (mode == DRM_MODE_DPMS_ON) { if (exynos_plane->enabled) return;
- exynos_drm_crtc_plane_enable(plane->crtc, overlay->zpos); + manager = to_exynos_crtc(plane->crtc)->manager; + if (manager->ops->win_enable) + manager->ops->win_enable(manager, overlay->zpos); + exynos_plane->enabled = true; } else { if (!exynos_plane->enabled) return;
- exynos_drm_crtc_plane_disable(plane->crtc, overlay->zpos); + manager = to_exynos_crtc(plane->crtc)->manager; + if (manager->ops->win_disable) + manager->ops->win_disable(manager, overlay->zpos); + exynos_plane->enabled = false; } }
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
struct exynos_drm_overlay has no practical advantage nor serves as important piece of the exynos API design. The only place it was used was inside the struct exynos_plane which was just causing a extra access overhead. Users had to access the overlay first and just then get the plane information it contains.
This patch merges struct exynos_drm_overlay into struct exynos_plane. It also renames struct exynos_plane to struct exynos_drm_plane.
The rational is to cut one step to access plane information.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/exynos/exynos_drm_crtc.h | 2 +- drivers/gpu/drm/exynos/exynos_drm_drv.h | 10 +++- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 44 ++++++++-------- drivers/gpu/drm/exynos/exynos_drm_plane.c | 85 ++++++++++++++----------------- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 38 +++++++------- drivers/gpu/drm/exynos/exynos_mixer.c | 50 +++++++++--------- 6 files changed, 112 insertions(+), 117 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.h b/drivers/gpu/drm/exynos/exynos_drm_crtc.h index e353d35..dbd4227 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.h +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.h @@ -24,7 +24,7 @@ void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int pipe); void exynos_drm_crtc_complete_scanout(struct drm_framebuffer *fb);
void exynos_drm_crtc_plane_mode_set(struct drm_crtc *crtc, - struct exynos_drm_overlay *overlay); + struct exynos_drm_plane *plane); void exynos_drm_crtc_plane_commit(struct drm_crtc *crtc, int zpos); void exynos_drm_crtc_plane_enable(struct drm_crtc *crtc, int zpos); void exynos_drm_crtc_plane_disable(struct drm_crtc *crtc, int zpos); diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index ffef077..ae0dce7 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -25,6 +25,7 @@
#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ drm_crtc) +#define to_exynos_plane(x) container_of(x, struct exynos_drm_plane, base)
/* This enumerates device type. */ enum exynos_drm_device_type { @@ -47,6 +48,7 @@ enum exynos_drm_output_type { /* * Exynos drm common overlay structure. * + * @base: plane object * @fb_x: offset x on a framebuffer to be displayed. * - the unit is screen coordinates. * @fb_y: offset y on a framebuffer to be displayed. @@ -76,11 +78,14 @@ enum exynos_drm_output_type { * @local_path: in case of lcd type, local path mode on or off. * @transparency: transparency on or off. * @activated: activated or not. + * @enabled: enabled or not. * * this structure is common to exynos SoC and its contents would be copied * to hardware specific overlay info. */ -struct exynos_drm_overlay { + +struct exynos_drm_plane { + struct drm_plane base; unsigned int fb_x; unsigned int fb_y; unsigned int fb_width; @@ -107,6 +112,7 @@ struct exynos_drm_overlay { bool local_path:1; bool transparency:1; bool activated:1; + bool enabled:1; };
/* @@ -188,7 +194,7 @@ struct exynos_drm_manager_ops { void (*disable_vblank)(struct exynos_drm_manager *mgr); void (*wait_for_vblank)(struct exynos_drm_manager *mgr); void (*win_mode_set)(struct exynos_drm_manager *mgr, - struct exynos_drm_overlay *overlay); + struct exynos_drm_plane *plane); void (*win_commit)(struct exynos_drm_manager *mgr, int zpos); void (*win_enable)(struct exynos_drm_manager *mgr, int zpos); void (*win_disable)(struct exynos_drm_manager *mgr, int zpos); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index e5810d1..abd2ca9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -518,44 +518,44 @@ static void fimd_disable_vblank(struct exynos_drm_manager *mgr) }
static void fimd_win_mode_set(struct exynos_drm_manager *mgr, - struct exynos_drm_overlay *overlay) + struct exynos_drm_plane *plane) { struct fimd_context *ctx = mgr_to_fimd(mgr); struct fimd_win_data *win_data; int win; unsigned long offset;
- if (!overlay) { - DRM_ERROR("overlay is NULL\n"); + if (!plane) { + DRM_ERROR("plane is NULL\n"); return; }
- win = overlay->zpos; + win = plane->zpos; if (win == DEFAULT_ZPOS) win = ctx->default_win;
if (win < 0 || win >= WINDOWS_NR) return;
- offset = overlay->fb_x * (overlay->bpp >> 3); - offset += overlay->fb_y * overlay->pitch; + offset = plane->fb_x * (plane->bpp >> 3); + offset += plane->fb_y * plane->pitch;
- DRM_DEBUG_KMS("offset = 0x%lx, pitch = %x\n", offset, overlay->pitch); + DRM_DEBUG_KMS("offset = 0x%lx, pitch = %x\n", offset, plane->pitch);
win_data = &ctx->win_data[win];
- win_data->offset_x = overlay->crtc_x; - win_data->offset_y = overlay->crtc_y; - win_data->ovl_width = overlay->crtc_width; - win_data->ovl_height = overlay->crtc_height; - win_data->fb_width = overlay->fb_width; - win_data->fb_height = overlay->fb_height; - win_data->dma_addr = overlay->dma_addr[0] + offset; - win_data->bpp = overlay->bpp; - win_data->pixel_format = overlay->pixel_format; - win_data->buf_offsize = (overlay->fb_width - overlay->crtc_width) * - (overlay->bpp >> 3); - win_data->line_size = overlay->crtc_width * (overlay->bpp >> 3); + win_data->offset_x = plane->crtc_x; + win_data->offset_y = plane->crtc_y; + win_data->ovl_width = plane->crtc_width; + win_data->ovl_height = plane->crtc_height; + win_data->fb_width = plane->fb_width; + win_data->fb_height = plane->fb_height; + win_data->dma_addr = plane->dma_addr[0] + offset; + win_data->bpp = plane->bpp; + win_data->pixel_format = plane->pixel_format; + win_data->buf_offsize = (plane->fb_width - plane->crtc_width) * + (plane->bpp >> 3); + win_data->line_size = plane->crtc_width * (plane->bpp >> 3);
DRM_DEBUG_KMS("offset_x = %d, offset_y = %d\n", win_data->offset_x, win_data->offset_y); @@ -563,7 +563,7 @@ static void fimd_win_mode_set(struct exynos_drm_manager *mgr, win_data->ovl_width, win_data->ovl_height); DRM_DEBUG_KMS("paddr = 0x%lx\n", (unsigned long)win_data->dma_addr); DRM_DEBUG_KMS("fb_width = %d, crtc_width = %d\n", - overlay->fb_width, overlay->crtc_width); + plane->fb_width, plane->crtc_width); }
static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win) @@ -623,8 +623,8 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win) /* * In case of exynos, setting dma-burst to 16Word causes permanent * tearing for very small buffers, e.g. cursor buffer. Burst Mode - * switching which is based on overlay size is not recommended as - * overlay size varies alot towards the end of the screen and rapid + * switching which is based on plane size is not recommended as + * plane size varies alot towards the end of the screen and rapid * movement causes unstable DMA which results into iommu crash/tear. */
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 7d76861..843f741 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -18,14 +18,6 @@ #include "exynos_drm_gem.h" #include "exynos_drm_plane.h"
-#define to_exynos_plane(x) container_of(x, struct exynos_plane, base) - -struct exynos_plane { - struct drm_plane base; - struct exynos_drm_overlay overlay; - bool enabled; -}; - static const uint32_t formats[] = { DRM_FORMAT_XRGB8888, DRM_FORMAT_ARGB8888, @@ -75,9 +67,8 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h) { - struct exynos_plane *exynos_plane = to_exynos_plane(plane); + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager; - struct exynos_drm_overlay *overlay = &exynos_plane->overlay; unsigned int actual_w; unsigned int actual_h; int nr; @@ -92,10 +83,10 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, return -EFAULT; }
- overlay->dma_addr[i] = buffer->dma_addr; + exynos_plane->dma_addr[i] = buffer->dma_addr;
DRM_DEBUG_KMS("buffer: %d, dma_addr = 0x%lx\n", - i, (unsigned long)overlay->dma_addr[i]); + i, (unsigned long)exynos_plane->dma_addr[i]); }
actual_w = exynos_plane_get_size(crtc_x, crtc_w, crtc->mode.hdisplay); @@ -114,54 +105,52 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, }
/* set drm framebuffer data. */ - overlay->fb_x = src_x; - overlay->fb_y = src_y; - overlay->fb_width = fb->width; - overlay->fb_height = fb->height; - overlay->src_width = src_w; - overlay->src_height = src_h; - overlay->bpp = fb->bits_per_pixel; - overlay->pitch = fb->pitches[0]; - overlay->pixel_format = fb->pixel_format; - - /* set overlay range to be displayed. */ - overlay->crtc_x = crtc_x; - overlay->crtc_y = crtc_y; - overlay->crtc_width = actual_w; - overlay->crtc_height = actual_h; + exynos_plane->fb_x = src_x; + exynos_plane->fb_y = src_y; + exynos_plane->fb_width = fb->width; + exynos_plane->fb_height = fb->height; + exynos_plane->src_width = src_w; + exynos_plane->src_height = src_h; + exynos_plane->bpp = fb->bits_per_pixel; + exynos_plane->pitch = fb->pitches[0]; + exynos_plane->pixel_format = fb->pixel_format; + + /* set plane range to be displayed. */ + exynos_plane->crtc_x = crtc_x; + exynos_plane->crtc_y = crtc_y; + exynos_plane->crtc_width = actual_w; + exynos_plane->crtc_height = actual_h;
/* set drm mode data. */ - overlay->mode_width = crtc->mode.hdisplay; - overlay->mode_height = crtc->mode.vdisplay; - overlay->refresh = crtc->mode.vrefresh; - overlay->scan_flag = crtc->mode.flags; + exynos_plane->mode_width = crtc->mode.hdisplay; + exynos_plane->mode_height = crtc->mode.vdisplay; + exynos_plane->refresh = crtc->mode.vrefresh; + exynos_plane->scan_flag = crtc->mode.flags;
- DRM_DEBUG_KMS("overlay : offset_x/y(%d,%d), width/height(%d,%d)", - overlay->crtc_x, overlay->crtc_y, - overlay->crtc_width, overlay->crtc_height); + DRM_DEBUG_KMS("plane : offset_x/y(%d,%d), width/height(%d,%d)", + exynos_plane->crtc_x, exynos_plane->crtc_y, + exynos_plane->crtc_width, exynos_plane->crtc_height);
plane->crtc = crtc;
if (manager->ops->win_mode_set) - manager->ops->win_mode_set(manager, overlay); + manager->ops->win_mode_set(manager, exynos_plane);
return 0; }
void exynos_plane_commit(struct drm_plane *plane) { - struct exynos_plane *exynos_plane = to_exynos_plane(plane); - struct exynos_drm_overlay *overlay = &exynos_plane->overlay; + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); struct exynos_drm_manager *manager = to_exynos_crtc(plane->crtc)->manager;
if (manager->ops->win_commit) - manager->ops->win_commit(manager, overlay->zpos); + manager->ops->win_commit(manager, exynos_plane->zpos); }
void exynos_plane_dpms(struct drm_plane *plane, int mode) { - struct exynos_plane *exynos_plane = to_exynos_plane(plane); - struct exynos_drm_overlay *overlay = &exynos_plane->overlay; + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); struct exynos_drm_manager *manager;
if (mode == DRM_MODE_DPMS_ON) { @@ -170,7 +159,7 @@ void exynos_plane_dpms(struct drm_plane *plane, int mode)
manager = to_exynos_crtc(plane->crtc)->manager; if (manager->ops->win_enable) - manager->ops->win_enable(manager, overlay->zpos); + manager->ops->win_enable(manager, exynos_plane->zpos);
exynos_plane->enabled = true; } else { @@ -179,7 +168,7 @@ void exynos_plane_dpms(struct drm_plane *plane, int mode)
manager = to_exynos_crtc(plane->crtc)->manager; if (manager->ops->win_disable) - manager->ops->win_disable(manager, overlay->zpos); + manager->ops->win_disable(manager, exynos_plane->zpos);
exynos_plane->enabled = false; } @@ -215,7 +204,7 @@ static int exynos_disable_plane(struct drm_plane *plane)
static void exynos_plane_destroy(struct drm_plane *plane) { - struct exynos_plane *exynos_plane = to_exynos_plane(plane); + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
exynos_disable_plane(plane); drm_plane_cleanup(plane); @@ -227,11 +216,11 @@ static int exynos_plane_set_property(struct drm_plane *plane, uint64_t val) { struct drm_device *dev = plane->dev; - struct exynos_plane *exynos_plane = to_exynos_plane(plane); + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); struct exynos_drm_private *dev_priv = dev->dev_private;
if (property == dev_priv->plane_zpos_property) { - exynos_plane->overlay.zpos = val; + exynos_plane->zpos = val; return 0; }
@@ -268,10 +257,10 @@ struct drm_plane *exynos_plane_init(struct drm_device *dev, unsigned long possible_crtcs, enum drm_plane_type type) { - struct exynos_plane *exynos_plane; + struct exynos_drm_plane *exynos_plane; int err;
- exynos_plane = kzalloc(sizeof(struct exynos_plane), GFP_KERNEL); + exynos_plane = kzalloc(sizeof(struct exynos_drm_plane), GFP_KERNEL); if (!exynos_plane) return ERR_PTR(-ENOMEM);
@@ -285,7 +274,7 @@ struct drm_plane *exynos_plane_init(struct drm_device *dev, }
if (type == DRM_PLANE_TYPE_PRIMARY) - exynos_plane->overlay.zpos = DEFAULT_ZPOS; + exynos_plane->zpos = DEFAULT_ZPOS; else exynos_plane_attach_zpos_property(&exynos_plane->base);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c index 3b6fdd6..c137545 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c @@ -161,43 +161,43 @@ static void vidi_disable_vblank(struct exynos_drm_manager *mgr) }
static void vidi_win_mode_set(struct exynos_drm_manager *mgr, - struct exynos_drm_overlay *overlay) + struct exynos_drm_plane *plane) { struct vidi_context *ctx = manager_to_vidi(mgr); struct vidi_win_data *win_data; int win; unsigned long offset;
- if (!overlay) { - DRM_ERROR("overlay is NULL\n"); + if (!plane) { + DRM_ERROR("plane is NULL\n"); return; }
- win = overlay->zpos; + win = plane->zpos; if (win == DEFAULT_ZPOS) win = ctx->default_win;
if (win < 0 || win >= WINDOWS_NR) return;
- offset = overlay->fb_x * (overlay->bpp >> 3); - offset += overlay->fb_y * overlay->pitch; + offset = plane->fb_x * (plane->bpp >> 3); + offset += plane->fb_y * plane->pitch;
- DRM_DEBUG_KMS("offset = 0x%lx, pitch = %x\n", offset, overlay->pitch); + DRM_DEBUG_KMS("offset = 0x%lx, pitch = %x\n", offset, plane->pitch);
win_data = &ctx->win_data[win];
- win_data->offset_x = overlay->crtc_x; - win_data->offset_y = overlay->crtc_y; - win_data->ovl_width = overlay->crtc_width; - win_data->ovl_height = overlay->crtc_height; - win_data->fb_width = overlay->fb_width; - win_data->fb_height = overlay->fb_height; - win_data->dma_addr = overlay->dma_addr[0] + offset; - win_data->bpp = overlay->bpp; - win_data->buf_offsize = (overlay->fb_width - overlay->crtc_width) * - (overlay->bpp >> 3); - win_data->line_size = overlay->crtc_width * (overlay->bpp >> 3); + win_data->offset_x = plane->crtc_x; + win_data->offset_y = plane->crtc_y; + win_data->ovl_width = plane->crtc_width; + win_data->ovl_height = plane->crtc_height; + win_data->fb_width = plane->fb_width; + win_data->fb_height = plane->fb_height; + win_data->dma_addr = plane->dma_addr[0] + offset; + win_data->bpp = plane->bpp; + win_data->buf_offsize = (plane->fb_width - plane->crtc_width) * + (plane->bpp >> 3); + win_data->line_size = plane->crtc_width * (plane->bpp >> 3);
/* * some parts of win_data should be transferred to user side @@ -210,7 +210,7 @@ static void vidi_win_mode_set(struct exynos_drm_manager *mgr, win_data->ovl_width, win_data->ovl_height); DRM_DEBUG_KMS("paddr = 0x%lx\n", (unsigned long)win_data->dma_addr); DRM_DEBUG_KMS("fb_width = %d, crtc_width = %d\n", - overlay->fb_width, overlay->crtc_width); + plane->fb_width, plane->crtc_width); }
static void vidi_win_commit(struct exynos_drm_manager *mgr, int zpos) diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 820b762..b64674a 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -922,24 +922,24 @@ static void mixer_disable_vblank(struct exynos_drm_manager *mgr) }
static void mixer_win_mode_set(struct exynos_drm_manager *mgr, - struct exynos_drm_overlay *overlay) + struct exynos_drm_plane *plane) { struct mixer_context *mixer_ctx = mgr_to_mixer(mgr); struct hdmi_win_data *win_data; int win;
- if (!overlay) { - DRM_ERROR("overlay is NULL\n"); + if (!plane) { + DRM_ERROR("plane is NULL\n"); return; }
DRM_DEBUG_KMS("set [%d]x[%d] at (%d,%d) to [%d]x[%d] at (%d,%d)\n", - overlay->fb_width, overlay->fb_height, - overlay->fb_x, overlay->fb_y, - overlay->crtc_width, overlay->crtc_height, - overlay->crtc_x, overlay->crtc_y); + plane->fb_width, plane->fb_height, + plane->fb_x, plane->fb_y, + plane->crtc_width, plane->crtc_height, + plane->crtc_x, plane->crtc_y);
- win = overlay->zpos; + win = plane->zpos; if (win == DEFAULT_ZPOS) win = MIXER_DEFAULT_WIN;
@@ -950,27 +950,27 @@ static void mixer_win_mode_set(struct exynos_drm_manager *mgr,
win_data = &mixer_ctx->win_data[win];
- win_data->dma_addr = overlay->dma_addr[0]; - win_data->chroma_dma_addr = overlay->dma_addr[1]; - win_data->pixel_format = overlay->pixel_format; - win_data->bpp = overlay->bpp; + win_data->dma_addr = plane->dma_addr[0]; + win_data->chroma_dma_addr = plane->dma_addr[1]; + win_data->pixel_format = plane->pixel_format; + win_data->bpp = plane->bpp;
- win_data->crtc_x = overlay->crtc_x; - win_data->crtc_y = overlay->crtc_y; - win_data->crtc_width = overlay->crtc_width; - win_data->crtc_height = overlay->crtc_height; + win_data->crtc_x = plane->crtc_x; + win_data->crtc_y = plane->crtc_y; + win_data->crtc_width = plane->crtc_width; + win_data->crtc_height = plane->crtc_height;
- win_data->fb_x = overlay->fb_x; - win_data->fb_y = overlay->fb_y; - win_data->fb_width = overlay->fb_width; - win_data->fb_height = overlay->fb_height; - win_data->src_width = overlay->src_width; - win_data->src_height = overlay->src_height; + win_data->fb_x = plane->fb_x; + win_data->fb_y = plane->fb_y; + win_data->fb_width = plane->fb_width; + win_data->fb_height = plane->fb_height; + win_data->src_width = plane->src_width; + win_data->src_height = plane->src_height;
- win_data->mode_width = overlay->mode_width; - win_data->mode_height = overlay->mode_height; + win_data->mode_width = plane->mode_width; + win_data->mode_height = plane->mode_height;
- win_data->scan_flags = overlay->scan_flag; + win_data->scan_flags = plane->scan_flag; }
static void mixer_win_commit(struct exynos_drm_manager *mgr, int zpos)
Hi Inki,
Can you please review this? I also have sent other two patch sets that sits on top of this one. Thanks.
Gustavo
2014-11-24 Gustavo Padovan gustavo@padovan.org:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Hi Inki,
In this series I've removed some level of indirection from the exynos_drm_code. There two moves in these patches, first we remove all exynos_drm_crtc_plane_*() wrappers and call the manager specific functions directly. The other change is the removal of struct exynos_drm_overlay(). In my understanding the overlay struct was just storing plane data in a 1:1 relationship so it made sense to merge its fields in struct exynos_drm_plane and remove another abstraction from the driver.
Next steps:
During our initial investigation on the Exynos DRM driver we've spoted a few abstractions that could be removed to get a more clean and less abstract code. - struct exynos_drm_manager: this is just a helper of struct exynos_drm_crtc, I suggest we could merge them both. - struct *_win_data: Most of the share common fields and could be merged int struct exynos_drm_plane. - some more function wrapper can be removed as well.
After these changes intead of looking to manager and win_data we will look into crtc and planes. The new names give us more clue about what a piece of code is doing since they are already defined and used by the whole DRM ecossytem.
What your thoughts on this? I've seen that you pushed some patches to remove static usage of managers so I would like to check with you which direction are you planning to go with this. I've done some code[0] around this but now it needs a rebase against you exynos-drm-next.
[0] https://git.kernel.org/cgit/linux/kernel/git/padovan/drm-exynos.git/log/?h=c...
Gustavo Padovan (4): drm/exynos: move to_exynos_crtc() macro to main header drm/exynos: expose struct exynos_drm_crtc drm/exynos: remove exynos_drm_crtc_plane_* wrappers drm/exynos: remove struct exynos_drm_overlay
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 66 --------------------- drivers/gpu/drm/exynos/exynos_drm_crtc.h | 2 +- drivers/gpu/drm/exynos/exynos_drm_drv.h | 43 +++++++++++++- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 44 +++++++------- drivers/gpu/drm/exynos/exynos_drm_plane.c | 96 +++++++++++++++---------------- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 38 ++++++------ drivers/gpu/drm/exynos/exynos_mixer.c | 50 ++++++++-------- 7 files changed, 156 insertions(+), 183 deletions(-)
-- 1.9.3
On 2 December 2014 at 22:38, Gustavo Padovan gustavo@padovan.org wrote:
Hi Inki,
Can you please review this? I also have sent other two patch sets that sits on top of this one. Thanks.
Inki, any plans on when you can get to looking at this?
I think cleaning up exynos so we can get atomic using it is something that will benefit it heavily.
Dave.
Hi Inki,
2014-12-06 Dave Airlie airlied@gmail.com:
On 2 December 2014 at 22:38, Gustavo Padovan gustavo@padovan.org wrote:
Hi Inki,
Can you please review this? I also have sent other two patch sets that sits on top of this one. Thanks.
Inki, any plans on when you can get to looking at this?
I think cleaning up exynos so we can get atomic using it is something that will benefit it heavily.
Besides these patches I have other two clean up patchsets[0][1] that are very important for atomic support, can you review these two too?
[0] http://www.spinics.net/lists/linux-samsung-soc/msg39614.html [1] http://www.spinics.net/lists/linux-samsung-soc/msg39632.html
Gustavo
Hi,
On 8 December 2014 at 17:03, Gustavo Padovan gustavo@padovan.org wrote:
2014-12-06 Dave Airlie airlied@gmail.com:
On 2 December 2014 at 22:38, Gustavo Padovan gustavo@padovan.org
wrote:
Can you please review this? I also have sent other two patch sets that
sits on
top of this one. Thanks.
Inki, any plans on when you can get to looking at this?
I think cleaning up exynos so we can get atomic using it is something that will benefit it heavily.
Besides these patches I have other two clean up patchsets[0][1] that are very important for atomic support, can you review these two too?
It'd be fantastic to get the START == START_S fix merged too: http://www.spinics.net/lists/linux-samsung-soc/msg40128.html
Cheers, Daniel
dri-devel@lists.freedesktop.org
-
Daniel Stone -
Dave Airlie -
Gustavo Padovan