From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 43423C61DA4 for ; Fri, 10 Feb 2023 01:22:00 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7A6A285C5F; Fri, 10 Feb 2023 02:21:57 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 7F05B85CF8; Fri, 10 Feb 2023 02:21:56 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by phobos.denx.de (Postfix) with ESMTP id 714E485C5F for ; Fri, 10 Feb 2023 02:21:53 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=andre.przywara@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 08E9D4B3; Thu, 9 Feb 2023 17:22:35 -0800 (PST) Received: from slackpad.lan (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9FA743F71E; Thu, 9 Feb 2023 17:21:51 -0800 (PST) Date: Fri, 10 Feb 2023 01:19:53 +0000 From: Andre Przywara To: Dzmitry Sankouski Cc: u-boot@lists.denx.de, Anatolij Gustschin , Simon Glass Subject: Re: [PATCH 3/5] video console: add support for fonts wider than 1 byte Message-ID: <20230210011953.44593268@slackpad.lan> In-Reply-To: <20221226194929.166369-4-dsankouski@gmail.com> References: <20221226194929.166369-1-dsankouski@gmail.com> <20221226194929.166369-4-dsankouski@gmail.com> Organization: Arm Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.31; x86_64-slackware-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Mon, 26 Dec 2022 22:49:27 +0300 Dzmitry Sankouski wrote: Hi Dzmitry, > Devices with high ppi may benefit from wider fonts. > > Current width implementation is limited by 1 byte, i.e. 8 bits. > New version iterates VIDEO_FONT_BYTE_WIDTH times, to process all > width bytes, thus allowing fonts wider than 1 byte. In general I think this is probably better than my version, not only because it unifies rotated and normal consoles. Some minor/stylistic comments below. > > Signed-off-by: Dzmitry Sankouski > --- > drivers/video/console_simple.c | 79 ++++++++++++++++++---------------- > include/video_font_4x6.h | 2 + > include/video_font_data.h | 2 + > 3 files changed, 47 insertions(+), 36 deletions(-) > > diff --git a/drivers/video/console_simple.c b/drivers/video/console_simple.c > index 20087a30af..3ae1fbdc89 100644 > --- a/drivers/video/console_simple.c > +++ b/drivers/video/console_simple.c > @@ -61,30 +61,32 @@ static int fill_char_horizontally(uchar *pfont, void **line, struct video_priv * > { > int ret; > void *dst; > - u8 mask = 0x80; > - > + u8 mask; > ret = check_bpix_support(vid_priv->bpix); > if (ret) > return ret; > > - for (int col = 0; col < VIDEO_FONT_WIDTH; col++) { > - dst = *line; > - for (int row = 0; row < VIDEO_FONT_HEIGHT; row++) { > - u32 value = (pfont[row * VIDEO_FONT_BYTE_WIDTH + col] & mask) ? > - vid_priv->colour_fg : > - vid_priv->colour_bg; > - > - ret = fill_pixel_and_goto_next(&dst, > - value, > - VNBYTES(vid_priv->bpix), > - direction > - ); > + for (int col = 0; col < VIDEO_FONT_BYTE_WIDTH; col++) { > + mask = 0x80; > + for (int bit = 0; bit < 8; bit++) { I think it's cleaner to collapse this: for (int col = 0; col < VIDEO_FONT_WIDTH; col++) { if ((col % 8) == 0) mask = 0x80; ... > + dst = *line; > + for (int row = 0; row < VIDEO_FONT_HEIGHT; row++) { > + u32 value = (pfont[row * VIDEO_FONT_BYTE_WIDTH + col] & mask) ? ... and use (col / 8) here. > + vid_priv->colour_fg : > + vid_priv->colour_bg; > + > + ret = fill_pixel_and_goto_next(&dst, > + value, > + VNBYTES(vid_priv->bpix), > + direction > + ); > + } > + if (direction) > + *line += vid_priv->line_length; > + else > + *line -= vid_priv->line_length; > + mask >>= 1; > } > - if (direction) > - *line += vid_priv->line_length; > - else > - *line -= vid_priv->line_length; > - mask >>= 1; > } > return ret; > } > @@ -101,24 +103,29 @@ static int fill_char_vertically(uchar *pfont, void **line, struct video_priv *vi > return ret; > for (int row = 0; row < VIDEO_FONT_HEIGHT; row++) { > dst = *line; > - uchar bits = pfont[row]; > - > - for (int i = 0; i < VIDEO_FONT_WIDTH; i++) { > - u32 value = (bits & 0x80) ? > - vid_priv->colour_fg : > - vid_priv->colour_bg; > - > - ret = fill_pixel_and_goto_next(&dst, > - value, > - VNBYTES(vid_priv->bpix), > - direction > - ); > - bits <<= 1; > + uchar bits; > + > + for (int i = 0; i < VIDEO_FONT_BYTE_WIDTH; i++) { > + bits = pfont[i]; > + for (int bit = 0; bit < 8; bit++) { Same as above, those two loops should become one. > + u32 value = (bits & 0x80) ? > + vid_priv->colour_fg : > + vid_priv->colour_bg; > + > + ret = fill_pixel_and_goto_next(&dst, > + value, > + VNBYTES(vid_priv->bpix), > + direction > + ); > + bits <<= 1; > + } > } > if (direction) > *line -= vid_priv->line_length; > else > *line += vid_priv->line_length; > + > + pfont += VIDEO_FONT_BYTE_WIDTH; > } > return ret; > } > @@ -177,7 +184,7 @@ static int console_putc_xy(struct udevice *dev, uint x_frac, uint y, char ch) > int pbytes = VNBYTES(vid_priv->bpix); > int x, linenum, ret; > void *start, *line; > - uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT; > + uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_CHAR_PIXEL_DATA_SIZE; > > if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac) > return -EAGAIN; > @@ -286,7 +293,7 @@ static int console_putc_xy_1(struct udevice *dev, uint x_frac, uint y, char ch) > int pbytes = VNBYTES(vid_priv->bpix); > int x, linenum, ret; > void *start, *line; > - uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT; > + uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_CHAR_PIXEL_DATA_SIZE; > > if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac) > return -EAGAIN; > @@ -357,7 +364,7 @@ static int console_putc_xy_2(struct udevice *dev, uint x_frac, uint y, char ch) > int pbytes = VNBYTES(vid_priv->bpix); > int linenum, x, ret; > void *start, *line; > - uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT; > + uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_CHAR_PIXEL_DATA_SIZE; > > if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac) > return -EAGAIN; > @@ -432,7 +439,7 @@ static int console_putc_xy_3(struct udevice *dev, uint x_frac, uint y, char ch) > int pbytes = VNBYTES(vid_priv->bpix); > int linenum, x, ret; > void *start, *line; > - uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT; > + uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_CHAR_PIXEL_DATA_SIZE; > > if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac) > return -EAGAIN; > diff --git a/include/video_font_4x6.h b/include/video_font_4x6.h > index c7e6351b64..f1eb2af861 100644 > --- a/include/video_font_4x6.h > +++ b/include/video_font_4x6.h > @@ -43,7 +43,9 @@ __END__; > > #define VIDEO_FONT_CHARS 256 > #define VIDEO_FONT_WIDTH 4 > +#define VIDEO_FONT_BYTE_WIDTH VIDEO_FONT_WIDTH / 8 + (VIDEO_FONT_WIDTH % 8 > 0) I think the canonical way for rounding up is: ((VIDEO_FONT_WIDTH + 7) / 8) And as Simon said: you need parentheses, otherwise it failed for me with a width of 12 (sun12x22 font). > #define VIDEO_FONT_HEIGHT 6 > +#define VIDEO_FONT_CHAR_PIXEL_DATA_SIZE VIDEO_FONT_HEIGHT * VIDEO_FONT_BYTE_WIDTH Those two new lines do not contain any specific data, so can be moved to console_simple.c. > #define VIDEO_FONT_SIZE (VIDEO_FONT_CHARS * VIDEO_FONT_HEIGHT) > > static unsigned char video_fontdata[VIDEO_FONT_SIZE] = { > diff --git a/include/video_font_data.h b/include/video_font_data.h > index 6e64198d1a..6a575e6d15 100644 > --- a/include/video_font_data.h > +++ b/include/video_font_data.h > @@ -11,7 +11,9 @@ > > #define VIDEO_FONT_CHARS 256 > #define VIDEO_FONT_WIDTH 8 > +#define VIDEO_FONT_BYTE_WIDTH VIDEO_FONT_WIDTH / 8 + (VIDEO_FONT_WIDTH % 8 > 0) > #define VIDEO_FONT_HEIGHT 16 > +#define VIDEO_FONT_CHAR_PIXEL_DATA_SIZE VIDEO_FONT_HEIGHT * VIDEO_FONT_BYTE_WIDTH same here, remove from here and use one copy in console_simple.c. Cheers, Andre > #define VIDEO_FONT_SIZE (VIDEO_FONT_CHARS * VIDEO_FONT_HEIGHT) > > static unsigned char __maybe_unused video_fontdata[VIDEO_FONT_SIZE] = {