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

Daniel Vetter
2016-04-20T21:22:52Z

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