diff mbox series

[v2,06/10] drm/fb-helper: Add support for DRM_FORMAT_C[124]

Message ID c665d6ba072f3debb3fa0a139454fefef6a55011.1646683502.git.geert@linux-m68k.org
State Superseded
Headers show
Series drm: Add support for low-color frame buffer formats | expand

Commit Message

Geert Uytterhoeven March 7, 2022, 8:52 p.m. UTC
Add support for color-indexed frame buffer formats with two, four, and
sixteen colors to the DRM framebuffer helper functions:
  1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
  2. For color-indexed modes, the length of the color bitfields must be
     set to the color depth, else the logo code may pick a logo with too
     many colors.  Drop the incorrect DAC width comment, which
     originates from the i915 driver.
  3. Accept C[124] modes when validating or filling in struct
     fb_var_screeninfo, and use the correct number of bits per pixel.
  4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
     modes.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - Use drm_format_info_bpp() helper instead of deprecated .depth field
    or format-dependent calculations,
  - Use new .is_color_indexed field instead of checking against a list
    of formats.
---
 drivers/gpu/drm/drm_fb_helper.c | 101 ++++++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 26 deletions(-)

Comments

Javier Martinez Canillas March 9, 2022, 1:10 p.m. UTC | #1
On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Add support for color-indexed frame buffer formats with two, four, and
> sixteen colors to the DRM framebuffer helper functions:
>   1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
>   2. For color-indexed modes, the length of the color bitfields must be
>      set to the color depth, else the logo code may pick a logo with too
>      many colors.  Drop the incorrect DAC width comment, which
>      originates from the i915 driver.
>   3. Accept C[124] modes when validating or filling in struct
>      fb_var_screeninfo, and use the correct number of bits per pixel.
>   4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
>      modes.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

[snip]

>  static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> -				   uint32_t depth)
> +				   bool is_color_indexed)
>  {
>  	info->fix.type = FB_TYPE_PACKED_PIXELS;
> -	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> -		FB_VISUAL_TRUECOLOR;
> +	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
> +					    : info->fix.visual;

Shouldn't this be the following instead ?

  +	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
  +					    : FB_VISUAL_TRUECOLOR;

Other than that the patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Geert Uytterhoeven March 9, 2022, 1:14 p.m. UTC | #2
Hi Javier,

On Wed, Mar 9, 2022 at 2:10 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 3/7/22 21:52, Geert Uytterhoeven wrote:
> > Add support for color-indexed frame buffer formats with two, four, and
> > sixteen colors to the DRM framebuffer helper functions:
> >   1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
> >   2. For color-indexed modes, the length of the color bitfields must be
> >      set to the color depth, else the logo code may pick a logo with too
> >      many colors.  Drop the incorrect DAC width comment, which
> >      originates from the i915 driver.
> >   3. Accept C[124] modes when validating or filling in struct
> >      fb_var_screeninfo, and use the correct number of bits per pixel.
> >   4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
> >      modes.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> [snip]
>
> >  static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> > -                                uint32_t depth)
> > +                                bool is_color_indexed)
> >  {
> >       info->fix.type = FB_TYPE_PACKED_PIXELS;
> > -     info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> > -             FB_VISUAL_TRUECOLOR;
> > +     info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
> > +                                         : info->fix.visual;
>
> Shouldn't this be the following instead ?
>
>   +     info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
>   +                                         : FB_VISUAL_TRUECOLOR;

Indeed. Will fix in v3.
Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ed43b987d306afce..ba1c303a294d8c6f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -376,12 +376,31 @@  static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
 					   struct iosys_map *dst)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
-	unsigned int cpp = fb->format->cpp[0];
-	size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
-	void *src = fb_helper->fbdev->screen_buffer + offset;
-	size_t len = (clip->x2 - clip->x1) * cpp;
+	size_t offset = clip->y1 * fb->pitches[0];
+	size_t len = clip->x2 - clip->x1;
 	unsigned int y;
+	void *src;
 
+	switch (drm_format_info_bpp(fb->format, 0)) {
+	case 1:
+		offset += clip->x1 / 8;
+		len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
+		break;
+	case 2:
+		offset += clip->x1 / 4;
+		len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
+		break;
+	case 4:
+		offset += clip->x1 / 2;
+		len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
+		break;
+	default:
+		offset += clip->x1 * fb->format->cpp[0];
+		len *= fb->format->cpp[0];
+		break;
+	}
+
+	src = fb_helper->fbdev->screen_buffer + offset;
 	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
 
 	for (y = clip->y1; y < clip->y2; y++) {
@@ -1231,19 +1250,23 @@  static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
 }
 
 static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
-					 u8 depth)
+					 const struct drm_format_info *format)
 {
-	switch (depth) {
-	case 8:
+	u8 depth = format->depth;
+
+	if (format->is_color_indexed) {
 		var->red.offset = 0;
 		var->green.offset = 0;
 		var->blue.offset = 0;
-		var->red.length = 8; /* 8bit DAC */
-		var->green.length = 8;
-		var->blue.length = 8;
+		var->red.length = depth;
+		var->green.length = depth;
+		var->blue.length = depth;
 		var->transp.offset = 0;
 		var->transp.length = 0;
-		break;
+		return;
+	}
+
+	switch (depth) {
 	case 15:
 		var->red.offset = 10;
 		var->green.offset = 5;
@@ -1298,7 +1321,9 @@  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 {
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_framebuffer *fb = fb_helper->fb;
+	const struct drm_format_info *format = fb->format;
 	struct drm_device *dev = fb_helper->dev;
+	unsigned int bpp;
 
 	if (in_dbg_master())
 		return -EINVAL;
@@ -1308,22 +1333,33 @@  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 		var->pixclock = 0;
 	}
 
-	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
-	    (drm_format_info_block_height(fb->format, 0) > 1))
-		return -EINVAL;
+	switch (format->format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+		/* supported format with sub-byte pixels */
+		break;
+
+	default:
+		if ((drm_format_info_block_width(format, 0) > 1) ||
+		    (drm_format_info_block_height(format, 0) > 1))
+			return -EINVAL;
+		break;
+	}
 
 	/*
 	 * Changes struct fb_var_screeninfo are currently not pushed back
 	 * to KMS, hence fail if different settings are requested.
 	 */
-	if (var->bits_per_pixel > fb->format->cpp[0] * 8 ||
+	bpp = drm_format_info_bpp(format, 0);
+	if (var->bits_per_pixel > bpp ||
 	    var->xres > fb->width || var->yres > fb->height ||
 	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
 		drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
 			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
 			  var->xres, var->yres, var->bits_per_pixel,
 			  var->xres_virtual, var->yres_virtual,
-			  fb->width, fb->height, fb->format->cpp[0] * 8);
+			  fb->width, fb->height, bpp);
 		return -EINVAL;
 	}
 
@@ -1338,13 +1374,13 @@  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	    !var->blue.length    && !var->transp.length   &&
 	    !var->red.msb_right  && !var->green.msb_right &&
 	    !var->blue.msb_right && !var->transp.msb_right) {
-		drm_fb_helper_fill_pixel_fmt(var, fb->format->depth);
+		drm_fb_helper_fill_pixel_fmt(var, format);
 	}
 
 	/*
 	 * Likewise, bits_per_pixel should be rounded up to a supported value.
 	 */
-	var->bits_per_pixel = fb->format->cpp[0] * 8;
+	var->bits_per_pixel = bpp;
 
 	/*
 	 * drm fbdev emulation doesn't support changing the pixel format at all,
@@ -1680,11 +1716,11 @@  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 }
 
 static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
-				   uint32_t depth)
+				   bool is_color_indexed)
 {
 	info->fix.type = FB_TYPE_PACKED_PIXELS;
-	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
-		FB_VISUAL_TRUECOLOR;
+	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
+					    : info->fix.visual;
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
 	info->fix.type_aux = 0;
@@ -1701,19 +1737,31 @@  static void drm_fb_helper_fill_var(struct fb_info *info,
 				   uint32_t fb_width, uint32_t fb_height)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
+	const struct drm_format_info *format = fb->format;
+
+	switch (format->format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+		/* supported format with sub-byte pixels */
+		break;
+
+	default:
+		WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
+			(drm_format_info_block_height(format, 0) > 1));
+		break;
+	}
 
-	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
-		(drm_format_info_block_height(fb->format, 0) > 1));
 	info->pseudo_palette = fb_helper->pseudo_palette;
 	info->var.xres_virtual = fb->width;
 	info->var.yres_virtual = fb->height;
-	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
+	info->var.bits_per_pixel = drm_format_info_bpp(format, 0);
 	info->var.accel_flags = FB_ACCELF_TEXT;
 	info->var.xoffset = 0;
 	info->var.yoffset = 0;
 	info->var.activate = FB_ACTIVATE_NOW;
 
-	drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
+	drm_fb_helper_fill_pixel_fmt(&info->var, format);
 
 	info->var.xres = fb_width;
 	info->var.yres = fb_height;
@@ -1738,7 +1786,8 @@  void drm_fb_helper_fill_info(struct fb_info *info,
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
 
-	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth);
+	drm_fb_helper_fill_fix(info, fb->pitches[0],
+			       fb->format->is_color_indexed);
 	drm_fb_helper_fill_var(info, fb_helper,
 			       sizes->fb_width, sizes->fb_height);