[PATCH 2/2] drm: Add new driver for MXSFB controller

[Daniel Vetter]

On Sun, Sep 25, 2016 at 9:26 PM, Marek Vasut <marex at denx.de> wrote:
> On 08/28/2016 06:44 PM, Daniel Vetter wrote:
>> On Fri, Aug 26, 2016 at 04:27:42PM +0200, Marek Vasut wrote:
>>> +static void mxsfb_crtc_mode_set_nofb(struct drm_crtc *crtc)
>>> +{
>>> +    struct mxsfb_drm_private *mxsfb = crtc_to_mxsfb_priv(crtc);
>>> +    struct drm_display_mode *m = &crtc->state->adjusted_mode;
>>> +    const u32 bus_flags = mxsfb->output.connector.display_info.bus_flags;
>>> +    u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
>>> +    bool reenable = false;
>>> +    int err;
>>> +
>>> +    /*
>>> +     * It seems, you can't re-program the controller if it is still
>>> +     * running. This may lead to shifted pictures (FIFO issue?), so
>>> +     * first stop the controller and drain its FIFOs.
>>> +     */
>>> +    if (mxsfb->enabled) {
>>> +            reenable = true;
>>> +            mxsfb_disable_controller(mxsfb);
>>> +    }
>>
>> The atomic core should keep perfect track of the state of your controller
>> and never asky ou to re-enable it when it's already enabled. Please remove
>> this code (and the ->enabled tracking, it should be redundant).
>>
>> If this doesn't work then we have a bug in the atomic core.
>
> What this code does is it temporarily disables the controller if it was
> enabled when this function is invoked and re-enables it before exiting
> this function. This is needed for this particular controller to
> reconfigure it without odd misbehavior of the hardware itself.
>
> Is there a better way ?

Atomic helpers shouldn't ever call your ->mode_set_nofb hook when the
crtc is on. Instead they will do exactly what you're doing here
already, and first shut down the entire pipeline (crtc and encoders),
then call ->mode_set_nofb, then re-enable again.

This code shouldn't be needed at all (or there's a bug somewhere in
the helpers ..).

>>> +static void mxsfb_fb_output_poll_changed(struct drm_device *drm)
>>> +{
>>> +    struct mxsfb_drm_private *mxsfb = drm->dev_private;
>>> +
>>> +    if (mxsfb->fbdev) {
>>> +            drm_fbdev_cma_hotplug_event(mxsfb->fbdev);
>>> +    } else {
>>> +            mxsfb->fbdev = drm_fbdev_cma_init(drm, 32,
>>> +                                    drm->mode_config.num_crtc,
>>> +                                    drm->mode_config.num_connector);
>>> +            if (IS_ERR(mxsfb->fbdev))
>>> +                    mxsfb->fbdev = NULL;
>>> +    }
>>> +}
>>
>> There's patches from Thierry Redding to make delayed fbdev init supported
>> in the fbdev helpers themselves (instead of reinventing it in every driver
>> like you do here). Please help get those patches landed&reviewed, I'll
>> ping Thierry to give you the relevant pointers.
>
> It seems I won't even need this, since my output is not polled and
> doesn't change. I moved the drm_fbdev_cma_init() into the mxsfb_load()
> function.
>
>>> +
>>> +static int mxsfb_atomic_commit(struct drm_device *dev,
>>> +                           struct drm_atomic_state *state, bool nonblock)
>>> +{
>>> +    return drm_atomic_helper_commit(dev, state, false);
>>
>> Atomic helpers support async commit nowadays, no need any more for this
>> hack.
>
> I had to add the same fence check as imx drm driver, so this function
> grew in V2.

Please check out my reply to Lucas patch. tldr: The fence check should
be done in prepare_fb plane hook, and we should have one
implementation in the cma fb helper library for all drivers. Would be
great if you could create that helper (and switch imx over to it).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch