Re: [Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, Dec 17, 2013 at 11:51 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, Dec 17, 2013 at 10:58:51PM +0100, Daniel Vetter wrote: On Tue, Dec 17, 2013 at 10:30 PM, Jesse Barnes jbar...@virtuousgeek.org wrote: So in the unlikely event that the fb helper code fails I don't want to fall over. But that shouldn't happen in practice. I only have the checks in place to catch when I failed to set the fbdev field in one path (which is now fixed) Imo if a core piece of the driver fails to initialize we should just fail driver loading. We've had tons of crazy lore around contexts failing, ppgtt failing and other similar stuff, and I think it just makes the normal code more fragile with no real gain. If you think something slips through the cracks maybe just throw a BUG_ON in there instead. I tripped over it earlier when I was disabling parts of the driver. I think allowing dev_priv-fbdev to be NULL fits nicely with your crusade against fbcon. (In fact it was quite fun to load the driver without modesetting as a headless accelerator...) Well for fully disabled fbdev we nuke all those functions completely. If we want to go with this safety net here then I think at least a WARN_ON or something should be put in there, since this really shouldn't happen. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
@@ -333,7 +535,8 @@ MODULE_LICENSE(GPL and additional rights);
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
+ if (dev_priv-fbdev)
+ drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
}
Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
int intel_fbdev_init(struct drm_device *dev)
@@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
- ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
- if (!ifbdev)
- return -ENOMEM;
+ /* This may fail, if so, dev_priv-fbdev won't be set below */
+ intel_fbdev_init_bios(dev);
- dev_priv-fbdev = ifbdev;
- ifbdev-helper.funcs = intel_fb_helper_funcs;
+ if ((ifbdev = dev_priv-fbdev) == NULL) {
+ ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
+ if (ifbdev == NULL)
+ return -ENOMEM;
+
+ ifbdev-helper.funcs = intel_fb_helper_funcs;
+ ifbdev-preferred_bpp = 32;
+
+ dev_priv-fbdev = ifbdev;
+ }
Since Daniel also mentioned the same bikeshed, I think this would look
neater with the allocation here and ifbdev being passed to
intel_fbdev_init_bios to fill in:
ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
if (ifbdev == NULL)
return -ENODEV;
if (!intel_fbdev_init_bios(dev, ifbdev)) {
ifbdev-helper.funcs = intel_fb_helper_funcs;
ifbdev-preferred_bpp = 32;
}
dev_priv-fbdev = ifbdev;
return 0;
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, 17 Dec 2013 10:34:39 +
Chris Wilson ch...@chris-wilson.co.uk wrote:
On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
@@ -333,7 +535,8 @@ MODULE_LICENSE(GPL and additional rights);
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
+ if (dev_priv-fbdev)
+ drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
}
Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
Fixed.
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, Dec 17, 2013 at 10:05 PM, Jesse Barnes jbar...@virtuousgeek.org wrote:
On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
@@ -333,7 +535,8 @@ MODULE_LICENSE(GPL and additional rights);
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
+ if (dev_priv-fbdev)
+ drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
}
Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
Fixed.
I still don't get why we need this check - for CONFIG_FB=n we have a
special dummy function and we are really careful in the setup code to
only enable the interrupt handling code once fbdev is fully set up. Or
do I miss some change here which makes this required? If so the right
fix imo would be to shuffle the init sequence again (and update all
the tons of comments about it, ofc).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, 17 Dec 2013 22:17:22 +0100
Daniel Vetter dan...@ffwll.ch wrote:
On Tue, Dec 17, 2013 at 10:05 PM, Jesse Barnes jbar...@virtuousgeek.org
wrote:
On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
@@ -333,7 +535,8 @@ MODULE_LICENSE(GPL and additional rights);
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
+ if (dev_priv-fbdev)
+ drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
}
Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
Fixed.
I still don't get why we need this check - for CONFIG_FB=n we have a
special dummy function and we are really careful in the setup code to
only enable the interrupt handling code once fbdev is fully set up. Or
do I miss some change here which makes this required? If so the right
fix imo would be to shuffle the init sequence again (and update all
the tons of comments about it, ofc).
In the init code I'm more careful now to avoid leaving a bogus
pointer around:
ret = drm_fb_helper_init(dev, ifbdev-helper,
INTEL_INFO(dev)-num_pipes,
4);
if (ret) {
+ dev_priv-fbdev = NULL;
kfree(ifbdev);
return ret;
}
So in the unlikely event that the fb helper code fails I don't want to
fall over.
But that shouldn't happen in practice. I only have the checks in place
to catch when I failed to set the fbdev field in one path (which is now
fixed).
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, Dec 17, 2013 at 10:30 PM, Jesse Barnes jbar...@virtuousgeek.org wrote:
On Tue, Dec 17, 2013 at 10:05 PM, Jesse Barnes jbar...@virtuousgeek.org
wrote:
On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
@@ -333,7 +535,8 @@ MODULE_LICENSE(GPL and additional rights);
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
+ if (dev_priv-fbdev)
+ drm_fb_helper_hotplug_event(dev_priv-fbdev-helper);
}
Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
Fixed.
I still don't get why we need this check - for CONFIG_FB=n we have a
special dummy function and we are really careful in the setup code to
only enable the interrupt handling code once fbdev is fully set up. Or
do I miss some change here which makes this required? If so the right
fix imo would be to shuffle the init sequence again (and update all
the tons of comments about it, ofc).
In the init code I'm more careful now to avoid leaving a bogus
pointer around:
ret = drm_fb_helper_init(dev, ifbdev-helper,
INTEL_INFO(dev)-num_pipes,
4);
if (ret) {
+ dev_priv-fbdev = NULL;
kfree(ifbdev);
return ret;
}
So in the unlikely event that the fb helper code fails I don't want to
fall over.
But that shouldn't happen in practice. I only have the checks in place
to catch when I failed to set the fbdev field in one path (which is now
fixed)
Imo if a core piece of the driver fails to initialize we should just
fail driver loading. We've had tons of crazy lore around contexts
failing, ppgtt failing and other similar stuff, and I think it just
makes the normal code more fragile with no real gain.
If you think something slips through the cracks maybe just throw a
BUG_ON in there instead.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, 17 Dec 2013 19:29:00 +
Chris Wilson ch...@chris-wilson.co.uk wrote:
On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
int intel_fbdev_init(struct drm_device *dev)
@@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
- ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
- if (!ifbdev)
- return -ENOMEM;
+ /* This may fail, if so, dev_priv-fbdev won't be set below */
+ intel_fbdev_init_bios(dev);
- dev_priv-fbdev = ifbdev;
- ifbdev-helper.funcs = intel_fb_helper_funcs;
+ if ((ifbdev = dev_priv-fbdev) == NULL) {
+ ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
+ if (ifbdev == NULL)
+ return -ENOMEM;
+
+ ifbdev-helper.funcs = intel_fb_helper_funcs;
+ ifbdev-preferred_bpp = 32;
+
+ dev_priv-fbdev = ifbdev;
+ }
Since Daniel also mentioned the same bikeshed, I think this would look
neater with the allocation here and ifbdev being passed to
intel_fbdev_init_bios to fill in:
ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
if (ifbdev == NULL)
return -ENODEV;
if (!intel_fbdev_init_bios(dev, ifbdev)) {
ifbdev-helper.funcs = intel_fb_helper_funcs;
ifbdev-preferred_bpp = 32;
}
dev_priv-fbdev = ifbdev;
return 0;
Yeah, looks better that way. Fixed.
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, Dec 17, 2013 at 10:58:51PM +0100, Daniel Vetter wrote: On Tue, Dec 17, 2013 at 10:30 PM, Jesse Barnes jbar...@virtuousgeek.org wrote: So in the unlikely event that the fb helper code fails I don't want to fall over. But that shouldn't happen in practice. I only have the checks in place to catch when I failed to set the fbdev field in one path (which is now fixed) Imo if a core piece of the driver fails to initialize we should just fail driver loading. We've had tons of crazy lore around contexts failing, ppgtt failing and other similar stuff, and I think it just makes the normal code more fragile with no real gain. If you think something slips through the cracks maybe just throw a BUG_ON in there instead. I tripped over it earlier when I was disabling parts of the driver. I think allowing dev_priv-fbdev to be NULL fits nicely with your crusade against fbcon. (In fact it was quite fun to load the driver without modesetting as a headless accelerator...) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
Retrieve current framebuffer config info from the regs and create an fb
object for the buffer the BIOS or boot loader left us. This should
allow for smooth transitions to userspace apps once we finish the
initial configuration construction.
v2: check for non-native modes and adjust (Jesse)
fixup aperture and cmap frees (Imre)
use unlocked unref if init_bios fails (Jesse)
fix curly brace around DSPADDR check (Imre)
comment failure path for pin_and_fence (Imre)
v3: fixup fixup of aperture frees (Chris)
v4: update to current bits (locking pin_and_fence hack) (Jesse)
v5: move fb config fetch to display code (Jesse)
re-order hw state readout on initial load to suit fb inherit (Jesse)
re-add pin_and_fence in fbdev code to make sure we refcount properly (Je
v6: rename to plane_config (Daniel)
check for valid object when initializing BIOS fb (Jesse)
split from plane_config readout and other display changes (Jesse)
drop use_bios_fb option (Chris)
update comments (Jesse)
rework fbdev_init_bios for clarity (Jesse)
drop fb obj ref under lock (Chris)
v7: use fb object from plane_config instead (Ville)
take ref on fb object (Jesse)
Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org
---
drivers/gpu/drm/i915/intel_display.c | 4 +-
drivers/gpu/drm/i915/intel_drv.h | 4 +-
drivers/gpu/drm/i915/intel_fbdev.c | 235 ---
3 files changed, 224 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 41f625b..7d73b13 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7826,11 +7826,11 @@ mode_fits_in_fbdev(struct drm_device *dev,
if (dev_priv-fbdev == NULL)
return NULL;
- obj = dev_priv-fbdev-ifb.obj;
+ obj = dev_priv-fbdev-fb-obj;
if (obj == NULL)
return NULL;
- fb = dev_priv-fbdev-ifb.base;
+ fb = dev_priv-fbdev-fb-base;
if (fb-pitches[0] intel_framebuffer_pitch_for_width(mode-hdisplay,
fb-bits_per_pixel))
return NULL;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4787773..5f9182e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -110,9 +110,10 @@ struct intel_framebuffer {
struct intel_fbdev {
struct drm_fb_helper helper;
- struct intel_framebuffer ifb;
+ struct intel_framebuffer *fb;
struct list_head fbdev_list;
struct drm_display_mode *our_mode;
+ int preferred_bpp;
};
struct intel_encoder {
@@ -671,6 +672,7 @@ int intel_framebuffer_init(struct drm_device *dev,
struct intel_framebuffer *ifb,
struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_i915_gem_object *obj);
+void intel_fbdev_init_bios(struct drm_device *dev);
void intel_framebuffer_fini(struct intel_framebuffer *fb);
void intel_prepare_page_flip(struct drm_device *dev, int plane);
void intel_finish_page_flip(struct drm_device *dev, int pipe);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index 284c3eb..db75f22 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -62,11 +62,20 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
{
struct intel_fbdev *ifbdev =
container_of(helper, struct intel_fbdev, helper);
+ struct intel_framebuffer *fb;
struct drm_device *dev = helper-dev;
struct drm_mode_fb_cmd2 mode_cmd = {};
struct drm_i915_gem_object *obj;
int size, ret;
+ fb = kzalloc(sizeof(*fb), GFP_KERNEL);
+ if (!fb) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ifbdev-fb = fb;
+
/* we don't do packed 24bpp */
if (sizes-surface_bpp == 24)
sizes-surface_bpp = 32;
@@ -97,7 +106,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
goto out_unref;
}
- ret = intel_framebuffer_init(dev, ifbdev-ifb, mode_cmd, obj);
+ ret = intel_framebuffer_init(dev, ifbdev-fb, mode_cmd, obj);
if (ret)
goto out_unpin;
@@ -116,7 +125,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
{
struct intel_fbdev *ifbdev =
container_of(helper, struct intel_fbdev, helper);
- struct intel_framebuffer *intel_fb = ifbdev-ifb;
+ struct intel_framebuffer *intel_fb = ifbdev-fb;
struct drm_device *dev = helper-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct fb_info *info;
@@ -148,7 +157,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
info-par = helper;
- fb = ifbdev-ifb.base;
+ fb = ifbdev-fb-base;
