On 09/25/2016 10:29 PM, Daniel Vetter wrote:
> On Sun, Sep 25, 2016 at 9:26 PM, Marek Vasut <marex@xxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
