Hi Tomasz,
Am Dienstag, den 24.11.2015, 17:27 +0900 schrieb Tomasz Figa:
Hi Philipp, CK,
Please see my comments inline.
Thank you for your comments.
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 @@
[...]
+struct mtk_crtc_ddp_context;
Is this forward declaration really necessary?
Leftover, will remove.
+/*
- 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).
I'll fix that.
- */
+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?
And that.
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."?
Ok.
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()
Yes, and the same for the following instances of this issue you point out below.
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?
Ultimately these are called by the enable/disable drm_crtc_helper_funcs, which aren't allowed to fail. And clk_enable of core SoC clocks should never fail either. If we hit this error, something else is very wrong with the system already. I'll have mtk_crtc_ddp_power_on propagate the error and mtk_crtc_ddp_hw_init below bail out, and I'll also re-add the mtk_crtc->enabled bool again to let crtc_disable warn and bail out in case crtc_enable failed.
[...]
+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.
Ok.
[...]
/* disp_mtcmos */
I'll drop this comment.
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...
Ok, will be fixed as part of the error handling cleanup.
[...]
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).
Absolutely yes, this was found really quickly. I missed it at first just because I forgot to plug in/out the HDMI connector a second time when testing. The NULL assignment needs to be removed here.
/* disp_mtcmos */This comment doesn't seem to be very meaningful.
I'll drop it.
pm_runtime_put(drm->dev);+}
To be continued.
Thanks!
regards Philipp