Hi Am 14.04.23 um 09:56 schrieb Daniel Vetter:
On Fri, 14 Apr 2023 at 09:34, Thomas Zimmermann <tzimmermann@xxxxxxx> 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
-DanielBest regards ThomasThe 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. -DanielThe 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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
