Message-ID: <572373EC.9010503@tronnes.org>
Date:	Fri, 29 Apr 2016 16:47:08 +0200
From:	Noralf Trønnes <noralf@...nnes.org>
To:	Tomi Valkeinen <tomi.valkeinen@...com>,
	dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org
Cc:	daniel@...ll.ch, laurent.pinchart@...asonboard.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support


Den 29.04.2016 14:50, skrev Tomi Valkeinen:
> Hi,
>
> On 28/04/16 18:18, Noralf Trønnes wrote:
>> This adds deferred io support to drm_fb_helper.
>> The fbdev framebuffer changes are flushed using the callback
>> (struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
>> ensuring that it always runs in process context.
>>
>> Signed-off-by: Noralf Trønnes <noralf@...nnes.org>
>> Reviewed-by: Daniel Vetter <daniel.vetter@...ll.ch>
>> ---
> Thanks for the series! Unfortunately I haven't been able to follow the
> discussions properly, so I hope my questions haven't been covered earlier.
>
>> Changes since v3:
>> - Don't use forward decl, move drm_fb_helper_dirty_work()
>> - Use DIV_ROUND_UP in drm_fb_helper_deferred_io()
>>
>> Changes since v2:
>> - FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
>> - The drm_clip_rect utility functions are dropped, so open code it
>> - docs: use & to denote structs
>>
>> Changes since v1:
>> - Use a dedicated worker to run the framebuffer flushing like qxl does
>> - Add parameter descriptions to drm_fb_helper_deferred_io
>>
>>   drivers/gpu/drm/Kconfig         |   1 +
>>   drivers/gpu/drm/drm_fb_helper.c | 103 +++++++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_fb_helper.h     |  15 ++++++
>>   3 files changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 9e4f2f1..8e6f34b 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -52,6 +52,7 @@ config DRM_KMS_FB_HELPER
>>   	select FB_CFB_FILLRECT
>>   	select FB_CFB_COPYAREA
>>   	select FB_CFB_IMAGEBLIT
>> +	select FB_DEFERRED_IO
>>   	help
>>   	  FBDEV helpers for KMS drivers.
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 855108e..62f849f 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -84,6 +84,15 @@ static LIST_HEAD(kernel_fb_helper_list);
>>    * and set up an initial configuration using the detected hardware, drivers
>>    * should call drm_fb_helper_single_add_all_connectors() followed by
>>    * drm_fb_helper_initial_config().
>> + *
>> + * If CONFIG_FB_DEFERRED_IO is enabled and &drm_framebuffer_funcs ->dirty is
>> + * set, the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit}
>> + * functions will accumulate changes and schedule &fb_helper .dirty_work to run
>> + * right away. This worker then calls the dirty() function ensuring that it
>> + * will always run in process context since the fb_*() function could be
>> + * running in atomic context. If drm_fb_helper_deferred_io() is used as the
> Who's calling {write,fillrect,copyarea,imageblit} in an atomic context?
> That sounds like a very bad idea to me...

I haven't verified it myself, but took it as fact based on
commit: bcb39af4486be07e896fc374a2336bad3104ae0a

drm/udl: make usage as a console safer
Okay you don't really want to use udl devices as your console, but if
you are unlucky enough to do so, you run into a lot of schedule while atomic
due to printk being called from all sorts of funky places. So check if we
are in an atomic context, and queue the damage for later, the next printk
should cause it to appear. This isn't ideal, but it is simple, and seems to
work okay in my testing here.

(dirty area idea came from xenfb)

fixes a bunch of sleeping while atomic issues running fbcon on udl devices.

Cc: stable@...r.kernel.org
Signed-off-by: Dave Airlie <airlied@...hat.com>

> If this is only for accumulating changes, I think it may be better to
> leave that to the driver as it may have better idea of how to accumulate.
>
> But, of course, this is a helper, so if all the drivers use this kind of
> accumulation, it makes sense =).

qxl already accumulates damage this way and upcoming tinydrm also.
udl has handled this damage inline except when in_atomic() which results
in damage being deferred to the next fb_*() call.

It is possible for drivers to have their own fb_*() handling and still
use drm_fb_helper_deferred_io().


For those who likes details, this is fbcon unblanking which results in a
full display update on a 320x240 display, shell with blinking cursor on
the last line of the console:

[ 5505.164150] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.164186] [drm:drm_atomic_state_init] Allocated atomic state b859e400
[...more drm atomic msgs...]
[ 5505.164713] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.164746] [drm:drm_atomic_state_init] Allocated atomic state b859e440
[...more drm atomic msgs...]
[ 5505.165086] drm_fb_helper_dirty: x=0,width=320,y=0,height=16
[ 5505.165153] drm_fb_helper_dirty: x=0,width=320,y=16,height=16
[ 5505.165220] drm_fb_helper_dirty: x=0,width=320,y=32,height=16
[ 5505.165287] drm_fb_helper_dirty: x=0,width=320,y=48,height=16
[ 5505.165354] drm_fb_helper_dirty: x=0,width=320,y=64,height=16
[ 5505.165420] drm_fb_helper_dirty: x=0,width=320,y=80,height=16
[ 5505.165487] drm_fb_helper_dirty: x=0,width=320,y=96,height=16
[ 5505.165553] drm_fb_helper_dirty: x=0,width=320,y=112,height=16
[ 5505.165619] drm_fb_helper_dirty: x=0,width=320,y=128,height=16
[ 5505.165686] drm_fb_helper_dirty: x=0,width=320,y=144,height=16
[ 5505.165752] drm_fb_helper_dirty: x=0,width=320,y=160,height=16
[ 5505.165818] drm_fb_helper_dirty: x=0,width=320,y=176,height=16
[ 5505.165884] drm_fb_helper_dirty: x=0,width=320,y=192,height=16
[ 5505.165949] drm_fb_helper_dirty: x=0,width=320,y=208,height=16
[ 5505.165978] drm_fb_helper_dirty: x=0,width=112,y=224,height=16
[ 5505.165988] drm_fb_helper_dirty: x=112,width=8,y=224,height=16
[ 5505.166002] drm_fb_helper_dirty: x=120,width=24,y=224,height=16
[ 5505.166041] drm_fb_helper_dirty: x=144,width=176,y=224,height=16
[ 5505.166058] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.166079] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.166424] drm_fb_helper_dirty_work: x1=0,x2=320,y1=0,y2=240
[ 5505.166452] adafruit-tft spi0.0: mipi_dbi_dirtyfb: vmem=bac40000, 
x1=0, x2=320, y1=0, y2=240

Cursor blinking:

[ 5505.363478] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.363509] drm_fb_helper_dirty_work: x1=152,x2=160,y1=224,y2=240
[ 5505.363539] adafruit-tft spi0.0: mipi_dbi_dirtyfb: vmem=bac40000, 
x1=152, x2=160, y1=224, y2=240

[ 5505.563488] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.563514] drm_fb_helper_dirty_work: x1=152,x2=160,y1=224,y2=240
[ 5505.563542] adafruit-tft spi0.0: mipi_dbi_dirtyfb: vmem=bac40000, 
x1=152, x2=160, y1=224, y2=240


Noralf.

Powered by blists - more mailing lists