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@xxxxxxxxxxxxxx> 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);
> > + else
> > + mtk_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
--
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
