drm/fbdev-generic: fix potential out-of-bounds access

  From: Sui Jingfeng <suijingfeng@loongson.cn>

We should setting the screen buffer size according to the screen's actual
size, rather than the size of the GEM object backing the front framebuffer.
The size of GEM buffer is page size aligned, while the size of active area
of a specific screen is *NOT* necessarily page size aliged. For example,
1680x1050, 1600x900, 1440x900, 800x6000 etc. In those case, the damage rect
computed by drm_fb_helper_memory_range_to_clip() goes out of bottom bounds
of the display.

Run fbdev test of IGT on a x86+ast2400 platform with 1680x1050 resolution
will cause the system hang with the following call trace:

  Oops: 0000 [#1] PREEMPT SMP PTI
  [IGT] fbdev: starting subtest eof
  Workqueue: events drm_fb_helper_damage_work [drm_kms_helper]
  [IGT] fbdev: starting subtest nullptr

  RIP: 0010:memcpy_erms+0xa/0x20
  RSP: 0018:ffffa17d40167d98 EFLAGS: 00010246
  RAX: ffffa17d4eb7fa80 RBX: ffffa17d40e0aa80 RCX: 00000000000014c0
  RDX: 0000000000001a40 RSI: ffffa17d40e0b000 RDI: ffffa17d4eb80000
  RBP: ffffa17d40167e20 R08: 0000000000000000 R09: ffff89522ecff8c0
  R10: ffffa17d4e4c5000 R11: 0000000000000000 R12: ffffa17d4eb7fa80
  R13: 0000000000001a40 R14: 000000000000041a R15: ffffa17d40167e30
  FS:  0000000000000000(0000) GS:ffff895257380000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: ffffa17d40e0b000 CR3: 00000001eaeca006 CR4: 00000000001706e0
  Call Trace:
   <TASK>
   ? drm_fbdev_generic_helper_fb_dirty+0x207/0x330 [drm_kms_helper]
   drm_fb_helper_damage_work+0x8f/0x170 [drm_kms_helper]
   process_one_work+0x21f/0x430
   worker_thread+0x4e/0x3c0
   ? __pfx_worker_thread+0x10/0x10
   kthread+0xf4/0x120
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x2c/0x50
   </TASK>
  CR2: ffffa17d40e0b000
  ---[ end trace 0000000000000000 ]---

We also add trival code in this patch to restrict the damage rect beyond
the last line of the framebuffer.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/drm_fb_helper.c     | 2 +-
 drivers/gpu/drm/drm_fbdev_generic.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)
  

On Sun, Apr 09, 2023 at 09:21:10PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> We should setting the screen buffer size according to the screen's actual
> size, rather than the size of the GEM object backing the front framebuffer.
> The size of GEM buffer is page size aligned, while the size of active area
> of a specific screen is *NOT* necessarily page size aliged. For example,
> 1680x1050, 1600x900, 1440x900, 800x6000 etc. In those case, the damage rect
> computed by drm_fb_helper_memory_range_to_clip() goes out of bottom bounds
> of the display.
> 
> Run fbdev test of IGT on a x86+ast2400 platform with 1680x1050 resolution
> will cause the system hang with the following call trace:
> 
>   Oops: 0000 [#1] PREEMPT SMP PTI
>   [IGT] fbdev: starting subtest eof
>   Workqueue: events drm_fb_helper_damage_work [drm_kms_helper]
>   [IGT] fbdev: starting subtest nullptr
> 
>   RIP: 0010:memcpy_erms+0xa/0x20
>   RSP: 0018:ffffa17d40167d98 EFLAGS: 00010246
>   RAX: ffffa17d4eb7fa80 RBX: ffffa17d40e0aa80 RCX: 00000000000014c0
>   RDX: 0000000000001a40 RSI: ffffa17d40e0b000 RDI: ffffa17d4eb80000
>   RBP: ffffa17d40167e20 R08: 0000000000000000 R09: ffff89522ecff8c0
>   R10: ffffa17d4e4c5000 R11: 0000000000000000 R12: ffffa17d4eb7fa80
>   R13: 0000000000001a40 R14: 000000000000041a R15: ffffa17d40167e30
>   FS:  0000000000000000(0000) GS:ffff895257380000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: ffffa17d40e0b000 CR3: 00000001eaeca006 CR4: 00000000001706e0
>   Call Trace:
>    <TASK>
>    ? drm_fbdev_generic_helper_fb_dirty+0x207/0x330 [drm_kms_helper]
>    drm_fb_helper_damage_work+0x8f/0x170 [drm_kms_helper]
>    process_one_work+0x21f/0x430
>    worker_thread+0x4e/0x3c0
>    ? __pfx_worker_thread+0x10/0x10
>    kthread+0xf4/0x120
>    ? __pfx_kthread+0x10/0x10
>    ret_from_fork+0x2c/0x50
>    </TASK>
>   CR2: ffffa17d40e0b000
>   ---[ end trace 0000000000000000 ]---
> 
> We also add trival code in this patch to restrict the damage rect beyond
> the last line of the framebuffer.

Nice catch!

> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/drm_fb_helper.c     | 2 +-
>  drivers/gpu/drm/drm_fbdev_generic.c | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 64458982be40..a2b749372759 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -645,7 +645,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
>  	u32 x1 = 0;
>  	u32 y1 = off / info->fix.line_length;
>  	u32 x2 = info->var.xres;
> -	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> +	u32 y2 = min_t(u32, DIV_ROUND_UP(end, info->fix.line_length), info->var.yres);

So for additional robustness I think it'd be good if we change the entire
computation here to use drm_framebuffer data and not fb_info data, because
fundamentally that's what the drm kms code consumes. It should all match
anyway, but I think it makes the code more obviously correct.

So in the entire function instead of looking at fb_info->fix we should
probably look at

	struct drm_fb_helper *helper = info->par;

And then helper->fb->pitches[0] and helper->fb->height.

If you agree would be great if you can please respin with that (and the
commit message augmented to explain why we do the change)?
>  
>  	if ((y2 - y1) == 1) {
>  		/*
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index 8e5148bf40bb..a6daecb5f640 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -95,6 +95,8 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
>  	fb_helper->fb = buffer->fb;
>  
>  	screen_size = buffer->gem->size;

I guess you forgot to remove this line here? Also I'm not understanding
why this matters, I think you're fix only needs the above chunk, not this
one? If I got this right then please drop this part, there's drivers which
only use drm_fb_helper.c but not drm_fbdev_generic.c, and from what I can
tell they all still set the gem buffer size here.

If otoh we need this too, then there's a few more places that need to be
fixed.

> +	screen_size = sizes->surface_height * buffer->fb->pitches[0];
> +
>  	screen_buffer = vzalloc(screen_size);
>  	if (!screen_buffer) {
>  		ret = -ENOMEM;

Cheers, Daniel

> -- 
> 2.25.1
>
  

Hi,

On 2023/4/11 22:53, Daniel Vetter wrote:
> On Sun, Apr 09, 2023 at 09:21:10PM +0800, Sui Jingfeng wrote:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> We should setting the screen buffer size according to the screen's actual
>> size, rather than the size of the GEM object backing the front framebuffer.
>> The size of GEM buffer is page size aligned, while the size of active area
>> of a specific screen is *NOT* necessarily page size aliged. For example,
>> 1680x1050, 1600x900, 1440x900, 800x6000 etc. In those case, the damage rect
>> computed by drm_fb_helper_memory_range_to_clip() goes out of bottom bounds
>> of the display.
>>
>> Run fbdev test of IGT on a x86+ast2400 platform with 1680x1050 resolution
>> will cause the system hang with the following call trace:
>>
>>    Oops: 0000 [#1] PREEMPT SMP PTI
>>    [IGT] fbdev: starting subtest eof
>>    Workqueue: events drm_fb_helper_damage_work [drm_kms_helper]
>>    [IGT] fbdev: starting subtest nullptr
>>
>>    RIP: 0010:memcpy_erms+0xa/0x20
>>    RSP: 0018:ffffa17d40167d98 EFLAGS: 00010246
>>    RAX: ffffa17d4eb7fa80 RBX: ffffa17d40e0aa80 RCX: 00000000000014c0
>>    RDX: 0000000000001a40 RSI: ffffa17d40e0b000 RDI: ffffa17d4eb80000
>>    RBP: ffffa17d40167e20 R08: 0000000000000000 R09: ffff89522ecff8c0
>>    R10: ffffa17d4e4c5000 R11: 0000000000000000 R12: ffffa17d4eb7fa80
>>    R13: 0000000000001a40 R14: 000000000000041a R15: ffffa17d40167e30
>>    FS:  0000000000000000(0000) GS:ffff895257380000(0000) knlGS:0000000000000000
>>    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>    CR2: ffffa17d40e0b000 CR3: 00000001eaeca006 CR4: 00000000001706e0
>>    Call Trace:
>>     <TASK>
>>     ? drm_fbdev_generic_helper_fb_dirty+0x207/0x330 [drm_kms_helper]
>>     drm_fb_helper_damage_work+0x8f/0x170 [drm_kms_helper]
>>     process_one_work+0x21f/0x430
>>     worker_thread+0x4e/0x3c0
>>     ? __pfx_worker_thread+0x10/0x10
>>     kthread+0xf4/0x120
>>     ? __pfx_kthread+0x10/0x10
>>     ret_from_fork+0x2c/0x50
>>     </TASK>
>>    CR2: ffffa17d40e0b000
>>    ---[ end trace 0000000000000000 ]---
>>
>> We also add trival code in this patch to restrict the damage rect beyond
>> the last line of the framebuffer.
> Nice catch!
  :)
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c     | 2 +-
>>   drivers/gpu/drm/drm_fbdev_generic.c | 2 ++
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 64458982be40..a2b749372759 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -645,7 +645,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
>>   	u32 x1 = 0;
>>   	u32 y1 = off / info->fix.line_length;
>>   	u32 x2 = info->var.xres;
>> -	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
>> +	u32 y2 = min_t(u32, DIV_ROUND_UP(end, info->fix.line_length), info->var.yres);
> So for additional robustness I think it'd be good if we change the entire
> computation here to use drm_framebuffer data and not fb_info data, because
> fundamentally that's what the drm kms code consumes. It should all match
> anyway, but I think it makes the code more obviously correct.
>
> So in the entire function instead of looking at fb_info->fix we should
> probably look at
>
> 	struct drm_fb_helper *helper = info->par;
>
> And then helper->fb->pitches[0] and helper->fb->height.
>
> If you agree would be great if you can please respin with that (and the
> commit message augmented to explain why we do the change)?

Yes, I'm agree.

Thank you for guidance, I will refine this patch with `helper = info->par`.

I will send a v2 when I finished.

>>   
>>   	if ((y2 - y1) == 1) {
>>   		/*
>> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
>> index 8e5148bf40bb..a6daecb5f640 100644
>> --- a/drivers/gpu/drm/drm_fbdev_generic.c
>> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
>> @@ -95,6 +95,8 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
>>   	fb_helper->fb = buffer->fb;
>>   
>>   	screen_size = buffer->gem->size;
> I guess you forgot to remove this line here?

Yes, this line should be removed in this patch. I overlooked this, sorry.

> Also I'm not understanding
> why this matters, I think you're fix only needs the above chunk, not this
> one? If I got this right then please drop this part, there's drivers which
> only use drm_fb_helper.c but not drm_fbdev_generic.c, and from what I can
> tell they all still set the gem buffer size here.
>
> If otoh we need this too, then there's a few more places that need to be
> fixed.

I think we need this line, otherwise wrapped around will be happen.

Because I found that the value of variable`y1` will be larger in number 
than the variable `y2` by 1,

which are computed  in drm_fb_helper_memory_range_to_clip().


This phenomenon will emerged on platforms with large page size or

non page size divisiable display resolution case. Take the LoongArch and 
Mips as an example,

the default page size is 16KB(to avoid cache alias).  Even with the most 
frequently used

1920x1080 screen, the screen_size can not be divided exactly.

The total size of the shadow buffer is 1920x1080x4 bytes, 1920x1080x4 / 
16384 = 506.25

TTM manage the vram in the term of pages, so TTM will allocate 507 pages 
for us.

507x16384 = 8306688 bytes.


drm_fb_helper_memory_range_to_clip() will be called when running fbdev 
eof test in the IGT.

with 8306688 as its second parameter. while 8306688 / (1920x4) = 1081, 
this cause y1 out of bound.

Simply restrict y2 with a min_t() function yeild 1080 in this case, but 
y2 - y1 cause *wrap around* here.

because they are both unsigned number.


drm_rect_init() function cast this unsigned int type to int type in end 
of drm_fb_helper_memory_range_to_clip(),

but the last argument of drm_fb_helper_damage() function is a u32 type,

it cast the return value of  drm_rect_height(&damage_area) back to 
unsigned type.

Yet, another wrapped around with truncation happened in 
drm_fb_helper_add_damage_clip()

called by subsequent drm_fb_helper_damage() function.

I finally got reject by drm_fbdev_generic_helper_fb_dirty() with follow 
code:

```

     /* Call damage handlers only if necessary */
     if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
         return 0;

```

On x86-64 platform, because 1920x1080x4 dumb buffer is lucky, it be 
divided exactly by 4KB(page size).

But other resolution will not as luck as this one. Right, fbdev test 
will be pasted, but wrap around

happens many time.

Therefore, as long as a larger buffer is allowed to exposed to the 
user-space.

A chance is given to the user-space,  to go beyond of the bottom bound 
of the actual active display area.

I not sure if this is intended, I feel it should not be allowable by 
intuition.

>> +	screen_size = sizes->surface_height * buffer->fb->pitches[0];
>> +
>>   	screen_buffer = vzalloc(screen_size);
>>   	if (!screen_buffer) {
>>   		ret = -ENOMEM;
> Cheers, Daniel
>
>> -- 
>> 2.25.1
>>
  

On 2023/4/13 01:13, Sui Jingfeng wrote:
> I finally got reject by drm_fbdev_generic_helper_fb_dirty() with 
> follow code: 


It finally got rejected by drm_fbdev_generic_helper_fb_dirty() function 
with follow code:
  

On Thu, Apr 13, 2023 at 01:13:37AM +0800, Sui Jingfeng wrote:
> Hi,
> 
> On 2023/4/11 22:53, Daniel Vetter wrote:
> > On Sun, Apr 09, 2023 at 09:21:10PM +0800, Sui Jingfeng wrote:
> > > From: Sui Jingfeng <suijingfeng@loongson.cn>
> > > 
> > > We should setting the screen buffer size according to the screen's actual
> > > size, rather than the size of the GEM object backing the front framebuffer.
> > > The size of GEM buffer is page size aligned, while the size of active area
> > > of a specific screen is *NOT* necessarily page size aliged. For example,
> > > 1680x1050, 1600x900, 1440x900, 800x6000 etc. In those case, the damage rect
> > > computed by drm_fb_helper_memory_range_to_clip() goes out of bottom bounds
> > > of the display.
> > > 
> > > Run fbdev test of IGT on a x86+ast2400 platform with 1680x1050 resolution
> > > will cause the system hang with the following call trace:
> > > 
> > >    Oops: 0000 [#1] PREEMPT SMP PTI
> > >    [IGT] fbdev: starting subtest eof
> > >    Workqueue: events drm_fb_helper_damage_work [drm_kms_helper]
> > >    [IGT] fbdev: starting subtest nullptr
> > > 
> > >    RIP: 0010:memcpy_erms+0xa/0x20
> > >    RSP: 0018:ffffa17d40167d98 EFLAGS: 00010246
> > >    RAX: ffffa17d4eb7fa80 RBX: ffffa17d40e0aa80 RCX: 00000000000014c0
> > >    RDX: 0000000000001a40 RSI: ffffa17d40e0b000 RDI: ffffa17d4eb80000
> > >    RBP: ffffa17d40167e20 R08: 0000000000000000 R09: ffff89522ecff8c0
> > >    R10: ffffa17d4e4c5000 R11: 0000000000000000 R12: ffffa17d4eb7fa80
> > >    R13: 0000000000001a40 R14: 000000000000041a R15: ffffa17d40167e30
> > >    FS:  0000000000000000(0000) GS:ffff895257380000(0000) knlGS:0000000000000000
> > >    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >    CR2: ffffa17d40e0b000 CR3: 00000001eaeca006 CR4: 00000000001706e0
> > >    Call Trace:
> > >     <TASK>
> > >     ? drm_fbdev_generic_helper_fb_dirty+0x207/0x330 [drm_kms_helper]
> > >     drm_fb_helper_damage_work+0x8f/0x170 [drm_kms_helper]
> > >     process_one_work+0x21f/0x430
> > >     worker_thread+0x4e/0x3c0
> > >     ? __pfx_worker_thread+0x10/0x10
> > >     kthread+0xf4/0x120
> > >     ? __pfx_kthread+0x10/0x10
> > >     ret_from_fork+0x2c/0x50
> > >     </TASK>
> > >    CR2: ffffa17d40e0b000
> > >    ---[ end trace 0000000000000000 ]---
> > > 
> > > We also add trival code in this patch to restrict the damage rect beyond
> > > the last line of the framebuffer.
> > Nice catch!
>  :)
> > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> > > ---
> > >   drivers/gpu/drm/drm_fb_helper.c     | 2 +-
> > >   drivers/gpu/drm/drm_fbdev_generic.c | 2 ++
> > >   2 files changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 64458982be40..a2b749372759 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -645,7 +645,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
> > >   	u32 x1 = 0;
> > >   	u32 y1 = off / info->fix.line_length;
> > >   	u32 x2 = info->var.xres;
> > > -	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> > > +	u32 y2 = min_t(u32, DIV_ROUND_UP(end, info->fix.line_length), info->var.yres);
> > So for additional robustness I think it'd be good if we change the entire
> > computation here to use drm_framebuffer data and not fb_info data, because
> > fundamentally that's what the drm kms code consumes. It should all match
> > anyway, but I think it makes the code more obviously correct.
> > 
> > So in the entire function instead of looking at fb_info->fix we should
> > probably look at
> > 
> > 	struct drm_fb_helper *helper = info->par;
> > 
> > And then helper->fb->pitches[0] and helper->fb->height.
> > 
> > If you agree would be great if you can please respin with that (and the
> > commit message augmented to explain why we do the change)?
> 
> Yes, I'm agree.
> 
> Thank you for guidance, I will refine this patch with `helper = info->par`.
> 
> I will send a v2 when I finished.
> 
> > >   	if ((y2 - y1) == 1) {
> > >   		/*
> > > diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> > > index 8e5148bf40bb..a6daecb5f640 100644
> > > --- a/drivers/gpu/drm/drm_fbdev_generic.c
> > > +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> > > @@ -95,6 +95,8 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
> > >   	fb_helper->fb = buffer->fb;
> > >   	screen_size = buffer->gem->size;
> > I guess you forgot to remove this line here?
> 
> Yes, this line should be removed in this patch. I overlooked this, sorry.
> 
> > Also I'm not understanding
> > why this matters, I think you're fix only needs the above chunk, not this
> > one? If I got this right then please drop this part, there's drivers which
> > only use drm_fb_helper.c but not drm_fbdev_generic.c, and from what I can
> > tell they all still set the gem buffer size here.
> > 
> > If otoh we need this too, then there's a few more places that need to be
> > fixed.
> 
> I think we need this line, otherwise wrapped around will be happen.
> 
> Because I found that the value of variable`y1` will be larger in number than
> the variable `y2` by 1,
> 
> which are computed  in drm_fb_helper_memory_range_to_clip().
> 
> 
> This phenomenon will emerged on platforms with large page size or
> 
> non page size divisiable display resolution case. Take the LoongArch and
> Mips as an example,
> 
> the default page size is 16KB(to avoid cache alias).  Even with the most
> frequently used
> 
> 1920x1080 screen, the screen_size can not be divided exactly.
> 
> The total size of the shadow buffer is 1920x1080x4 bytes, 1920x1080x4 /
> 16384 = 506.25
> 
> TTM manage the vram in the term of pages, so TTM will allocate 507 pages for
> us.
> 
> 507x16384 = 8306688 bytes.
> 
> 
> drm_fb_helper_memory_range_to_clip() will be called when running fbdev eof
> test in the IGT.
> 
> with 8306688 as its second parameter. while 8306688 / (1920x4) = 1081, this
> cause y1 out of bound.
> 
> Simply restrict y2 with a min_t() function yeild 1080 in this case, but y2 -
> y1 cause *wrap around* here.
> 
> because they are both unsigned number.
> 
> 
> drm_rect_init() function cast this unsigned int type to int type in end of
> drm_fb_helper_memory_range_to_clip(),
> 
> but the last argument of drm_fb_helper_damage() function is a u32 type,
> 
> it cast the return value of  drm_rect_height(&damage_area) back to unsigned
> type.
> 
> Yet, another wrapped around with truncation happened in
> drm_fb_helper_add_damage_clip()
> 
> called by subsequent drm_fb_helper_damage() function.
> 
> I finally got reject by drm_fbdev_generic_helper_fb_dirty() with follow
> code:
> 
> ```
> 
>     /* Call damage handlers only if necessary */
>     if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>         return 0;
> 
> ```
> 
> On x86-64 platform, because 1920x1080x4 dumb buffer is lucky, it be divided
> exactly by 4KB(page size).
> 
> But other resolution will not as luck as this one. Right, fbdev test will be
> pasted, but wrap around
> 
> happens many time.
> 
> Therefore, as long as a larger buffer is allowed to exposed to the
> user-space.
> 
> A chance is given to the user-space,  to go beyond of the bottom bound of
> the actual active display area.
> 
> I not sure if this is intended, I feel it should not be allowable by
> intuition.

Ah yes, thanks for the in-depth explanation. But I think we need a
different fix, by also limiting y1. Otherwise for really big page sizes
(64k on arm64 iirc) and really small screens (there's i2c panels with just
a few lines) we might still run into the issue of y1 being too large.

So we need to limit both y1 and y2. I think it's ok to let y1 == y2 slip
through, since as you point out that's filtered later on.

The userspace api is that we should expose the full fbdev buffer and allow
writes into the entire thing. It's just that for the explicit upload with
damage rects we need to make sure we're staying within the real buffer.
-Daniel

> > > +	screen_size = sizes->surface_height * buffer->fb->pitches[0];
> > > +
> > >   	screen_buffer = vzalloc(screen_size);
> > >   	if (!screen_buffer) {
> > >   		ret = -ENOMEM;
> > Cheers, Daniel
> > 
> > > -- 
> > > 2.25.1
> > >
  

Hi,

On 2023/4/13 01:44, Daniel Vetter wrote:
> On Thu, Apr 13, 2023 at 01:13:37AM +0800, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2023/4/11 22:53, Daniel Vetter wrote:
>>> On Sun, Apr 09, 2023 at 09:21:10PM +0800, Sui Jingfeng wrote:
>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>
>>>> We should setting the screen buffer size according to the screen's actual
>>>> size, rather than the size of the GEM object backing the front framebuffer.
>>>> The size of GEM buffer is page size aligned, while the size of active area
>>>> of a specific screen is *NOT* necessarily page size aliged. For example,
>>>> 1680x1050, 1600x900, 1440x900, 800x6000 etc. In those case, the damage rect
>>>> computed by drm_fb_helper_memory_range_to_clip() goes out of bottom bounds
>>>> of the display.
>>>>
>>>> Run fbdev test of IGT on a x86+ast2400 platform with 1680x1050 resolution
>>>> will cause the system hang with the following call trace:
>>>>
>>>>     Oops: 0000 [#1] PREEMPT SMP PTI
>>>>     [IGT] fbdev: starting subtest eof
>>>>     Workqueue: events drm_fb_helper_damage_work [drm_kms_helper]
>>>>     [IGT] fbdev: starting subtest nullptr
>>>>
>>>>     RIP: 0010:memcpy_erms+0xa/0x20
>>>>     RSP: 0018:ffffa17d40167d98 EFLAGS: 00010246
>>>>     RAX: ffffa17d4eb7fa80 RBX: ffffa17d40e0aa80 RCX: 00000000000014c0
>>>>     RDX: 0000000000001a40 RSI: ffffa17d40e0b000 RDI: ffffa17d4eb80000
>>>>     RBP: ffffa17d40167e20 R08: 0000000000000000 R09: ffff89522ecff8c0
>>>>     R10: ffffa17d4e4c5000 R11: 0000000000000000 R12: ffffa17d4eb7fa80
>>>>     R13: 0000000000001a40 R14: 000000000000041a R15: ffffa17d40167e30
>>>>     FS:  0000000000000000(0000) GS:ffff895257380000(0000) knlGS:0000000000000000
>>>>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>     CR2: ffffa17d40e0b000 CR3: 00000001eaeca006 CR4: 00000000001706e0
>>>>     Call Trace:
>>>>      <TASK>
>>>>      ? drm_fbdev_generic_helper_fb_dirty+0x207/0x330 [drm_kms_helper]
>>>>      drm_fb_helper_damage_work+0x8f/0x170 [drm_kms_helper]
>>>>      process_one_work+0x21f/0x430
>>>>      worker_thread+0x4e/0x3c0
>>>>      ? __pfx_worker_thread+0x10/0x10
>>>>      kthread+0xf4/0x120
>>>>      ? __pfx_kthread+0x10/0x10
>>>>      ret_from_fork+0x2c/0x50
>>>>      </TASK>
>>>>     CR2: ffffa17d40e0b000
>>>>     ---[ end trace 0000000000000000 ]---
>>>>
>>>> We also add trival code in this patch to restrict the damage rect beyond
>>>> the last line of the framebuffer.
>>> Nice catch!
>>   :)
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>>    drivers/gpu/drm/drm_fb_helper.c     | 2 +-
>>>>    drivers/gpu/drm/drm_fbdev_generic.c | 2 ++
>>>>    2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>> index 64458982be40..a2b749372759 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -645,7 +645,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
>>>>    	u32 x1 = 0;
>>>>    	u32 y1 = off / info->fix.line_length;
>>>>    	u32 x2 = info->var.xres;
>>>> -	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
>>>> +	u32 y2 = min_t(u32, DIV_ROUND_UP(end, info->fix.line_length), info->var.yres);
>>> So for additional robustness I think it'd be good if we change the entire
>>> computation here to use drm_framebuffer data and not fb_info data, because
>>> fundamentally that's what the drm kms code consumes. It should all match
>>> anyway, but I think it makes the code more obviously correct.
>>>
>>> So in the entire function instead of looking at fb_info->fix we should
>>> probably look at
>>>
>>> 	struct drm_fb_helper *helper = info->par;
>>>
>>> And then helper->fb->pitches[0] and helper->fb->height.
>>>
>>> If you agree would be great if you can please respin with that (and the
>>> commit message augmented to explain why we do the change)?
>> Yes, I'm agree.
>>
>> Thank you for guidance, I will refine this patch with `helper = info->par`.
>>
>> I will send a v2 when I finished.
>>
>>>>    	if ((y2 - y1) == 1) {
>>>>    		/*
>>>> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
>>>> index 8e5148bf40bb..a6daecb5f640 100644
>>>> --- a/drivers/gpu/drm/drm_fbdev_generic.c
>>>> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
>>>> @@ -95,6 +95,8 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
>>>>    	fb_helper->fb = buffer->fb;
>>>>    	screen_size = buffer->gem->size;
>>> I guess you forgot to remove this line here?
>> Yes, this line should be removed in this patch. I overlooked this, sorry.
>>
>>> Also I'm not understanding
>>> why this matters, I think you're fix only needs the above chunk, not this
>>> one? If I got this right then please drop this part, there's drivers which
>>> only use drm_fb_helper.c but not drm_fbdev_generic.c, and from what I can
>>> tell they all still set the gem buffer size here.
>>>
>>> If otoh we need this too, then there's a few more places that need to be
>>> fixed.
>> I think we need this line, otherwise wrapped around will be happen.
>>
>> Because I found that the value of variable`y1` will be larger in number than
>> the variable `y2` by 1,
>>
>> which are computed  in drm_fb_helper_memory_range_to_clip().
>>
>>
>> This phenomenon will emerged on platforms with large page size or
>>
>> non page size divisiable display resolution case. Take the LoongArch and
>> Mips as an example,
>>
>> the default page size is 16KB(to avoid cache alias).  Even with the most
>> frequently used
>>
>> 1920x1080 screen, the screen_size can not be divided exactly.
>>
>> The total size of the shadow buffer is 1920x1080x4 bytes, 1920x1080x4 /
>> 16384 = 506.25
>>
>> TTM manage the vram in the term of pages, so TTM will allocate 507 pages for
>> us.
>>
>> 507x16384 = 8306688 bytes.
>>
>>
>> drm_fb_helper_memory_range_to_clip() will be called when running fbdev eof
>> test in the IGT.
>>
>> with 8306688 as its second parameter. while 8306688 / (1920x4) = 1081, this
>> cause y1 out of bound.
>>
>> Simply restrict y2 with a min_t() function yeild 1080 in this case, but y2 -
>> y1 cause *wrap around* here.
>>
>> because they are both unsigned number.
>>
>>
>> drm_rect_init() function cast this unsigned int type to int type in end of
>> drm_fb_helper_memory_range_to_clip(),
>>
>> but the last argument of drm_fb_helper_damage() function is a u32 type,
>>
>> it cast the return value of  drm_rect_height(&damage_area) back to unsigned
>> type.
>>
>> Yet, another wrapped around with truncation happened in
>> drm_fb_helper_add_damage_clip()
>>
>> called by subsequent drm_fb_helper_damage() function.
>>
>> I finally got reject by drm_fbdev_generic_helper_fb_dirty() with follow
>> code:
>>
>> ```
>>
>>      /* Call damage handlers only if necessary */
>>      if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>>          return 0;
>>
>> ```
>>
>> On x86-64 platform, because 1920x1080x4 dumb buffer is lucky, it be divided
>> exactly by 4KB(page size).
>>
>> But other resolution will not as luck as this one. Right, fbdev test will be
>> pasted, but wrap around
>>
>> happens many time.
>>
>> Therefore, as long as a larger buffer is allowed to exposed to the
>> user-space.
>>
>> A chance is given to the user-space,  to go beyond of the bottom bound of
>> the actual active display area.
>>
>> I not sure if this is intended, I feel it should not be allowable by
>> intuition.
> Ah yes, thanks for the in-depth explanation. But I think we need a
> different fix, by also limiting y1. Otherwise for really big page sizes
> (64k on arm64 iirc) and really small screens (there's i2c panels with just
> a few lines) we might still run into the issue of y1 being too large.
>
> So we need to limit both y1 and y2. I think it's ok to let y1 == y2 slip
> through, since as you point out that's filtered later on.
>
> The userspace api is that we should expose the full fbdev buffer and allow
> writes into the entire thing. It's just that for the explicit upload with
> damage rects we need to make sure we're staying within the real buffer.
> -Daniel
>
Limiting y1 is easy, and this is necessary, because it is the crazy 
fbdev test of IGT writing after EOF intentionally.

But there some difficulties for me to avoid using info->fix and info->var ,

I found all other functions are surrounding the info->fix and info-var.

There seems no good variable to replace info->var related data structure.

Partially replacement may introduce confusion, this somewhat beyond my 
ability.

I'm afraid of introducing out-of-bound in horizontal direction for 
multi-screen case.

Using fb_info->fix is still more safe.

Can I  respin my patch by still using fb_info->fix here?

>>>> +	screen_size = sizes->surface_height * buffer->fb->pitches[0];
>>>> +
>>>>    	screen_buffer = vzalloc(screen_size);
>>>>    	if (!screen_buffer) {
>>>>    		ret = -ENOMEM;
>>> Cheers, Daniel
>>>
>>>> -- 
>>>> 2.25.1
>>>>
  

On Thu, 13 Apr 2023 at 17:35, Sui Jingfeng <15330273260@189.cn> wrote:
>
> Hi,
>
> On 2023/4/13 01:44, Daniel Vetter wrote:
> > On Thu, Apr 13, 2023 at 01:13:37AM +0800, Sui Jingfeng wrote:
> >> Hi,
> >>
> >> On 2023/4/11 22:53, Daniel Vetter wrote:
> >>> On Sun, Apr 09, 2023 at 09:21:10PM +0800, Sui Jingfeng wrote:
> >>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>>
> >>>> We should setting the screen buffer size according to the screen's actual
> >>>> size, rather than the size of the GEM object backing the front framebuffer.
> >>>> The size of GEM buffer is page size aligned, while the size of active area
> >>>> of a specific screen is *NOT* necessarily page size aliged. For example,
> >>>> 1680x1050, 1600x900, 1440x900, 800x6000 etc. In those case, the damage rect
> >>>> computed by drm_fb_helper_memory_range_to_clip() goes out of bottom bounds
> >>>> of the display.
> >>>>
> >>>> Run fbdev test of IGT on a x86+ast2400 platform with 1680x1050 resolution
> >>>> will cause the system hang with the following call trace:
> >>>>
> >>>>     Oops: 0000 [#1] PREEMPT SMP PTI
> >>>>     [IGT] fbdev: starting subtest eof
> >>>>     Workqueue: events drm_fb_helper_damage_work [drm_kms_helper]
> >>>>     [IGT] fbdev: starting subtest nullptr
> >>>>
> >>>>     RIP: 0010:memcpy_erms+0xa/0x20
> >>>>     RSP: 0018:ffffa17d40167d98 EFLAGS: 00010246
> >>>>     RAX: ffffa17d4eb7fa80 RBX: ffffa17d40e0aa80 RCX: 00000000000014c0
> >>>>     RDX: 0000000000001a40 RSI: ffffa17d40e0b000 RDI: ffffa17d4eb80000
> >>>>     RBP: ffffa17d40167e20 R08: 0000000000000000 R09: ffff89522ecff8c0
> >>>>     R10: ffffa17d4e4c5000 R11: 0000000000000000 R12: ffffa17d4eb7fa80
> >>>>     R13: 0000000000001a40 R14: 000000000000041a R15: ffffa17d40167e30
> >>>>     FS:  0000000000000000(0000) GS:ffff895257380000(0000) knlGS:0000000000000000
> >>>>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>>>     CR2: ffffa17d40e0b000 CR3: 00000001eaeca006 CR4: 00000000001706e0
> >>>>     Call Trace:
> >>>>      <TASK>
> >>>>      ? drm_fbdev_generic_helper_fb_dirty+0x207/0x330 [drm_kms_helper]
> >>>>      drm_fb_helper_damage_work+0x8f/0x170 [drm_kms_helper]
> >>>>      process_one_work+0x21f/0x430
> >>>>      worker_thread+0x4e/0x3c0
> >>>>      ? __pfx_worker_thread+0x10/0x10
> >>>>      kthread+0xf4/0x120
> >>>>      ? __pfx_kthread+0x10/0x10
> >>>>      ret_from_fork+0x2c/0x50
> >>>>      </TASK>
> >>>>     CR2: ffffa17d40e0b000
> >>>>     ---[ end trace 0000000000000000 ]---
> >>>>
> >>>> We also add trival code in this patch to restrict the damage rect beyond
> >>>> the last line of the framebuffer.
> >>> Nice catch!
> >>   :)
> >>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>> ---
> >>>>    drivers/gpu/drm/drm_fb_helper.c     | 2 +-
> >>>>    drivers/gpu/drm/drm_fbdev_generic.c | 2 ++
> >>>>    2 files changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>>> index 64458982be40..a2b749372759 100644
> >>>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>>> @@ -645,7 +645,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
> >>>>            u32 x1 = 0;
> >>>>            u32 y1 = off / info->fix.line_length;
> >>>>            u32 x2 = info->var.xres;
> >>>> -  u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> >>>> +  u32 y2 = min_t(u32, DIV_ROUND_UP(end, info->fix.line_length), info->var.yres);
> >>> So for additional robustness I think it'd be good if we change the entire
> >>> computation here to use drm_framebuffer data and not fb_info data, because
> >>> fundamentally that's what the drm kms code consumes. It should all match
> >>> anyway, but I think it makes the code more obviously correct.
> >>>
> >>> So in the entire function instead of looking at fb_info->fix we should
> >>> probably look at
> >>>
> >>>     struct drm_fb_helper *helper = info->par;
> >>>
> >>> And then helper->fb->pitches[0] and helper->fb->height.
> >>>
> >>> If you agree would be great if you can please respin with that (and the
> >>> commit message augmented to explain why we do the change)?
> >> Yes, I'm agree.
> >>
> >> Thank you for guidance, I will refine this patch with `helper = info->par`.
> >>
> >> I will send a v2 when I finished.
> >>
> >>>>            if ((y2 - y1) == 1) {
> >>>>                    /*
> >>>> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> >>>> index 8e5148bf40bb..a6daecb5f640 100644
> >>>> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> >>>> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> >>>> @@ -95,6 +95,8 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
> >>>>            fb_helper->fb = buffer->fb;
> >>>>            screen_size = buffer->gem->size;
> >>> I guess you forgot to remove this line here?
> >> Yes, this line should be removed in this patch. I overlooked this, sorry.
> >>
> >>> Also I'm not understanding
> >>> why this matters, I think you're fix only needs the above chunk, not this
> >>> one? If I got this right then please drop this part, there's drivers which
> >>> only use drm_fb_helper.c but not drm_fbdev_generic.c, and from what I can
> >>> tell they all still set the gem buffer size here.
> >>>
> >>> If otoh we need this too, then there's a few more places that need to be
> >>> fixed.
> >> I think we need this line, otherwise wrapped around will be happen.
> >>
> >> Because I found that the value of variable`y1` will be larger in number than
> >> the variable `y2` by 1,
> >>
> >> which are computed  in drm_fb_helper_memory_range_to_clip().
> >>
> >>
> >> This phenomenon will emerged on platforms with large page size or
> >>
> >> non page size divisiable display resolution case. Take the LoongArch and
> >> Mips as an example,
> >>
> >> the default page size is 16KB(to avoid cache alias).  Even with the most
> >> frequently used
> >>
> >> 1920x1080 screen, the screen_size can not be divided exactly.
> >>
> >> The total size of the shadow buffer is 1920x1080x4 bytes, 1920x1080x4 /
> >> 16384 = 506.25
> >>
> >> TTM manage the vram in the term of pages, so TTM will allocate 507 pages for
> >> us.
> >>
> >> 507x16384 = 8306688 bytes.
> >>
> >>
> >> drm_fb_helper_memory_range_to_clip() will be called when running fbdev eof
> >> test in the IGT.
> >>
> >> with 8306688 as its second parameter. while 8306688 / (1920x4) = 1081, this
> >> cause y1 out of bound.
> >>
> >> Simply restrict y2 with a min_t() function yeild 1080 in this case, but y2 -
> >> y1 cause *wrap around* here.
> >>
> >> because they are both unsigned number.
> >>
> >>
> >> drm_rect_init() function cast this unsigned int type to int type in end of
> >> drm_fb_helper_memory_range_to_clip(),
> >>
> >> but the last argument of drm_fb_helper_damage() function is a u32 type,
> >>
> >> it cast the return value of  drm_rect_height(&damage_area) back to unsigned
> >> type.
> >>
> >> Yet, another wrapped around with truncation happened in
> >> drm_fb_helper_add_damage_clip()
> >>
> >> called by subsequent drm_fb_helper_damage() function.
> >>
> >> I finally got reject by drm_fbdev_generic_helper_fb_dirty() with follow
> >> code:
> >>
> >> ```
> >>
> >>      /* Call damage handlers only if necessary */
> >>      if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
> >>          return 0;
> >>
> >> ```
> >>
> >> On x86-64 platform, because 1920x1080x4 dumb buffer is lucky, it be divided
> >> exactly by 4KB(page size).
> >>
> >> But other resolution will not as luck as this one. Right, fbdev test will be
> >> pasted, but wrap around
> >>
> >> happens many time.
> >>
> >> Therefore, as long as a larger buffer is allowed to exposed to the
> >> user-space.
> >>
> >> A chance is given to the user-space,  to go beyond of the bottom bound of
> >> the actual active display area.
> >>
> >> I not sure if this is intended, I feel it should not be allowable by
> >> intuition.
> > Ah yes, thanks for the in-depth explanation. But I think we need a
> > different fix, by also limiting y1. Otherwise for really big page sizes
> > (64k on arm64 iirc) and really small screens (there's i2c panels with just
> > a few lines) we might still run into the issue of y1 being too large.
> >
> > So we need to limit both y1 and y2. I think it's ok to let y1 == y2 slip
> > through, since as you point out that's filtered later on.
> >
> > The userspace api is that we should expose the full fbdev buffer and allow
> > writes into the entire thing. It's just that for the explicit upload with
> > damage rects we need to make sure we're staying within the real buffer.
> > -Daniel
> >
> Limiting y1 is easy, and this is necessary, because it is the crazy
> fbdev test of IGT writing after EOF intentionally.
>
> But there some difficulties for me to avoid using info->fix and info->var ,
>
> I found all other functions are surrounding the info->fix and info-var.
>
> There seems no good variable to replace info->var related data structure.
>
> Partially replacement may introduce confusion, this somewhat beyond my
> ability.
>
> I'm afraid of introducing out-of-bound in horizontal direction for
> multi-screen case.
>
> Using fb_info->fix is still more safe.
>
> Can I  respin my patch by still using fb_info->fix here?

Which one do you have an issue with finding the right drm variable? I
can help with that.
-Daniel

> >>>> +  screen_size = sizes->surface_height * buffer->fb->pitches[0];
> >>>> +
> >>>>            screen_buffer = vzalloc(screen_size);
> >>>>            if (!screen_buffer) {
> >>>>                    ret = -ENOMEM;
> >>> Cheers, Daniel
> >>>
> >>>> --
> >>>> 2.25.1
> >>>>
  

On 2023/4/13 23:56, Daniel Vetter wrote:
> On Thu, 13 Apr 2023 at 17:35, Sui Jingfeng <15330273260@189.cn> wrote:
>> Hi,
>>
>> On 2023/4/13 01:44, Daniel Vetter wrote:
>>> On Thu, Apr 13, 2023 at 01:13:37AM +0800, Sui Jingfeng wrote:
>>>> Hi,
>>>>
>>>> On 2023/4/11 22:53, Daniel Vetter wrote:
>>>>> On Sun, Apr 09, 2023 at 09:21:10PM +0800, Sui Jingfeng wrote:
>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>
>>>>>> We should setting the screen buffer size according to the screen's actual
>>>>>> size, rather than the size of the GEM object backing the front framebuffer.
>>>>>> The size of GEM buffer is page size aligned, while the size of active area
>>>>>> of a specific screen is *NOT* necessarily page size aliged. For example,
>>>>>> 1680x1050, 1600x900, 1440x900, 800x6000 etc. In those case, the damage rect
>>>>>> computed by drm_fb_helper_memory_range_to_clip() goes out of bottom bounds
>>>>>> of the display.
>>>>>>
>>>>>> Run fbdev test of IGT on a x86+ast2400 platform with 1680x1050 resolution
>>>>>> will cause the system hang with the following call trace:
>>>>>>
>>>>>>      Oops: 0000 [#1] PREEMPT SMP PTI
>>>>>>      [IGT] fbdev: starting subtest eof
>>>>>>      Workqueue: events drm_fb_helper_damage_work [drm_kms_helper]
>>>>>>      [IGT] fbdev: starting subtest nullptr
>>>>>>
>>>>>>      RIP: 0010:memcpy_erms+0xa/0x20
>>>>>>      RSP: 0018:ffffa17d40167d98 EFLAGS: 00010246
>>>>>>      RAX: ffffa17d4eb7fa80 RBX: ffffa17d40e0aa80 RCX: 00000000000014c0
>>>>>>      RDX: 0000000000001a40 RSI: ffffa17d40e0b000 RDI: ffffa17d4eb80000
>>>>>>      RBP: ffffa17d40167e20 R08: 0000000000000000 R09: ffff89522ecff8c0
>>>>>>      R10: ffffa17d4e4c5000 R11: 0000000000000000 R12: ffffa17d4eb7fa80
>>>>>>      R13: 0000000000001a40 R14: 000000000000041a R15: ffffa17d40167e30
>>>>>>      FS:  0000000000000000(0000) GS:ffff895257380000(0000) knlGS:0000000000000000
>>>>>>      CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>>      CR2: ffffa17d40e0b000 CR3: 00000001eaeca006 CR4: 00000000001706e0
>>>>>>      Call Trace:
>>>>>>       <TASK>
>>>>>>       ? drm_fbdev_generic_helper_fb_dirty+0x207/0x330 [drm_kms_helper]
>>>>>>       drm_fb_helper_damage_work+0x8f/0x170 [drm_kms_helper]
>>>>>>       process_one_work+0x21f/0x430
>>>>>>       worker_thread+0x4e/0x3c0
>>>>>>       ? __pfx_worker_thread+0x10/0x10
>>>>>>       kthread+0xf4/0x120
>>>>>>       ? __pfx_kthread+0x10/0x10
>>>>>>       ret_from_fork+0x2c/0x50
>>>>>>       </TASK>
>>>>>>      CR2: ffffa17d40e0b000
>>>>>>      ---[ end trace 0000000000000000 ]---
>>>>>>
>>>>>> We also add trival code in this patch to restrict the damage rect beyond
>>>>>> the last line of the framebuffer.
>>>>> Nice catch!
>>>>    :)
>>>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_fb_helper.c     | 2 +-
>>>>>>     drivers/gpu/drm/drm_fbdev_generic.c | 2 ++
>>>>>>     2 files changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> index 64458982be40..a2b749372759 100644
>>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> @@ -645,7 +645,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
>>>>>>             u32 x1 = 0;
>>>>>>             u32 y1 = off / info->fix.line_length;
>>>>>>             u32 x2 = info->var.xres;
>>>>>> -  u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
>>>>>> +  u32 y2 = min_t(u32, DIV_ROUND_UP(end, info->fix.line_length), info->var.yres);
>>>>> So for additional robustness I think it'd be good if we change the entire
>>>>> computation here to use drm_framebuffer data and not fb_info data, because
>>>>> fundamentally that's what the drm kms code consumes. It should all match
>>>>> anyway, but I think it makes the code more obviously correct.
>>>>>
>>>>> So in the entire function instead of looking at fb_info->fix we should
>>>>> probably look at
>>>>>
>>>>>      struct drm_fb_helper *helper = info->par;
>>>>>
>>>>> And then helper->fb->pitches[0] and helper->fb->height.
>>>>>
>>>>> If you agree would be great if you can please respin with that (and the
>>>>> commit message augmented to explain why we do the change)?
>>>> Yes, I'm agree.
>>>>
>>>> Thank you for guidance, I will refine this patch with `helper = info->par`.
>>>>
>>>> I will send a v2 when I finished.
>>>>
>>>>>>             if ((y2 - y1) == 1) {
>>>>>>                     /*
>>>>>> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
>>>>>> index 8e5148bf40bb..a6daecb5f640 100644
>>>>>> --- a/drivers/gpu/drm/drm_fbdev_generic.c
>>>>>> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
>>>>>> @@ -95,6 +95,8 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
>>>>>>             fb_helper->fb = buffer->fb;
>>>>>>             screen_size = buffer->gem->size;
>>>>> I guess you forgot to remove this line here?
>>>> Yes, this line should be removed in this patch. I overlooked this, sorry.
>>>>
>>>>> Also I'm not understanding
>>>>> why this matters, I think you're fix only needs the above chunk, not this
>>>>> one? If I got this right then please drop this part, there's drivers which
>>>>> only use drm_fb_helper.c but not drm_fbdev_generic.c, and from what I can
>>>>> tell they all still set the gem buffer size here.
>>>>>
>>>>> If otoh we need this too, then there's a few more places that need to be
>>>>> fixed.
>>>> I think we need this line, otherwise wrapped around will be happen.
>>>>
>>>> Because I found that the value of variable`y1` will be larger in number than
>>>> the variable `y2` by 1,
>>>>
>>>> which are computed  in drm_fb_helper_memory_range_to_clip().
>>>>
>>>>
>>>> This phenomenon will emerged on platforms with large page size or
>>>>
>>>> non page size divisiable display resolution case. Take the LoongArch and
>>>> Mips as an example,
>>>>
>>>> the default page size is 16KB(to avoid cache alias).  Even with the most
>>>> frequently used
>>>>
>>>> 1920x1080 screen, the screen_size can not be divided exactly.
>>>>
>>>> The total size of the shadow buffer is 1920x1080x4 bytes, 1920x1080x4 /
>>>> 16384 = 506.25
>>>>
>>>> TTM manage the vram in the term of pages, so TTM will allocate 507 pages for
>>>> us.
>>>>
>>>> 507x16384 = 8306688 bytes.
>>>>
>>>>
>>>> drm_fb_helper_memory_range_to_clip() will be called when running fbdev eof
>>>> test in the IGT.
>>>>
>>>> with 8306688 as its second parameter. while 8306688 / (1920x4) = 1081, this
>>>> cause y1 out of bound.
>>>>
>>>> Simply restrict y2 with a min_t() function yeild 1080 in this case, but y2 -
>>>> y1 cause *wrap around* here.
>>>>
>>>> because they are both unsigned number.
>>>>
>>>>
>>>> drm_rect_init() function cast this unsigned int type to int type in end of
>>>> drm_fb_helper_memory_range_to_clip(),
>>>>
>>>> but the last argument of drm_fb_helper_damage() function is a u32 type,
>>>>
>>>> it cast the return value of  drm_rect_height(&damage_area) back to unsigned
>>>> type.
>>>>
>>>> Yet, another wrapped around with truncation happened in
>>>> drm_fb_helper_add_damage_clip()
>>>>
>>>> called by subsequent drm_fb_helper_damage() function.
>>>>
>>>> I finally got reject by drm_fbdev_generic_helper_fb_dirty() with follow
>>>> code:
>>>>
>>>> ```
>>>>
>>>>       /* Call damage handlers only if necessary */
>>>>       if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>>>>           return 0;
>>>>
>>>> ```
>>>>
>>>> On x86-64 platform, because 1920x1080x4 dumb buffer is lucky, it be divided
>>>> exactly by 4KB(page size).
>>>>
>>>> But other resolution will not as luck as this one. Right, fbdev test will be
>>>> pasted, but wrap around
>>>>
>>>> happens many time.
>>>>
>>>> Therefore, as long as a larger buffer is allowed to exposed to the
>>>> user-space.
>>>>
>>>> A chance is given to the user-space,  to go beyond of the bottom bound of
>>>> the actual active display area.
>>>>
>>>> I not sure if this is intended, I feel it should not be allowable by
>>>> intuition.
>>> Ah yes, thanks for the in-depth explanation. But I think we need a
>>> different fix, by also limiting y1. Otherwise for really big page sizes
>>> (64k on arm64 iirc) and really small screens (there's i2c panels with just
>>> a few lines) we might still run into the issue of y1 being too large.
>>>
>>> So we need to limit both y1 and y2. I think it's ok to let y1 == y2 slip
>>> through, since as you point out that's filtered later on.
>>>
>>> The userspace api is that we should expose the full fbdev buffer and allow
>>> writes into the entire thing. It's just that for the explicit upload with
>>> damage rects we need to make sure we're staying within the real buffer.
>>> -Daniel
>>>
>> Limiting y1 is easy, and this is necessary, because it is the crazy
>> fbdev test of IGT writing after EOF intentionally.
>>
>> But there some difficulties for me to avoid using info->fix and info->var ,
>>
>> I found all other functions are surrounding the info->fix and info-var.
>>
>> There seems no good variable to replace info->var related data structure.
>>
>> Partially replacement may introduce confusion, this somewhat beyond my
>> ability.
>>
>> I'm afraid of introducing out-of-bound in horizontal direction for
>> multi-screen case.
>>
>> Using fb_info->fix is still more safe.
>>
>> Can I  respin my patch by still using fb_info->fix here?
> Which one do you have an issue with finding the right drm variable? I
> can help with that.
> -Daniel

The info->var.xres and info->var.bits_per_pixel in 
drm_fb_helper_memory_range_to_clip() function.

>>>>>> +  screen_size = sizes->surface_height * buffer->fb->pitches[0];
>>>>>> +
>>>>>>             screen_buffer = vzalloc(screen_size);
>>>>>>             if (!screen_buffer) {
>>>>>>                     ret = -ENOMEM;
>>>>> Cheers, Daniel
>>>>>
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>
>
  

On Fri, Apr 14, 2023 at 01:00:07AM +0800, Sui Jingfeng wrote:
> 
> On 2023/4/13 23:56, Daniel Vetter wrote:
> > On Thu, 13 Apr 2023 at 17:35, Sui Jingfeng <15330273260@189.cn> wrote:
> > > Hi,
> > > 
> > > On 2023/4/13 01:44, Daniel Vetter wrote:
> > > > On Thu, Apr 13, 2023 at 01:13:37AM +0800, Sui Jingfeng wrote:
> > > > > Hi,
> > > > > 
> > > > > On 2023/4/11 22:53, Daniel Vetter wrote:
> > > > > > On Sun, Apr 09, 2023 at 09:21:10PM +0800, Sui Jingfeng wrote:
> > > > > > > From: Sui Jingfeng <suijingfeng@loongson.cn>
> > > > > > > 
> > > > > > > We should setting the screen buffer size according to the screen's actual
> > > > > > > size, rather than the size of the GEM object backing the front framebuffer.
> > > > > > > The size of GEM buffer is page size aligned, while the size of active area
> > > > > > > of a specific screen is *NOT* necessarily page size aliged. For example,
> > > > > > > 1680x1050, 1600x900, 1440x900, 800x6000 etc. In those case, the damage rect
> > > > > > > computed by drm_fb_helper_memory_range_to_clip() goes out of bottom bounds
> > > > > > > of the display.
> > > > > > > 
> > > > > > > Run fbdev test of IGT on a x86+ast2400 platform with 1680x1050 resolution
> > > > > > > will cause the system hang with the following call trace:
> > > > > > > 
> > > > > > >      Oops: 0000 [#1] PREEMPT SMP PTI
> > > > > > >      [IGT] fbdev: starting subtest eof
> > > > > > >      Workqueue: events drm_fb_helper_damage_work [drm_kms_helper]
> > > > > > >      [IGT] fbdev: starting subtest nullptr
> > > > > > > 
> > > > > > >      RIP: 0010:memcpy_erms+0xa/0x20
> > > > > > >      RSP: 0018:ffffa17d40167d98 EFLAGS: 00010246
> > > > > > >      RAX: ffffa17d4eb7fa80 RBX: ffffa17d40e0aa80 RCX: 00000000000014c0
> > > > > > >      RDX: 0000000000001a40 RSI: ffffa17d40e0b000 RDI: ffffa17d4eb80000
> > > > > > >      RBP: ffffa17d40167e20 R08: 0000000000000000 R09: ffff89522ecff8c0
> > > > > > >      R10: ffffa17d4e4c5000 R11: 0000000000000000 R12: ffffa17d4eb7fa80
> > > > > > >      R13: 0000000000001a40 R14: 000000000000041a R15: ffffa17d40167e30
> > > > > > >      FS:  0000000000000000(0000) GS:ffff895257380000(0000) knlGS:0000000000000000
> > > > > > >      CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > > >      CR2: ffffa17d40e0b000 CR3: 00000001eaeca006 CR4: 00000000001706e0
> > > > > > >      Call Trace:
> > > > > > >       <TASK>
> > > > > > >       ? drm_fbdev_generic_helper_fb_dirty+0x207/0x330 [drm_kms_helper]
> > > > > > >       drm_fb_helper_damage_work+0x8f/0x170 [drm_kms_helper]
> > > > > > >       process_one_work+0x21f/0x430
> > > > > > >       worker_thread+0x4e/0x3c0
> > > > > > >       ? __pfx_worker_thread+0x10/0x10
> > > > > > >       kthread+0xf4/0x120
> > > > > > >       ? __pfx_kthread+0x10/0x10
> > > > > > >       ret_from_fork+0x2c/0x50
> > > > > > >       </TASK>
> > > > > > >      CR2: ffffa17d40e0b000
> > > > > > >      ---[ end trace 0000000000000000 ]---
> > > > > > > 
> > > > > > > We also add trival code in this patch to restrict the damage rect beyond
> > > > > > > the last line of the framebuffer.
> > > > > > Nice catch!
> > > > >    :)
> > > > > > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/drm_fb_helper.c     | 2 +-
> > > > > > >     drivers/gpu/drm/drm_fbdev_generic.c | 2 ++
> > > > > > >     2 files changed, 3 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > > > > index 64458982be40..a2b749372759 100644
> > > > > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > > > > @@ -645,7 +645,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
> > > > > > >             u32 x1 = 0;
> > > > > > >             u32 y1 = off / info->fix.line_length;
> > > > > > >             u32 x2 = info->var.xres;
> > > > > > > -  u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> > > > > > > +  u32 y2 = min_t(u32, DIV_ROUND_UP(end, info->fix.line_length), info->var.yres);
> > > > > > So for additional robustness I think it'd be good if we change the entire
> > > > > > computation here to use drm_framebuffer data and not fb_info data, because
> > > > > > fundamentally that's what the drm kms code consumes. It should all match
> > > > > > anyway, but I think it makes the code more obviously correct.
> > > > > > 
> > > > > > So in the entire function instead of looking at fb_info->fix we should
> > > > > > probably look at
> > > > > > 
> > > > > >      struct drm_fb_helper *helper = info->par;
> > > > > > 
> > > > > > And then helper->fb->pitches[0] and helper->fb->height.
> > > > > > 
> > > > > > If you agree would be great if you can please respin with that (and the
> > > > > > commit message augmented to explain why we do the change)?
> > > > > Yes, I'm agree.
> > > > > 
> > > > > Thank you for guidance, I will refine this patch with `helper = info->par`.
> > > > > 
> > > > > I will send a v2 when I finished.
> > > > > 
> > > > > > >             if ((y2 - y1) == 1) {
> > > > > > >                     /*
> > > > > > > diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> > > > > > > index 8e5148bf40bb..a6daecb5f640 100644
> > > > > > > --- a/drivers/gpu/drm/drm_fbdev_generic.c
> > > > > > > +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> > > > > > > @@ -95,6 +95,8 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
> > > > > > >             fb_helper->fb = buffer->fb;
> > > > > > >             screen_size = buffer->gem->size;
> > > > > > I guess you forgot to remove this line here?
> > > > > Yes, this line should be removed in this patch. I overlooked this, sorry.
> > > > > 
> > > > > > Also I'm not understanding
> > > > > > why this matters, I think you're fix only needs the above chunk, not this
> > > > > > one? If I got this right then please drop this part, there's drivers which
> > > > > > only use drm_fb_helper.c but not drm_fbdev_generic.c, and from what I can
> > > > > > tell they all still set the gem buffer size here.
> > > > > > 
> > > > > > If otoh we need this too, then there's a few more places that need to be
> > > > > > fixed.
> > > > > I think we need this line, otherwise wrapped around will be happen.
> > > > > 
> > > > > Because I found that the value of variable`y1` will be larger in number than
> > > > > the variable `y2` by 1,
> > > > > 
> > > > > which are computed  in drm_fb_helper_memory_range_to_clip().
> > > > > 
> > > > > 
> > > > > This phenomenon will emerged on platforms with large page size or
> > > > > 
> > > > > non page size divisiable display resolution case. Take the LoongArch and
> > > > > Mips as an example,
> > > > > 
> > > > > the default page size is 16KB(to avoid cache alias).  Even with the most
> > > > > frequently used
> > > > > 
> > > > > 1920x1080 screen, the screen_size can not be divided exactly.
> > > > > 
> > > > > The total size of the shadow buffer is 1920x1080x4 bytes, 1920x1080x4 /
> > > > > 16384 = 506.25
> > > > > 
> > > > > TTM manage the vram in the term of pages, so TTM will allocate 507 pages for
> > > > > us.
> > > > > 
> > > > > 507x16384 = 8306688 bytes.
> > > > > 
> > > > > 
> > > > > drm_fb_helper_memory_range_to_clip() will be called when running fbdev eof
> > > > > test in the IGT.
> > > > > 
> > > > > with 8306688 as its second parameter. while 8306688 / (1920x4) = 1081, this
> > > > > cause y1 out of bound.
> > > > > 
> > > > > Simply restrict y2 with a min_t() function yeild 1080 in this case, but y2 -
> > > > > y1 cause *wrap around* here.
> > > > > 
> > > > > because they are both unsigned number.
> > > > > 
> > > > > 
> > > > > drm_rect_init() function cast this unsigned int type to int type in end of
> > > > > drm_fb_helper_memory_range_to_clip(),
> > > > > 
> > > > > but the last argument of drm_fb_helper_damage() function is a u32 type,
> > > > > 
> > > > > it cast the return value of  drm_rect_height(&damage_area) back to unsigned
> > > > > type.
> > > > > 
> > > > > Yet, another wrapped around with truncation happened in
> > > > > drm_fb_helper_add_damage_clip()
> > > > > 
> > > > > called by subsequent drm_fb_helper_damage() function.
> > > > > 
> > > > > I finally got reject by drm_fbdev_generic_helper_fb_dirty() with follow
> > > > > code:
> > > > > 
> > > > > ```
> > > > > 
> > > > >       /* Call damage handlers only if necessary */
> > > > >       if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
> > > > >           return 0;
> > > > > 
> > > > > ```
> > > > > 
> > > > > On x86-64 platform, because 1920x1080x4 dumb buffer is lucky, it be divided
> > > > > exactly by 4KB(page size).
> > > > > 
> > > > > But other resolution will not as luck as this one. Right, fbdev test will be
> > > > > pasted, but wrap around
> > > > > 
> > > > > happens many time.
> > > > > 
> > > > > Therefore, as long as a larger buffer is allowed to exposed to the
> > > > > user-space.
> > > > > 
> > > > > A chance is given to the user-space,  to go beyond of the bottom bound of
> > > > > the actual active display area.
> > > > > 
> > > > > I not sure if this is intended, I feel it should not be allowable by
> > > > > intuition.
> > > > Ah yes, thanks for the in-depth explanation. But I think we need a
> > > > different fix, by also limiting y1. Otherwise for really big page sizes
> > > > (64k on arm64 iirc) and really small screens (there's i2c panels with just
> > > > a few lines) we might still run into the issue of y1 being too large.
> > > > 
> > > > So we need to limit both y1 and y2. I think it's ok to let y1 == y2 slip
> > > > through, since as you point out that's filtered later on.
> > > > 
> > > > The userspace api is that we should expose the full fbdev buffer and allow
> > > > writes into the entire thing. It's just that for the explicit upload with
> > > > damage rects we need to make sure we're staying within the real buffer.
> > > > -Daniel
> > > > 
> > > Limiting y1 is easy, and this is necessary, because it is the crazy
> > > fbdev test of IGT writing after EOF intentionally.
> > > 
> > > But there some difficulties for me to avoid using info->fix and info->var ,
> > > 
> > > I found all other functions are surrounding the info->fix and info-var.
> > > 
> > > There seems no good variable to replace info->var related data structure.
> > > 
> > > Partially replacement may introduce confusion, this somewhat beyond my
> > > ability.
> > > 
> > > I'm afraid of introducing out-of-bound in horizontal direction for
> > > multi-screen case.
> > > 
> > > Using fb_info->fix is still more safe.
> > > 
> > > Can I  respin my patch by still using fb_info->fix here?
> > Which one do you have an issue with finding the right drm variable? I
> > can help with that.
> > -Daniel
> 
> The info->var.xres and info->var.bits_per_pixel in
> drm_fb_helper_memory_range_to_clip() function.

This should switch the existing code over to using drm_framebuffer instead
of fbdev:


diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ef4eb8b12766..99ca69dd432f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
 static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
 					       struct drm_rect *clip)
 {
+	struct drm_fb_helper *helper = info->par;
+
 	off_t end = off + len;
 	u32 x1 = 0;
 	u32 y1 = off / info->fix.line_length;
-	u32 x2 = info->var.xres;
-	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
+	u32 x2 = helper->fb->height;
+	unsigned stride = helper->fb->pitches[0];
+	u32 y2 = DIV_ROUND_UP(end, stride);
+	int bpp = drm_format_info_bpp(helper->fb->format, 0);
 
 	if ((y2 - y1) == 1) {
 		/*
 		 * We've only written to a single scanline. Try to reduce
 		 * the number of horizontal pixels that need an update.
 		 */
-		off_t bit_off = (off % info->fix.line_length) * 8;
-		off_t bit_end = (end % info->fix.line_length) * 8;
+		off_t bit_off = (off % stride) * 8;
+		off_t bit_end = (end % stride) * 8;
 
-		x1 = bit_off / info->var.bits_per_pixel;
-		x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);
+		x1 = bit_off / bpp;
+		x2 = DIV_ROUND_UP(bit_end, bpp);
 	}
 
 	drm_rect_init(clip, x1, y1, x2 - x1, y2 - y1);

> 
> > > > > > > +  screen_size = sizes->surface_height * buffer->fb->pitches[0];
> > > > > > > +
> > > > > > >             screen_buffer = vzalloc(screen_size);
> > > > > > >             if (!screen_buffer) {
> > > > > > >                     ret = -ENOMEM;
> > > > > > Cheers, Daniel
> > > > > > 
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > > 
> > 
> >
  

Hi

Am 13.04.23 um 20:56 schrieb Daniel Vetter:
[...]
> 
> This should switch the existing code over to using drm_framebuffer instead
> of fbdev:
> 
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index ef4eb8b12766..99ca69dd432f 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
>   static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
>   					       struct drm_rect *clip)
>   {
> +	struct drm_fb_helper *helper = info->par;
> +
>   	off_t end = off + len;
>   	u32 x1 = 0;
>   	u32 y1 = off / info->fix.line_length;
> -	u32 x2 = info->var.xres;
> -	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> +	u32 x2 = helper->fb->height;
> +	unsigned stride = helper->fb->pitches[0];
> +	u32 y2 = DIV_ROUND_UP(end, stride);
> +	int bpp = drm_format_info_bpp(helper->fb->format, 0);

Please DONT do that. The code here is fbdev code and shouldn't bother 
about DRM data structures. Actually, it shouldn't be here: a number of 
fbdev drivers with deferred I/O contain similar code and the fbdev 
module should provide us with a helper. (I think I even had some patches 
somewhere.)

Best regards
Thomas

>   
>   	if ((y2 - y1) == 1) {
>   		/*
>   		 * We've only written to a single scanline. Try to reduce
>   		 * the number of horizontal pixels that need an update.
>   		 */
> -		off_t bit_off = (off % info->fix.line_length) * 8;
> -		off_t bit_end = (end % info->fix.line_length) * 8;
> +		off_t bit_off = (off % stride) * 8;
> +		off_t bit_end = (end % stride) * 8;
>   
> -		x1 = bit_off / info->var.bits_per_pixel;
> -		x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);
> +		x1 = bit_off / bpp;
> +		x2 = DIV_ROUND_UP(bit_end, bpp);
>   	}
>   
>   	drm_rect_init(clip, x1, y1, x2 - x1, y2 - y1);
> 
>>
>>>>>>>> +  screen_size = sizes->surface_height * buffer->fb->pitches[0];
>>>>>>>> +
>>>>>>>>              screen_buffer = vzalloc(screen_size);
>>>>>>>>              if (!screen_buffer) {
>>>>>>>>                      ret = -ENOMEM;
>>>>>>> Cheers, Daniel
>>>>>>>
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>>>
>>>
>>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
  

On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 13.04.23 um 20:56 schrieb Daniel Vetter:
> [...]
> > 
> > This should switch the existing code over to using drm_framebuffer instead
> > of fbdev:
> > 
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index ef4eb8b12766..99ca69dd432f 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> >   static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
> >   					       struct drm_rect *clip)
> >   {
> > +	struct drm_fb_helper *helper = info->par;
> > +
> >   	off_t end = off + len;
> >   	u32 x1 = 0;
> >   	u32 y1 = off / info->fix.line_length;
> > -	u32 x2 = info->var.xres;
> > -	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> > +	u32 x2 = helper->fb->height;
> > +	unsigned stride = helper->fb->pitches[0];
> > +	u32 y2 = DIV_ROUND_UP(end, stride);
> > +	int bpp = drm_format_info_bpp(helper->fb->format, 0);
> 
> Please DONT do that. The code here is fbdev code and shouldn't bother about
> DRM data structures. Actually, it shouldn't be here: a number of fbdev
> drivers with deferred I/O contain similar code and the fbdev module should
> provide us with a helper. (I think I even had some patches somewhere.)

Well my thinking is that it's a drm driver, so if we have issue with limit
checks blowing up it makes more sense to check them against drm limits.
Plus a lot more people understand those than fbdev. They should all match
anyway, or if they dont, we have a bug. The thing is, if you change this
further to just pass the drm_framebuffer, then this 100% becomes a drm
function, which could be used by anything in drm really.

But also *shrug*.
-Daniel
  

Hi,

On 2023/4/14 04:01, Daniel Vetter wrote:
> On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 13.04.23 um 20:56 schrieb Daniel Vetter:
>> [...]
>>> This should switch the existing code over to using drm_framebuffer instead
>>> of fbdev:
>>>
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>> index ef4eb8b12766..99ca69dd432f 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
>>>    static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
>>>    					       struct drm_rect *clip)
>>>    {
>>> +	struct drm_fb_helper *helper = info->par;
>>> +
>>>    	off_t end = off + len;
>>>    	u32 x1 = 0;
>>>    	u32 y1 = off / info->fix.line_length;
>>> -	u32 x2 = info->var.xres;
>>> -	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
>>> +	u32 x2 = helper->fb->height;
>>> +	unsigned stride = helper->fb->pitches[0];
>>> +	u32 y2 = DIV_ROUND_UP(end, stride);
>>> +	int bpp = drm_format_info_bpp(helper->fb->format, 0);
>> Please DONT do that. The code here is fbdev code and shouldn't bother about
>> DRM data structures. Actually, it shouldn't be here: a number of fbdev
>> drivers with deferred I/O contain similar code and the fbdev module should
>> provide us with a helper. (I think I even had some patches somewhere.)
> Well my thinking is that it's a drm driver,

Yes, I actually agree with this, and the code looks quite good. And I 
really want to adopt it.

Because here is DRM, we should emulate the fbdev in the DRM's way.

The DRM is really the big brother on the behind of emulated fbdev,

who provide the real displayable backing memory and scant out engine to 
display such a buffer.


But currently, the fact is,  drm_fb_helper.c still initializes lots of 
data structure come from fbdev world.

For example, the drm_fb_helper_fill_fix() and drm_fb_helper_fill_var()  
in drm_fb_helper_fill_info() function.

This saying implicitly that the fbdev-generic is a glue layer which copy 
damage frame buffer data

from the front end(fbdev layer) to its drm backend.  It's not a 100% 
replacement its fbdev front end,

rather, it relay on it.


> so if we have issue with limit
> checks blowing up it makes more sense to check them against drm limits.
> Plus a lot more people understand those than fbdev. They should all match
> anyway, or if they dont, we have a bug.

Yes, this is really what I'm worry about.

I see that  members of struct fb_var_screeninfo can be changed by 
user-space. For example, xoffset and yoffset.

There is no change about 'info->var.xres' and 'info->var.yres' from the 
userspace,

therefore, they should all match in practice.


User-space can choose a use a smaller dispaly area than 'var.xres x 
var.yres',

but that is implemented with 'var.xoffset' and 'var.xoffset'.

But consider that the name 'var', which may stand for 'variation' or 
'vary'. This is terrifying.

I imagine, with a shadow buffer, the front end can do any modeset on the 
runtime freely,

including change the format of frame buffer on the runtime.  Just do not 
out-of-bound.

The middle do the conversion on the fly.


We might also create double shadow buffer size to allow the front end to 
pan?

> The thing is, if you change this
> further to just pass the drm_framebuffer, then this 100% becomes a drm
> function, which could be used by anything in drm really.

I agree with you.

If I use fb_width/fb_height directly and bypassing 'info->var.xres" and 
"info->var.yres",

the code style diverged then. As far as I am understanding,  the clip 
happen on the front end,

the actual damage update happen at back end.

Using the data structure come with the front end is more reasonable for 
current implement.

> But also *shrug*.

I can convert this single function to 100% drm with another patch.

But, maybe there also have other functions are not 100% drm

I would like do something to help achieve this in the future,

let me help to fix this bug first?

> -Daniel
  

On Fri, 14 Apr 2023 at 06:24, Sui Jingfeng <15330273260@189.cn> wrote:
>
> Hi,
>
> On 2023/4/14 04:01, Daniel Vetter wrote:
> > On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 13.04.23 um 20:56 schrieb Daniel Vetter:
> >> [...]
> >>> This should switch the existing code over to using drm_framebuffer instead
> >>> of fbdev:
> >>>
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>> index ef4eb8b12766..99ca69dd432f 100644
> >>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>> @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> >>>    static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
> >>>                                            struct drm_rect *clip)
> >>>    {
> >>> +   struct drm_fb_helper *helper = info->par;
> >>> +
> >>>     off_t end = off + len;
> >>>     u32 x1 = 0;
> >>>     u32 y1 = off / info->fix.line_length;
> >>> -   u32 x2 = info->var.xres;
> >>> -   u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> >>> +   u32 x2 = helper->fb->height;
> >>> +   unsigned stride = helper->fb->pitches[0];
> >>> +   u32 y2 = DIV_ROUND_UP(end, stride);
> >>> +   int bpp = drm_format_info_bpp(helper->fb->format, 0);
> >> Please DONT do that. The code here is fbdev code and shouldn't bother about
> >> DRM data structures. Actually, it shouldn't be here: a number of fbdev
> >> drivers with deferred I/O contain similar code and the fbdev module should
> >> provide us with a helper. (I think I even had some patches somewhere.)
> > Well my thinking is that it's a drm driver,
>
> Yes, I actually agree with this, and the code looks quite good. And I
> really want to adopt it.
>
> Because here is DRM, we should emulate the fbdev in the DRM's way.
>
> The DRM is really the big brother on the behind of emulated fbdev,
>
> who provide the real displayable backing memory and scant out engine to
> display such a buffer.
>
>
> But currently, the fact is,  drm_fb_helper.c still initializes lots of
> data structure come from fbdev world.
>
> For example, the drm_fb_helper_fill_fix() and drm_fb_helper_fill_var()
> in drm_fb_helper_fill_info() function.
>
> This saying implicitly that the fbdev-generic is a glue layer which copy
> damage frame buffer data
>
> from the front end(fbdev layer) to its drm backend.  It's not a 100%
> replacement its fbdev front end,
>
> rather, it relay on it.
>
>
> > so if we have issue with limit
> > checks blowing up it makes more sense to check them against drm limits.
> > Plus a lot more people understand those than fbdev. They should all match
> > anyway, or if they dont, we have a bug.
>
> Yes, this is really what I'm worry about.
>
> I see that  members of struct fb_var_screeninfo can be changed by
> user-space. For example, xoffset and yoffset.
>
> There is no change about 'info->var.xres' and 'info->var.yres' from the
> userspace,
>
> therefore, they should all match in practice.
>
>
> User-space can choose a use a smaller dispaly area than 'var.xres x
> var.yres',
>
> but that is implemented with 'var.xoffset' and 'var.xoffset'.
>
> But consider that the name 'var', which may stand for 'variation' or
> 'vary'. This is terrifying.
>
> I imagine, with a shadow buffer, the front end can do any modeset on the
> runtime freely,
>
> including change the format of frame buffer on the runtime.  Just do not
> out-of-bound.
>
> The middle do the conversion on the fly.
>
>
> We might also create double shadow buffer size to allow the front end to
> pan?

Yeah so the front should be able to pan. And the front-end can
actually make xres/yres smalle than drm_fb->height/width, so we _have_
to use the fb side of things. Otherwise it's a bug I just realized.

The xres_virtual/yres_virtual should always match drm_fb sizes (but
we've had bugs in the past for that, only recently fixed all in
linux-next), because that's supposed to be the max size. And since we
never reallocate the fbdev emulation fb (at least with the current
code) this should never change.

But fundamentally you're bringing up a very good point, we've had
piles of bugs in the past with not properly validating the fbdev side
information in info->var, and a bunch of resulting bugs. So validating
against the drm side of things should be a bit more robust.

It's kinda the same we do for legacy kms ioctls: We translate that to
atomic kms as fast as possible, and then do the entire subsequent
validation with atomic kms data structures.
-Daniel

> > The thing is, if you change this
> > further to just pass the drm_framebuffer, then this 100% becomes a drm
> > function, which could be used by anything in drm really.
>
> I agree with you.
>
> If I use fb_width/fb_height directly and bypassing 'info->var.xres" and
> "info->var.yres",
>
> the code style diverged then. As far as I am understanding,  the clip
> happen on the front end,
>
> the actual damage update happen at back end.
>
> Using the data structure come with the front end is more reasonable for
> current implement.
>
> > But also *shrug*.
>
> I can convert this single function to 100% drm with another patch.
>
> But, maybe there also have other functions are not 100% drm
>
> I would like do something to help achieve this in the future,
>
> let me help to fix this bug first?
>
> > -Daniel
  

Hi

Am 13.04.23 um 22:01 schrieb Daniel Vetter:
> On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 13.04.23 um 20:56 schrieb Daniel Vetter:
>> [...]
>>>
>>> This should switch the existing code over to using drm_framebuffer instead
>>> of fbdev:
>>>
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>> index ef4eb8b12766..99ca69dd432f 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
>>>    static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
>>>    					       struct drm_rect *clip)
>>>    {
>>> +	struct drm_fb_helper *helper = info->par;
>>> +
>>>    	off_t end = off + len;
>>>    	u32 x1 = 0;
>>>    	u32 y1 = off / info->fix.line_length;
>>> -	u32 x2 = info->var.xres;
>>> -	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
>>> +	u32 x2 = helper->fb->height;
>>> +	unsigned stride = helper->fb->pitches[0];
>>> +	u32 y2 = DIV_ROUND_UP(end, stride);
>>> +	int bpp = drm_format_info_bpp(helper->fb->format, 0);
>>
>> Please DONT do that. The code here is fbdev code and shouldn't bother about
>> DRM data structures. Actually, it shouldn't be here: a number of fbdev
>> drivers with deferred I/O contain similar code and the fbdev module should
>> provide us with a helper. (I think I even had some patches somewhere.)
> 
> Well my thinking is that it's a drm driver, so if we have issue with limit

Technically, it's not a driver, but a client.

> checks blowing up it makes more sense to check them against drm limits.

You can still do this in fb_dirty during the actual update.

> Plus a lot more people understand those than fbdev. They should all match
> anyway, or if they dont, we have a bug. The thing is, if you change this
> further to just pass the drm_framebuffer, then this 100% becomes a drm
> function, which could be used by anything in drm really.

The code is only useful to fbdev-generic and a number older of fbdev 
drivers. Nothing else uses deferred I/O and the rsp damage handling. 
Anything else operates directly on memory buffers. (i915 is a bit 
special, but doesn't use this damage handling.)  The function really 
should not be here.

I'd push back on additional DRM code with fbdev deferred I/O. Drivers' 
fbdev should either operate directly on GEM buffers; or the driver 
should employ fbdev-generic.

Best regards
Thomas

> 
> But also *shrug*.
> -Daniel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
  

Hi

Am 14.04.23 um 07:36 schrieb Daniel Vetter:
> On Fri, 14 Apr 2023 at 06:24, Sui Jingfeng <15330273260@189.cn> wrote:
>>
>> Hi,
>>
>> On 2023/4/14 04:01, Daniel Vetter wrote:
>>> On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 13.04.23 um 20:56 schrieb Daniel Vetter:
>>>> [...]
>>>>> This should switch the existing code over to using drm_framebuffer instead
>>>>> of fbdev:
>>>>>
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>>> index ef4eb8b12766..99ca69dd432f 100644
>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>> @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
>>>>>     static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
>>>>>                                             struct drm_rect *clip)
>>>>>     {
>>>>> +   struct drm_fb_helper *helper = info->par;
>>>>> +
>>>>>      off_t end = off + len;
>>>>>      u32 x1 = 0;
>>>>>      u32 y1 = off / info->fix.line_length;
>>>>> -   u32 x2 = info->var.xres;
>>>>> -   u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
>>>>> +   u32 x2 = helper->fb->height;
>>>>> +   unsigned stride = helper->fb->pitches[0];
>>>>> +   u32 y2 = DIV_ROUND_UP(end, stride);
>>>>> +   int bpp = drm_format_info_bpp(helper->fb->format, 0);
>>>> Please DONT do that. The code here is fbdev code and shouldn't bother about
>>>> DRM data structures. Actually, it shouldn't be here: a number of fbdev
>>>> drivers with deferred I/O contain similar code and the fbdev module should
>>>> provide us with a helper. (I think I even had some patches somewhere.)
>>> Well my thinking is that it's a drm driver,
>>
>> Yes, I actually agree with this, and the code looks quite good. And I
>> really want to adopt it.
>>
>> Because here is DRM, we should emulate the fbdev in the DRM's way.
>>
>> The DRM is really the big brother on the behind of emulated fbdev,
>>
>> who provide the real displayable backing memory and scant out engine to
>> display such a buffer.
>>
>>
>> But currently, the fact is,  drm_fb_helper.c still initializes lots of
>> data structure come from fbdev world.
>>
>> For example, the drm_fb_helper_fill_fix() and drm_fb_helper_fill_var()
>> in drm_fb_helper_fill_info() function.
>>
>> This saying implicitly that the fbdev-generic is a glue layer which copy
>> damage frame buffer data
>>
>> from the front end(fbdev layer) to its drm backend.  It's not a 100%
>> replacement its fbdev front end,
>>
>> rather, it relay on it.
>>
>>
>>> so if we have issue with limit
>>> checks blowing up it makes more sense to check them against drm limits.
>>> Plus a lot more people understand those than fbdev. They should all match
>>> anyway, or if they dont, we have a bug.
>>
>> Yes, this is really what I'm worry about.
>>
>> I see that  members of struct fb_var_screeninfo can be changed by
>> user-space. For example, xoffset and yoffset.
>>
>> There is no change about 'info->var.xres' and 'info->var.yres' from the
>> userspace,
>>
>> therefore, they should all match in practice.
>>
>>
>> User-space can choose a use a smaller dispaly area than 'var.xres x
>> var.yres',
>>
>> but that is implemented with 'var.xoffset' and 'var.xoffset'.
>>
>> But consider that the name 'var', which may stand for 'variation' or
>> 'vary'. This is terrifying.
>>
>> I imagine, with a shadow buffer, the front end can do any modeset on the
>> runtime freely,
>>
>> including change the format of frame buffer on the runtime.  Just do not
>> out-of-bound.
>>
>> The middle do the conversion on the fly.
>>
>>
>> We might also create double shadow buffer size to allow the front end to
>> pan?
> 
> Yeah so the front should be able to pan. And the front-end can
> actually make xres/yres smalle than drm_fb->height/width, so we _have_
> to use the fb side of things. Otherwise it's a bug I just realized.

What are you talking about?  The GEM buffer is a full fbdev framebuffer, 
which is screen resolution multiplied by the overall factor.  The shadow 
buffer is exactly the same size. You can already easily pan within these 
buffers.

There's also no need/way to change video modes or formats in the shadow 
buffer. If we'd ever support that, it would be implemented in the DRM 
driver's modesetting.  The relationship between GEM buffer and shadow 
buffer remains unaffected by this.

Best regards
Thomas

> 
> The xres_virtual/yres_virtual should always match drm_fb sizes (but
> we've had bugs in the past for that, only recently fixed all in
> linux-next), because that's supposed to be the max size. And since we
> never reallocate the fbdev emulation fb (at least with the current
> code) this should never change.
> 
> But fundamentally you're bringing up a very good point, we've had
> piles of bugs in the past with not properly validating the fbdev side
> information in info->var, and a bunch of resulting bugs. So validating
> against the drm side of things should be a bit more robust.
> 
> It's kinda the same we do for legacy kms ioctls: We translate that to
> atomic kms as fast as possible, and then do the entire subsequent
> validation with atomic kms data structures.
> -Daniel
> 
>>> The thing is, if you change this
>>> further to just pass the drm_framebuffer, then this 100% becomes a drm
>>> function, which could be used by anything in drm really.
>>
>> I agree with you.
>>
>> If I use fb_width/fb_height directly and bypassing 'info->var.xres" and
>> "info->var.yres",
>>
>> the code style diverged then. As far as I am understanding,  the clip
>> happen on the front end,
>>
>> the actual damage update happen at back end.
>>
>> Using the data structure come with the front end is more reasonable for
>> current implement.
>>
>>> But also *shrug*.
>>
>> I can convert this single function to 100% drm with another patch.
>>
>> But, maybe there also have other functions are not 100% drm
>>
>> I would like do something to help achieve this in the future,
>>
>> let me help to fix this bug first?
>>
>>> -Daniel
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
  

Am 14.04.23 um 09:34 schrieb Thomas Zimmermann:
> Hi
> 
> Am 14.04.23 um 07:36 schrieb Daniel Vetter:
>> On Fri, 14 Apr 2023 at 06:24, Sui Jingfeng <15330273260@189.cn> wrote:
>>>
>>> Hi,
>>>
>>> On 2023/4/14 04:01, Daniel Vetter wrote:
>>>> On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
>>>>> Hi
>>>>>
>>>>> Am 13.04.23 um 20:56 schrieb Daniel Vetter:
>>>>> [...]
>>>>>> This should switch the existing code over to using drm_framebuffer 
>>>>>> instead
>>>>>> of fbdev:
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c 
>>>>>> b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> index ef4eb8b12766..99ca69dd432f 100644
>>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct 
>>>>>> drm_fb_helper *helper, u32 x, u32 y,
>>>>>>     static void drm_fb_helper_memory_range_to_clip(struct fb_info 
>>>>>> *info, off_t off, size_t len,
>>>>>>                                             struct drm_rect *clip)
>>>>>>     {
>>>>>> +   struct drm_fb_helper *helper = info->par;
>>>>>> +
>>>>>>      off_t end = off + len;
>>>>>>      u32 x1 = 0;
>>>>>>      u32 y1 = off / info->fix.line_length;
>>>>>> -   u32 x2 = info->var.xres;
>>>>>> -   u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
>>>>>> +   u32 x2 = helper->fb->height;
>>>>>> +   unsigned stride = helper->fb->pitches[0];
>>>>>> +   u32 y2 = DIV_ROUND_UP(end, stride);
>>>>>> +   int bpp = drm_format_info_bpp(helper->fb->format, 0);
>>>>> Please DONT do that. The code here is fbdev code and shouldn't 
>>>>> bother about
>>>>> DRM data structures. Actually, it shouldn't be here: a number of fbdev
>>>>> drivers with deferred I/O contain similar code and the fbdev module 
>>>>> should
>>>>> provide us with a helper. (I think I even had some patches somewhere.)
>>>> Well my thinking is that it's a drm driver,
>>>
>>> Yes, I actually agree with this, and the code looks quite good. And I
>>> really want to adopt it.
>>>
>>> Because here is DRM, we should emulate the fbdev in the DRM's way.
>>>
>>> The DRM is really the big brother on the behind of emulated fbdev,
>>>
>>> who provide the real displayable backing memory and scant out engine to
>>> display such a buffer.
>>>
>>>
>>> But currently, the fact is,  drm_fb_helper.c still initializes lots of
>>> data structure come from fbdev world.
>>>
>>> For example, the drm_fb_helper_fill_fix() and drm_fb_helper_fill_var()
>>> in drm_fb_helper_fill_info() function.
>>>
>>> This saying implicitly that the fbdev-generic is a glue layer which copy
>>> damage frame buffer data
>>>
>>> from the front end(fbdev layer) to its drm backend.  It's not a 100%
>>> replacement its fbdev front end,
>>>
>>> rather, it relay on it.
>>>
>>>
>>>> so if we have issue with limit
>>>> checks blowing up it makes more sense to check them against drm limits.
>>>> Plus a lot more people understand those than fbdev. They should all 
>>>> match
>>>> anyway, or if they dont, we have a bug.
>>>
>>> Yes, this is really what I'm worry about.
>>>
>>> I see that  members of struct fb_var_screeninfo can be changed by
>>> user-space. For example, xoffset and yoffset.
>>>
>>> There is no change about 'info->var.xres' and 'info->var.yres' from the
>>> userspace,
>>>
>>> therefore, they should all match in practice.
>>>
>>>
>>> User-space can choose a use a smaller dispaly area than 'var.xres x
>>> var.yres',
>>>
>>> but that is implemented with 'var.xoffset' and 'var.xoffset'.
>>>
>>> But consider that the name 'var', which may stand for 'variation' or
>>> 'vary'. This is terrifying.
>>>
>>> I imagine, with a shadow buffer, the front end can do any modeset on the
>>> runtime freely,
>>>
>>> including change the format of frame buffer on the runtime.  Just do not
>>> out-of-bound.
>>>
>>> The middle do the conversion on the fly.
>>>
>>>
>>> We might also create double shadow buffer size to allow the front end to
>>> pan?
>>
>> Yeah so the front should be able to pan. And the front-end can
>> actually make xres/yres smalle than drm_fb->height/width, so we _have_
>> to use the fb side of things. Otherwise it's a bug I just realized.
> 
> What are you talking about?  The GEM buffer is a full fbdev framebuffer, 
> which is screen resolution multiplied by the overall factor.  The shadow 

s/overall/overalloc'

> buffer is exactly the same size. You can already easily pan within these 
> buffers.
> 
> There's also no need/way to change video modes or formats in the shadow 
> buffer. If we'd ever support that, it would be implemented in the DRM 
> driver's modesetting.  The relationship between GEM buffer and shadow 
> buffer remains unaffected by this.
> 
> Best regards
> Thomas
> 
>>
>> The xres_virtual/yres_virtual should always match drm_fb sizes (but
>> we've had bugs in the past for that, only recently fixed all in
>> linux-next), because that's supposed to be the max size. And since we
>> never reallocate the fbdev emulation fb (at least with the current
>> code) this should never change.
>>
>> But fundamentally you're bringing up a very good point, we've had
>> piles of bugs in the past with not properly validating the fbdev side
>> information in info->var, and a bunch of resulting bugs. So validating
>> against the drm side of things should be a bit more robust.
>>
>> It's kinda the same we do for legacy kms ioctls: We translate that to
>> atomic kms as fast as possible, and then do the entire subsequent
>> validation with atomic kms data structures.
>> -Daniel
>>
>>>> The thing is, if you change this
>>>> further to just pass the drm_framebuffer, then this 100% becomes a drm
>>>> function, which could be used by anything in drm really.
>>>
>>> I agree with you.
>>>
>>> If I use fb_width/fb_height directly and bypassing 'info->var.xres" and
>>> "info->var.yres",
>>>
>>> the code style diverged then. As far as I am understanding,  the clip
>>> happen on the front end,
>>>
>>> the actual damage update happen at back end.
>>>
>>> Using the data structure come with the front end is more reasonable for
>>> current implement.
>>>
>>>> But also *shrug*.
>>>
>>> I can convert this single function to 100% drm with another patch.
>>>
>>> But, maybe there also have other functions are not 100% drm
>>>
>>> I would like do something to help achieve this in the future,
>>>
>>> let me help to fix this bug first?
>>>
>>>> -Daniel
>>
>>
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
  

On Fri, 14 Apr 2023 at 09:34, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 14.04.23 um 07:36 schrieb Daniel Vetter:
> > On Fri, 14 Apr 2023 at 06:24, Sui Jingfeng <15330273260@189.cn> wrote:
> >>
> >> Hi,
> >>
> >> On 2023/4/14 04:01, Daniel Vetter wrote:
> >>> On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
> >>>> Hi
> >>>>
> >>>> Am 13.04.23 um 20:56 schrieb Daniel Vetter:
> >>>> [...]
> >>>>> This should switch the existing code over to using drm_framebuffer instead
> >>>>> of fbdev:
> >>>>>
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>>>> index ef4eb8b12766..99ca69dd432f 100644
> >>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>>>> @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> >>>>>     static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
> >>>>>                                             struct drm_rect *clip)
> >>>>>     {
> >>>>> +   struct drm_fb_helper *helper = info->par;
> >>>>> +
> >>>>>      off_t end = off + len;
> >>>>>      u32 x1 = 0;
> >>>>>      u32 y1 = off / info->fix.line_length;
> >>>>> -   u32 x2 = info->var.xres;
> >>>>> -   u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> >>>>> +   u32 x2 = helper->fb->height;
> >>>>> +   unsigned stride = helper->fb->pitches[0];
> >>>>> +   u32 y2 = DIV_ROUND_UP(end, stride);
> >>>>> +   int bpp = drm_format_info_bpp(helper->fb->format, 0);
> >>>> Please DONT do that. The code here is fbdev code and shouldn't bother about
> >>>> DRM data structures. Actually, it shouldn't be here: a number of fbdev
> >>>> drivers with deferred I/O contain similar code and the fbdev module should
> >>>> provide us with a helper. (I think I even had some patches somewhere.)
> >>> Well my thinking is that it's a drm driver,
> >>
> >> Yes, I actually agree with this, and the code looks quite good. And I
> >> really want to adopt it.
> >>
> >> Because here is DRM, we should emulate the fbdev in the DRM's way.
> >>
> >> The DRM is really the big brother on the behind of emulated fbdev,
> >>
> >> who provide the real displayable backing memory and scant out engine to
> >> display such a buffer.
> >>
> >>
> >> But currently, the fact is,  drm_fb_helper.c still initializes lots of
> >> data structure come from fbdev world.
> >>
> >> For example, the drm_fb_helper_fill_fix() and drm_fb_helper_fill_var()
> >> in drm_fb_helper_fill_info() function.
> >>
> >> This saying implicitly that the fbdev-generic is a glue layer which copy
> >> damage frame buffer data
> >>
> >> from the front end(fbdev layer) to its drm backend.  It's not a 100%
> >> replacement its fbdev front end,
> >>
> >> rather, it relay on it.
> >>
> >>
> >>> so if we have issue with limit
> >>> checks blowing up it makes more sense to check them against drm limits.
> >>> Plus a lot more people understand those than fbdev. They should all match
> >>> anyway, or if they dont, we have a bug.
> >>
> >> Yes, this is really what I'm worry about.
> >>
> >> I see that  members of struct fb_var_screeninfo can be changed by
> >> user-space. For example, xoffset and yoffset.
> >>
> >> There is no change about 'info->var.xres' and 'info->var.yres' from the
> >> userspace,
> >>
> >> therefore, they should all match in practice.
> >>
> >>
> >> User-space can choose a use a smaller dispaly area than 'var.xres x
> >> var.yres',
> >>
> >> but that is implemented with 'var.xoffset' and 'var.xoffset'.
> >>
> >> But consider that the name 'var', which may stand for 'variation' or
> >> 'vary'. This is terrifying.
> >>
> >> I imagine, with a shadow buffer, the front end can do any modeset on the
> >> runtime freely,
> >>
> >> including change the format of frame buffer on the runtime.  Just do not
> >> out-of-bound.
> >>
> >> The middle do the conversion on the fly.
> >>
> >>
> >> We might also create double shadow buffer size to allow the front end to
> >> pan?
> >
> > Yeah so the front should be able to pan. And the front-end can
> > actually make xres/yres smalle than drm_fb->height/width, so we _have_
> > to use the fb side of things. Otherwise it's a bug I just realized.
>
> What are you talking about?  The GEM buffer is a full fbdev framebuffer,
> which is screen resolution multiplied by the overall factor.  The shadow
> buffer is exactly the same size. You can already easily pan within these
> buffers.
>
> There's also no need/way to change video modes or formats in the shadow
> buffer. If we'd ever support that, it would be implemented in the DRM
> driver's modesetting.  The relationship between GEM buffer and shadow
> buffer remains unaffected by this.

Try it and be amazed :-) I've seen enough syzkaller bugs and screamed
at them that yes we do this. Also xres/yres is the wrong thing even if
you don't use fb ioctl to change things up in multi-monitor cases (we
allocate the drm_fb/fbdev virtual size to match the biggest
resolution, but then set fbinfo->var.x/yres to match the smallest to
make sure fbcon is fully visible everywhere).

I think you're confusion is the perfect case for why we really should
use fb->height/width/pitches[0] here.
-Daniel

>
> Best regards
> Thomas
>
> >
> > The xres_virtual/yres_virtual should always match drm_fb sizes (but
> > we've had bugs in the past for that, only recently fixed all in
> > linux-next), because that's supposed to be the max size. And since we
> > never reallocate the fbdev emulation fb (at least with the current
> > code) this should never change.
> >
> > But fundamentally you're bringing up a very good point, we've had
> > piles of bugs in the past with not properly validating the fbdev side
> > information in info->var, and a bunch of resulting bugs. So validating
> > against the drm side of things should be a bit more robust.
> >
> > It's kinda the same we do for legacy kms ioctls: We translate that to
> > atomic kms as fast as possible, and then do the entire subsequent
> > validation with atomic kms data structures.
> > -Daniel
> >
> >>> The thing is, if you change this
> >>> further to just pass the drm_framebuffer, then this 100% becomes a drm
> >>> function, which could be used by anything in drm really.
> >>
> >> I agree with you.
> >>
> >> If I use fb_width/fb_height directly and bypassing 'info->var.xres" and
> >> "info->var.yres",
> >>
> >> the code style diverged then. As far as I am understanding,  the clip
> >> happen on the front end,
> >>
> >> the actual damage update happen at back end.
> >>
> >> Using the data structure come with the front end is more reasonable for
> >> current implement.
> >>
> >>> But also *shrug*.
> >>
> >> I can convert this single function to 100% drm with another patch.
> >>
> >> But, maybe there also have other functions are not 100% drm
> >>
> >> I would like do something to help achieve this in the future,
> >>
> >> let me help to fix this bug first?
> >>
> >>> -Daniel
> >
> >
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
  

Hi,

On 2023/4/14 15:56, Daniel Vetter wrote:
> On Fri, 14 Apr 2023 at 09:34, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 14.04.23 um 07:36 schrieb Daniel Vetter:
>>> On Fri, 14 Apr 2023 at 06:24, Sui Jingfeng <15330273260@189.cn> wrote:
>>>> Hi,
>>>>
>>>> On 2023/4/14 04:01, Daniel Vetter wrote:
>>>>> On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
>>>>>> Hi
>>>>>>
>>>>>> Am 13.04.23 um 20:56 schrieb Daniel Vetter:
>>>>>> [...]
>>>>>>> This should switch the existing code over to using drm_framebuffer instead
>>>>>>> of fbdev:
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>>>>> index ef4eb8b12766..99ca69dd432f 100644
>>>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>>>> @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
>>>>>>>      static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
>>>>>>>                                              struct drm_rect *clip)
>>>>>>>      {
>>>>>>> +   struct drm_fb_helper *helper = info->par;
>>>>>>> +
>>>>>>>       off_t end = off + len;
>>>>>>>       u32 x1 = 0;
>>>>>>>       u32 y1 = off / info->fix.line_length;
>>>>>>> -   u32 x2 = info->var.xres;
>>>>>>> -   u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
>>>>>>> +   u32 x2 = helper->fb->height;
>>>>>>> +   unsigned stride = helper->fb->pitches[0];
>>>>>>> +   u32 y2 = DIV_ROUND_UP(end, stride);
>>>>>>> +   int bpp = drm_format_info_bpp(helper->fb->format, 0);
>>>>>> Please DONT do that. The code here is fbdev code and shouldn't bother about
>>>>>> DRM data structures. Actually, it shouldn't be here: a number of fbdev
>>>>>> drivers with deferred I/O contain similar code and the fbdev module should
>>>>>> provide us with a helper. (I think I even had some patches somewhere.)
>>>>> Well my thinking is that it's a drm driver,
>>>> Yes, I actually agree with this, and the code looks quite good. And I
>>>> really want to adopt it.
>>>>
>>>> Because here is DRM, we should emulate the fbdev in the DRM's way.
>>>>
>>>> The DRM is really the big brother on the behind of emulated fbdev,
>>>>
>>>> who provide the real displayable backing memory and scant out engine to
>>>> display such a buffer.
>>>>
>>>>
>>>> But currently, the fact is,  drm_fb_helper.c still initializes lots of
>>>> data structure come from fbdev world.
>>>>
>>>> For example, the drm_fb_helper_fill_fix() and drm_fb_helper_fill_var()
>>>> in drm_fb_helper_fill_info() function.
>>>>
>>>> This saying implicitly that the fbdev-generic is a glue layer which copy
>>>> damage frame buffer data
>>>>
>>>> from the front end(fbdev layer) to its drm backend.  It's not a 100%
>>>> replacement its fbdev front end,
>>>>
>>>> rather, it relay on it.
>>>>
>>>>
>>>>> so if we have issue with limit
>>>>> checks blowing up it makes more sense to check them against drm limits.
>>>>> Plus a lot more people understand those than fbdev. They should all match
>>>>> anyway, or if they dont, we have a bug.
>>>> Yes, this is really what I'm worry about.
>>>>
>>>> I see that  members of struct fb_var_screeninfo can be changed by
>>>> user-space. For example, xoffset and yoffset.
>>>>
>>>> There is no change about 'info->var.xres' and 'info->var.yres' from the
>>>> userspace,
>>>>
>>>> therefore, they should all match in practice.
>>>>
>>>>
>>>> User-space can choose a use a smaller dispaly area than 'var.xres x
>>>> var.yres',
>>>>
>>>> but that is implemented with 'var.xoffset' and 'var.xoffset'.
>>>>
>>>> But consider that the name 'var', which may stand for 'variation' or
>>>> 'vary'. This is terrifying.
>>>>
>>>> I imagine, with a shadow buffer, the front end can do any modeset on the
>>>> runtime freely,
>>>>
>>>> including change the format of frame buffer on the runtime.  Just do not
>>>> out-of-bound.
>>>>
>>>> The middle do the conversion on the fly.
>>>>
>>>>
>>>> We might also create double shadow buffer size to allow the front end to
>>>> pan?
>>> Yeah so the front should be able to pan. And the front-end can
>>> actually make xres/yres smalle than drm_fb->height/width, so we _have_
>>> to use the fb side of things. Otherwise it's a bug I just realized.
>> What are you talking about?  The GEM buffer is a full fbdev framebuffer,
>> which is screen resolution multiplied by the overall factor.  The shadow
>> buffer is exactly the same size. You can already easily pan within these
>> buffers.
>>
>> There's also no need/way to change video modes or formats in the shadow
>> buffer. If we'd ever support that, it would be implemented in the DRM
>> driver's modesetting.  The relationship between GEM buffer and shadow
>> buffer remains unaffected by this.
> Try it and be amazed :-) I've seen enough syzkaller bugs and screamed
> at them that yes we do this. Also xres/yres is the wrong thing even if
> you don't use fb ioctl to change things up in multi-monitor cases (we
> allocate the drm_fb/fbdev virtual size to match the biggest
> resolution, but then set fbinfo->var.x/yres to match the smallest to
> make sure fbcon is fully visible everywhere).
>
> I think you're confusion is the perfect case for why we really should
> use fb->height/width/pitches[0] here.
> -Daniel

Exactly,

This what I'm worry about, if someone add code with changing xres/yres 
from userspace

via fb ioctl implemented.  Then, xres/yres and 
fb->height/width/pitches[0] may not match.

so fetch data from fbinfo->var.x/yres still more safe.


Yet, on our platform with drm/loongson driver with two screen,  I just 
tested,

fbinfo->var.x/yres do match the smallest of the display resolution.

V2 of this patch also have been respin, and sent, welcome to review.


It also will be proved that it's more easy  to do the modeset on the 
emulated fbdev side.

It just adjust a few parameter, without the need of touch the real 
hardware.

The drm/GEM backend side do not need to do anything, just using their 
native display mode supported respectively.

Which allow the emulated fbdev side use the smaller display area freely 
and may support any resolution smaller

than the common display area.

>> Best regards
>> Thomas
>>
>>> The xres_virtual/yres_virtual should always match drm_fb sizes (but
>>> we've had bugs in the past for that, only recently fixed all in
>>> linux-next), because that's supposed to be the max size. And since we
>>> never reallocate the fbdev emulation fb (at least with the current
>>> code) this should never change.
>>>
>>> But fundamentally you're bringing up a very good point, we've had
>>> piles of bugs in the past with not properly validating the fbdev side
>>> information in info->var, and a bunch of resulting bugs. So validating
>>> against the drm side of things should be a bit more robust.
>>>
>>> It's kinda the same we do for legacy kms ioctls: We translate that to
>>> atomic kms as fast as possible, and then do the entire subsequent
>>> validation with atomic kms data structures.
>>> -Daniel
>>>
>>>>> The thing is, if you change this
>>>>> further to just pass the drm_framebuffer, then this 100% becomes a drm
>>>>> function, which could be used by anything in drm really.
>>>> I agree with you.
>>>>
>>>> If I use fb_width/fb_height directly and bypassing 'info->var.xres" and
>>>> "info->var.yres",
>>>>
>>>> the code style diverged then. As far as I am understanding,  the clip
>>>> happen on the front end,
>>>>
>>>> the actual damage update happen at back end.
>>>>
>>>> Using the data structure come with the front end is more reasonable for
>>>> current implement.
>>>>
>>>>> But also *shrug*.
>>>> I can convert this single function to 100% drm with another patch.
>>>>
>>>> But, maybe there also have other functions are not 100% drm
>>>>
>>>> I would like do something to help achieve this in the future,
>>>>
>>>> let me help to fix this bug first?
>>>>
>>>>> -Daniel
>>>
>>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
>
>
  

On Fri, Apr 14, 2023 at 07:30:53PM +0800, Sui Jingfeng wrote:
> Hi,
> 
> On 2023/4/14 15:56, Daniel Vetter wrote:
> > On Fri, 14 Apr 2023 at 09:34, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > > Hi
> > > 
> > > Am 14.04.23 um 07:36 schrieb Daniel Vetter:
> > > > On Fri, 14 Apr 2023 at 06:24, Sui Jingfeng <15330273260@189.cn> wrote:
> > > > > Hi,
> > > > > 
> > > > > On 2023/4/14 04:01, Daniel Vetter wrote:
> > > > > > On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
> > > > > > > Hi
> > > > > > > 
> > > > > > > Am 13.04.23 um 20:56 schrieb Daniel Vetter:
> > > > > > > [...]
> > > > > > > > This should switch the existing code over to using drm_framebuffer instead
> > > > > > > > of fbdev:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > > > > > index ef4eb8b12766..99ca69dd432f 100644
> > > > > > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > > > > > @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> > > > > > > >      static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
> > > > > > > >                                              struct drm_rect *clip)
> > > > > > > >      {
> > > > > > > > +   struct drm_fb_helper *helper = info->par;
> > > > > > > > +
> > > > > > > >       off_t end = off + len;
> > > > > > > >       u32 x1 = 0;
> > > > > > > >       u32 y1 = off / info->fix.line_length;
> > > > > > > > -   u32 x2 = info->var.xres;
> > > > > > > > -   u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> > > > > > > > +   u32 x2 = helper->fb->height;
> > > > > > > > +   unsigned stride = helper->fb->pitches[0];
> > > > > > > > +   u32 y2 = DIV_ROUND_UP(end, stride);
> > > > > > > > +   int bpp = drm_format_info_bpp(helper->fb->format, 0);
> > > > > > > Please DONT do that. The code here is fbdev code and shouldn't bother about
> > > > > > > DRM data structures. Actually, it shouldn't be here: a number of fbdev
> > > > > > > drivers with deferred I/O contain similar code and the fbdev module should
> > > > > > > provide us with a helper. (I think I even had some patches somewhere.)
> > > > > > Well my thinking is that it's a drm driver,
> > > > > Yes, I actually agree with this, and the code looks quite good. And I
> > > > > really want to adopt it.
> > > > > 
> > > > > Because here is DRM, we should emulate the fbdev in the DRM's way.
> > > > > 
> > > > > The DRM is really the big brother on the behind of emulated fbdev,
> > > > > 
> > > > > who provide the real displayable backing memory and scant out engine to
> > > > > display such a buffer.
> > > > > 
> > > > > 
> > > > > But currently, the fact is,  drm_fb_helper.c still initializes lots of
> > > > > data structure come from fbdev world.
> > > > > 
> > > > > For example, the drm_fb_helper_fill_fix() and drm_fb_helper_fill_var()
> > > > > in drm_fb_helper_fill_info() function.
> > > > > 
> > > > > This saying implicitly that the fbdev-generic is a glue layer which copy
> > > > > damage frame buffer data
> > > > > 
> > > > > from the front end(fbdev layer) to its drm backend.  It's not a 100%
> > > > > replacement its fbdev front end,
> > > > > 
> > > > > rather, it relay on it.
> > > > > 
> > > > > 
> > > > > > so if we have issue with limit
> > > > > > checks blowing up it makes more sense to check them against drm limits.
> > > > > > Plus a lot more people understand those than fbdev. They should all match
> > > > > > anyway, or if they dont, we have a bug.
> > > > > Yes, this is really what I'm worry about.
> > > > > 
> > > > > I see that  members of struct fb_var_screeninfo can be changed by
> > > > > user-space. For example, xoffset and yoffset.
> > > > > 
> > > > > There is no change about 'info->var.xres' and 'info->var.yres' from the
> > > > > userspace,
> > > > > 
> > > > > therefore, they should all match in practice.
> > > > > 
> > > > > 
> > > > > User-space can choose a use a smaller dispaly area than 'var.xres x
> > > > > var.yres',
> > > > > 
> > > > > but that is implemented with 'var.xoffset' and 'var.xoffset'.
> > > > > 
> > > > > But consider that the name 'var', which may stand for 'variation' or
> > > > > 'vary'. This is terrifying.
> > > > > 
> > > > > I imagine, with a shadow buffer, the front end can do any modeset on the
> > > > > runtime freely,
> > > > > 
> > > > > including change the format of frame buffer on the runtime.  Just do not
> > > > > out-of-bound.
> > > > > 
> > > > > The middle do the conversion on the fly.
> > > > > 
> > > > > 
> > > > > We might also create double shadow buffer size to allow the front end to
> > > > > pan?
> > > > Yeah so the front should be able to pan. And the front-end can
> > > > actually make xres/yres smalle than drm_fb->height/width, so we _have_
> > > > to use the fb side of things. Otherwise it's a bug I just realized.
> > > What are you talking about?  The GEM buffer is a full fbdev framebuffer,
> > > which is screen resolution multiplied by the overall factor.  The shadow
> > > buffer is exactly the same size. You can already easily pan within these
> > > buffers.
> > > 
> > > There's also no need/way to change video modes or formats in the shadow
> > > buffer. If we'd ever support that, it would be implemented in the DRM
> > > driver's modesetting.  The relationship between GEM buffer and shadow
> > > buffer remains unaffected by this.
> > Try it and be amazed :-) I've seen enough syzkaller bugs and screamed
> > at them that yes we do this. Also xres/yres is the wrong thing even if
> > you don't use fb ioctl to change things up in multi-monitor cases (we
> > allocate the drm_fb/fbdev virtual size to match the biggest
> > resolution, but then set fbinfo->var.x/yres to match the smallest to
> > make sure fbcon is fully visible everywhere).
> > 
> > I think you're confusion is the perfect case for why we really should
> > use fb->height/width/pitches[0] here.
> > -Daniel
> 
> Exactly,
> 
> This what I'm worry about, if someone add code with changing xres/yres from
> userspace
> 
> via fb ioctl implemented.  Then, xres/yres and fb->height/width/pitches[0]
> may not match.
> 
> so fetch data from fbinfo->var.x/yres still more safe.

Other way round, because what we stuff into the drm must be limited by
fb->height/width/pitches. If fbinfo->var.x/yres is wrong, then that's a
pontential problem.

> Yet, on our platform with drm/loongson driver with two screen,  I just
> tested,
> 
> fbinfo->var.x/yres do match the smallest of the display resolution.
> 
> V2 of this patch also have been respin, and sent, welcome to review.
> 
> 
> It also will be proved that it's more easy  to do the modeset on the
> emulated fbdev side.
> 
> It just adjust a few parameter, without the need of touch the real hardware.
> 
> The drm/GEM backend side do not need to do anything, just using their native
> display mode supported respectively.
> 
> Which allow the emulated fbdev side use the smaller display area freely and
> may support any resolution smaller
> 
> than the common display area.

This is already what's happening. Which is why looking at fbinfo->var is
risky (it can change and doesn't have a strict relationship to the reality
on the drm modeset side). Whereas drm_fb data is fixed, and is always
correct.

Note that it's not a problem when part of our damage clip is not visible,
the drm drivers take care of that. The only thing we must ensure is that
the damage rects are within the drm_framebuffer coodrinations.

The other thing I just realized is that we also allow changing
fbinfo->var.x/yoffset. Which means just checking against info->var.x/yres
is wrong when the buffer is panned. If we instead just check against
drm_framebuffer coordinations then we don't have that issue.

Also on this topic, there's als fb->offset[0]. If that's not zero, we
again have the issue that we limit the damage rect to the wrong area, but
luckily that's always zero. Would still be good to put a

	WARN_ON_ONCE(fb->offsets[0] != 0);

in the code to document that assumption.

Can you please respin v3 with all that?

Thanks, Daniel

> 
> > > Best regards
> > > Thomas
> > > 
> > > > The xres_virtual/yres_virtual should always match drm_fb sizes (but
> > > > we've had bugs in the past for that, only recently fixed all in
> > > > linux-next), because that's supposed to be the max size. And since we
> > > > never reallocate the fbdev emulation fb (at least with the current
> > > > code) this should never change.
> > > > 
> > > > But fundamentally you're bringing up a very good point, we've had
> > > > piles of bugs in the past with not properly validating the fbdev side
> > > > information in info->var, and a bunch of resulting bugs. So validating
> > > > against the drm side of things should be a bit more robust.
> > > > 
> > > > It's kinda the same we do for legacy kms ioctls: We translate that to
> > > > atomic kms as fast as possible, and then do the entire subsequent
> > > > validation with atomic kms data structures.
> > > > -Daniel
> > > > 
> > > > > > The thing is, if you change this
> > > > > > further to just pass the drm_framebuffer, then this 100% becomes a drm
> > > > > > function, which could be used by anything in drm really.
> > > > > I agree with you.
> > > > > 
> > > > > If I use fb_width/fb_height directly and bypassing 'info->var.xres" and
> > > > > "info->var.yres",
> > > > > 
> > > > > the code style diverged then. As far as I am understanding,  the clip
> > > > > happen on the front end,
> > > > > 
> > > > > the actual damage update happen at back end.
> > > > > 
> > > > > Using the data structure come with the front end is more reasonable for
> > > > > current implement.
> > > > > 
> > > > > > But also *shrug*.
> > > > > I can convert this single function to 100% drm with another patch.
> > > > > 
> > > > > But, maybe there also have other functions are not 100% drm
> > > > > 
> > > > > I would like do something to help achieve this in the future,
> > > > > 
> > > > > let me help to fix this bug first?
> > > > > 
> > > > > > -Daniel
> > > > 
> > > > 
> > > --
> > > Thomas Zimmermann
> > > Graphics Driver Developer
> > > SUSE Software Solutions Germany GmbH
> > > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > (HRB 36809, AG Nürnberg)
> > > Geschäftsführer: Ivo Totev
> > 
> >
  

Hi

Am 14.04.23 um 09:56 schrieb Daniel Vetter:
> On Fri, 14 Apr 2023 at 09:34, Thomas Zimmermann <tzimmermann@suse.de> wrote:
[...]
>>
>> There's also no need/way to change video modes or formats in the shadow
>> buffer. If we'd ever support that, it would be implemented in the DRM
>> driver's modesetting.  The relationship between GEM buffer and shadow
>> buffer remains unaffected by this.
> 
> Try it and be amazed :-)

It's not supported. I don't know if we catch all the cases, but at least 
we try. And I don't think we will ever support changes to the video 
mode. The framebuffer width/height binds us to certain constrains during 
the damage handling.  Figuring all this out is probably not worth the 
effort.

> I've seen enough syzkaller bugs and screamed
> at them that yes we do this. Also xres/yres is the wrong thing even if
> you don't use fb ioctl to change things up in multi-monitor cases (we
> allocate the drm_fb/fbdev virtual size to match the biggest
> resolution, but then set fbinfo->var.x/yres to match the smallest to
> make sure fbcon is fully visible everywhere).
> 
> I think you're confusion is the perfect case for why we really should
> use fb->height/width/pitches[0] here.

I really don't see the point of building a DRM-only variant when there's 
the same code in fbdev drivers. Required information is all stored in 
the fb_info. The helper code should be seen as part of fbdev's deferred I/O.

This, however, is independent from the limitation where the memory size 
has to be a multiple of the framebuffer resolution. That's a limitation 
imposed by DRM. Please also note that this is only relevant for 
fbdev-generic. I intent to move some of those damage helpers there. I'd 
assume that will make the whole thing a bit more understandable. 
(Unfortunately, the fbdev emulation has been a victim of false starts 
and complexity. It takes time to fix all this.)

I'm not sure why you refer to xres/yres; I think, the smem_length and 
line_length is what we'd need in most cases.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> The xres_virtual/yres_virtual should always match drm_fb sizes (but
>>> we've had bugs in the past for that, only recently fixed all in
>>> linux-next), because that's supposed to be the max size. And since we
>>> never reallocate the fbdev emulation fb (at least with the current
>>> code) this should never change.
>>>
>>> But fundamentally you're bringing up a very good point, we've had
>>> piles of bugs in the past with not properly validating the fbdev side
>>> information in info->var, and a bunch of resulting bugs. So validating
>>> against the drm side of things should be a bit more robust.
>>>
>>> It's kinda the same we do for legacy kms ioctls: We translate that to
>>> atomic kms as fast as possible, and then do the entire subsequent
>>> validation with atomic kms data structures.
>>> -Daniel
>>>
>>>>> The thing is, if you change this
>>>>> further to just pass the drm_framebuffer, then this 100% becomes a drm
>>>>> function, which could be used by anything in drm really.
>>>>
>>>> I agree with you.
>>>>
>>>> If I use fb_width/fb_height directly and bypassing 'info->var.xres" and
>>>> "info->var.yres",
>>>>
>>>> the code style diverged then. As far as I am understanding,  the clip
>>>> happen on the front end,
>>>>
>>>> the actual damage update happen at back end.
>>>>
>>>> Using the data structure come with the front end is more reasonable for
>>>> current implement.
>>>>
>>>>> But also *shrug*.
>>>>
>>>> I can convert this single function to 100% drm with another patch.
>>>>
>>>> But, maybe there also have other functions are not 100% drm
>>>>
>>>> I would like do something to help achieve this in the future,
>>>>
>>>> let me help to fix this bug first?
>>>>
>>>>> -Daniel
>>>
>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev