Hi Daniel,
Am Donnerstag, den 05.11.2015, 02:49 +0800 schrieb Daniel Kurtz:
Hi Philipp,
A bunch of review comments inline.
A bunch indeed. Thank you for the feedback.
On Wed, Nov 4, 2015 at 7:44 PM, Philipp Zabel p.zabel@pengutronix.de wrote:
[...]
+struct mtk_drm_crtc {
struct drm_crtc base;unsigned int pipe;There is one pipe too many :-) I think this one is not used."
bool enabled;struct mtk_crtc_ddp_context *ctx;I think you should be able to just embed the "mtk_crtc_ddp_context" right into mtk_drm_crtc. Or maybe just get rid of mtk_crtc_ddp_context completely and just use mtk_drm_crtc eveywhere.
I'll combine them and get rid of the superfluous pipe.
[...]
+static void mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *crtc) +{
[...]
/* disp_mtcmos */ret = pm_runtime_get_sync(drm->dev);if (ret < 0)DRM_ERROR("Failed to enable power domain: %d\n", ret);mtk_ddp_clock_on(ctx->mutex_dev);mtk_crtc_ddp_power_on(ctx);get_sync(), clock_on() and ddp_power_on() really can fail; we are just ignoring errors here. Since they can fail, shouldn't they be moved out of the atomic "->enable()" path into the ->check() path that is allowed to fail?
You suggest I move them into atomic_check?
To me it sounds like this should not be called from check, but from the drm_mode_config_funcs atomic_commit callback, right after calling drm_atomic_helper_prepare_planes. But there is no prepare equivalent to drm_atomic_helper_commit_modeset_enables.
DRM_INFO("mediatek_ddp_ddp_path_setup\n");for (i = 0; i < (ctx->ddp_comp_nr - 1); i++) {nit: the inner () are not necessary.
Will remove those.
mtk_ddp_add_comp_to_path(ctx->config_regs, ctx->pipe,ctx->ddp_comp[i].funcs->comp_id,ctx->ddp_comp[i+1].funcs->comp_id);mtk_ddp_add_comp_to_mutex(ctx->mutex_dev, ctx->pipe,ctx->ddp_comp[i].funcs->comp_id,ctx->ddp_comp[i+1].funcs->comp_id);}Do you really have to do this here in the enable path? This looks like something that should be done in bind()?
Perhaps all we really need here is to walk the path and write to DISP_REG_MUTEX_EN at the end of mtk_ddp_add_comp_to_mutex(). By the way, where are those bits cleared in the disable path?
Disabling or changing the path is not implemented, the currently static setup could well be moved into bind().
[...]
+static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *crtc) +{
[...]
pm_runtime_put_sync(drm->dev);Why sync?
I can think of no reason, will use the async version here.
[...]
+static void mtk_drm_crtc_disable(struct drm_crtc *crtc) +{
struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);struct mtk_crtc_state *state = to_mtk_crtc_state(crtc->state);DRM_INFO("mtk_drm_crtc_disable %d\n", crtc->base.id);if (WARN_ON(!mtk_crtc->enabled))return;mtk_crtc_ddp_hw_fini(mtk_crtc);mtk_crtc->do_flush = false;if (state->event)mtk_drm_crtc_finish_page_flip(mtk_crtc);It is a bit awkward to send the page flip event before the actual page flip has completed (in this case, for a page flip that you are canceling by disabling the crtc). Would it be better to wait for vblank here in crtc_disable instead?
Yes, I will wait for vblank here instead.
[...]
+static void mtk_drm_crtc_atomic_begin(struct drm_crtc *crtc,
struct drm_crtc_state *old_crtc_state)+{
struct mtk_crtc_state *state = to_mtk_crtc_state(crtc->state);if (state->base.event) {state->base.event->pipe = drm_crtc_index(crtc);WARN_ON(drm_crtc_vblank_get(crtc) != 0);state->event = state->base.event;state->base.event = NULL;Hmm. Why are we "stealing" event from drm_crtc_state and handing it over to the wrapper mtk_crtc_state? Won't we still be able to retrieve state->base.event later when we need it in the mtk_drm_crtc_finish_page_flip?
Hmm, good point. I'll try that.
[...]
+static void mtk_crtc_ddp_irq(struct mtk_crtc_ddp_context *ctx)
==> So, this is the part of the driver that makes me the most nervous. Why are we writing the actual hardware registers in the *vblank interrupt handler*?! Every other display controller that I've ever seen has shadow registers that are used to stage a page flip at the next vblank. Are you sure this hardware doesn't support this?
In any case, how do we get that first interrupt in which to apply the register?
I'll try to rework this using the DISP_MUTEX to trigger register updates on SOF.
+{
struct mtk_drm_crtc *mtk_crtc = ctx->crtc;struct mtk_ddp_comp *ovl = &ctx->ddp_comp[0];ddp_comp[0] here looks a bit like black magic. "The 0th component is always the ovl". Perhaps make an explicitly named "ovl" field for mtk_crtc_ddp_context.
Ok.
struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);unsigned int i;if (state->pending_config) {state->pending_config = false;for (i = 0; i < OVL_LAYER_NR; i++)ovl->funcs->comp_layer_off(ovl->regs, i);Why do you need to turn off all layers before setting mode?
I am not sure if this is necessary. It seems to work without.
ovl->funcs->comp_config(ovl->regs, state->pending_width,state->pending_height,state->pending_vrefresh);}for (i = 0; i < OVL_LAYER_NR; i++) {if (state->pending_ovl_config[i]) {if (!state->pending_ovl_enable[i])ovl->funcs->comp_layer_off(ovl->regs, i);ovl->funcs->comp_layer_config(ovl->regs, i,state->pending_ovl_addr[i],state->pending_ovl_pitch[i],state->pending_ovl_format[i],state->pending_ovl_x[i],state->pending_ovl_y[i],state->pending_ovl_size[i]);if (state->pending_ovl_enable[i])ovl->funcs->comp_layer_on(ovl->regs, i);}I'd move this all all with an mtk_plane function, and let each mtk_plane store its own pending state.
Ok.
[...]
+static int mtk_crtc_ddp_bind(struct device *dev, struct device *master,
void *data)+{
[...]
for (zpos = 0; zpos < OVL_LAYER_NR; zpos++) {type = (zpos == 0) ? DRM_PLANE_TYPE_PRIMARY :(zpos == 1) ? DRM_PLANE_TYPE_CURSOR :DRM_PLANE_TYPE_OVERLAY;ret = mtk_plane_init(drm_dev, &ctx->planes[zpos],1 << ctx->pipe, type, zpos, OVL_LAYER_NR);if (ret)goto unprepare;}I think you should just let mtk_drm_crtc_create() create & init its own planes. Currently, its overlay planes are unassigned. Furthermore, the crtc context doesn't really need an array of planes at all. It only really uses the array to know which planes to update during the vblank irq. But, that information is actually already present in the atomic state to be applied itself.
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h new file mode 100644 index 0000000..b696066 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h @@ -0,0 +1,56 @@
[...]
+struct mtk_crtc_state {
struct drm_crtc_state base;struct drm_pending_vblank_event *event;unsigned int pending_update;bool pending_needs_vblank;bool pending_config;unsigned int pending_width;unsigned int pending_height;unsigned int pending_vrefresh;bool pending_ovl_config[OVL_LAYER_NR];bool pending_ovl_enable[OVL_LAYER_NR];unsigned int pending_ovl_addr[OVL_LAYER_NR];unsigned int pending_ovl_pitch[OVL_LAYER_NR];unsigned int pending_ovl_format[OVL_LAYER_NR];int pending_ovl_x[OVL_LAYER_NR];int pending_ovl_y[OVL_LAYER_NR];unsigned int pending_ovl_size[OVL_LAYER_NR];bool pending_ovl_dirty[OVL_LAYER_NR];All of these OVL_LAYER_NR length arrays look like mtk_plane_state to me. I think we want to do something similar and wrap drm_plane_state.
[...]
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c @@ -0,0 +1,218 @@
[...]
+#define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * n) +#define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * n) +#define DISP_REG_MUTEX_MOD(n) (0x2c + 0x20 * n) +#define DISP_REG_MUTEX_SOF(n) (0x30 + 0x20 * n)
The n should be in parens here.
Ok.
[...]
+void mtk_ddp_add_comp_to_path(void __iomem *config_regs, unsigned int pipe,
enum mtk_ddp_comp_id cur,enum mtk_ddp_comp_id next)Why pass pipe if we don't use it?
Leftover from before the split of mtk_ddp_add_to_path/mutex.
[...]
if (value) {unsigned int reg = readl(config_regs + addr) | value;writel(reg, config_regs + addr);Almost all of the writel() in this patch can really be writel_relaxed(). The only ones that need writel() are when you really need a barrier.
Ok.
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c new file mode 100644 index 0000000..2f3b32b --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c @@ -0,0 +1,424 @@
[...]
+static void mtk_ovl_config(void __iomem *ovl_base,
unsigned int w, unsigned int h, unsigned int vrefresh)+{
if (w != 0 && h != 0)writel(h << 16 | w, ovl_base + DISP_REG_OVL_ROI_SIZE);Should you just write 0 to DISP_REG_OVL_ROI_SIZE if w or h are 0?
According to the docs, width and height must be at least 1. But of course I should check this earlier.
[...]
+static unsigned int ovl_fmt_convert(unsigned int fmt)
return int so the error code stays negative?
I'll just return RGB888 on error.
[...]
+static void mtk_ovl_layer_config(void __iomem *ovl_base, unsigned int idx,
unsigned int addr, unsigned int pitch, unsigned int fmt,int x, int y, unsigned int size)+{
unsigned int reg;reg = has_rb_swapped(fmt) << 24 | ovl_fmt_convert(fmt) << 12;ovl_fmt_convert() can return < 0 for unsupported fmt. But, perhaps we can remove that check, since it should actually already be guaranteed by the drm core that a plane is only ever asked to display an FB with a format that the plane advertises.
Exactly. If something unsupported gets through here, it's a bug.
[...]
+static const struct mtk_ddp_comp_funcs ddp_color0 = {
.comp_id = DDP_COMPONENT_COLOR0,.comp_power_on = mtk_color_start,For consistency, can you name all of the "*_start()" functions "*_power_on", or change the .comp_power_on to .comp_start. Actually, just drop the "comp_" from all of the fields of mtk_ddp_comp_funcs, since it is redundant.
I'd change them all to .enable/.disable.
+static const struct mtk_ddp_comp_funcs ddp_ovl1 = {
.comp_id = DDP_COMPONENT_OVL1,.comp_config = mtk_ovl_config,.comp_power_on = mtk_ovl_start,.comp_enable_vblank = mtk_ovl_enable_vblank,.comp_disable_vblank = mtk_ovl_disable_vblank,.comp_clear_vblank = mtk_ovl_clear_vblank,.comp_layer_on = mtk_ovl_layer_on,.comp_layer_off = mtk_ovl_layer_off,.comp_layer_config = mtk_ovl_layer_config,+};
The callback duplication here suggests the component model can be refined a bit. I think you probably want a single one of these:
static const struct mtk_ddp_comp_funcs ddp_ovl = { .config = mtk_ovl_config, .start = mtk_ovl_start, .enable_vblank = mtk_ovl_enable_vblank, .disable_vblank = mtk_ovl_disable_vblank, .clear_vblank = mtk_ovl_clear_vblank, .layer_on = mtk_ovl_layer_on, .layer_off = mtk_ovl_layer_off, .layer_config = mtk_ovl_layer_config, };
Yes, the next version will drop .comp_id from the funcs.
and then to use it for both ovl's. Maybe something like this:
static const struct mtk_ddp_comp_desc ddp_ovl0 = { .id = DDP_COMPONENT_OVL0, .ops = ddp_ovl };
static const struct mtk_ddp_comp_desc ddp_ovl1 = { .id = DDP_COMPONENT_OVL1, .ops = ddp_ovl };
These will not be necessary, I'll store the id in mtk_ddp_comp.
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h new file mode 100644 index 0000000..ae3a6e8 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h @@ -0,0 +1,86 @@
[...]
+static void mtk_atomic_complete(struct mtk_drm_private *private,
struct drm_atomic_state *state)+{
struct drm_device *drm = private->drm;drm_atomic_helper_commit_modeset_disables(drm, state);drm_atomic_helper_commit_planes(drm, state);drm_atomic_helper_commit_modeset_enables(drm, state);drm_atomic_helper_wait_for_vblanks(drm, state);Why wait for vblank here? Are you sure we waited long enough here? This will return after the very next vblank. However, what guarantees that the new state was really transferred to the hardware in time?
I'll check out how to use the DISP_MUTEX for this. It has an interrupt to signal when the register settings were applied.
[...]
+static int mtk_atomic_commit(struct drm_device *drm,
struct drm_atomic_state *state,bool async)+{
struct mtk_drm_private *private = drm->dev_private;int ret;ret = drm_atomic_helper_prepare_planes(drm, state);if (ret)return ret;mutex_lock(&private->commit.lock);flush_work(&private->commit.work);drm_atomic_helper_swap_state(drm, state);if (async)mtk_atomic_schedule(private, state);elsemtk_atomic_complete(private, state);mutex_unlock(&private->commit.lock);What does this lock do? AFAICT, it only ensures that the second half of mtk_atomic_commit() cannot execute twice at the same time. However, I expect simultaneous calls to atomic_commit() should already be prohibited by the drm core?
I don't see it. DRM_IOCTL_MODE_ATOMIC is unlocked, and drm_mode_atomic_ioctl only calls drm_modeset_acquire_init but doesn't get any lock before calling drm_atomic_(async_)commit.
[...]
+static int mtk_drm_kms_init(struct drm_device *drm) +{
struct mtk_drm_private *private = drm->dev_private;struct platform_device *pdev;int ret;int i;pdev = of_find_device_by_node(private->mutex_node);if (!pdev) {dev_err(drm->dev, "Waiting for disp-mutex device %s\n",private->mutex_node->full_name);of_node_put(private->mutex_node);return -EPROBE_DEFER;}private->mutex_dev = &pdev->dev;for (i = 0; i < MAX_CRTC; i++) {if (!private->larb_node[i])break;pdev = of_find_device_by_node(private->larb_node[i]);if (!pdev) {dev_err(drm->dev, "Waiting for larb device %s\n",private->larb_node[i]->full_name);Nit: dev_warn() perhaps?
Ok.
[...]
for (i = 0; i < MAX_CRTC; i++) {if (!private->larb_dev[i])break;ret = mtk_smi_larb_get(private->larb_dev[i]);Hmm. this looks like it should be done by a crtc function, and, only for CRTCs that are actually going to be used.
Yes, I suppose these should only be enabled while the respective DMA components (OVL, RDMA, WDMA) are active. I think the correct place for this would be in the component's .enable (or a new .prepare if necessary).
if (ret) {DRM_ERROR("Failed to get larb: %d\n", ret);goto err_larb_get;}}drm_kms_helper_poll_init(drm);drm_mode_config_reset(drm);return 0;+err_larb_get:
for (i = i - 1; i >= 0; i--)mtk_smi_larb_put(private->larb_dev[i]);drm_kms_helper_poll_fini(drm);Not drm_kms_helper_poll_fini().
Ok.
[...]
+static void mtk_drm_kms_deinit(struct drm_device *drm) +{
put the larbs?
Ok, unless I move them elsewhere.
drm_kms_helper_poll_fini(drm);drm_vblank_cleanup(drm);unbind_all ?
Ok.
[...]
+static struct drm_driver mtk_drm_driver = {
.driver_features = DRIVER_MODESET | DRIVER_GEM,| DRIVER_ATOMIC ?
Yes.
[...]
+static int mtk_drm_probe(struct platform_device *pdev) +{
[...]
/* Iterate over sibling DISP function blocks */for_each_child_of_node(dev->of_node->parent, node) {const struct of_device_id *of_id;enum mtk_ddp_comp_type comp_type;int comp_id;of_id = of_match_node(mtk_ddp_comp_dt_ids, node);if (!of_id)continue;comp_type = (enum mtk_ddp_comp_type)of_id->data;if (comp_type == MTK_DISP_MUTEX) {private->mutex_node = of_node_get(node);continue;}I'd move the MUTEX check after "is_available()", to catch the odd case where the MUTEX node is disabled (it will still be NULL, and fail below).
Ok.
if (!of_device_is_available(node)) {dev_dbg(dev, "Skipping disabled component %s\n",node->full_name);continue;}comp_id = mtk_ddp_comp_get_id(node, comp_type);if (comp_id < 0) {dev_info(dev, "Skipping unknown component %s\n",node->full_name);dev_warn()
Ok.
[...]
if (comp_type == MTK_DISP_OVL) {struct device_node *larb_node;larb_node = of_parse_phandle(node, "mediatek,larb", 0);if (larb_node && num_larbs < MAX_CRTC)private->larb_node[num_larbs++] = larb_node;It feels like the larb_nodes should be handled directly by the ovl driver.
Yes. And the rdma/wdma, probably. For that I'd like to split the ovl driver from the crtc implementation.
}}if (!private->mutex_node) {dev_err(dev, "Failed to find disp-mutex node\n");Cleanup: of_node_put() any nodes already in comp_node[] Anything to clean after component_match_add()? of_node_put() larb_nodes?
[...]
+static int mtk_drm_remove(struct platform_device *pdev) +{
component_master_del(&pdev->dev, &mtk_drm_ops);pm_runtime_disable(&pdev->dev);Cleanup: of_node_put() any nodes already in comp_node[]
Ok.
Anything to clean after component_match_add()?
No, that just calls devm_kmalloc internally.
of_node_put() larb_nodes?
Ok.
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h new file mode 100644 index 0000000..5e5128e --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h @@ -0,0 +1,61 @@
[...]
+struct mtk_drm_private {
struct drm_fb_helper *fb_helper;Not used?
Will drop, this belongs into a separate fb_helper patch (not part of this series).
struct drm_device *drm;/** created crtc object would be contained at this array and* this array is used to be aware of which crtc did it request vblank.I don't understand this comment.
I'll just drop it. This array of pointers is needed to translate from pipe number to struct mtk_drm_crtc in crtc_enable/disable_vblank.
*/struct drm_crtc *crtc[MAX_CRTC];struct drm_property *plane_zpos_property;Not used.
Will drop, this belongs into a separate zpos property patch (not part of this series).
unsigned int pipe;what pipe is this?
That should be renamed to num_pipes.
struct device_node *larb_node[MAX_CRTC];struct device *larb_dev[MAX_CRTC];struct device_node *mutex_node;struct device *mutex_dev;void __iomem *config_regs;unsigned int path_len[MAX_CRTC];const enum mtk_ddp_comp_id *path[MAX_CRTC];Having 5 arrays of length MAX_CRTC suggests you really want one to combine these fields into a struct, and have an MAX_CRTC array of the struct. In fact, this struct sounds like it should be "mtk_crtc", which wraps a drm_crtc.
The larb_node/dev should be moved into DMA component drivers, but a static path indeed currently maps directly to a crtc.
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c new file mode 100644 index 0000000..dfa931b --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.c @@ -0,0 +1,151 @@
[...]
+struct mtk_drm_fb {
struct drm_framebuffer base;struct drm_gem_object *gem_obj[MAX_FB_OBJ];Nit: The display controller driver does not handle multi-buffer formats. So, maybe just add the array later when it does. Arguably, for now it is just adding complexity with no benefit.
Ok.
[...]
+struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev,
struct drm_file *file,struct drm_mode_fb_cmd2 *cmd)+{
unsigned int hsub, vsub, i;struct mtk_drm_fb *mtk_fb;struct drm_gem_object *gem[MAX_FB_OBJ];int err;hsub = drm_format_horz_chroma_subsampling(cmd->pixel_format);vsub = drm_format_vert_chroma_subsampling(cmd->pixel_format);nit: blank line here would help readability.
for (i = 0; i < drm_format_num_planes(cmd->pixel_format); i++) {
Ok.
[...]
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..9ce7307 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.h @@ -0,0 +1,29 @@ +struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device *dev,
unsigned long size)static? But actually, just folding this directly into mtk_drm_gem_create() would make the code flow clearer, since this function has no corresponding _fini().
Yes.
[...]
+struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev,
unsigned long size, bool alloc_kmap)static?
It is referenced from mtk_drm_drv.c
[...]
+void mtk_drm_gem_free_object(struct drm_gem_object *obj) +{
struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);drm_gem_free_mmap_offset(obj);/* release file pointer to gem object. */drm_gem_object_release(obj);drm_gem_object_release calls drm_gem_free_mmap_offset().
Ok.
dma_free_attrs(obj->dev->dev, obj->size, mtk_gem->cookie,mtk_gem->dma_addr, &mtk_gem->dma_attrs);I think you want to do this before calling drm_gem_object_release.
Why?
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.h b/drivers/gpu/drm/mediatek/mtk_drm_gem.h new file mode 100644 index 0000000..fb7953e --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.h @@ -0,0 +1,56 @@
[...]
+struct mtk_drm_gem_obj {
struct drm_gem_object base;void __iomem *cookie;void __iomem *kvaddr;Is kvaddr really __iomem?
No, and neither is cookie.
[...]
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..3a8843c --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c @@ -0,0 +1,167 @@
[...]
+static void mtk_plane_atomic_update(struct drm_plane *plane,
struct drm_plane_state *old_state)+{
struct drm_plane_state *state = plane->state;struct drm_gem_object *gem_obj;struct drm_crtc *crtc = state->crtc;struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane);struct drm_rect dest = {.x1 = state->crtc_x,.y1 = state->crtc_y,.x2 = state->crtc_x + state->crtc_w,.y2 = state->crtc_y + state->crtc_h,};struct drm_rect clip = { 0, };if (!crtc)return;clip.x2 = state->crtc->state->mode.hdisplay;clip.y2 = state->crtc->state->mode.vdisplay;drm_rect_intersect(&dest, &clip);mtk_plane->disp_size = (dest.y2 - dest.y1) << 16 | (dest.x2 - dest.x1);plane->fb = state->fb;This doesn't look right. The drm core should be doing this for us. Why do you need this here?
I really think we want to be using a "struct mtk_plane_state" which wraps a drm_plane_state and accumulates our ovl specific changes. During init, when creating crtcs & planes, we should tell each plane its id, and which ovl it should use. The mtk_plane code can then provide a function for the vblank irq to apply mtk_plane's pending state.
I agree.
gem_obj = mtk_fb_get_gem_obj(state->fb, 0);mtk_plane->flip_obj = to_mtk_gem_obj(gem_obj);We do not need to store "flip_obj". We only use it twice, +3 and +5 lines down from here. Also, this doesn't look safe. What if state->fb is NULL?
Since we implement atomic_disable, atomic_update should never be called with state->fb == NULL.
It works because to_mtk_gem_obj(NULL) == NULL, but it relies on an internal implementation detail - gem_obj.base is at offset 0.
mtk_plane->crtc = crtc;This isn't used, either.
Will drop.
[...]
+static void mtk_plane_atomic_disable(struct drm_plane *plane,
struct drm_plane_state *old_state)+{
struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane);struct drm_crtc *crtc = old_state->crtc;if (!crtc)return;mtk_drm_crtc_plane_config(crtc, mtk_plane->idx, false, 0);mtk_drm_crtc_commit(crtc);Why this here? Won't this happen during mtk_drm_crtc_atomic_flush?
Because that just marks a configuration change in pending_ovl_config, which you already suggested rightfully belongs in mtk_plane_state. I'll fix that.
[...]
+int mtk_plane_init(struct drm_device *dev, struct mtk_drm_plane *mtk_plane,
unsigned long possible_crtcs, enum drm_plane_type type,unsigned int zpos, unsigned int max_plane)max_plane is not used.
Ok... enough for today :-).
This is also part of the zpos property patch, I'll drop it.
regards Philipp