Re: [PATCH v3 2/3] drm/arm: Add support for Mali Display Processors

[Daniel Vetter]

On Fri, Jun 10, 2016 at 09:52:59AM +0100, Liviu Dudau wrote:
> On Thu, Jun 09, 2016 at 08:56:29PM +0200, Daniel Vetter wrote:
> I'm seeing ->disable being called at HPD event time, which is soon after ->probe.
> 
> > This is the case when enabling a crtc when it is fully
> > disabled. Just mentioning in case ->enter_config_mode() is something that
> > must be called symmetrically with ->leave_config_mode().
> 
> ->leave_config_mode() should be the default mode when HW is disabled or not active,
> and the reset default value. Regardless of that, ->enter_config_mode() can be
> called at any time, even if HW is already in config mode.
> 
> > > +
> > > +	/* mclk needs to be set to the same or higher rate than pxlclk */
> > > +	clk_set_rate(hwdev->mclk, crtc->state->adjusted_mode.crtc_clock * 1000);
> > > +	clk_set_rate(hwdev->pxlclk, crtc->state->adjusted_mode.crtc_clock * 1000);
> > > +
> > > +	hwdev->modeset(hwdev, &vm);
> > > +	hwdev->leave_config_mode(hwdev);
> > > +}
> > > +
> > > +static void malidp_crtc_disable(struct drm_crtc *crtc)
> > > +{
> > > +	struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
> > > +	struct malidp_hw_device *hwdev = malidp->dev;
> > > +
> > > +	/*
> > > +	 * avoid disabling already disabled clocks and hardware
> > > +	 * (as is the case at device probe time)
> > > +	 */
> > > +	if (crtc->state->active) {
> > 
> > Comment doesn't match code. Checking crtc->state->active in ->disable
> > looks at the _new_ state, not at the current state of the crtc. Atomic
> > helpers already guarantee you that ->disable is only called when the CRTC
> > is still on. 
> 
> Except at probe* time, when the framework calls ->disable before modeset to
> make sure that the hardware is in a known state. And I'm not sure how to
> check the _current_ state of the crtc other than by using crtc->state.
> 
> * actually, it is HPD event after probe.

Surprising. And you don't have a call to
drm_helper_disable_unused_functions, which is the usual culprit. Where
exactly do you see that ->disable call? Can you pls capture a backtrace
with full drm debugging enabled?

This shouldn't happen with atomic ...

> > > +static void malidp_unbind(struct device *dev)
> > > +{
> > > +	struct drm_device *drm = dev_get_drvdata(dev);
> > > +	struct malidp_drm *malidp = drm->dev_private;
> > > +	struct malidp_hw_device *hwdev = malidp->dev;
> > > +
> > > +	if (malidp->fbdev) {
> > > +		drm_fbdev_cma_fini(malidp->fbdev);
> > > +		malidp->fbdev = NULL;
> > > +	}
> > > +	drm_kms_helper_poll_fini(drm);
> > > +	malidp_se_irq_fini(drm);
> > > +	malidp_de_irq_fini(drm);
> > > +	drm_vblank_cleanup(drm);
> > > +	component_unbind_all(dev, drm);
> > > +	drm_dev_unregister(drm);
> > 
> > Unregister first, also need to unregister connectors too.
> 
> Bah, you are right. Does unregister have to come even before
> drm_kms_helper_poll_fini() ?

It's just generally the safest approach to first unregister everything,
and only then start cleaning up.

> As for the connectors, because my driver uses an encoder that
> is also a component slave, component_[un]bind_all() should take
> care of [un]registering that.

E.g. this sounds unsafe, because drm assume that encoder lists are static
over the lifetime of the driver. You should make sure no one can get at it
any more first before cleaning up any components.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch