[PATCH 0/3] drm/msm: hijack firmware fb's memory

[PATCH 0/3] drm/msm: hijack firmware fb's memory

[Rob Clark]

The first patch I've already sent to list and plan to include in a
v4.13-fixes pull request, but I'm resending as part of this patchset
since 3/3 depends on it.

The 2nd patch exports get_fb_info()/put_fb_info() so that in the 3rd
patch we can iterate over the registered fb's (before kicking out fw
fb) and find the stolen memory from bootloader display.

Note that the firmware fb might either be efifb or simplefb, depending
on EFI boot or not.  So a devicetree based solution to this is not
useful.

This is prep-work for a larger patchset for drm/msm to readback and
take over display setup by bootloader (although that also requires
some CCF and other changes.. RFC patchset coming soon).  In the RFC
patchset, drm/msm will wrap the stolen mem in a GEM object and use
that to create a drm_framebuffer object for the read-back plane state
(and if the drm fbdev emulation layer is enabled, it will try to re-
use this for the fbdev scanout buffer).

If no objections to the 2nd patch, I guess it is probably easiest if
it is merged via msm-next for 4.14.

Rob Clark (3):
  drm/msm: kick out firmware framebuffer
  fbdev: fbmem: export get/put_fb_info()
  drm/msm: hijack firmware fb's memory

 drivers/gpu/drm/msm/msm_drv.c    | 102 ++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/msm/msm_drv.h    |   2 +
 drivers/gpu/drm/msm/msm_fbdev.c  |   2 +-
 drivers/video/fbdev/core/fbmem.c |   6 ++-
 include/linux/fb.h               |   2 +
 5 files changed, 93 insertions(+), 21 deletions(-)

-- 
2.13.0

[PATCH 1/3] drm/msm: kick out firmware framebuffer

[Rob Clark]

Fixes a problem with console not appearing when booting with EFI that
has GOP support, because fb0 would end up being efifb, even after drm
has taken over the display.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/msm_drv.c   | 20 ++++++++++++++++++++
 drivers/gpu/drm/msm/msm_drv.h   |  2 ++
 drivers/gpu/drm/msm/msm_fbdev.c |  2 +-
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f49f6ac5585c..f487437fb9d0 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -286,6 +286,24 @@ static int get_mdp_ver(struct platform_device *pdev)
 
 #include <linux/of_address.h>
 
+static void kick_out_firmware_fb(void)
+{
+	struct apertures_struct *ap;
+
+	ap = alloc_apertures(1);
+	if (!ap)
+		return;
+
+	/* Since msm is a UMA device, the simplefb or efifb node may
+	 * have been located anywhere in memory.
+	 */
+	ap->ranges[0].base = 0;
+	ap->ranges[0].size = MAX_RESOURCE;
+
+	drm_fb_helper_remove_conflicting_framebuffers(ap, FB_NAME, false);
+	kfree(ap);
+}
+
 static int msm_init_vram(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
@@ -416,6 +434,8 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv)
 	if (ret)
 		goto fail;
 
+	kick_out_firmware_fb();
+
 	msm_gem_shrinker_init(ddev);
 
 	switch (get_mdp_ver(pdev)) {
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index e5d8cadfdb75..e69c029b428c 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -55,6 +55,8 @@ struct msm_fence_cb;
 struct msm_gem_address_space;
 struct msm_gem_vma;
 
+#define FB_NAME "msm"
+
 struct msm_file_private {
 	/* currently we don't do anything useful with this.. but when
 	 * per-context address spaces are supported we'd keep track of
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index 5ecf4ff9a059..794265225cda 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -146,7 +146,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
 	fbi->flags = FBINFO_DEFAULT;
 	fbi->fbops = &msm_fb_ops;
 
-	strcpy(fbi->fix.id, "msm");
+	strcpy(fbi->fix.id, FB_NAME);
 
 	drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
 	drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, sizes->fb_height);
-- 
2.13.0

[PATCH 2/3] fbdev: fbmem: export get/put_fb_info()

[Rob Clark]

Needed for following patch.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/video/fbdev/core/fbmem.c | 6 ++++--
 include/linux/fb.h               | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 5324358f110f..db88c7bf3afe 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -50,7 +50,7 @@ EXPORT_SYMBOL(registered_fb);
 int num_registered_fb __read_mostly;
 EXPORT_SYMBOL(num_registered_fb);
 
-static struct fb_info *get_fb_info(unsigned int idx)
+struct fb_info *get_fb_info(unsigned int idx)
 {
 	struct fb_info *fb_info;
 
@@ -65,14 +65,16 @@ static struct fb_info *get_fb_info(unsigned int idx)
 
 	return fb_info;
 }
+EXPORT_SYMBOL(get_fb_info);
 
-static void put_fb_info(struct fb_info *fb_info)
+void put_fb_info(struct fb_info *fb_info)
 {
 	if (!atomic_dec_and_test(&fb_info->count))
 		return;
 	if (fb_info->fbops->fb_destroy)
 		fb_info->fbops->fb_destroy(fb_info);
 }
+EXPORT_SYMBOL(put_fb_info);
 
 int lock_fb_info(struct fb_info *info)
 {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index a964d076b4dc..eccae50db5ed 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -650,6 +650,8 @@ extern struct fb_info *registered_fb[FB_MAX];
 extern int num_registered_fb;
 extern struct class *fb_class;
 
+extern struct fb_info *get_fb_info(unsigned int idx);
+extern void put_fb_info(struct fb_info *fb_info);
 extern int lock_fb_info(struct fb_info *info);
 
 static inline void unlock_fb_info(struct fb_info *info)
-- 
2.13.0

Re: [PATCH 2/3] fbdev: fbmem: export get/put_fb_info()

[Bartlomiej Zolnierkiewicz]

On Tuesday, July 11, 2017 09:38:21 AM Rob Clark wrote:
> Needed for following patch.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 3/3] drm/msm: hijack firmware fb's memory

[Rob Clark]

If we are kicking out efifb or simplefb then we want to hijack the
outgoing fb's memory and wrap it in a gem object so that it can
be allocated for use by fbdev helpers.  This way we keep the same
scanout buffer that the display is already using.

This is prep-work for enabling drm/msm to take over a display that
is enabled already by the bootloader.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/msm_drv.c | 82 +++++++++++++++++++++++++++++++++----------
 1 file changed, 64 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f487437fb9d0..7c1ff26a3c13 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -304,6 +304,45 @@ static void kick_out_firmware_fb(void)
 	kfree(ap);
 }
 
+static unsigned long hijack_firmware_fb(struct drm_device *dev)
+{
+	struct msm_drm_private *priv = dev->dev_private;
+	unsigned long size;
+	int i;
+
+	/* if we have simplefb/efifb, find it's aperture and hijack
+	 * that before we kick out the firmware fb's.
+	 *
+	 * TODO we probably should hold registration_lock
+	 */
+	for (i = 0; i < FB_MAX; i++) {
+		struct fb_info *fb = get_fb_info(i);
+
+		if (IS_ERR_OR_NULL(fb))
+			continue;
+
+		if (!fb->apertures->count)
+			continue;
+
+		/* if we find efifb or simplefb, we are about to
+		 * kick them out, so hijack their memory:
+		 */
+		if ((strcmp(fb->fix.id, "EFI VGA") = 0) ||
+				(strcmp(fb->fix.id, "simple") = 0)) {
+
+			priv->vram.paddr = fb->apertures->ranges[0].base;
+			size = fb->apertures->ranges[0].size;
+		}
+
+		put_fb_info(fb);
+
+		if (size)
+			return size;
+	}
+
+	return 0;
+}
+
 static int msm_init_vram(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
@@ -335,39 +374,46 @@ static int msm_init_vram(struct drm_device *dev)
 		of_node_put(node);
 		if (ret)
 			return ret;
-		size = r.end - r.start;
+		size = r.end - r.start - 1;
 		DRM_INFO("using VRAM carveout: %lx@%pa\n", size, &r.start);
 
+	} else if ((size = hijack_firmware_fb(dev))) {
+		DRM_INFO("hijacking VRAM carveout: %lx@%pa\n",
+				size, &priv->vram.paddr);
+	} else if (!iommu_present(&platform_bus_type)) {
 		/* if we have no IOMMU, then we need to use carveout allocator.
 		 * Grab the entire CMA chunk carved out in early startup in
 		 * mach-msm:
 		 */
-	} else if (!iommu_present(&platform_bus_type)) {
 		DRM_INFO("using %s VRAM carveout\n", vram);
 		size = memparse(vram, NULL);
 	}
 
 	if (size) {
-		unsigned long attrs = 0;
-		void *p;
-
 		priv->vram.size = size;
 
-		drm_mm_init(&priv->vram.mm, 0, (size >> PAGE_SHIFT) - 1);
+		drm_mm_init(&priv->vram.mm, 0, (size >> PAGE_SHIFT));
 		spin_lock_init(&priv->vram.lock);
 
-		attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
-		attrs |= DMA_ATTR_WRITE_COMBINE;
-
-		/* note that for no-kernel-mapping, the vaddr returned
-		 * is bogus, but non-null if allocation succeeded:
-		 */
-		p = dma_alloc_attrs(dev->dev, size,
-				&priv->vram.paddr, GFP_KERNEL, attrs);
-		if (!p) {
-			dev_err(dev->dev, "failed to allocate VRAM\n");
-			priv->vram.paddr = 0;
-			return -ENOMEM;
+		if (!priv->vram.paddr) {
+			unsigned long attrs = 0;
+			void *p;
+
+			attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
+			attrs |= DMA_ATTR_WRITE_COMBINE;
+
+			/* note that for no-kernel-mapping, the vaddr returned
+			 * is bogus, but non-null if allocation succeeded:
+			 */
+			p = dma_alloc_attrs(dev->dev, size,
+					&priv->vram.paddr, GFP_KERNEL, attrs);
+			if (!p) {
+				dev_err(dev->dev, "failed to allocate VRAM\n");
+				priv->vram.paddr = 0;
+				return -ENOMEM;
+			}
+		} else {
+			request_region(priv->vram.paddr, size, "stolen");
 		}
 
 		dev_info(dev->dev, "VRAM: %08x->%08x\n",
-- 
2.13.0

Re: [PATCH 3/3] drm/msm: hijack firmware fb's memory

[Daniel Vetter]

On Tue, Jul 11, 2017 at 3:38 PM, Rob Clark <robdclark@gmail.com> wrote:
> +static unsigned long hijack_firmware_fb(struct drm_device *dev)
> +{
> +       struct msm_drm_private *priv = dev->dev_private;
> +       unsigned long size;
> +       int i;
> +
> +       /* if we have simplefb/efifb, find it's aperture and hijack
> +        * that before we kick out the firmware fb's.
> +        *
> +        * TODO we probably should hold registration_lock
> +        */
> +       for (i = 0; i < FB_MAX; i++) {
> +               struct fb_info *fb = get_fb_info(i);
> +
> +               if (IS_ERR_OR_NULL(fb))
> +                       continue;
> +
> +               if (!fb->apertures->count)
> +                       continue;
> +
> +               /* if we find efifb or simplefb, we are about to
> +                * kick them out, so hijack their memory:
> +                */
> +               if ((strcmp(fb->fix.id, "EFI VGA") = 0) ||
> +                               (strcmp(fb->fix.id, "simple") = 0)) {
> +
> +                       priv->vram.paddr = fb->apertures->ranges[0].base;
> +                       size = fb->apertures->ranges[0].size;
> +               }
> +
> +               put_fb_info(fb);
> +
> +               if (size)
> +                       return size;
> +       }
> +
> +       return 0;
> +}

I think this should be a helper function in at least drm_fb_helper.c,
which would then fill in both base&size in a passed-in struct. But
yeah this seems a lot better than the old one.

In the future we could then also extend this with kicking out other
firmware fb things.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/msm: hijack firmware fb's memory

[Rob Clark]

On Tue, Jul 11, 2017 at 10:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Jul 11, 2017 at 3:38 PM, Rob Clark <robdclark@gmail.com> wrote:
>> +static unsigned long hijack_firmware_fb(struct drm_device *dev)
>> +{
>> +       struct msm_drm_private *priv = dev->dev_private;
>> +       unsigned long size;
>> +       int i;
>> +
>> +       /* if we have simplefb/efifb, find it's aperture and hijack
>> +        * that before we kick out the firmware fb's.
>> +        *
>> +        * TODO we probably should hold registration_lock
>> +        */
>> +       for (i = 0; i < FB_MAX; i++) {
>> +               struct fb_info *fb = get_fb_info(i);
>> +
>> +               if (IS_ERR_OR_NULL(fb))
>> +                       continue;
>> +
>> +               if (!fb->apertures->count)
>> +                       continue;
>> +
>> +               /* if we find efifb or simplefb, we are about to
>> +                * kick them out, so hijack their memory:
>> +                */
>> +               if ((strcmp(fb->fix.id, "EFI VGA") = 0) ||
>> +                               (strcmp(fb->fix.id, "simple") = 0)) {
>> +
>> +                       priv->vram.paddr = fb->apertures->ranges[0].base;
>> +                       size = fb->apertures->ranges[0].size;
>> +               }
>> +
>> +               put_fb_info(fb);
>> +
>> +               if (size)
>> +                       return size;
>> +       }
>> +
>> +       return 0;
>> +}
>
> I think this should be a helper function in at least drm_fb_helper.c,
> which would then fill in both base&size in a passed-in struct. But
> yeah this seems a lot better than the old one.

Yeah, I guess we could do that.. but probably not in drm_fb_helper.c
since that is compile-time optional.  Better suggestions about where
it should live?  If you have fbdev but not DRM_FBDEV_EMULATION you
still want to do this, I think.  Otherwise we can't completely take
over the display setup by firmware (ie. no way to create
plane->state->fb).

BR,
-R


> In the future we could then also extend this with kicking out other
> firmware fb things.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/msm: hijack firmware fb's memory

[Daniel Vetter]

On Tue, Jul 11, 2017 at 4:31 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Tue, Jul 11, 2017 at 10:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Jul 11, 2017 at 3:38 PM, Rob Clark <robdclark@gmail.com> wrote:
>>> +static unsigned long hijack_firmware_fb(struct drm_device *dev)
>>> +{
>>> +       struct msm_drm_private *priv = dev->dev_private;
>>> +       unsigned long size;
>>> +       int i;
>>> +
>>> +       /* if we have simplefb/efifb, find it's aperture and hijack
>>> +        * that before we kick out the firmware fb's.
>>> +        *
>>> +        * TODO we probably should hold registration_lock
>>> +        */
>>> +       for (i = 0; i < FB_MAX; i++) {
>>> +               struct fb_info *fb = get_fb_info(i);
>>> +
>>> +               if (IS_ERR_OR_NULL(fb))
>>> +                       continue;
>>> +
>>> +               if (!fb->apertures->count)
>>> +                       continue;
>>> +
>>> +               /* if we find efifb or simplefb, we are about to
>>> +                * kick them out, so hijack their memory:
>>> +                */
>>> +               if ((strcmp(fb->fix.id, "EFI VGA") = 0) ||
>>> +                               (strcmp(fb->fix.id, "simple") = 0)) {
>>> +
>>> +                       priv->vram.paddr = fb->apertures->ranges[0].base;
>>> +                       size = fb->apertures->ranges[0].size;
>>> +               }
>>> +
>>> +               put_fb_info(fb);
>>> +
>>> +               if (size)
>>> +                       return size;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>
>> I think this should be a helper function in at least drm_fb_helper.c,
>> which would then fill in both base&size in a passed-in struct. But
>> yeah this seems a lot better than the old one.
>
> Yeah, I guess we could do that.. but probably not in drm_fb_helper.c
> since that is compile-time optional.  Better suggestions about where
> it should live?  If you have fbdev but not DRM_FBDEV_EMULATION you
> still want to do this, I think.  Otherwise we can't completely take
> over the display setup by firmware (ie. no way to create
> plane->state->fb).

Hm right, maybe add a drm_fwfb_helper.c or so. If you look at
i915_kick_out_vgacon(), that might be another candidate for that file.
Putting it into fbdev itself seems like a bad idea, because
maintenance pains.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/msm: hijack firmware fb's memory

[Rob Clark]

On Tue, Jul 11, 2017 at 10:42 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Jul 11, 2017 at 4:31 PM, Rob Clark <robdclark@gmail.com> wrote:
>> On Tue, Jul 11, 2017 at 10:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Tue, Jul 11, 2017 at 3:38 PM, Rob Clark <robdclark@gmail.com> wrote:
>>>> +static unsigned long hijack_firmware_fb(struct drm_device *dev)
>>>> +{
>>>> +       struct msm_drm_private *priv = dev->dev_private;
>>>> +       unsigned long size;
>>>> +       int i;
>>>> +
>>>> +       /* if we have simplefb/efifb, find it's aperture and hijack
>>>> +        * that before we kick out the firmware fb's.
>>>> +        *
>>>> +        * TODO we probably should hold registration_lock
>>>> +        */
>>>> +       for (i = 0; i < FB_MAX; i++) {
>>>> +               struct fb_info *fb = get_fb_info(i);
>>>> +
>>>> +               if (IS_ERR_OR_NULL(fb))
>>>> +                       continue;
>>>> +
>>>> +               if (!fb->apertures->count)
>>>> +                       continue;
>>>> +
>>>> +               /* if we find efifb or simplefb, we are about to
>>>> +                * kick them out, so hijack their memory:
>>>> +                */
>>>> +               if ((strcmp(fb->fix.id, "EFI VGA") = 0) ||
>>>> +                               (strcmp(fb->fix.id, "simple") = 0)) {
>>>> +
>>>> +                       priv->vram.paddr = fb->apertures->ranges[0].base;
>>>> +                       size = fb->apertures->ranges[0].size;
>>>> +               }
>>>> +
>>>> +               put_fb_info(fb);
>>>> +
>>>> +               if (size)
>>>> +                       return size;
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>
>>> I think this should be a helper function in at least drm_fb_helper.c,
>>> which would then fill in both base&size in a passed-in struct. But
>>> yeah this seems a lot better than the old one.
>>
>> Yeah, I guess we could do that.. but probably not in drm_fb_helper.c
>> since that is compile-time optional.  Better suggestions about where
>> it should live?  If you have fbdev but not DRM_FBDEV_EMULATION you
>> still want to do this, I think.  Otherwise we can't completely take
>> over the display setup by firmware (ie. no way to create
>> plane->state->fb).
>
> Hm right, maybe add a drm_fwfb_helper.c or so. If you look at
> i915_kick_out_vgacon(), that might be another candidate for that file.
> Putting it into fbdev itself seems like a bad idea, because
> maintenance pains.

Hmm, would it be weird to have an:

  obj-$(CONFIG_FB) += drm_fbfw_helper.o

in drm/Makefile?  Or is there a better way to do that?

I'm also wondering a bit about the CONFIG_FB=n case.. you might still
have CONFIG_EFI, so maybe we should fall back to pulling this out of
screen_info and looking for a simple-framebuffer node in the CONFIG_OF
case?

(also maybe worth noting that on ARM/ARM64 we don't have
CONFIG_VGA_CONSOLE.. so there are a lot of fun permutations..)

BR,
-R

Re: [PATCH 3/3] drm/msm: hijack firmware fb's memory

[Daniel Vetter]

On Tue, Jul 11, 2017 at 9:53 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Tue, Jul 11, 2017 at 10:42 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Jul 11, 2017 at 4:31 PM, Rob Clark <robdclark@gmail.com> wrote:
>>> On Tue, Jul 11, 2017 at 10:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>> On Tue, Jul 11, 2017 at 3:38 PM, Rob Clark <robdclark@gmail.com> wrote:
>>>>> +static unsigned long hijack_firmware_fb(struct drm_device *dev)
>>>>> +{
>>>>> +       struct msm_drm_private *priv = dev->dev_private;
>>>>> +       unsigned long size;
>>>>> +       int i;
>>>>> +
>>>>> +       /* if we have simplefb/efifb, find it's aperture and hijack
>>>>> +        * that before we kick out the firmware fb's.
>>>>> +        *
>>>>> +        * TODO we probably should hold registration_lock
>>>>> +        */
>>>>> +       for (i = 0; i < FB_MAX; i++) {
>>>>> +               struct fb_info *fb = get_fb_info(i);
>>>>> +
>>>>> +               if (IS_ERR_OR_NULL(fb))
>>>>> +                       continue;
>>>>> +
>>>>> +               if (!fb->apertures->count)
>>>>> +                       continue;
>>>>> +
>>>>> +               /* if we find efifb or simplefb, we are about to
>>>>> +                * kick them out, so hijack their memory:
>>>>> +                */
>>>>> +               if ((strcmp(fb->fix.id, "EFI VGA") = 0) ||
>>>>> +                               (strcmp(fb->fix.id, "simple") = 0)) {
>>>>> +
>>>>> +                       priv->vram.paddr = fb->apertures->ranges[0].base;
>>>>> +                       size = fb->apertures->ranges[0].size;
>>>>> +               }
>>>>> +
>>>>> +               put_fb_info(fb);
>>>>> +
>>>>> +               if (size)
>>>>> +                       return size;
>>>>> +       }
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>
>>>> I think this should be a helper function in at least drm_fb_helper.c,
>>>> which would then fill in both base&size in a passed-in struct. But
>>>> yeah this seems a lot better than the old one.
>>>
>>> Yeah, I guess we could do that.. but probably not in drm_fb_helper.c
>>> since that is compile-time optional.  Better suggestions about where
>>> it should live?  If you have fbdev but not DRM_FBDEV_EMULATION you
>>> still want to do this, I think.  Otherwise we can't completely take
>>> over the display setup by firmware (ie. no way to create
>>> plane->state->fb).
>>
>> Hm right, maybe add a drm_fwfb_helper.c or so. If you look at
>> i915_kick_out_vgacon(), that might be another candidate for that file.
>> Putting it into fbdev itself seems like a bad idea, because
>> maintenance pains.
>
> Hmm, would it be weird to have an:
>
>   obj-$(CONFIG_FB) += drm_fbfw_helper.o
>
> in drm/Makefile?  Or is there a better way to do that?
>
> I'm also wondering a bit about the CONFIG_FB=n case.. you might still
> have CONFIG_EFI, so maybe we should fall back to pulling this out of
> screen_info and looking for a simple-framebuffer node in the CONFIG_OF
> case?
>
> (also maybe worth noting that on ARM/ARM64 we don't have
> CONFIG_VGA_CONSOLE.. so there are a lot of fun permutations..)

I'd include the source always (because of the above, e.g. kicking
vgacon doesn't depend on CONFIG_FB), and then we'll probably have to
sprinkle a pile of ugly #ifdef all over that file. Still better to
have these hacks in one place only at least.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/msm: hijack firmware fb's memory

[Chris Wilson]

Quoting Rob Clark (2017-07-11 14:38:22)
> +static unsigned long hijack_firmware_fb(struct drm_device *dev)
> +{
> +       struct msm_drm_private *priv = dev->dev_private;
> +       unsigned long size;
> +       int i;
> +
> +       /* if we have simplefb/efifb, find it's aperture and hijack
> +        * that before we kick out the firmware fb's.
> +        *
> +        * TODO we probably should hold registration_lock
> +        */
> +       for (i = 0; i < FB_MAX; i++) {
> +               struct fb_info *fb = get_fb_info(i);
> +
> +               if (IS_ERR_OR_NULL(fb))
> +                       continue;
> +
> +               if (!fb->apertures->count)

Does get_fb_info() not return a reference if its apertures->count=0?

> +                       continue;
> +
> +               /* if we find efifb or simplefb, we are about to
> +                * kick them out, so hijack their memory:
> +                */
> +               if ((strcmp(fb->fix.id, "EFI VGA") = 0) ||
> +                               (strcmp(fb->fix.id, "simple") = 0)) {
> +
> +                       priv->vram.paddr = fb->apertures->ranges[0].base;
> +                       size = fb->apertures->ranges[0].size;
> +               }
> +
> +               put_fb_info(fb);
> +
> +               if (size)
> +                       return size;

size is never initialised to 0. Perhaps just return the reference to the
matching fb? Hopefully sidestepping a few of the worries about it
disappearing during the probe.
-Chris

Re: [PATCH 3/3] drm/msm: hijack firmware fb's memory

[Rob Clark]

On Tue, Jul 11, 2017 at 10:17 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Rob Clark (2017-07-11 14:38:22)
>> +static unsigned long hijack_firmware_fb(struct drm_device *dev)
>> +{
>> +       struct msm_drm_private *priv = dev->dev_private;
>> +       unsigned long size;
>> +       int i;
>> +
>> +       /* if we have simplefb/efifb, find it's aperture and hijack
>> +        * that before we kick out the firmware fb's.
>> +        *
>> +        * TODO we probably should hold registration_lock
>> +        */
>> +       for (i = 0; i < FB_MAX; i++) {
>> +               struct fb_info *fb = get_fb_info(i);
>> +
>> +               if (IS_ERR_OR_NULL(fb))
>> +                       continue;
>> +
>> +               if (!fb->apertures->count)
>
> Does get_fb_info() not return a reference if its apertures->count=0?

yeah, you are right.. overlooked that when converting from iterating
registered_fb[] table directly without taking a reference..

>> +                       continue;
>> +
>> +               /* if we find efifb or simplefb, we are about to
>> +                * kick them out, so hijack their memory:
>> +                */
>> +               if ((strcmp(fb->fix.id, "EFI VGA") = 0) ||
>> +                               (strcmp(fb->fix.id, "simple") = 0)) {
>> +
>> +                       priv->vram.paddr = fb->apertures->ranges[0].base;
>> +                       size = fb->apertures->ranges[0].size;
>> +               }
>> +
>> +               put_fb_info(fb);
>> +
>> +               if (size)
>> +                       return size;
>
> size is never initialised to 0. Perhaps just return the reference to the
> matching fb? Hopefully sidestepping a few of the worries about it
> disappearing during the probe.

I guess that would be a useful approach if I wanted a single helper
that did both this and kicking out firmware fb's..

BR,
-R