[PATCH} efifb support for wsmoused + smaller fonts

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH} efifb support for wsmoused + smaller fonts

johnc
The efifb driver behaves almost identically to the inteldrm driver
for wscons, but did not implement the getchar() accessops, so
wsmoused would fail at startup.

Separately, I increased the maximum screen dimensions to 160x50 to
allow the 12x24 font to be used on an 1920 monitor, which looks great!

Unlike the intel driver, efifb has a static buffer of that size,
which is a non-trivial amount of memory.  Not setting the backing
store to save the memory results in an ultra-slow console until the
driver gets fully initialized, because it is doing scrolls by reading
from write combined memory instead of the redrawing all the
characters.

I plan on changing that to a dynamic allocation once I am more
comfortable with the reinitialization that goes on when autoconf
gets around to the console display device, so feel free to punt on
that if you want to wait.

Index: efifb.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/efifb.c,v
retrieving revision 1.27
diff -u -p -r1.27 efifb.c
--- efifb.c 24 Jan 2020 05:27:31 -0000 1.27
+++ efifb.c 24 May 2020 23:32:47 -0000
@@ -100,6 +100,7 @@ int efifb_alloc_screen(void *, const st
 void efifb_free_screen(void *, void *);
 int efifb_show_screen(void *, void *, int, void (*cb) (void *, int,
int),
     void *);
+int efifb_getchar(void *, int, int, struct wsdisplay_charcell *);
 int efifb_list_font(void *, struct wsdisplay_font *);
 int efifb_load_font(void *, void *, struct wsdisplay_font *);
 void efifb_scrollback(void *, void *, int lines);
@@ -114,8 +115,9 @@ const struct cfattach efifb_ca = {
  sizeof(struct efifb_softc), efifb_match, efifb_attach, NULL
 };
 
-#define EFIFB_WIDTH 100
-#define EFIFB_HEIGHT 31
+/* support a 12x24 font on a 1920x1200 screen */
+#define EFIFB_WIDTH 160
+#define EFIFB_HEIGHT 50
 
 struct wsscreen_descr efifb_std_descr = { "std" };
 
@@ -133,6 +135,7 @@ struct wsdisplay_accessops efifb_accesso
  .alloc_screen = efifb_alloc_screen,
  .free_screen = efifb_free_screen,
  .show_screen = efifb_show_screen,
+ .getchar = efifb_getchar,
  .load_font = efifb_load_font,
  .list_font = efifb_list_font,
  .scrollback = efifb_scrollback,
@@ -371,6 +374,15 @@ efifb_show_screen(void *v, void *cookie,
  struct rasops_info *ri = &sc->sc_fb->rinfo;
 
  return rasops_show_screen(ri, cookie, waitok, cb, cb_arg);
+}
+
+int
+efifb_getchar(void *v, int row, int col, struct wsdisplay_charcell
*cell)
+{
+ struct efifb_softc *sc = v;
+ struct rasops_info *ri = &sc->sc_fb->rinfo;
+
+ return rasops_getchar(ri, row, col, cell);
 }
 
 int


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH} efifb support for wsmoused + smaller fonts

Jonathan Gray-11
On Sun, May 24, 2020 at 05:01:16PM -0700, [hidden email] wrote:
> The efifb driver behaves almost identically to the inteldrm driver
> for wscons, but did not implement the getchar() accessops, so
> wsmoused would fail at startup.

This seems reasonable, though your mail client has line wrapped the
patch so it won't apply.  In this case it is simple enough to apply by
hand.

> Separately, I increased the maximum screen dimensions to 160x50 to
> allow the 12x24 font to be used on an 1920 monitor, which looks great!

Something similiar was done and reverted as it somehow broke inteldrm
taking the fb over from efifb on a 4k display.  Perhaps someone with a
4k display can confirm if this is still a problem.

https://marc.info/?l=openbsd-bugs&m=155337866226121&w=2

That was back before we deferred most of inteldrm to when the root fs is
mounted and interrupts are available.  And just before the last big drm
update by the look of it.

----------------------------
revision 1.22
date: 2019/03/25 14:17:58;  author: fcambus;  state: Exp;  lines: +3 -3;  commitid: hL9P1vyWGBp5FHhE;
Revert back to using previous values for EFIFB_WIDTH and EFIFB_HEIGHT,
as raising them expose an issue which breaks inteldrm on large screen
resolutions.

Reported by chris@, and by Lucas Raab on bugs@. Thanks!
----------------------------
revision 1.21
date: 2019/03/16 13:16:49;  author: fcambus;  state: Exp;  lines: +3 -3;  commitid: wsqjyS7rXDKbqGS3;
Now that efifb(4) supports remapping the framebuffer in write combining
mode, it's fast enough to raise the maximum size of the EFI framebuffer
console.

Set it to 160 columns and 160 rows, to match inteldrm and radeondrm.

OK jcs@, kettenis@
----------------------------

>
> Unlike the intel driver, efifb has a static buffer of that size,
> which is a non-trivial amount of memory.  Not setting the backing
> store to save the memory results in an ultra-slow console until the
> driver gets fully initialized, because it is doing scrolls by reading
> from write combined memory instead of the redrawing all the
> characters.
>
> I plan on changing that to a dynamic allocation once I am more
> comfortable with the reinitialization that goes on when autoconf
> gets around to the console display device, so feel free to punt on
> that if you want to wait.

Drivers which can act as a console have a cnattach function for very
early init and then later probe again and run attach.  In the case of
efifb there is also efifb_cnremap() which is called when mainbus
attaches.  The drm drivers call efifb_detach() to stop autoconf from
doing the normal probe/attach when taking over the console.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH} efifb support for wsmoused + smaller fonts

Lucas Raab
On Mon, May 25, 2020 at 09:13:43PM +1000, Jonathan Gray wrote:

> On Sun, May 24, 2020 at 05:01:16PM -0700, [hidden email] wrote:
> > The efifb driver behaves almost identically to the inteldrm driver
> > for wscons, but did not implement the getchar() accessops, so
> > wsmoused would fail at startup.
>
> This seems reasonable, though your mail client has line wrapped the
> patch so it won't apply.  In this case it is simple enough to apply by
> hand.
>
> > Separately, I increased the maximum screen dimensions to 160x50 to
> > allow the 12x24 font to be used on an 1920 monitor, which looks great!
>
> Something similiar was done and reverted as it somehow broke inteldrm
> taking the fb over from efifb on a 4k display.  Perhaps someone with a
> 4k display can confirm if this is still a problem.
>
> https://marc.info/?l=openbsd-bugs&m=155337866226121&w=2
>
> That was back before we deferred most of inteldrm to when the root fs is
> mounted and interrupts are available.  And just before the last big drm
> update by the look of it.

I tried out the patch twice, once with John's value for EFIFB_HEIGHT of
50 and once with the original original value of 160. The behavior of
the bug report I submitted doesn't reappear so it looks everything is
working as intended. This is with the same monitor as in the bug report

>
> ----------------------------
> revision 1.22
> date: 2019/03/25 14:17:58;  author: fcambus;  state: Exp;  lines: +3 -3;  commitid: hL9P1vyWGBp5FHhE;
> Revert back to using previous values for EFIFB_WIDTH and EFIFB_HEIGHT,
> as raising them expose an issue which breaks inteldrm on large screen
> resolutions.
>
> Reported by chris@, and by Lucas Raab on bugs@. Thanks!
> ----------------------------
> revision 1.21
> date: 2019/03/16 13:16:49;  author: fcambus;  state: Exp;  lines: +3 -3;  commitid: wsqjyS7rXDKbqGS3;
> Now that efifb(4) supports remapping the framebuffer in write combining
> mode, it's fast enough to raise the maximum size of the EFI framebuffer
> console.
>
> Set it to 160 columns and 160 rows, to match inteldrm and radeondrm.
>
> OK jcs@, kettenis@
> ----------------------------
>
> >
> > Unlike the intel driver, efifb has a static buffer of that size,
> > which is a non-trivial amount of memory.  Not setting the backing
> > store to save the memory results in an ultra-slow console until the
> > driver gets fully initialized, because it is doing scrolls by reading
> > from write combined memory instead of the redrawing all the
> > characters.
> >
> > I plan on changing that to a dynamic allocation once I am more
> > comfortable with the reinitialization that goes on when autoconf
> > gets around to the console display device, so feel free to punt on
> > that if you want to wait.
>
> Drivers which can act as a console have a cnattach function for very
> early init and then later probe again and run attach.  In the case of
> efifb there is also efifb_cnremap() which is called when mainbus
> attaches.  The drm drivers call efifb_detach() to stop autoconf from
> doing the normal probe/attach when taking over the console.
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH} efifb support for wsmoused + smaller fonts

Mark Kettenis
In reply to this post by Jonathan Gray-11
> Date: Mon, 25 May 2020 21:13:43 +1000
> From: Jonathan Gray <[hidden email]>
>
> On Sun, May 24, 2020 at 05:01:16PM -0700, [hidden email] wrote:
> > The efifb driver behaves almost identically to the inteldrm driver
> > for wscons, but did not implement the getchar() accessops, so
> > wsmoused would fail at startup.
>
> This seems reasonable, though your mail client has line wrapped the
> patch so it won't apply.  In this case it is simple enough to apply by
> hand.
>
> > Separately, I increased the maximum screen dimensions to 160x50 to
> > allow the 12x24 font to be used on an 1920 monitor, which looks great!
>
> Something similiar was done and reverted as it somehow broke inteldrm
> taking the fb over from efifb on a 4k display.  Perhaps someone with a
> 4k display can confirm if this is still a problem.
>
> https://marc.info/?l=openbsd-bugs&m=155337866226121&w=2
>
> That was back before we deferred most of inteldrm to when the root fs is
> mounted and interrupts are available.  And just before the last big drm
> update by the look of it.

I think we also handle "stolen" memory a bit better now.  Enough
reason to believe that this may be fixed now that we can change it
again and see what happens?

> ----------------------------
> revision 1.22
> date: 2019/03/25 14:17:58;  author: fcambus;  state: Exp;  lines: +3 -3;  commitid: hL9P1vyWGBp5FHhE;
> Revert back to using previous values for EFIFB_WIDTH and EFIFB_HEIGHT,
> as raising them expose an issue which breaks inteldrm on large screen
> resolutions.
>
> Reported by chris@, and by Lucas Raab on bugs@. Thanks!
> ----------------------------
> revision 1.21
> date: 2019/03/16 13:16:49;  author: fcambus;  state: Exp;  lines: +3 -3;  commitid: wsqjyS7rXDKbqGS3;
> Now that efifb(4) supports remapping the framebuffer in write combining
> mode, it's fast enough to raise the maximum size of the EFI framebuffer
> console.
>
> Set it to 160 columns and 160 rows, to match inteldrm and radeondrm.
>
> OK jcs@, kettenis@
> ----------------------------
>
> >
> > Unlike the intel driver, efifb has a static buffer of that size,
> > which is a non-trivial amount of memory.  Not setting the backing
> > store to save the memory results in an ultra-slow console until the
> > driver gets fully initialized, because it is doing scrolls by reading
> > from write combined memory instead of the redrawing all the
> > characters.

Yes, that's why this "shadow buffer" was implemented.

> > I plan on changing that to a dynamic allocation once I am more
> > comfortable with the reinitialization that goes on when autoconf
> > gets around to the console display device, so feel free to punt on
> > that if you want to wait.
>
> Drivers which can act as a console have a cnattach function for very
> early init and then later probe again and run attach.  In the case of
> efifb there is also efifb_cnremap() which is called when mainbus
> attaches.  The drm drivers call efifb_detach() to stop autoconf from
> doing the normal probe/attach when taking over the console.

Be aware that the first initailization (efifb_cnattach) happens before
allocating memory is possible; even pmap_steal_memory() won't work at
this point.  And we want to be able to print messages as early as
possible in the kernel.

The second initialization happens bear the start of autoconf straight
after printing "mainbus at root".  At this point memory allocation is
possible the normal way.  We don't print a lot of information between
these steps and shouldn't need to do any scrolling.  So maybe a
solution where we initialize without the RI_WRONLY flag and add it
later is feasable.  We need to keep track what has been printed though
such that the shadow buffer can be pre-initialized with watever is
already on the screen at that point.  That could be done in a simple
char buffer though, which takes up much less space.  In fact, it may
be possible to simply replay the contents of msgbuf to fill the shadow
buffer.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH} efifb support for wsmoused + smaller fonts

Jonathan Gray-11
In reply to this post by Lucas Raab
On Mon, May 25, 2020 at 08:49:23AM -0500, Lucas Raab wrote:

> On Mon, May 25, 2020 at 09:13:43PM +1000, Jonathan Gray wrote:
> > On Sun, May 24, 2020 at 05:01:16PM -0700, [hidden email] wrote:
> > > The efifb driver behaves almost identically to the inteldrm driver
> > > for wscons, but did not implement the getchar() accessops, so
> > > wsmoused would fail at startup.
> >
> > This seems reasonable, though your mail client has line wrapped the
> > patch so it won't apply.  In this case it is simple enough to apply by
> > hand.
> >
> > > Separately, I increased the maximum screen dimensions to 160x50 to
> > > allow the 12x24 font to be used on an 1920 monitor, which looks great!
> >
> > Something similiar was done and reverted as it somehow broke inteldrm
> > taking the fb over from efifb on a 4k display.  Perhaps someone with a
> > 4k display can confirm if this is still a problem.
> >
> > https://marc.info/?l=openbsd-bugs&m=155337866226121&w=2
> >
> > That was back before we deferred most of inteldrm to when the root fs is
> > mounted and interrupts are available.  And just before the last big drm
> > update by the look of it.
>
> I tried out the patch twice, once with John's value for EFIFB_HEIGHT of
> 50 and once with the original original value of 160. The behavior of
> the bug report I submitted doesn't reappear so it looks everything is
> working as intended. This is with the same monitor as in the bug report

Thanks for testing.  I've committed the wsmoused part on it's own, so
how about we try rev 1.21 again?

Index: efifb.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/efifb.c,v
retrieving revision 1.30
diff -u -p -r1.30 efifb.c
--- efifb.c 25 May 2020 14:12:04 -0000 1.30
+++ efifb.c 25 May 2020 14:20:25 -0000
@@ -115,8 +115,8 @@ const struct cfattach efifb_ca = {
  sizeof(struct efifb_softc), efifb_match, efifb_attach, NULL
 };
 
-#define EFIFB_WIDTH 100
-#define EFIFB_HEIGHT 31
+#define EFIFB_WIDTH 160
+#define EFIFB_HEIGHT 160
 
 struct wsscreen_descr efifb_std_descr = { "std" };
 

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH} efifb support for wsmoused + smaller fonts

Mark Kettenis
> Date: Tue, 26 May 2020 00:27:31 +1000
> From: Jonathan Gray <[hidden email]>
>
> On Mon, May 25, 2020 at 08:49:23AM -0500, Lucas Raab wrote:
> > On Mon, May 25, 2020 at 09:13:43PM +1000, Jonathan Gray wrote:
> > > On Sun, May 24, 2020 at 05:01:16PM -0700, [hidden email] wrote:
> > > > The efifb driver behaves almost identically to the inteldrm driver
> > > > for wscons, but did not implement the getchar() accessops, so
> > > > wsmoused would fail at startup.
> > >
> > > This seems reasonable, though your mail client has line wrapped the
> > > patch so it won't apply.  In this case it is simple enough to apply by
> > > hand.
> > >
> > > > Separately, I increased the maximum screen dimensions to 160x50 to
> > > > allow the 12x24 font to be used on an 1920 monitor, which looks great!
> > >
> > > Something similiar was done and reverted as it somehow broke inteldrm
> > > taking the fb over from efifb on a 4k display.  Perhaps someone with a
> > > 4k display can confirm if this is still a problem.
> > >
> > > https://marc.info/?l=openbsd-bugs&m=155337866226121&w=2
> > >
> > > That was back before we deferred most of inteldrm to when the root fs is
> > > mounted and interrupts are available.  And just before the last big drm
> > > update by the look of it.
> >
> > I tried out the patch twice, once with John's value for EFIFB_HEIGHT of
> > 50 and once with the original original value of 160. The behavior of
> > the bug report I submitted doesn't reappear so it looks everything is
> > working as intended. This is with the same monitor as in the bug report
>
> Thanks for testing.  I've committed the wsmoused part on it's own, so
> how about we try rev 1.21 again?

ok kettenis@

This adds roughly 175KB of uninitialized memory to the kernel (.bss),
but that is probably acceptable since it doesn't occupy any disk space.


> Index: efifb.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/efifb.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 efifb.c
> --- efifb.c 25 May 2020 14:12:04 -0000 1.30
> +++ efifb.c 25 May 2020 14:20:25 -0000
> @@ -115,8 +115,8 @@ const struct cfattach efifb_ca = {
>   sizeof(struct efifb_softc), efifb_match, efifb_attach, NULL
>  };
>  
> -#define EFIFB_WIDTH 100
> -#define EFIFB_HEIGHT 31
> +#define EFIFB_WIDTH 160
> +#define EFIFB_HEIGHT 160
>  
>  struct wsscreen_descr efifb_std_descr = { "std" };
>  
>
>