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@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 @@
[...]
> > +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
--
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
