[PATCH v6 02/12] drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.

Tomasz Figa tfiga at chromium.org
Tue Nov 24 00:27:13 PST 2015


Hi Philipp, CK,

Please see my comments inline.

On Thu, Nov 19, 2015 at 2:34 AM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
[snip]
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> new file mode 100644
> index 0000000..508c8f3
> --- /dev/null
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -0,0 +1,596 @@
[snip]
> +#include "mtk_drm_drv.h"
> +#include "mtk_drm_crtc.h"
> +#include "mtk_drm_ddp.h"
> +#include "mtk_drm_ddp_comp.h"
> +#include "mtk_drm_gem.h"
> +#include "mtk_drm_plane.h"
> +
> +struct mtk_crtc_ddp_context;

Is this forward declaration really necessary?

> +
> +/*
> + * MediaTek specific crtc structure.
> + *
> + * @base: crtc object.
> + * @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.

Only first two fields documented? Also this isn't proper kerneldoc
syntax (see Documentation/kernel-doc-nano-HOWTO.txt).

> + */
> +struct mtk_drm_crtc {
> +       struct drm_crtc                 base;
> +       unsigned int                    pipe;
> +
> +       bool                            do_flush;
> +
> +       struct mtk_drm_plane            planes[OVL_LAYER_NR];
> +
> +       void __iomem                    *config_regs;
> +       struct mtk_disp_mutex           *mutex;
> +       u32                             ddp_comp_nr;

I assume this is size of ddp_comp array? Why not just unsigned int then?

> +       struct mtk_ddp_comp             **ddp_comp;
> +};
[snip]
> +static bool mtk_drm_crtc_mode_fixup(struct drm_crtc *crtc,
> +               const struct drm_display_mode *mode,
> +               struct drm_display_mode *adjusted_mode)
> +{
> +       /* drm framework doesn't check NULL */

Maybe rephrase the comment to "Nothing to do here, but the callback is
mandatory."?

> +       return true;
> +}
[snip]
> +static void mtk_crtc_ddp_power_on(struct mtk_drm_crtc *mtk_crtc)
> +{
> +       int ret;
> +       int i;
> +
> +       DRM_INFO("mtk_crtc_ddp_power_on\n");

DRM_DEBUG_DRIVER()

> +       for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> +               ret = clk_enable(mtk_crtc->ddp_comp[i]->clk);
> +               if (ret)
> +                       DRM_ERROR("Failed to enable clock %d: %d\n", i, ret);

This is unsafe, because even if we fail here, mtk_crtc_ddp_power_off()
will try to disable the clocks anyway, which will lead to negative
enable counts (and a WARN() in best case). Can we add proper error
handling to this function and other functions in the call stack?

> +       }
> +}
> +
> +static void mtk_crtc_ddp_power_off(struct mtk_drm_crtc *mtk_crtc)
> +{
> +       int i;
> +
> +       DRM_INFO("mtk_crtc_ddp_power_off\n");

DRM_DEBUG_DRIVER()

> +       for (i = 0; i < mtk_crtc->ddp_comp_nr; i++)
> +               clk_disable(mtk_crtc->ddp_comp[i]->clk);
> +}
> +
> +static void 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;
> +
> +       if (crtc->state) {
> +               width = crtc->state->adjusted_mode.hdisplay;
> +               height = crtc->state->adjusted_mode.vdisplay;
> +               vrefresh = crtc->state->adjusted_mode.vrefresh;
> +       } else {
> +               WARN_ON(true);
> +               width = 1920;
> +               height = 1080;
> +               vrefresh = 60;

When can crtc->state be NULL? Also shouldn't we just fail here instead
of carrying on?

> +       }

nit: The if above can be replaced with the following.

if (WARN_ON(!crtc->state)) {
     // do the error handling
} else {
    // dereference crtc->state
}

This is better because the warning condition shows what's wrong.

> +
> +       DRM_INFO("mtk_crtc_ddp_hw_init\n");

DRM_DEBUG_DRIVER()

> +
> +       /* disp_mtcmos */
> +       ret = pm_runtime_get_sync(crtc->dev->dev);
> +       if (ret < 0)
> +               DRM_ERROR("Failed to enable power domain: %d\n", ret);

Carrying on here after pm runtime error is at least inappropriate and
in practice is a good way to lock the system up...

> +
> +       mtk_disp_mutex_prepare(mtk_crtc->mutex);
> +       mtk_crtc_ddp_power_on(mtk_crtc);
> +
> +       DRM_INFO("mediatek_ddp_ddp_path_setup\n");

DRM_DEBUG_DRIVER()

> +       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_INFO("ddp_disp_path_power_on %dx%d\n", width, height);

DRM_DEBUG_DRIVER()

> +       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_power_on(comp);
> +       }
> +}
> +
> +static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc)
> +{
> +       struct drm_device *drm = mtk_crtc->base.dev;
> +       int i;
> +
> +       DRM_INFO("mtk_crtc_ddp_hw_fini\n");

DRM_DEBUG_DRIVER()

> +       mtk_crtc_ddp_power_off(mtk_crtc);
> +       for (i = 0; i < mtk_crtc->ddp_comp_nr; i++)
> +               mtk_disp_mutex_remove_comp(mtk_crtc->mutex,
> +                                          mtk_crtc->ddp_comp[i]->id);
> +       mtk_disp_mutex_disable(mtk_crtc->mutex);
> +       for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) {
> +               mtk_ddp_remove_comp_from_path(mtk_crtc->config_regs,
> +                                             mtk_crtc->ddp_comp[i]->id,
> +                                             mtk_crtc->ddp_comp[i + 1]->id);
> +               mtk_disp_mutex_remove_comp(mtk_crtc->mutex,
> +                                          mtk_crtc->ddp_comp[i]->id);
> +       }
> +       mtk_disp_mutex_remove_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id);
> +       mtk_disp_mutex_unprepare(mtk_crtc->mutex);
> +       mtk_crtc->mutex = NULL;

mtk_crtc->mutex was acquired by mtk_disp_mutex_get(). Shouldn't it be
released with mtk_disp_mutex_put()? (In fact I can see
mtk_disp_ovl_unbind() already doing that).

> +
> +       /* disp_mtcmos */

This comment doesn't seem to be very meaningful.

> +       pm_runtime_put(drm->dev);
> +}

To be continued.

Best regards,
Tomasz



More information about the Linux-mediatek mailing list