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

[Marek Vasut]

On 09/25/2016 10:29 PM, Daniel Vetter wrote:
> 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.

I see, thanks for clarifying, that's convenient. This code is dropped.

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

It is not needed indeed, I double-checked now.

>>>> +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).

Done, although what I did might be wrong. I will submit the patches once
I am a bit more confident about that. I have a question -- should the
drm simple kms helper code be augmented with a new parameter to add
this prepare_fb helper into the plane helper functions or should the
drm simple kms helper always call the prepare_fb with the fence part ?

> Cheers, Daniel
> 


-- 
Best regards,
Marek Vasut