Florian Tobias Schandinat wrote:
>> Any comments on this patch? If you're okay with the change, I want to take
>> advantage of it in my framebuffer driver.
>
> Of course you want, otherwise I'd be wondering why you are sending this patch
> at all.
I frequently have to ping maintainers on patches that I've posted. I've lost count how many times I've been told, "Oh sorry, I forgot all about your patch".
> But I don't see any advantages of your approach. Instead of pointers to fb_info
> with this patch you could embed fb_info directly in your data structure but that
> is barely a difference for a programmer I'd think. You'd still have to call your
> new functions on init/exit so the amount of function calls needed is the same
> with or without the patch (I could see an advantage if alloc and release were
> pure memory allocations). Or is this all about handling the case when fb_alloc
> fails?
It's all about reducing the number of kmalloc calls. Here's an example of what my per-device data structure looks like:
struct fsl_diu_data {
dma_addr_t phys;
struct fb_info fsl_diu_info[NUM_AOIS]; <-----
struct mfb_info mfb[NUM_AOIS];
struct device_attribute dev_attr;
unsigned int irq;
int fb_enabled;
enum fsl_diu_monitor_port monitor_port;
struct diu __iomem *diu_reg;
spinlock_t reg_lock;
u8 dummy_aoi[4 * 4 * 4];
struct diu_ad dummy_ad __aligned(8);
struct diu_ad ad[NUM_AOIS] __aligned(8);
u8 gamma[256 * 3] __aligned(32);
u8 cursor[MAX_CURS * MAX_CURS * 2] __aligned(32);
} __aligned(32);
So this way, I have all of the memory I need allocated in one spot. I know need to allocate one block of memory.
> Historically some drivers don't even call alloc but have their own fb_info and
> call only register.
That's exactly the same thing I want to do. Do these drivers also initialize info->device and info->bl_curve_mutex by themselves? How would they know if the fb_info structure got new members that also need to be initialized?
Perhaps we need to move the fb_info initialization code from framebuffer_alloc() into register_framebuffer()?
> I do not want to add yet another way of doing framebuffer
> initialization unless you can clearly show its benefits.
I think the problem is that we already have two ways that framebuffers are initialized.
What do you think about this:
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index 67afa9c..ba47a38 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -57,12 +57,6 @@ struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
if (size)
info->par = p + fb_info_size;
- info->device = dev;
-
-#ifdef CONFIG_FB_BACKLIGHT
- mutex_init(&info->bl_curve_mutex);
-#endif
-
return info;
#undef PADDING
#undef BYTES_PER_LONG
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index ad93629..ea35bf8 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1591,6 +1591,12 @@ static int do_register_framebuffer(struct fb_info *fb_info)
mutex_init(&fb_info->lock);
mutex_init(&fb_info->mm_lock);
+ fb_info->device = dev;
+
+#ifdef CONFIG_FB_BACKLIGHT
+ mutex_init(&fb_info->bl_curve_mutex);
+#endif
+
fb_info->dev = device_create(fb_class, fb_info->device,
MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
if (IS_ERR(fb_info->dev)) {
There are a few instances where drivers use info->device after calling framebuffer_alloc() but before calling register_framebuffer(). These drivers will need to be fixed. But otherwise, I think this is a good idea.
--
Timur Tabi
Linux kernel developer at Freescale
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
