Re: drm fb helpers hotplug/resize

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 07, 2022 at 09:10:27AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.10.22 um 04:16 schrieb Zack Rusin:
> > On Thu, 2022-10-06 at 10:01 +0200, Thomas Zimmermann wrote:
> >> Hi Zack
> >>
> >> Am 05.10.22 um 21:49 schrieb Zack Rusin:
> >>> Hi, Thomas.
> >>>
> >>> Because you've been the one who's been working on drm_fb_helper.c the most the last
> >>> few years I wanted to pick your brain a bit.
> >>>
> >>> I was porting vmwgfx to drm_fb_helper code which is largely trivial, just removing
> >>> all of vmwgfx_fb.c and replacing it with a call to drm_fbdev_generic_setup. But
> >>
> >> Thanks a lot for this work. I have been looking into doing this
> >> conversion myself at some point, but never found the time to actually do
> >> it.
> >>
> >>> drm_fb_helper.c code never deals with resizes which is a bit of a problem.
> >>>
> >>> e.g. replacing the drm_sysfs_hotplug_event() call from
> >>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c#L2255
> >>> with drm_kms_helper_hotplug_event will call drm_fbdev_client_hotplug and end up in
> >>> drm_fb_helper_hotplug_event:
> >>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L2003
> >>>
> >>> Now drm_fb_helper_hotplug_event does drm_client_modeset_probe but it never resizes
> >>> drm_fb_helper::buffer and drm_fb_helper::fb so they're both incorrectly sized.
> >>>
> >>> In general I don't see drm_fb_helper code ever being able to deal with resizes. In
> >>> particular because the fbdev's xres_virtual/yres_virtual are sized exactly to the
> >>> initial xres/yres.
> >>>
> >>> It's definitely a lot bigger issue on virtualized environments where at boot we'll
> >>> have some very conservative size (800x600) on vmwgfx which is then usually resized
> >>> to the size of the window. drm_fb_helper breaks pretty bad in that case because it
> >>> can't deal with those resizes at all.
> 
> The initial resolution of 800x600 is imposed by the driver, right? 
> (VMW_MIN_INITIAL_{WIDTH,HEIGHT}) You can use video= on the kernel 
> command line to select a resolution. That gives at least a workaround 
> with fbdev emulation.
> 
> >>>
> >>> Is this scenario something that drm_fb_helper should be able to handle or is it not
> >>> worth pursuing it? I don't think there's a trivial way of handling it so my guess is
> >>> that it would make drm_fb_helper quite a bit more complicated.
> >>
> >> I'm aware that resizing is missing. It's one of the few things I'd like
> >> to see being added to generic fbdev emulation. But as you say, it's not
> >> easy. The generic fbdev emulation has all kinds of code paths for the
> >> various drivers' memory managers. That makes it complicated.
> >>
> >> The problem is that fbdev's mmap'ed memory cannot be reallocated. It is
> >> expected to behave like 'real video memory.' So either reserve a chunk
> >> of the video ram for fbdev's GEM objects or use deferred I/O, which
> >> provides mmaped pages from a shadow buffer in system memory. vmwgfx uses
> >> the latter IIRC.
> >>
> >> But ideally, we'd get rid of most of the shadow buffering and try to
> >> mmap pages directly from GEM objects. For modesetting, this means that
> >> the new mode's framebuffer has to inherit the old framebuffer's buffer
> >> objects. Probably the easiest solution is to allocate a framebuffer once
> >> and reconfigure its parameters (width, height, pitch) on each modeset
> >> operation.
> >>
> >> Switching to a higher resolution would require more video memory.
> >> Although we cannot reallocate, this problem can be solved with the
> >> drm_fbdev_overalloc parameter. It gives the percentage of allocated
> >> video memory. If you start with 800x600 with overalloc at 400, you'd get
> >> enough video memory for 2400 scanlines. This allows for fbdev panning
> >> (i.e., pageflipping). With that extra memory fbdev could switch to
> >> another display mode with a higher resolution. For example, changing to
> >> 1024x786 would result in 1875 scanlines at the given overalloc of 400.
> >>
> >> To implement this, I guess that some of fbdev's memory allocation needs
> >> to be changed. The check_var and set_par code needs an update to handle
> >> the modeset. And I suspect that there are other dark corners that need
> >> to be reworked as well.
> > 
> > That sounds good. In a similar fashion to drm_fbdev_overalloc another, rather hacky
> > but vastly simpler approach, would be to basically allow the drivers to specify the
> > maximum size of fb to support in drm_fbdev_generic_setup. This would just directly
> > set the drm_fb_helper_surface_size::surface_width and surface_height with the end
> > result being that drm_client_framebuffer_create would be called with those values
> > and xres_virtual/yres_virtual would be set to them. Resizing would basically just
> > work then, right? Of course at the cost of possibly large allocation, e.g. 4k fb
> > even when only 800x600 is actually used.
> 
> For the absolute size of fbdev memory, I think we should introduce a 
> module parameter in drm_fb_helper, which an option to set a default 
> value in the kernel config. It would benefit all drivers that use fbdev 
> emulation and work how overalloc works.
> 
> If no size is given, the current approach would be used.
> 
> I don't think resizing would work immediately. There isn't anything in 
> the check_var and set_par functions that implements the necessary atomic 
> check and commit.

set_par() is the thing tht does the commit.

-- 
Ville Syrjälä
Intel