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