Re: [PATCH 2/4] fbdev/stifb: Allocate fb_info instance with framebuffer_alloc()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Hi Helge

Am 30.12.23 um 09:35 schrieb Helge Deller:

Hi Thomas,

On 12/20/23 14:22, Thomas Zimmermann wrote:

Allocate stifb's instance of fb_info with framebuffer_alloc(). This
is the preferred way of creating fb_info with associated driver data
stored in struct fb_info.par. Requires several, but minor, changes
through out the driver's code.

The intended side effect of this patch is that the new instance of
struct fb_info now has its device field correctly set to the parent
device of the STI ROM. A later patch can detect if the device is the
firmware's primary output. It is also now correctly located within
the Linux device hierarchy.


Thanks for this cleanup series!
Sadly stifb then crashes at bootup.
Please include this copy&pasted hunk in this patch, which then fixes the crash: 

diff --git a/drivers/video/fbdev/stifb.c b/drivers/video/fbdev/stifb.c
index 2de0e675fd15..f8bbd8d6f5b2 100644
--- a/drivers/video/fbdev/stifb.c
+++ b/drivers/video/fbdev/stifb.c
@@ -1158,7 +1158,7 @@ stifb_init_display(struct stifb_info *fb)
             }
             break;
         }
-       stifb_blank(0, (struct fb_info *)fb);   /* 0=enable screen */
+       stifb_blank(0, fb->info);       /* 0=enable screen */



With this applied, you may add to the series:

Tested-by: Helge Deller <deller@xxxxxx>
Reviewed-by: Helge Deller <deller@xxxxxx>


Thanks so much for testing and fixing the problem.

Which tree should this go through?

Best regards
Thomas



Thanks!
Helge



Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
---
  drivers/video/fbdev/stifb.c | 106 +++++++++++++++++++-----------------
  1 file changed, 56 insertions(+), 50 deletions(-)

diff --git a/drivers/video/fbdev/stifb.c b/drivers/video/fbdev/stifb.c
index 548d992f8cb1..36e6bcab83aa 100644
--- a/drivers/video/fbdev/stifb.c
+++ b/drivers/video/fbdev/stifb.c
@@ -103,7 +103,7 @@ typedef struct {
  } ngle_rom_t;

  struct stifb_info {
-    struct fb_info info;
+    struct fb_info *info;
      unsigned int id;
      ngle_rom_t ngle_rom;
      struct sti_struct *sti;
@@ -153,15 +153,15 @@ static int __initdata stifb_bpp_pref[MAX_STI_ROMS];
  #define REG_44        0x210030
  #define REG_45        0x210034
-#define READ_BYTE(fb,reg)        gsc_readb((fb)->info.fix.mmio_start + (reg)) -#define READ_WORD(fb,reg)        gsc_readl((fb)->info.fix.mmio_start + (reg)) +#define READ_BYTE(fb, reg) gsc_readb((fb)->info->fix.mmio_start + (reg)) +#define READ_WORD(fb, reg) gsc_readl((fb)->info->fix.mmio_start + (reg)) 


  #ifndef DEBUG_STIFB_REGS
  # define  DEBUG_OFF()
  # define  DEBUG_ON()
-# define WRITE_BYTE(value,fb,reg) gsc_writeb((value),(fb)->info.fix.mmio_start + (reg)) -# define WRITE_WORD(value,fb,reg) gsc_writel((value),(fb)->info.fix.mmio_start + (reg)) +# define WRITE_BYTE(value, fb, reg)    gsc_writeb((value), (fb)->info->fix.mmio_start + (reg)) +# define WRITE_WORD(value, fb, reg)    gsc_writel((value), (fb)->info->fix.mmio_start + (reg)) 
  #else
    static int debug_on = 1;
  # define  DEBUG_OFF() debug_on=0
@@ -169,11 +169,11 @@ static int __initdata stifb_bpp_pref[MAX_STI_ROMS];
  # define WRITE_BYTE(value,fb,reg)    do { if (debug_on) \
                          printk(KERN_DEBUG "%30s: WRITE_BYTE(0x%06x) = 0x%02x (old=0x%02x)\n", \                               __func__, reg, value, READ_BYTE(fb,reg));           \ -                    gsc_writeb((value),(fb)->info.fix.mmio_start + (reg)); } while (0) +                    gsc_writeb((value), (fb)->info->fix.mmio_start + (reg)); } while (0) 
  # define WRITE_WORD(value,fb,reg)    do { if (debug_on) \
                          printk(KERN_DEBUG "%30s: WRITE_WORD(0x%06x) = 0x%08x (old=0x%08x)\n", \                               __func__, reg, value, READ_WORD(fb,reg));           \ -                    gsc_writel((value),(fb)->info.fix.mmio_start + (reg)); } while (0) +                    gsc_writel((value), (fb)->info->fix.mmio_start + (reg)); } while (0) 
  #endif /* DEBUG_STIFB_REGS */


@@ -210,13 +210,13 @@ SETUP_FB(struct stifb_info *fb)
              reg10_value = 0x13601000;
              break;
          case S9000_ID_A1439A:
-            if (fb->info.var.bits_per_pixel == 32)
+            if (fb->info->var.bits_per_pixel == 32)
                  reg10_value = 0xBBA0A000;
              else
                  reg10_value = 0x13601000;
              break;
          case S9000_ID_HCRX:
-            if (fb->info.var.bits_per_pixel == 32)
+            if (fb->info->var.bits_per_pixel == 32)
                  reg10_value = 0xBBA0A000;
              else
                  reg10_value = 0x13602000;
@@ -254,7 +254,7 @@ static void
  FINISH_IMAGE_COLORMAP_ACCESS(struct stifb_info *fb)
  {
      WRITE_WORD(0x400, fb, REG_2);
-    if (fb->info.var.bits_per_pixel == 32) {
+    if (fb->info->var.bits_per_pixel == 32) {
          WRITE_WORD(0x83000100, fb, REG_1);
      } else {
          if (fb->id == S9000_ID_ARTIST || fb->id == CRT_ID_VISUALIZE_EG)
@@ -503,7 +503,7 @@ static void
  ngleSetupAttrPlanes(struct stifb_info *fb, int BufferNumber)
  {
      SETUP_ATTR_ACCESS(fb, BufferNumber);
-    SET_ATTR_SIZE(fb, fb->info.var.xres, fb->info.var.yres);
+    SET_ATTR_SIZE(fb, fb->info->var.xres, fb->info->var.yres);
      FINISH_ATTR_ACCESS(fb);
      SETUP_FB(fb);
  }
@@ -526,9 +526,9 @@ rattlerSetupPlanes(struct stifb_info *fb)
      SETUP_FB(fb);
      fb->id = saved_id;

-    for (y = 0; y < fb->info.var.yres; ++y)
-        fb_memset_io(fb->info.screen_base + y * fb->info.fix.line_length, -                 0xff, fb->info.var.xres * fb->info.var.bits_per_pixel/8); 
+    for (y = 0; y < fb->info->var.yres; ++y)
+        fb_memset_io(fb->info->screen_base + y * fb->info->fix.line_length, +                 0xff, fb->info->var.xres * fb->info->var.bits_per_pixel/8); 

      CRX24_SET_OVLY_MASK(fb);
      SETUP_FB(fb);
@@ -607,7 +607,7 @@ setHyperLutBltCtl(struct stifb_info *fb, int offsetWithinLut, int length) 
      lutBltCtl.fields.lutType = HYPER_CMAP_TYPE;
      /* Expect lutIndex to be 0 or 1 for image cmaps, 2 or 3 for overlay cmaps */ 
-    if (fb->info.var.bits_per_pixel == 8)
+    if (fb->info->var.bits_per_pixel == 8)
          lutBltCtl.fields.lutOffset = 2 * 256;
      else
          lutBltCtl.fields.lutOffset = 0 * 256;
@@ -688,7 +688,7 @@ ngleResetAttrPlanes(struct stifb_info *fb, unsigned int ctlPlaneReg) 
                             DataDynamic, MaskOtc,
                             BGx(0), FGx(0)));
      packed_dst = 0;
-    packed_len = (fb->info.var.xres << 16) | fb->info.var.yres;
+    packed_len = (fb->info->var.xres << 16) | fb->info->var.yres;
      GET_FIFO_SLOTS(fb, nFreeFifoSlots, 2);
      NGLE_SET_DSTXY(fb, packed_dst);
      SET_LENXY_START_RECFILL(fb, packed_len);
@@ -738,7 +738,7 @@ ngleClearOverlayPlanes(struct stifb_info *fb, int mask, int data) 
          NGLE_REALLY_SET_IMAGE_PLANEMASK(fb, mask);

          packed_dst = 0;
-        packed_len = (fb->info.var.xres << 16) | fb->info.var.yres;
+    packed_len = (fb->info->var.xres << 16) | fb->info->var.yres;
          NGLE_SET_DSTXY(fb, packed_dst);

      /* Write zeroes to overlay planes */
@@ -760,7 +760,7 @@ hyperResetPlanes(struct stifb_info *fb, int enable)
      NGLE_LOCK(fb);

      if (IS_24_DEVICE(fb))
-        if (fb->info.var.bits_per_pixel == 32)
+        if (fb->info->var.bits_per_pixel == 32)
              controlPlaneReg = 0x04000F00;
          else
              controlPlaneReg = 0x00000F00;   /* 0x00000800 should be enough, but lets clear all 4 bits */ 
@@ -890,7 +890,7 @@ SETUP_HCRX(struct stifb_info *fb)
      GET_FIFO_SLOTS(fb, nFreeFifoSlots, 7);

      if (IS_24_DEVICE(fb)) {
-        hyperbowl = (fb->info.var.bits_per_pixel == 32) ?
+        hyperbowl = (fb->info->var.bits_per_pixel == 32) ?
              HYPERBOWL_MODE01_8_24_LUT0_TRANSPARENT_LUT1_OPAQUE :
              HYPERBOWL_MODE01_8_24_LUT0_OPAQUE_LUT1_OPAQUE;

@@ -924,21 +924,21 @@ SETUP_HCRX(struct stifb_info *fb)
  static int
  stifb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
  {
-    struct stifb_info *fb = container_of(info, struct stifb_info, info);
+    struct stifb_info *fb = info->par;

-    if (var->xres != fb->info.var.xres ||
-        var->yres != fb->info.var.yres ||
-        var->bits_per_pixel != fb->info.var.bits_per_pixel)
+    if (var->xres != fb->info->var.xres ||
+        var->yres != fb->info->var.yres ||
+        var->bits_per_pixel != fb->info->var.bits_per_pixel)
          return -EINVAL;

      var->xres_virtual = var->xres;
      var->yres_virtual = var->yres;
      var->xoffset = 0;
      var->yoffset = 0;
-    var->grayscale = fb->info.var.grayscale;
-    var->red.length = fb->info.var.red.length;
-    var->green.length = fb->info.var.green.length;
-    var->blue.length = fb->info.var.blue.length;
+    var->grayscale = fb->info->var.grayscale;
+    var->red.length = fb->info->var.red.length;
+    var->green.length = fb->info->var.green.length;
+    var->blue.length = fb->info->var.blue.length;

      return 0;
  }
@@ -947,7 +947,7 @@ static int
  stifb_setcolreg(u_int regno, u_int red, u_int green,
            u_int blue, u_int transp, struct fb_info *info)
  {
-    struct stifb_info *fb = container_of(info, struct stifb_info, info);
+    struct stifb_info *fb = info->par;
      u32 color;

      if (regno >= NR_PALETTE)
@@ -961,7 +961,7 @@ stifb_setcolreg(u_int regno, u_int red, u_int green,

      START_IMAGE_COLORMAP_ACCESS(fb);

-    if (unlikely(fb->info.var.grayscale)) {
+    if (unlikely(fb->info->var.grayscale)) {
          /* gray = 0.30*R + 0.59*G + 0.11*B */
          color = ((red * 77) +
               (green * 151) +
@@ -972,10 +972,10 @@ stifb_setcolreg(u_int regno, u_int red, u_int green, 
               (blue));
      }

-    if (fb->info.fix.visual == FB_VISUAL_DIRECTCOLOR) {
-        struct fb_var_screeninfo *var = &fb->info.var;
+    if (fb->info->fix.visual == FB_VISUAL_DIRECTCOLOR) {
+        struct fb_var_screeninfo *var = &fb->info->var;
          if (regno < 16)
-            ((u32 *)fb->info.pseudo_palette)[regno] =
+            ((u32 *)fb->info->pseudo_palette)[regno] =
                  regno << var->red.offset |
                  regno << var->green.offset |
                  regno << var->blue.offset;
@@ -1007,7 +1007,7 @@ stifb_setcolreg(u_int regno, u_int red, u_int green, 
  static int
  stifb_blank(int blank_mode, struct fb_info *info)
  {
-    struct stifb_info *fb = container_of(info, struct stifb_info, info);
+    struct stifb_info *fb = info->par;
      int enable = (blank_mode == 0) ? ENABLE : DISABLE;

      switch (fb->id) {
@@ -1036,12 +1036,12 @@ stifb_blank(int blank_mode, struct fb_info *info)
  static void
  stifb_copyarea(struct fb_info *info, const struct fb_copyarea *area)
  {
-    struct stifb_info *fb = container_of(info, struct stifb_info, info);
+    struct stifb_info *fb = info->par;

      SETUP_COPYAREA(fb);

      SETUP_HW(fb);
-    if (fb->info.var.bits_per_pixel == 32) {
+    if (fb->info->var.bits_per_pixel == 32) {
          WRITE_WORD(0xBBA0A000, fb, REG_10);

          NGLE_REALLY_SET_IMAGE_PLANEMASK(fb, 0xffffffff);
@@ -1075,15 +1075,15 @@ stifb_copyarea(struct fb_info *info, const struct fb_copyarea *area) 
  static void
  stifb_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
  {
-    struct stifb_info *fb = container_of(info, struct stifb_info, info);
+    struct stifb_info *fb = info->par;

      if (rect->rop != ROP_COPY ||
-        (fb->id == S9000_ID_HCRX && fb->info.var.bits_per_pixel == 32))
+        (fb->id == S9000_ID_HCRX && fb->info->var.bits_per_pixel == 32))
          return cfb_fillrect(info, rect);

      SETUP_HW(fb);

-    if (fb->info.var.bits_per_pixel == 32) {
+    if (fb->info->var.bits_per_pixel == 32) {
          WRITE_WORD(0xBBA0A000, fb, REG_10);

          NGLE_REALLY_SET_IMAGE_PLANEMASK(fb, 0xffffffff);
@@ -1141,7 +1141,7 @@ stifb_init_display(struct stifb_info *fb)
          switch (id) {
       case S9000_ID_A1659A:
       case S9000_ID_A1439A:
-        if (fb->info.var.bits_per_pixel == 32)
+        if (fb->info->var.bits_per_pixel == 32)
          ngleSetupAttrPlanes(fb, BUFF1_CMAP3);
          else {
          ngleSetupAttrPlanes(fb, BUFF1_CMAP0);
@@ -1151,7 +1151,7 @@ stifb_init_display(struct stifb_info *fb)
          break;
       case S9000_ID_ARTIST:
       case CRT_ID_VISUALIZE_EG:
-        if (fb->info.var.bits_per_pixel == 32)
+        if (fb->info->var.bits_per_pixel == 32)
          ngleSetupAttrPlanes(fb, BUFF1_CMAP3);
          else {
          ngleSetupAttrPlanes(fb, ARTIST_CMAP0);
@@ -1193,11 +1193,11 @@ static int __init stifb_init_fb(struct sti_struct *sti, int bpp_pref) 
      char *dev_name;
      int bpp, xres, yres;

-    fb = kzalloc(sizeof(*fb), GFP_ATOMIC);
-    if (!fb)
+    info = framebuffer_alloc(sizeof(*fb), sti->dev);
+    if (!info)
          return -ENOMEM;
-
-    info = &fb->info;
+    fb = info->par;
+    fb->info = info;

      /* set struct to a known state */
      fix = &info->fix;
@@ -1390,10 +1390,10 @@ static int __init stifb_init_fb(struct sti_struct *sti, int bpp_pref)
      /* save for primary gfx device detection & unregister_framebuffer() */ 
      sti->info = info;
-    if (register_framebuffer(&fb->info) < 0)
+    if (register_framebuffer(fb->info) < 0)
          goto out_err4;
-    fb_info(&fb->info, "%s %dx%d-%d frame buffer device, %s, id: %04x, mmio: 0x%04lx\n", +    fb_info(fb->info, "%s %dx%d-%d frame buffer device, %s, id: %04x, mmio: 0x%04lx\n", 
          fix->id,
          var->xres,
          var->yres,
@@ -1402,6 +1402,8 @@ static int __init stifb_init_fb(struct sti_struct *sti, int bpp_pref) 
          fb->id,
          fix->mmio_start);

+    dev_set_drvdata(sti->dev, info);
+
      return 0;
@@ -1414,7 +1416,7 @@ static int __init stifb_init_fb(struct sti_struct *sti, int bpp_pref) 
  out_err1:
      iounmap(info->screen_base);
  out_err0:
-    kfree(fb);
+    framebuffer_release(info);
      sti->info = NULL;
      return -ENXIO;
  }
@@ -1480,15 +1482,19 @@ stifb_cleanup(void)
          sti = sti_get_rom(i);
          if (!sti)
              break;
-        if (sti->info) {
-            struct fb_info *info = sti->info;
-            unregister_framebuffer(sti->info);
+        if (sti->dev) {
+            struct fb_info *info = dev_get_drvdata(sti->dev);
+
+            if (!info)
+                continue;
+            unregister_framebuffer(info);
              release_mem_region(info->fix.mmio_start, info->fix.mmio_len);                   release_mem_region(info->fix.smem_start, info->fix.smem_len); 
                  if (info->screen_base)
                      iounmap(info->screen_base);
                  fb_dealloc_cmap(&info->cmap);
                  framebuffer_release(info);
+            dev_set_drvdata(sti->dev, NULL);
          }
          sti->info = NULL;
      }




--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux