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

[Philipp Zabel <p.zabel@pengutronix.de>]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel