Hi Philipp, CK,
Please see my comments inline.
On Thu, Nov 19, 2015 at 2:34 AM, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> 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
--
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
