Hi Philipp, This driver is looking very good now to me. Sorry for the delay reviewing. Some comments inline below. On Tue, Jan 12, 2016 at 7:15 AM, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote: > From: CK Hu <ck.hu@xxxxxxxxxxxx> > > This patch adds an initial DRM driver for the Mediatek MT8173 DISP > subsystem. It currently supports two fixed output streams from the > OVL0/OVL1 sources to the DSI0/DPI0 sinks, respectively. > > Signed-off-by: CK Hu <ck.hu@xxxxxxxxxxxx> > Signed-off-by: YT Shen <yt.shen@xxxxxxxxxxxx> > Signed-off-by: Daniel Kurtz <djkurtz@xxxxxxxxxxxx> > Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> [snip...] > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c > new file mode 100644 > index 0000000..455e62e > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c > @@ -0,0 +1,301 @@ > +/* > + * Copyright (c) 2015 MediaTek Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <drm/drmP.h> > +#include <linux/clk.h> > +#include <linux/component.h> > +#include <linux/of_device.h> > +#include <linux/of_irq.h> > +#include <linux/platform_device.h> > + > +#include "mtk_drm_crtc.h" > +#include "mtk_drm_ddp_comp.h" > + > +#define DISP_REG_OVL_INTEN 0x0004 > +#define OVL_FME_CPL_INT BIT(1) > +#define DISP_REG_OVL_INTSTA 0x0008 > +#define DISP_REG_OVL_EN 0x000c > +#define DISP_REG_OVL_RST 0x0014 > +#define DISP_REG_OVL_ROI_SIZE 0x0020 > +#define DISP_REG_OVL_ROI_BGCLR 0x0028 > +#define DISP_REG_OVL_SRC_CON 0x002c > +#define DISP_REG_OVL_CON(n) (0x0030 + 0x20 * (n)) > +#define DISP_REG_OVL_SRC_SIZE(n) (0x0038 + 0x20 * (n)) > +#define DISP_REG_OVL_OFFSET(n) (0x003c + 0x20 * (n)) > +#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)) > + > +enum OVL_INPUT_FORMAT { > + OVL_INFMT_RGB565 = 0, > + OVL_INFMT_RGB888 = 1, > + OVL_INFMT_RGBA8888 = 2, > + OVL_INFMT_ARGB8888 = 3, > +}; > + > +#define OVL_RDMA_MEM_GMC 0x40402020 > +#define OVL_AEN BIT(8) > +#define OVL_ALPHA 0xff > + > +/** > + * struct mtk_disp_ovl - DISP_OVL driver structure > + * @ddp_comp - structure containing type enum and hardware resources > + * @drm_device - backlink to allow the irq handler to find the associated crtc > + */ > +struct mtk_disp_ovl { > + struct mtk_ddp_comp ddp_comp; > + struct drm_device *drm_dev; Storing the crtc here would be more consistent with the comment above. > +}; > + > +static irqreturn_t mtk_disp_ovl_irq_handler(int irq, void *dev_id) > +{ > + struct mtk_disp_ovl *priv = dev_id; > + struct mtk_ddp_comp *ovl = &priv->ddp_comp; > + > + /* Clear frame completion interrupt */ > + writel(0x0, ovl->regs + DISP_REG_OVL_INTSTA); > + > + mtk_crtc_ddp_irq(priv->drm_dev, ovl); Likewise, passing the crtc here would be a bit more consistent with the name of this function mtk_crtc_ddp_irq(). Also, see below; if the CRTC is not found, we can return IRQ_NONE here to indicate that this was a spurious (unhandled) interrupt. > + > + return IRQ_HANDLED; > +} [snip...] > +static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx, > + struct mtk_plane_state *state) > +{ > + struct mtk_plane_pending_state *pending = &state->pending; > + unsigned int addr = pending->addr; > + unsigned int pitch = pending->pitch & 0xffff; > + unsigned int fmt = pending->format; > + unsigned int offset = (pending->y << 16) | pending->x; > + unsigned int src_size = (pending->height << 16) | pending->width; > + unsigned int con; > + > + con = has_rb_swapped(fmt) << 24 | ovl_fmt_convert(fmt) << 12; Since has_rb_swapped() and ovl_fmt_convert() can theoretically fail, but we can't fail here during .layer_config, can we instead do this in an atomic state check phase, and just store the resulting .has_rb_swapped and .ovl_infmt values to mtk_plane_pending_state for later application during .layer_config? > + if (idx != 0) > + con |= OVL_AEN | OVL_ALPHA; > + > + writel(con, comp->regs + DISP_REG_OVL_CON(idx)); > + writel(pitch, comp->regs + DISP_REG_OVL_PITCH(idx)); > + writel(src_size, comp->regs + DISP_REG_OVL_SRC_SIZE(idx)); > + writel(offset, comp->regs + DISP_REG_OVL_OFFSET(idx)); > + writel(addr, comp->regs + DISP_REG_OVL_ADDR(idx)); Can the above all be writel_relaxed() ? > +} [snip...] > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc) > +{ > + struct drm_crtc *crtc = &mtk_crtc->base; > + unsigned int width, height, vrefresh; > + int ret; > + int i; > + > + DRM_DEBUG_DRIVER("%s\n", __func__); > + if (WARN_ON(!crtc->state)) > + return -EINVAL; > + > + width = crtc->state->adjusted_mode.hdisplay; > + height = crtc->state->adjusted_mode.vdisplay; > + vrefresh = crtc->state->adjusted_mode.vrefresh; > + > + ret = pm_runtime_get_sync(crtc->dev->dev); > + if (ret < 0) { > + DRM_ERROR("Failed to enable power domain: %d\n", ret); > + return ret; > + } > + > + ret = mtk_disp_mutex_prepare(mtk_crtc->mutex); > + if (ret < 0) { > + DRM_ERROR("Failed to enable mutex clock: %d\n", ret); > + goto err_pm_runtime_put; > + } > + > + ret = mtk_crtc_ddp_clk_enable(mtk_crtc); > + if (ret < 0) { > + DRM_ERROR("Failed to enable component clocks: %d\n", ret); > + goto err_mutex_unprepare; > + } > + > + DRM_DEBUG_DRIVER("mediatek_ddp_ddp_path_setup\n"); > + for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) { > + mtk_ddp_add_comp_to_path(mtk_crtc->config_regs, > + mtk_crtc->ddp_comp[i]->id, > + mtk_crtc->ddp_comp[i + 1]->id); > + mtk_disp_mutex_add_comp(mtk_crtc->mutex, > + mtk_crtc->ddp_comp[i]->id); > + } > + mtk_disp_mutex_add_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id); > + mtk_disp_mutex_enable(mtk_crtc->mutex); > + > + DRM_DEBUG_DRIVER("ddp_disp_path_power_on %dx%d\n", width, height); I think this debug message is now stale? > + for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) { > + struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i]; > + > + mtk_ddp_comp_config(comp, width, height, vrefresh); > + mtk_ddp_comp_start(comp); > + } > + > + return 0; > + > +err_mutex_unprepare: > + mtk_disp_mutex_unprepare(mtk_crtc->mutex); > +err_pm_runtime_put: > + pm_runtime_put(crtc->dev->dev); > + return ret; > +} [snip...] > +void mtk_drm_crtc_check_flush(struct drm_crtc *crtc) > +{ > + struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc); > + int i; > + > + if (mtk_crtc->do_flush) { > + if (mtk_crtc->event) > + mtk_crtc->pending_needs_vblank = true; > + for (i = 0; i < OVL_LAYER_NR; i++) { > + struct drm_plane *plane = &mtk_crtc->planes[i].base; > + struct mtk_plane_state *plane_state; > + > + plane_state = to_mtk_plane_state(plane->state); > + if (plane_state->pending.dirty) { > + plane_state->pending.config = true; > + plane_state->pending.dirty = false; > + } > + } > + mtk_crtc->pending_planes = true; Only set pending_planes is true iff at least 1 of the ovl was pending.dirty, right? > + mtk_crtc->do_flush = false; > + } > +} > + > +static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc, > + struct drm_crtc_state *old_crtc_state) > +{ > + struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc); > + > + mtk_crtc->do_flush = true; > + mtk_drm_crtc_check_flush(crtc); I can't figure out how this is supposed to work. mtk_drm_crtc_check_flush() only does something if do_flush is set. And, do_flush is only set here in mtk_drm_crtc_atomic_flush(), just above mtk_drm_crtc_check_flush(), and it is cleared by that function. So, why do we also call mtk_drm_crtc_check_flush() from mtk_plane_atomic_update()? In what condition will the call to mtk_drm_crtc_check_flush() from mtk_plane_atomic_update() have do_flush == true? > +} [snip...] > +void mtk_crtc_ddp_irq(struct drm_device *drm_dev, struct mtk_ddp_comp *ovl) > +{ > + struct mtk_drm_private *priv = drm_dev->dev_private; > + struct mtk_drm_crtc *mtk_crtc; > + struct mtk_crtc_state *state; > + unsigned int i; > + > + mtk_crtc = mtk_crtc_by_src_comp(priv, ovl); Hmm. Can we do this lookup once and store it somewhere to avoid this lookup during every IRQ? > + if (WARN_ON(!mtk_crtc)) > + return; I think the typical thing to do if we can't handle an interrupt is to return IRQ_NONE, so that it can be tracked by the kernel's spurious interrupt infrastructure. We can probably remove the WARN_ON, too. > + > + state = to_mtk_crtc_state(mtk_crtc->base.state); > + > + /* > + * TODO: instead of updating the registers here, we should prepare > + * working registers in atomic_commit and let the hardware command > + * queue update module registers on vblank. > + */ > + if (state->pending_config) { > + mtk_ddp_comp_config(ovl, state->pending_width, > + state->pending_height, > + state->pending_vrefresh); > + > + state->pending_config = false; > + } > + > + if (mtk_crtc->pending_planes) { > + for (i = 0; i < OVL_LAYER_NR; i++) { > + struct drm_plane *plane = &mtk_crtc->planes[i].base; > + struct mtk_plane_state *plane_state; > + > + plane_state = to_mtk_plane_state(plane->state); > + > + if (plane_state->pending.config) { > + if (!plane_state->pending.enable) > + mtk_ddp_comp_layer_off(ovl, i); > + > + mtk_ddp_comp_layer_config(ovl, i, plane_state); > + > + if (plane_state->pending.enable) > + mtk_ddp_comp_layer_on(ovl, i); .layer_off() and .layer_on() are redundant, and can just be handled inside .layer_config(), since we pass plane_state to .layer_config() anyway. That will also be slightly more efficient, since the mtk_ddp_comp_layer*() will then all be static functions in the same file and on/off can be inlined. > + > + plane_state->pending.config = false; > + } > + } > + mtk_crtc->pending_planes = false; > + } > + > + mtk_drm_finish_page_flip(mtk_crtc); > +} [snip...] > + > +struct mtk_ddp_comp_match { > + enum mtk_ddp_comp_type type; > + int alias_id; > + const struct mtk_ddp_comp_funcs *funcs; > +}; > + > +static struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = { static const struct ... > + [DDP_COMPONENT_AAL] = { MTK_DISP_AAL, 0, NULL }, > + [DDP_COMPONENT_COLOR0] = { MTK_DISP_COLOR, 0, &ddp_color }, > + [DDP_COMPONENT_COLOR1] = { MTK_DISP_COLOR, 1, &ddp_color }, > + [DDP_COMPONENT_DPI0] = { MTK_DPI, 0, NULL }, > + [DDP_COMPONENT_DSI0] = { MTK_DSI, 0, NULL }, > + [DDP_COMPONENT_DSI1] = { MTK_DSI, 1, NULL }, > + [DDP_COMPONENT_GAMMA] = { MTK_DISP_GAMMA, 0, NULL }, > + [DDP_COMPONENT_OD] = { MTK_DISP_OD, 0, &ddp_od }, > + [DDP_COMPONENT_OVL0] = { MTK_DISP_OVL, 0, NULL }, > + [DDP_COMPONENT_OVL1] = { MTK_DISP_OVL, 1, NULL }, > + [DDP_COMPONENT_PWM0] = { MTK_DISP_PWM, 0, NULL }, > + [DDP_COMPONENT_RDMA0] = { MTK_DISP_RDMA, 0, &ddp_rdma }, > + [DDP_COMPONENT_RDMA1] = { MTK_DISP_RDMA, 1, &ddp_rdma }, > + [DDP_COMPONENT_RDMA2] = { MTK_DISP_RDMA, 2, &ddp_rdma }, > + [DDP_COMPONENT_UFOE] = { MTK_DISP_UFOE, 0, &ddp_ufoe }, > + [DDP_COMPONENT_WDMA0] = { MTK_DISP_WDMA, 0, NULL }, > + [DDP_COMPONENT_WDMA1] = { MTK_DISP_WDMA, 1, NULL }, > +}; [snip...] > +int mtk_ddp_comp_init(struct device *dev, struct device_node *node, > + struct mtk_ddp_comp *comp, enum mtk_ddp_comp_id comp_id, > + const struct mtk_ddp_comp_funcs *funcs) > +{ > + enum mtk_ddp_comp_type type; > + struct device_node *larb_node; > + struct platform_device *larb_pdev; > + > + if (comp_id < 0 || comp_id >= DDP_COMPONENT_ID_MAX) > + return -EINVAL; > + > + comp->id = comp_id; > + comp->funcs = funcs ?: mtk_ddp_matches[comp_id].funcs; > + > + if (comp_id == DDP_COMPONENT_DPI0 || > + comp_id == DDP_COMPONENT_DSI0 || > + comp_id == DDP_COMPONENT_PWM0) { > + comp->regs = NULL; > + comp->clk = NULL; > + comp->irq = 0; > + return 0; > + } > + > + comp->regs = of_iomap(node, 0); > + comp->irq = of_irq_get(node, 0); > + comp->clk = of_clk_get(node, 0); > + if (IS_ERR(comp->clk)) > + comp->clk = NULL; > + > + type = mtk_ddp_matches[comp_id].type; > + > + /* Only DMA capable components need the LARB property */ > + comp->larb_dev = NULL; > + if (type != MTK_DISP_OVL && > + type != MTK_DISP_RDMA && > + type != MTK_DISP_WDMA) > + return 0; > + > + larb_node = of_parse_phandle(node, "mediatek,larb", 0); Need to call of_node_put() for larb_node somewhere. > + if (!larb_node) { > + dev_err(dev, > + "Missing mediadek,larb phandle in %s node\n", > + node->full_name); > + return -EINVAL; > + } > + > + larb_pdev = of_find_device_by_node(larb_node); > + if (!larb_pdev) { > + dev_warn(dev, "Waiting for larb device %s\n", > + larb_node->full_name); > + return -EPROBE_DEFER; > + } > + > + comp->larb_dev = &larb_pdev->dev; > + > + return 0; > +} [snip...] > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.h b/drivers/gpu/drm/mediatek/mtk_drm_fb.h > new file mode 100644 > index 0000000..a89c7af > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.h > @@ -0,0 +1,29 @@ > +/* > + * Copyright (c) 2015 MediaTek Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef MTK_DRM_FB_H > +#define MTK_DRM_FB_H > + > +#define MAX_FB_OBJ 3 Remove from this patch > + > +struct drm_gem_object *mtk_fb_get_gem_obj(struct drm_framebuffer *fb); > +int mtk_fb_wait(struct drm_framebuffer *fb); > +struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev, > + struct drm_file *file, > + struct drm_mode_fb_cmd2 *cmd); > + > +void mtk_drm_mode_output_poll_changed(struct drm_device *dev); > +int mtk_fbdev_create(struct drm_device *dev); > +void mtk_fbdev_destroy(struct drm_device *dev); Remove these last three function prototypes from this patch. > + > +#endif /* MTK_DRM_FB_H */ > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c > new file mode 100644 > index 0000000..96cc980 > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c > @@ -0,0 +1,227 @@ > +/* > + * Copyright (c) 2015 MediaTek Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <drm/drmP.h> > +#include <drm/drm_gem.h> > + > +#include "mtk_drm_gem.h" > + > +static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device *dev, > + unsigned long size) nit: I think these "size in bytes" parameters should use size_t. > +{ > + struct mtk_drm_gem_obj *mtk_gem_obj; > + int ret; > + > + size = round_up(size, PAGE_SIZE); > + > + mtk_gem_obj = kzalloc(sizeof(*mtk_gem_obj), GFP_KERNEL); > + if (!mtk_gem_obj) > + return ERR_PTR(-ENOMEM); > + > + ret = drm_gem_object_init(dev, &mtk_gem_obj->base, size); > + if (ret < 0) { > + DRM_ERROR("failed to initialize gem object\n"); > + kfree(mtk_gem_obj); > + return ERR_PTR(ret); > + } > + > + return mtk_gem_obj; > +} > + > +struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev, > + unsigned long size, bool alloc_kmap) > +{ > + struct mtk_drm_gem_obj *mtk_gem; > + struct drm_gem_object *obj; > + int ret; > + > + mtk_gem = mtk_drm_gem_init(dev, size); > + if (IS_ERR(mtk_gem)) > + return ERR_CAST(mtk_gem); > + > + obj = &mtk_gem->base; > + > + init_dma_attrs(&mtk_gem->dma_attrs); > + dma_set_attr(DMA_ATTR_WRITE_COMBINE, &mtk_gem->dma_attrs); > + > + if (!alloc_kmap) > + dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, &mtk_gem->dma_attrs); > + > + mtk_gem->cookie = dma_alloc_attrs(dev->dev, obj->size, > + (dma_addr_t *)&mtk_gem->dma_addr, GFP_KERNEL, Cast here is not necessary, since dma_addr is dma_addr_t. > + &mtk_gem->dma_attrs); > + if (!mtk_gem->cookie) { > + DRM_ERROR("failed to allocate %zx byte dma buffer", obj->size); > + ret = -ENOMEM; > + goto err_gem_free; > + } > + > + if (alloc_kmap) > + mtk_gem->kvaddr = mtk_gem->cookie; > + > + DRM_DEBUG_DRIVER("cookie = %p dma_addr = %pad\n", > + mtk_gem->cookie, &mtk_gem->dma_addr); Print size too. > + > + return mtk_gem; > + > +err_gem_free: > + drm_gem_object_release(obj); > + kfree(mtk_gem); > + return ERR_PTR(ret); > +} > + > +void mtk_drm_gem_free_object(struct drm_gem_object *obj) > +{ > + struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj); > + > + dma_free_attrs(obj->dev->dev, obj->size, mtk_gem->cookie, > + mtk_gem->dma_addr, &mtk_gem->dma_attrs); > + > + /* release file pointer to gem object. */ > + drm_gem_object_release(obj); > + > + kfree(mtk_gem); > +} > + > +int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev, > + struct drm_mode_create_dumb *args) > +{ > + struct mtk_drm_gem_obj *mtk_gem; > + int ret; > + > + args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8); Perhaps this more correctly aligns the pitch (this is what cma does): args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); > + args->size = args->pitch * args->height; > + > + mtk_gem = mtk_drm_gem_create(dev, args->size, false); > + if (IS_ERR(mtk_gem)) > + return PTR_ERR(mtk_gem); > + > + /* > + * allocate a id of idr table where the obj is registered > + * and handle has the id what user can see. > + */ > + ret = drm_gem_handle_create(file_priv, &mtk_gem->base, &args->handle); > + if (ret) > + goto err_handle_create; > + > + /* drop reference from allocate - handle holds it now. */ > + drm_gem_object_unreference_unlocked(&mtk_gem->base); > + > + return 0; > + > +err_handle_create: > + mtk_drm_gem_free_object(&mtk_gem->base); > + return ret; > +} > + > +int mtk_drm_gem_dumb_map_offset(struct drm_file *file_priv, > + struct drm_device *dev, uint32_t handle, > + uint64_t *offset) > +{ > + struct drm_gem_object *obj; > + int ret; > + The cma helper locks struct_mutex here. > + obj = drm_gem_object_lookup(dev, file_priv, handle); > + if (!obj) { > + DRM_ERROR("failed to lookup gem object.\n"); > + return -EINVAL; > + } > + > + ret = drm_gem_create_mmap_offset(obj); > + if (ret) > + goto out; > + > + *offset = drm_vma_node_offset_addr(&obj->vma_node); > + DRM_DEBUG_KMS("offset = 0x%llx\n", *offset); > + > +out: > + drm_gem_object_unreference_unlocked(obj); > + return ret; > +} [snip...] > +/* > + * Allocate a sg_table for this GEM object. > + * Note: Both the table's contents, and the sg_table itself must be freed by > + * the caller. > + * Returns a pointer to the newly allocated sg_table, or an ERR_PTR() error. > + */ > +struct sg_table *mtk_gem_prime_get_sg_table(struct drm_gem_object *obj) > +{ > + struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj); > + struct drm_device *drm = obj->dev; > + struct sg_table *sgt; > + int ret; > + > + sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); > + if (!sgt) > + return ERR_PTR(-ENOMEM); > + > + ret = dma_get_sgtable_attrs(drm->dev, sgt, mtk_gem->cookie, > + mtk_gem->dma_addr, obj->size, > + &mtk_gem->dma_attrs); > + if (ret) { > + DRM_ERROR("failed to allocate sgt, %d\n", ret); > + kfree(sgt); > + return ERR_PTR(ret); > + } > + > + return sgt; > +} Do we also want a prime_import_sg_table() here for importing foreign allocated dma bufs for display? [snip...] > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > new file mode 100644 > index 0000000..e9b6bf6 > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > @@ -0,0 +1,242 @@ > +/* > + * Copyright (c) 2015 MediaTek Inc. > + * Author: CK Hu <ck.hu@xxxxxxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <drm/drmP.h> > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_plane_helper.h> > + > +#include "mtk_drm_crtc.h" > +#include "mtk_drm_ddp_comp.h" > +#include "mtk_drm_drv.h" > +#include "mtk_drm_fb.h" > +#include "mtk_drm_gem.h" > +#include "mtk_drm_plane.h" > + > +static const uint32_t formats[] = { > + DRM_FORMAT_XRGB8888, > + DRM_FORMAT_ARGB8888, > + DRM_FORMAT_RGB565, > +}; > + > +static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable, > + dma_addr_t addr, struct drm_rect *dest) > +{ > + struct drm_plane *plane = &mtk_plane->base; > + struct mtk_plane_state *state = to_mtk_plane_state(plane->state); > + unsigned int pitch, format; > + int x, y; > + > + if (WARN_ON(!plane->state || (enable && !plane->state->fb))) > + return; > + > + if (plane->state->fb) { > + pitch = plane->state->fb->pitches[0]; > + format = plane->state->fb->pixel_format; > + } else { > + pitch = 0; > + format = DRM_FORMAT_RGBA8888; > + } > + > + x = plane->state->crtc_x; > + y = plane->state->crtc_y; > + > + if (x < 0) { > + addr -= x * 4; > + x = 0; > + } > + > + if (y < 0) { > + addr -= y * pitch; > + y = 0; > + } > + > + state->pending.enable = enable; > + state->pending.pitch = pitch; > + state->pending.format = format; > + state->pending.addr = addr; > + state->pending.x = x; > + state->pending.y = y; > + state->pending.width = dest->x2 - dest->x1; > + state->pending.height = dest->y2 - dest->y1; wmb(); ? BTW, it is a good idea to document all wmb() calls. If you forget, I believe checkpatch.pl will remind you. > + state->pending.dirty = true; > +} > + That's it! Thanks, -Dan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html