Re: [PATCH 8/8] drm/udl: Use drm_fb_helper deferred_io support

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 20, 2016 at 9:20 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>>> @@ -330,20 +203,20 @@ static int udl_fb_open(struct fb_info *info, int
>>> user)
>>>         ufbdev->fb_count++;
>>>   -     if (fb_defio && (info->fbdefio == NULL)) {
>>> -               /* enable defio at last moment if not disabled by client
>>> */
>>> +       if (!info->fbdefio) {
>>> +               /* enable defio at last moment */
>>>                 struct fb_deferred_io *fbdefio;
>>>                 fbdefio = kmalloc(sizeof(struct fb_deferred_io),
>>> GFP_KERNEL);
>>> -
>>>                 if (fbdefio) {
>>>                         fbdefio->delay = DL_DEFIO_WRITE_DELAY;
>>> -                       fbdefio->deferred_io = udlfb_dpy_deferred_io;
>>> +                       fbdefio->deferred_io = drm_fb_helper_deferred_io;
>>
>> Why all these changes here? I figured just exchanging the deferred_io
>> pointer should be all that's really needed in the setup code?
>
>
> Because we always need to initialize deferred_io since we use it's worker
> to handle fb_{fillrect,copyarea,imageblit} damage in drm_fb_helper.
>
> The previous code didn't use deferred_io to handle these, it just handled
> the damage directly unless it was running in atomic context, in which case
> it just recorded the damage and returned, leaving it to the next call to
> push the changes.

That kind of explanation needs to be added to the commit message. I
completely missed that udl doesn't have an async work item for defio
from atomic.

> And in the following code I fixed a null pointer problem as well, maybe I
> shouldn't have packed it in here. If fbdefio allocation fails == NULL,
> fb_deferred_io_init() will trigger a BUG().

Yeah, better to split that out into a separate bugfix I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch