Re: [PATCH v3 08/14] drm: rcar-du: Add support for CMM

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

On Thu, Sep 05, 2019 at 03:14:53PM +0200, Jacopo Mondi wrote:
> On Thu, Sep 05, 2019 at 02:17:12PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> >>>>>> +/**
> >>>>>> + * rcar_cmm_enable() - enable the CMM unit
> >>>>>> + *
> >>>>>> + * @pdev: The platform device associated with the CMM instance
> >>>>>> + *
> >>>>>> + * Enable the CMM unit by enabling the parent clock and enabling the CMM
> >>>>>> + * components, such as 1-D LUT, if requested.
> >>>>>> + */
> >>>>>> +int rcar_cmm_enable(struct platform_device *pdev)
> >>>>>> +{
> >>>>>> +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	if (!rcmm)
> >>>>>> +		return -EPROBE_DEFER;
> >>>>>
> >>>>> This function is called in rcar_du_crtc_atomic_enable(), so that's not
> >>>>> the right error code. It seems we need another function for the CMM API
> >>>>> to defer probing :-/ I would call it rcar_cmm_init(). This check would
> >>>>> then be removed.
> >>>>
> >>>> I agree about the return code, but not the name, as this function
> >>>> actually enables the CMM.
> >>>
> >>> I meant creating a new rcar_cmm_init() function that would just have the
> >>> !rcmm check.
> >>>
> >>>> PROBE_DEFER does not make any sense here, I
> >>>> wonder where it come from, as the probing of CMM and DU has long
> >>>> happened once we get here (at least, I assume so, if we receive a
> >>>> gamma_table, userspace has already been running, and both DU and CMM
> >>>> should have probed. Otherwise, we can exploit the newly created device
> >>>> link, and make sure DU probes after the CMM).
> >>>>
> >>>> I would just change the return value here, and possibly use the device
> >>>> link to ensure the correct probing sequence.
> >>>
> >>> How does device link help here ?
> >>
> >> Currently it doesn't, as we are creating a stateless link.
> >>
> >> But if we go for a managed device link (which is the default, by the
> >> way, you have to opt-out from it) we can guarantee the CMM has probed
> >> before the DU probes, so that we have a guarantee when we get here
> >> !rcmm cannot happen.
> >>
> >> https://www.kernel.org/doc/html/v5.2-rc7/driver-api/device_link.html
> >> "The consumer devices are not probed before the supplier is bound to a driver,
> >>  and they’re unbound before the supplier is unbound."
> >>
> >> As we create the link, the CMM is the supplier of DU, so we could just
> >> drop the DL_FLAG_STATELESS flag in device_link_add() in 10/14.
> >>
> >> Does this match your understanding ?
> >
> > Except there's a bit of a chicken and egg issue, as you call
> > device_link_add() from rcar_du_cmm_init(), which thus require the DU
> > driver to probe first :-) For this to work we would probably need an
> > early initcall in the DU driver.
> 
> Yes indeed, the point where the link is created at the moment is too
> late... Is it worth an early initcall, or should we just assume that
> at the point where the LUT is operated userspace has already been
> running and both the CMM and the DU have probed already?

We should at least guard against crashes, that's why I've proposed an
init function in the CMM driver for the sole purpose of making sure the
device has been probed, and deferring probe of the DU.

-- 
Regards,

Laurent Pinchart