* [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
2013-12-12 20:41 [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time Jesse Barnes
@ 2013-12-12 20:41 ` Jesse Barnes
2013-12-14 11:01 ` Daniel Vetter
2013-12-12 20:41 ` [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7 Jesse Barnes
` (4 subsequent siblings)
5 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-12 20:41 UTC (permalink / raw)
To: intel-gfx
Read out the current plane configuration at init time into a new
plane_config structure. This allows us to track any existing
framebuffers attached to the plane and potentially re-use them in our
fbdev code for a smooth handoff.
v2: update for new pitch_for_width function (Jesse)
comment how get_plane_config works with shared fbs (Jesse)
v3: s/ARGB/XRGB (Ville)
use pipesrc width/height (Ville)
fix fourcc comment (Bob)
use drm_format_plane_cpp (Ville)
v4: use fb for tracking fb data object (Ville)
v5: fix up gen2 pitch limits (Ville)
v6: read out stride as well (Daniel)
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
drivers/gpu/drm/i915/i915_drv.h | 3 +
drivers/gpu/drm/i915/intel_display.c | 130 +++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_drv.h | 8 +++
3 files changed, 136 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index efc57fe..2ea151d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -366,6 +366,7 @@ struct drm_i915_error_state {
struct intel_connector;
struct intel_crtc_config;
+struct intel_plane_config;
struct intel_crtc;
struct intel_limit;
struct dpll;
@@ -404,6 +405,8 @@ struct drm_i915_display_funcs {
* fills out the pipe-config with the hw state. */
bool (*get_pipe_config)(struct intel_crtc *,
struct intel_crtc_config *);
+ void (*get_plane_config)(struct intel_crtc *,
+ struct intel_plane_config *);
int (*crtc_mode_set)(struct drm_crtc *crtc,
int x, int y,
struct drm_framebuffer *old_fb);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1ae3d44..94183af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2008,6 +2008,27 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
}
}
+int intel_format_to_fourcc(int format)
+{
+ switch (format) {
+ case DISPPLANE_8BPP:
+ return DRM_FORMAT_C8;
+ case DISPPLANE_BGRX555:
+ return DRM_FORMAT_XRGB1555;
+ case DISPPLANE_BGRX565:
+ return DRM_FORMAT_RGB565;
+ default:
+ case DISPPLANE_BGRX888:
+ return DRM_FORMAT_XRGB8888;
+ case DISPPLANE_RGBX888:
+ return DRM_FORMAT_XBGR8888;
+ case DISPPLANE_BGRX101010:
+ return DRM_FORMAT_XRGB2101010;
+ case DISPPLANE_RGBX101010:
+ return DRM_FORMAT_XBGR2101010;
+ }
+}
+
static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
int x, int y)
{
@@ -5463,6 +5484,92 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
pipe_config->port_clock = clock.dot / 5;
}
+static void i9xx_get_plane_config(struct intel_crtc *crtc,
+ struct intel_plane_config *plane_config)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_i915_gem_object *obj = NULL;
+ struct drm_mode_fb_cmd2 mode_cmd = { 0 };
+ u32 val, base, offset;
+ int pipe = crtc->pipe, plane = crtc->plane;
+ int fourcc, pixel_format;
+
+ plane_config->fb = kzalloc(sizeof(*plane_config->fb), GFP_KERNEL);
+ if (!plane_config->fb) {
+ DRM_DEBUG_KMS("failed to alloc fb\n");
+ return;
+ }
+
+ val = I915_READ(DSPCNTR(plane));
+
+ if (INTEL_INFO(dev)->gen >= 4)
+ if (val & DISPPLANE_TILED)
+ plane_config->tiled = true;
+
+ pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
+ fourcc = intel_format_to_fourcc(pixel_format);
+ plane_config->fb->base.pixel_format = fourcc;
+ plane_config->fb->base.bits_per_pixel =
+ drm_format_plane_cpp(fourcc, 0) * 8;
+
+ if (INTEL_INFO(dev)->gen >= 4) {
+ if (plane_config->tiled)
+ offset = I915_READ(DSPTILEOFF(plane));
+ else
+ offset = I915_READ(DSPLINOFF(plane));
+ base = I915_READ(DSPSURF(plane)) & 0xfffff000;
+ } else {
+ base = I915_READ(DSPADDR(plane));
+ }
+
+ val = I915_READ(PIPESRC(pipe));
+ plane_config->fb->base.width = ((val >> 16) & 0xfff) + 1;
+ plane_config->fb->base.height = ((val >> 0) & 0xfff) + 1;
+
+ val = I915_READ(DSPSTRIDE(pipe));
+ plane_config->fb->base.pitches[0] = val & 0xffffff80;
+
+ plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
+ plane_config->fb->base.height, PAGE_SIZE);
+
+ DRM_DEBUG_KMS("pipe/plane %d/%d with fb: size=%dx%d@%d, offset=%x, pitch %d, size 0x%x\n",
+ pipe, plane, plane_config->fb->base.width,
+ plane_config->fb->base.height,
+ plane_config->fb->base.bits_per_pixel, base,
+ plane_config->fb->base.pitches[0],
+ plane_config->size);
+
+ /*
+ * If the fb is shared between multiple heads, we'll just get the
+ * first one.
+ */
+ obj = i915_gem_object_create_stolen_for_preallocated(dev, base, base,
+ plane_config->size);
+ if (!obj)
+ return;
+
+ mode_cmd.pixel_format = fourcc;
+ mode_cmd.width = plane_config->fb->base.width;
+ mode_cmd.height = plane_config->fb->base.height;
+ mode_cmd.pitches[0] = plane_config->fb->base.pitches[0];
+
+ mutex_lock(&dev->struct_mutex);
+
+ if (intel_framebuffer_init(dev, plane_config->fb, &mode_cmd, obj)) {
+ DRM_DEBUG_KMS("intel fb init failed\n");
+ goto out_unref_obj;
+ }
+
+ mutex_unlock(&dev->struct_mutex);
+ DRM_DEBUG_KMS("plane fb obj %p\n", plane_config->fb->obj);
+ return;
+
+out_unref_obj:
+ drm_gem_object_unreference(&obj->base);
+ mutex_unlock(&dev->struct_mutex);
+}
+
static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
struct intel_crtc_config *pipe_config)
{
@@ -10553,6 +10660,7 @@ static void intel_init_display(struct drm_device *dev)
dev_priv->display.update_plane = ironlake_update_plane;
} else if (IS_VALLEYVIEW(dev)) {
dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
+ dev_priv->display.get_plane_config = i9xx_get_plane_config;
dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
dev_priv->display.crtc_enable = valleyview_crtc_enable;
dev_priv->display.crtc_disable = i9xx_crtc_disable;
@@ -10560,6 +10668,7 @@ static void intel_init_display(struct drm_device *dev)
dev_priv->display.update_plane = i9xx_update_plane;
} else {
dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
+ dev_priv->display.get_plane_config = i9xx_get_plane_config;
dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
dev_priv->display.crtc_enable = i9xx_crtc_enable;
dev_priv->display.crtc_disable = i9xx_crtc_disable;
@@ -10814,6 +10923,7 @@ void intel_modeset_suspend_hw(struct drm_device *dev)
void intel_modeset_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_crtc *crtc;
int i, j, ret;
drm_mode_config_init(dev);
@@ -10870,6 +10980,21 @@ void intel_modeset_init(struct drm_device *dev)
/* Just in case the BIOS is doing something questionable. */
intel_disable_fbc(dev);
+
+ drm_modeset_lock_all(dev);
+ drm_mode_config_reset(dev);
+ intel_modeset_setup_hw_state(dev, false);
+ drm_modeset_unlock_all(dev);
+
+ list_for_each_entry(crtc, &dev->mode_config.crtc_list,
+ base.head) {
+ if (!crtc->active)
+ continue;
+
+ if (dev_priv->display.get_plane_config)
+ dev_priv->display.get_plane_config(crtc,
+ &crtc->plane_config);
+ }
}
static void
@@ -11237,11 +11362,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
intel_modeset_init_hw(dev);
intel_setup_overlay(dev);
-
- drm_modeset_lock_all(dev);
- drm_mode_config_reset(dev);
- intel_modeset_setup_hw_state(dev, false);
- drm_modeset_unlock_all(dev);
}
void intel_modeset_cleanup(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ea62673..4787773 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -209,6 +209,12 @@ typedef struct dpll {
int p;
} intel_clock_t;
+struct intel_plane_config {
+ struct intel_framebuffer *fb; /* ends up managed by intel_fbdev.c */
+ bool tiled;
+ int size;
+};
+
struct intel_crtc_config {
/**
* quirks - bitfield with hw state readout quirks
@@ -358,6 +364,7 @@ struct intel_crtc {
bool cursor_visible;
struct intel_crtc_config config;
+ struct intel_plane_config plane_config;
uint32_t ddi_pll_sel;
@@ -707,6 +714,7 @@ void hsw_enable_ips(struct intel_crtc *crtc);
void hsw_disable_ips(struct intel_crtc *crtc);
void intel_display_set_init_power(struct drm_device *dev, bool enable);
int valleyview_get_vco(struct drm_i915_private *dev_priv);
+int intel_format_to_fourcc(int format);
/* intel_dp.c */
void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
--
1.8.4.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
2013-12-12 20:41 ` [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init Jesse Barnes
@ 2013-12-14 11:01 ` Daniel Vetter
2013-12-17 0:01 ` Jesse Barnes
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-14 11:01 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Dec 12, 2013 at 12:41:53PM -0800, Jesse Barnes wrote:
> Read out the current plane configuration at init time into a new
> plane_config structure. This allows us to track any existing
> framebuffers attached to the plane and potentially re-use them in our
> fbdev code for a smooth handoff.
>
> v2: update for new pitch_for_width function (Jesse)
> comment how get_plane_config works with shared fbs (Jesse)
> v3: s/ARGB/XRGB (Ville)
> use pipesrc width/height (Ville)
> fix fourcc comment (Bob)
> use drm_format_plane_cpp (Ville)
> v4: use fb for tracking fb data object (Ville)
> v5: fix up gen2 pitch limits (Ville)
> v6: read out stride as well (Daniel)
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 +
> drivers/gpu/drm/i915/intel_display.c | 130 +++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_drv.h | 8 +++
> 3 files changed, 136 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index efc57fe..2ea151d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -366,6 +366,7 @@ struct drm_i915_error_state {
>
> struct intel_connector;
> struct intel_crtc_config;
> +struct intel_plane_config;
> struct intel_crtc;
> struct intel_limit;
> struct dpll;
> @@ -404,6 +405,8 @@ struct drm_i915_display_funcs {
> * fills out the pipe-config with the hw state. */
> bool (*get_pipe_config)(struct intel_crtc *,
> struct intel_crtc_config *);
> + void (*get_plane_config)(struct intel_crtc *,
> + struct intel_plane_config *);
> int (*crtc_mode_set)(struct drm_crtc *crtc,
> int x, int y,
> struct drm_framebuffer *old_fb);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1ae3d44..94183af 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2008,6 +2008,27 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
> }
> }
>
> +int intel_format_to_fourcc(int format)
> +{
> + switch (format) {
> + case DISPPLANE_8BPP:
> + return DRM_FORMAT_C8;
> + case DISPPLANE_BGRX555:
> + return DRM_FORMAT_XRGB1555;
> + case DISPPLANE_BGRX565:
> + return DRM_FORMAT_RGB565;
> + default:
> + case DISPPLANE_BGRX888:
> + return DRM_FORMAT_XRGB8888;
> + case DISPPLANE_RGBX888:
> + return DRM_FORMAT_XBGR8888;
> + case DISPPLANE_BGRX101010:
> + return DRM_FORMAT_XRGB2101010;
> + case DISPPLANE_RGBX101010:
> + return DRM_FORMAT_XBGR2101010;
> + }
> +}
> +
> static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> int x, int y)
> {
> @@ -5463,6 +5484,92 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> pipe_config->port_clock = clock.dot / 5;
> }
>
> +static void i9xx_get_plane_config(struct intel_crtc *crtc,
> + struct intel_plane_config *plane_config)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_i915_gem_object *obj = NULL;
> + struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> + u32 val, base, offset;
> + int pipe = crtc->pipe, plane = crtc->plane;
> + int fourcc, pixel_format;
> +
> + plane_config->fb = kzalloc(sizeof(*plane_config->fb), GFP_KERNEL);
> + if (!plane_config->fb) {
> + DRM_DEBUG_KMS("failed to alloc fb\n");
> + return;
> + }
> +
> + val = I915_READ(DSPCNTR(plane));
> +
> + if (INTEL_INFO(dev)->gen >= 4)
> + if (val & DISPPLANE_TILED)
> + plane_config->tiled = true;
> +
> + pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> + fourcc = intel_format_to_fourcc(pixel_format);
> + plane_config->fb->base.pixel_format = fourcc;
> + plane_config->fb->base.bits_per_pixel =
> + drm_format_plane_cpp(fourcc, 0) * 8;
> +
> + if (INTEL_INFO(dev)->gen >= 4) {
> + if (plane_config->tiled)
> + offset = I915_READ(DSPTILEOFF(plane));
> + else
> + offset = I915_READ(DSPLINOFF(plane));
> + base = I915_READ(DSPSURF(plane)) & 0xfffff000;
> + } else {
> + base = I915_READ(DSPADDR(plane));
> + }
> +
> + val = I915_READ(PIPESRC(pipe));
> + plane_config->fb->base.width = ((val >> 16) & 0xfff) + 1;
> + plane_config->fb->base.height = ((val >> 0) & 0xfff) + 1;
> +
> + val = I915_READ(DSPSTRIDE(pipe));
> + plane_config->fb->base.pitches[0] = val & 0xffffff80;
> +
> + plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
> + plane_config->fb->base.height, PAGE_SIZE);
> +
> + DRM_DEBUG_KMS("pipe/plane %d/%d with fb: size=%dx%d@%d, offset=%x, pitch %d, size 0x%x\n",
> + pipe, plane, plane_config->fb->base.width,
> + plane_config->fb->base.height,
> + plane_config->fb->base.bits_per_pixel, base,
> + plane_config->fb->base.pitches[0],
> + plane_config->size);
> +
> + /*
> + * If the fb is shared between multiple heads, we'll just get the
> + * first one.
> + */
> + obj = i915_gem_object_create_stolen_for_preallocated(dev, base, base,
> + plane_config->size);
> + if (!obj)
> + return;
> +
> + mode_cmd.pixel_format = fourcc;
> + mode_cmd.width = plane_config->fb->base.width;
> + mode_cmd.height = plane_config->fb->base.height;
> + mode_cmd.pitches[0] = plane_config->fb->base.pitches[0];
> +
> + mutex_lock(&dev->struct_mutex);
> +
> + if (intel_framebuffer_init(dev, plane_config->fb, &mode_cmd, obj)) {
> + DRM_DEBUG_KMS("intel fb init failed\n");
> + goto out_unref_obj;
> + }
> +
> + mutex_unlock(&dev->struct_mutex);
> + DRM_DEBUG_KMS("plane fb obj %p\n", plane_config->fb->obj);
> + return;
> +
> +out_unref_obj:
> + drm_gem_object_unreference(&obj->base);
> + mutex_unlock(&dev->struct_mutex);
> +}
> +
> static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> struct intel_crtc_config *pipe_config)
> {
> @@ -10553,6 +10660,7 @@ static void intel_init_display(struct drm_device *dev)
> dev_priv->display.update_plane = ironlake_update_plane;
> } else if (IS_VALLEYVIEW(dev)) {
> dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
> + dev_priv->display.get_plane_config = i9xx_get_plane_config;
> dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
> dev_priv->display.crtc_enable = valleyview_crtc_enable;
> dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -10560,6 +10668,7 @@ static void intel_init_display(struct drm_device *dev)
> dev_priv->display.update_plane = i9xx_update_plane;
> } else {
> dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
> + dev_priv->display.get_plane_config = i9xx_get_plane_config;
> dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
> dev_priv->display.crtc_enable = i9xx_crtc_enable;
> dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -10814,6 +10923,7 @@ void intel_modeset_suspend_hw(struct drm_device *dev)
> void intel_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_crtc *crtc;
> int i, j, ret;
>
> drm_mode_config_init(dev);
> @@ -10870,6 +10980,21 @@ void intel_modeset_init(struct drm_device *dev)
>
> /* Just in case the BIOS is doing something questionable. */
> intel_disable_fbc(dev);
> +
> + drm_modeset_lock_all(dev);
> + drm_mode_config_reset(dev);
> + intel_modeset_setup_hw_state(dev, false);
> + drm_modeset_unlock_all(dev);
> +
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> + base.head) {
> + if (!crtc->active)
> + continue;
> +
> + if (dev_priv->display.get_plane_config)
> + dev_priv->display.get_plane_config(crtc,
> + &crtc->plane_config);
Still not really sold on the plane_config, but I think the current code
looks neat enough that we can frob it later on ;-)
But I still think the fb lifetime management is a bit broken here and we
need a few small changes:
1. Right here in this loop we need to assign the fb from the plane_config
ot the crtc->fb pointer and grab an fb reference for that.
If we don't do that we'll fall over for CONFIG_FB=n
A side-effect of that is that plane_config is now fairly redundant and we
have the problem of cleaning up the fb referenced in there somehow
(especially for CONFIG_FB=n). That's kinda the reason why I don't like it
very much ...
The below points are for the next patch, just noting them here for the
full picture. I haven't read carefully through that patch yet, so might
all be correct already.
2. We need to clean up fb reference in the plane config. Iirc your current
patch 3 fails that for CONFIG_FB=n
3. fbdev needs to grab one more reference (if it decides to steal the bios
framebuffer) to make sure the fbdev survives. But besides that I don't
think we need anything else - any subsequent modeset will update
references correctly. And for the fbdev config we can rely on the fastboot
modeset paths to ellide any real updates when fbcon sets its desired
config (which hopefully matches what the bios has set up already).
> + }
> }
>
> static void
> @@ -11237,11 +11362,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
> intel_modeset_init_hw(dev);
>
> intel_setup_overlay(dev);
> -
> - drm_modeset_lock_all(dev);
> - drm_mode_config_reset(dev);
> - intel_modeset_setup_hw_state(dev, false);
> - drm_modeset_unlock_all(dev);
As mention in the dpio fixup patch I'd like this code block move to be
split out in a small prep patch.
Cheers, Daniel
> }
>
> void intel_modeset_cleanup(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ea62673..4787773 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -209,6 +209,12 @@ typedef struct dpll {
> int p;
> } intel_clock_t;
>
> +struct intel_plane_config {
> + struct intel_framebuffer *fb; /* ends up managed by intel_fbdev.c */
> + bool tiled;
> + int size;
> +};
> +
> struct intel_crtc_config {
> /**
> * quirks - bitfield with hw state readout quirks
> @@ -358,6 +364,7 @@ struct intel_crtc {
> bool cursor_visible;
>
> struct intel_crtc_config config;
> + struct intel_plane_config plane_config;
>
> uint32_t ddi_pll_sel;
>
> @@ -707,6 +714,7 @@ void hsw_enable_ips(struct intel_crtc *crtc);
> void hsw_disable_ips(struct intel_crtc *crtc);
> void intel_display_set_init_power(struct drm_device *dev, bool enable);
> int valleyview_get_vco(struct drm_i915_private *dev_priv);
> +int intel_format_to_fourcc(int format);
>
> /* intel_dp.c */
> void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
2013-12-14 11:01 ` Daniel Vetter
@ 2013-12-17 0:01 ` Jesse Barnes
2013-12-17 8:38 ` Daniel Vetter
0 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-17 0:01 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Sat, 14 Dec 2013 12:01:47 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> But I still think the fb lifetime management is a bit broken here and we
> need a few small changes:
>
> 1. Right here in this loop we need to assign the fb from the plane_config
> ot the crtc->fb pointer and grab an fb reference for that.
>
> If we don't do that we'll fall over for CONFIG_FB=n
>
> A side-effect of that is that plane_config is now fairly redundant and we
> have the problem of cleaning up the fb referenced in there somehow
> (especially for CONFIG_FB=n). That's kinda the reason why I don't like it
> very much ...
>
> The below points are for the next patch, just noting them here for the
> full picture. I haven't read carefully through that patch yet, so might
> all be correct already.
>
> 2. We need to clean up fb reference in the plane config. Iirc your current
> patch 3 fails that for CONFIG_FB=n
Hm yeah the ownership is less clear in the CONFIG_FB=n case. I think
the driver will own the buffer, and it'll get dropped on the first mode
set with a new buffer. But even then there will be no process to deref
the object finally, so it'll stick around. Hm... maybe just disable it
if CONFIG_FB=n is the right answer for now.
> 3. fbdev needs to grab one more reference (if it decides to steal the bios
> framebuffer) to make sure the fbdev survives. But besides that I don't
> think we need anything else - any subsequent modeset will update
> references correctly. And for the fbdev config we can rely on the fastboot
> modeset paths to ellide any real updates when fbcon sets its desired
> config (which hopefully matches what the bios has set up already).
I thought this was correct already... I'll post with the CONFIG_FB
changes and you can check again. But I take a ref in the fbdev layer
on both the GEM object and the fb that we end up using, so it should
have the appropriate ref in that case.
> > -
> > - drm_modeset_lock_all(dev);
> > - drm_mode_config_reset(dev);
> > - intel_modeset_setup_hw_state(dev, false);
> > - drm_modeset_unlock_all(dev);
>
> As mention in the dpio fixup patch I'd like this code block move to be
> split out in a small prep patch.
Ok will do.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
2013-12-17 0:01 ` Jesse Barnes
@ 2013-12-17 8:38 ` Daniel Vetter
2013-12-17 21:04 ` Jesse Barnes
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-17 8:38 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Mon, Dec 16, 2013 at 04:01:41PM -0800, Jesse Barnes wrote:
> On Sat, 14 Dec 2013 12:01:47 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> > But I still think the fb lifetime management is a bit broken here and we
> > need a few small changes:
> >
> > 1. Right here in this loop we need to assign the fb from the plane_config
> > ot the crtc->fb pointer and grab an fb reference for that.
> >
> > If we don't do that we'll fall over for CONFIG_FB=n
> >
> > A side-effect of that is that plane_config is now fairly redundant and we
> > have the problem of cleaning up the fb referenced in there somehow
> > (especially for CONFIG_FB=n). That's kinda the reason why I don't like it
> > very much ...
> >
> > The below points are for the next patch, just noting them here for the
> > full picture. I haven't read carefully through that patch yet, so might
> > all be correct already.
> >
> > 2. We need to clean up fb reference in the plane config. Iirc your current
> > patch 3 fails that for CONFIG_FB=n
>
> Hm yeah the ownership is less clear in the CONFIG_FB=n case. I think
> the driver will own the buffer, and it'll get dropped on the first mode
> set with a new buffer. But even then there will be no process to deref
> the object finally, so it'll stick around. Hm... maybe just disable it
> if CONFIG_FB=n is the right answer for now.
If you switch the fbdev code to look at crtc->fb instead of
crtc->plane_config.fb and just drop the plane_config.fb pointer (and it's
reference) it should pan out. Then the only reference+pointers we have are
the ones in crtc->fb, and the drm core will take care of those.
fbdev then needs to grab an additional reference for ifbdev->fb, but it
needs to do that anyway. Your current code seems to just steal the initial
reference from creating the framebuffer in ->get_plane_config.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
2013-12-17 8:38 ` Daniel Vetter
@ 2013-12-17 21:04 ` Jesse Barnes
2013-12-17 21:19 ` Daniel Vetter
0 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-17 21:04 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Tue, 17 Dec 2013 09:38:36 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 16, 2013 at 04:01:41PM -0800, Jesse Barnes wrote:
> > On Sat, 14 Dec 2013 12:01:47 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > But I still think the fb lifetime management is a bit broken here and we
> > > need a few small changes:
> > >
> > > 1. Right here in this loop we need to assign the fb from the plane_config
> > > ot the crtc->fb pointer and grab an fb reference for that.
> > >
> > > If we don't do that we'll fall over for CONFIG_FB=n
> > >
> > > A side-effect of that is that plane_config is now fairly redundant and we
> > > have the problem of cleaning up the fb referenced in there somehow
> > > (especially for CONFIG_FB=n). That's kinda the reason why I don't like it
> > > very much ...
> > >
> > > The below points are for the next patch, just noting them here for the
> > > full picture. I haven't read carefully through that patch yet, so might
> > > all be correct already.
> > >
> > > 2. We need to clean up fb reference in the plane config. Iirc your current
> > > patch 3 fails that for CONFIG_FB=n
> >
> > Hm yeah the ownership is less clear in the CONFIG_FB=n case. I think
> > the driver will own the buffer, and it'll get dropped on the first mode
> > set with a new buffer. But even then there will be no process to deref
> > the object finally, so it'll stick around. Hm... maybe just disable it
> > if CONFIG_FB=n is the right answer for now.
>
> If you switch the fbdev code to look at crtc->fb instead of
> crtc->plane_config.fb and just drop the plane_config.fb pointer (and it's
> reference) it should pan out. Then the only reference+pointers we have are
> the ones in crtc->fb, and the drm core will take care of those.
How can I switch to looking at crtc->fb? Or do you mean
get_plane_config should stuff a full fb into crtc->fb instead of the
plane_config struct?
> fbdev then needs to grab an additional reference for ifbdev->fb, but it
> needs to do that anyway. Your current code seems to just steal the initial
> reference from creating the framebuffer in ->get_plane_config.
Right.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
2013-12-17 21:04 ` Jesse Barnes
@ 2013-12-17 21:19 ` Daniel Vetter
0 siblings, 0 replies; 36+ messages in thread
From: Daniel Vetter @ 2013-12-17 21:19 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Tue, Dec 17, 2013 at 10:04 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > Hm yeah the ownership is less clear in the CONFIG_FB=n case. I think
>> > the driver will own the buffer, and it'll get dropped on the first mode
>> > set with a new buffer. But even then there will be no process to deref
>> > the object finally, so it'll stick around. Hm... maybe just disable it
>> > if CONFIG_FB=n is the right answer for now.
>>
>> If you switch the fbdev code to look at crtc->fb instead of
>> crtc->plane_config.fb and just drop the plane_config.fb pointer (and it's
>> reference) it should pan out. Then the only reference+pointers we have are
>> the ones in crtc->fb, and the drm core will take care of those.
>
> How can I switch to looking at crtc->fb? Or do you mean
> get_plane_config should stuff a full fb into crtc->fb instead of the
> plane_config struct?
Yeah, that would be my idea. Since crtc->fb is managed by the drm core
we could also enable the recovery for CONFIG_FB=n and so enable smooth
transitions without fbdev being present. Well, super-smooth only with
fastboot ofc ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-12 20:41 [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time Jesse Barnes
2013-12-12 20:41 ` [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init Jesse Barnes
@ 2013-12-12 20:41 ` Jesse Barnes
2013-12-12 22:54 ` Daniel Vetter
2013-12-14 11:13 ` Daniel Vetter
2013-12-12 20:41 ` [PATCH 4/6] drm/i915: don't memset the fb buffer if preallocated Jesse Barnes
` (3 subsequent siblings)
5 siblings, 2 replies; 36+ messages in thread
From: Jesse Barnes @ 2013-12-12 20:41 UTC (permalink / raw)
To: intel-gfx
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 <jbarnes@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 94183af..b868331 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7817,11 +7817,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;
ifbdev->helper.fb = fb;
ifbdev->helper.fbdev = info;
@@ -194,14 +203,14 @@ static int intelfb_create(struct drm_fb_helper *helper,
* If the object is stolen however, it will be full of whatever
* garbage was left in there.
*/
- if (ifbdev->ifb.obj->stolen)
+ if (ifbdev->fb->obj->stolen)
memset_io(info->screen_base, 0, info->screen_size);
/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
- DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08lx, bo %p\n",
+ DRM_DEBUG_KMS("allocated %dx%d fb: map %p, bo %p, size 0x%lx\n",
fb->width, fb->height,
- i915_gem_obj_ggtt_offset(obj), obj);
+ info->screen_base, obj, info->screen_size);
mutex_unlock(&dev->struct_mutex);
vga_switcheroo_client_fb_set(dev->pdev, info);
@@ -236,6 +245,96 @@ static void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
*blue = intel_crtc->lut_b[regno] << 8;
}
+static struct drm_fb_helper_crtc *
+intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
+{
+ int i;
+
+ for (i = 0; i < fb_helper->crtc_count; i++)
+ if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
+ return &fb_helper->crtc_info[i];
+
+ return NULL;
+}
+
+/*
+ * Try to read the BIOS display configuration and use it for the initial
+ * fb configuration.
+ *
+ * The BIOS or boot loader will generally create an initial display
+ * configuration for us that includes some set of active pipes and displays.
+ * This routine tries to figure out which pipes and connectors are active
+ * and stuffs them into the crtcs and modes array given to us by the
+ * drm_fb_helper code.
+ *
+ * The overall sequence is:
+ * intel_fbdev_init - from driver load
+ * intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data
+ * drm_fb_helper_init - build fb helper structs
+ * drm_fb_helper_single_add_all_connectors - more fb helper structs
+ * intel_fbdev_initial_config - apply the config
+ * drm_fb_helper_initial_config - call ->probe then register_framebuffer()
+ * drm_setup_crtcs - build crtc config for fbdev
+ * intel_fb_initial_config - find active connectors etc
+ * drm_fb_helper_single_fb_probe - set up fbdev
+ * intelfb_create - re-use or alloc fb, build out fbdev structs
+ *
+ * If the BIOS or boot loader leaves the display in VGA mode, there's not
+ * much we can do; switching out of that mode involves allocating a new,
+ * high res buffer, and also recalculating bandwidth requirements for the
+ * new bpp configuration.
+ */
+static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
+ struct drm_fb_helper_crtc **crtcs,
+ struct drm_display_mode **modes,
+ bool *enabled, int width, int height)
+{
+ int i;
+
+ for (i = 0; i < fb_helper->connector_count; i++) {
+ struct drm_connector *connector;
+ struct drm_encoder *encoder;
+
+ connector = fb_helper->connector_info[i]->connector;
+ if (!enabled[i]) {
+ DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
+ connector->base.id);
+ continue;
+ }
+
+ encoder = connector->encoder;
+ if (!encoder || !encoder->crtc) {
+ DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
+ connector->base.id);
+ continue;
+ }
+
+ if (WARN_ON(!encoder->crtc->enabled)) {
+ DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n",
+ drm_get_connector_name(connector),
+ encoder->crtc->base.id);
+ return false;
+ }
+
+ if (!to_intel_crtc(encoder->crtc)->active) {
+ DRM_DEBUG_KMS("connector %s on inactive crtc %d, borting\n",
+ drm_get_connector_name(connector),
+ encoder->crtc->base.id);
+ return false;
+ }
+
+ modes[i] = &encoder->crtc->mode;
+ crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);
+
+ DRM_DEBUG_KMS("connector %s on crtc %d: %s\n",
+ drm_get_connector_name(connector),
+ encoder->crtc->base.id,
+ modes[i]->name);
+ }
+
+ return true;
+}
+
static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
.gamma_set = intel_crtc_fb_gamma_set,
.gamma_get = intel_crtc_fb_gamma_get,
@@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
drm_fb_helper_fini(&ifbdev->helper);
- drm_framebuffer_unregister_private(&ifbdev->ifb.base);
- intel_framebuffer_fini(&ifbdev->ifb);
+ drm_framebuffer_unregister_private(&ifbdev->fb->base);
+ intel_framebuffer_fini(ifbdev->fb);
+ kfree(ifbdev->fb);
+}
+
+/*
+ * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible.
+ * The core display code will have read out the current plane configuration,
+ * so we use that to figure out if there's an object for us to use as the
+ * fb, and if so, we re-use it for the fbdev configuration.
+ *
+ * Note we only support a single fb shared across pipes for boot (mostly for
+ * fbcon), so we just find the biggest and use that.
+ */
+void intel_fbdev_init_bios(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_fbdev *ifbdev;
+ struct intel_framebuffer *fb = NULL;
+ struct drm_crtc *crtc;
+ struct intel_crtc *intel_crtc;
+ struct intel_plane_config *plane_config = NULL;
+ unsigned int last_size = 0;
+
+ ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
+ if (ifbdev == NULL) {
+ DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
+ return;
+ }
+
+ /* Find the largest framebuffer to use, then free the others */
+ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+ intel_crtc = to_intel_crtc(crtc);
+
+ if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) {
+ DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
+ pipe_name(intel_crtc->pipe));
+ continue;
+ }
+
+ if (intel_crtc->plane_config.size > last_size) {
+ plane_config = &intel_crtc->plane_config;
+ last_size = plane_config->size;
+ fb = plane_config->fb;
+ }
+ }
+
+ /* Free unused fbs */
+ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+ struct intel_framebuffer *cur_fb;
+
+ intel_crtc = to_intel_crtc(crtc);
+ cur_fb = intel_crtc->plane_config.fb;
+
+ if (cur_fb && cur_fb != fb)
+ intel_framebuffer_fini(cur_fb);
+ }
+
+ if (!fb) {
+ DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
+ goto out_free;
+ }
+
+ ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel;
+ ifbdev->helper.funcs = &intel_fb_helper_funcs;
+ ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
+ ifbdev->fb = fb;
+
+ /* Assuming a single fb across all pipes here */
+ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+ intel_crtc = to_intel_crtc(crtc);
+
+ if (!intel_crtc->active)
+ continue;
+
+ /*
+ * This should only fail on the first one so we don't need
+ * to cleanup any secondary crtc->fbs
+ */
+ if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL))
+ goto out_unref_obj;
+
+ crtc->fb = &fb->base;
+ drm_gem_object_reference(&fb->obj->base);
+ drm_framebuffer_reference(&fb->base);
+ }
+
+ dev_priv->fbdev = ifbdev;
+
+ DRM_DEBUG_KMS("using BIOS fb for initial console\n");
+ return;
+
+out_unref_obj:
+ intel_framebuffer_fini(fb);
+out_free:
+ kfree(ifbdev);
}
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;
+ }
ret = drm_fb_helper_init(dev, &ifbdev->helper,
INTEL_INFO(dev)->num_pipes,
4);
if (ret) {
+ dev_priv->fbdev = NULL;
kfree(ifbdev);
return ret;
}
@@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev)
void intel_fbdev_initial_config(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_fbdev *ifbdev = dev_priv->fbdev;
/* Due to peculiar init order wrt to hpd handling this is separate. */
- drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
+ drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
}
void intel_fbdev_fini(struct drm_device *dev)
@@ -322,7 +524,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
* been restored from swap. If the object is stolen however, it will be
* full of whatever garbage was left in there.
*/
- if (state == FBINFO_STATE_RUNNING && ifbdev->ifb.obj->stolen)
+ if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
memset_io(info->screen_base, 0, info->screen_size);
fb_set_suspend(info, state);
@@ -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);
}
void intel_fbdev_restore_mode(struct drm_device *dev)
--
1.8.4.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-12 20:41 ` [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7 Jesse Barnes
@ 2013-12-12 22:54 ` Daniel Vetter
2013-12-12 23:45 ` Jesse Barnes
2013-12-13 19:09 ` Jesse Barnes
2013-12-14 11:13 ` Daniel Vetter
1 sibling, 2 replies; 36+ messages in thread
From: Daniel Vetter @ 2013-12-12 22:54 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote:
> 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 <jbarnes@virtuousgeek.org>
[snip]
> @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>
> drm_fb_helper_fini(&ifbdev->helper);
>
> - drm_framebuffer_unregister_private(&ifbdev->ifb.base);
> - intel_framebuffer_fini(&ifbdev->ifb);
> + drm_framebuffer_unregister_private(&ifbdev->fb->base);
> + intel_framebuffer_fini(ifbdev->fb);
> + kfree(ifbdev->fb);
No need to go the private fb route here anymore since now the fb is
free-standing. Normal refcounting should work. But a separate prep/cleanup
patch (prep since switching ifbdev->fb from struct to point would look
neat as a separate patch).
> +}
> +
> +/*
> + * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible.
> + * The core display code will have read out the current plane configuration,
> + * so we use that to figure out if there's an object for us to use as the
> + * fb, and if so, we re-use it for the fbdev configuration.
> + *
> + * Note we only support a single fb shared across pipes for boot (mostly for
> + * fbcon), so we just find the biggest and use that.
> + */
> +void intel_fbdev_init_bios(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_fbdev *ifbdev;
> + struct intel_framebuffer *fb = NULL;
> + struct drm_crtc *crtc;
> + struct intel_crtc *intel_crtc;
> + struct intel_plane_config *plane_config = NULL;
> + unsigned int last_size = 0;
> +
> + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> + if (ifbdev == NULL) {
> + DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
> + return;
> + }
> +
> + /* Find the largest framebuffer to use, then free the others */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) {
> + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
> + pipe_name(intel_crtc->pipe));
> + continue;
> + }
> +
> + if (intel_crtc->plane_config.size > last_size) {
> + plane_config = &intel_crtc->plane_config;
> + last_size = plane_config->size;
> + fb = plane_config->fb;
> + }
> + }
> +
> + /* Free unused fbs */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + struct intel_framebuffer *cur_fb;
> +
> + intel_crtc = to_intel_crtc(crtc);
> + cur_fb = intel_crtc->plane_config.fb;
> +
> + if (cur_fb && cur_fb != fb)
> + intel_framebuffer_fini(cur_fb);
> + }
> +
> + if (!fb) {
> + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> + goto out_free;
> + }
> +
> + ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel;
> + ifbdev->helper.funcs = &intel_fb_helper_funcs;
> + ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
> + ifbdev->fb = fb;
> +
> + /* Assuming a single fb across all pipes here */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!intel_crtc->active)
> + continue;
> +
> + /*
> + * This should only fail on the first one so we don't need
> + * to cleanup any secondary crtc->fbs
> + */
> + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL))
> + goto out_unref_obj;
> +
> + crtc->fb = &fb->base;
> + drm_gem_object_reference(&fb->obj->base);
> + drm_framebuffer_reference(&fb->base);
> + }
> +
> + dev_priv->fbdev = ifbdev;
> +
> + DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> + return;
> +
> +out_unref_obj:
> + intel_framebuffer_fini(fb);
> +out_free:
> + kfree(ifbdev);
> }
>
> 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 */
If you need a comment to explain your control flow, it's probably too
clever ;-)
> + 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;
> + }
>
> ret = drm_fb_helper_init(dev, &ifbdev->helper,
> INTEL_INFO(dev)->num_pipes,
> 4);
> if (ret) {
> + dev_priv->fbdev = NULL;
> kfree(ifbdev);
> return ret;
> }
> @@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev)
> void intel_fbdev_initial_config(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_fbdev *ifbdev = dev_priv->fbdev;
>
> /* Due to peculiar init order wrt to hpd handling this is separate. */
> - drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
> + drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
> }
>
> void intel_fbdev_fini(struct drm_device *dev)
> @@ -322,7 +524,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> * been restored from swap. If the object is stolen however, it will be
> * full of whatever garbage was left in there.
> */
> - if (state == FBINFO_STATE_RUNNING && ifbdev->ifb.obj->stolen)
> + if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
> memset_io(info->screen_base, 0, info->screen_size);
>
> fb_set_suspend(info, state);
> @@ -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);
Ok, here's the real reason I've actually replied to this patch. This looks
like a separate bugfix. And there's not mention of it in the commit
message. Please split it out and give it the nice commit message
explanation it deserves (whatever it's doing here).
Cheers, Daniel
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-12 22:54 ` Daniel Vetter
@ 2013-12-12 23:45 ` Jesse Barnes
2013-12-13 19:09 ` Jesse Barnes
1 sibling, 0 replies; 36+ messages in thread
From: Jesse Barnes @ 2013-12-12 23:45 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Thu, 12 Dec 2013 23:54:37 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote:
> > 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 <jbarnes@virtuousgeek.org>
>
> [snip]
>
> > @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> >
> > drm_fb_helper_fini(&ifbdev->helper);
> >
> > - drm_framebuffer_unregister_private(&ifbdev->ifb.base);
> > - intel_framebuffer_fini(&ifbdev->ifb);
> > + drm_framebuffer_unregister_private(&ifbdev->fb->base);
> > + intel_framebuffer_fini(ifbdev->fb);
> > + kfree(ifbdev->fb);
>
> No need to go the private fb route here anymore since now the fb is
> free-standing. Normal refcounting should work. But a separate prep/cleanup
> patch (prep since switching ifbdev->fb from struct to point would look
> neat as a separate patch).
>
> > +}
> > +
> > +/*
> > + * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible.
> > + * The core display code will have read out the current plane configuration,
> > + * so we use that to figure out if there's an object for us to use as the
> > + * fb, and if so, we re-use it for the fbdev configuration.
> > + *
> > + * Note we only support a single fb shared across pipes for boot (mostly for
> > + * fbcon), so we just find the biggest and use that.
> > + */
> > +void intel_fbdev_init_bios(struct drm_device *dev)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_fbdev *ifbdev;
> > + struct intel_framebuffer *fb = NULL;
> > + struct drm_crtc *crtc;
> > + struct intel_crtc *intel_crtc;
> > + struct intel_plane_config *plane_config = NULL;
> > + unsigned int last_size = 0;
> > +
> > + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> > + if (ifbdev == NULL) {
> > + DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
> > + return;
> > + }
> > +
> > + /* Find the largest framebuffer to use, then free the others */
> > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > + intel_crtc = to_intel_crtc(crtc);
> > +
> > + if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) {
> > + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
> > + pipe_name(intel_crtc->pipe));
> > + continue;
> > + }
> > +
> > + if (intel_crtc->plane_config.size > last_size) {
> > + plane_config = &intel_crtc->plane_config;
> > + last_size = plane_config->size;
> > + fb = plane_config->fb;
> > + }
> > + }
> > +
> > + /* Free unused fbs */
> > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > + struct intel_framebuffer *cur_fb;
> > +
> > + intel_crtc = to_intel_crtc(crtc);
> > + cur_fb = intel_crtc->plane_config.fb;
> > +
> > + if (cur_fb && cur_fb != fb)
> > + intel_framebuffer_fini(cur_fb);
> > + }
> > +
> > + if (!fb) {
> > + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> > + goto out_free;
> > + }
> > +
> > + ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel;
> > + ifbdev->helper.funcs = &intel_fb_helper_funcs;
> > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
> > + ifbdev->fb = fb;
> > +
> > + /* Assuming a single fb across all pipes here */
> > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > + intel_crtc = to_intel_crtc(crtc);
> > +
> > + if (!intel_crtc->active)
> > + continue;
> > +
> > + /*
> > + * This should only fail on the first one so we don't need
> > + * to cleanup any secondary crtc->fbs
> > + */
> > + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL))
> > + goto out_unref_obj;
> > +
> > + crtc->fb = &fb->base;
> > + drm_gem_object_reference(&fb->obj->base);
> > + drm_framebuffer_reference(&fb->base);
> > + }
> > +
> > + dev_priv->fbdev = ifbdev;
> > +
> > + DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> > + return;
> > +
> > +out_unref_obj:
> > + intel_framebuffer_fini(fb);
> > +out_free:
> > + kfree(ifbdev);
> > }
> >
> > 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 */
>
> If you need a comment to explain your control flow, it's probably too
> clever ;-)
I could add a return value I suppose, but I didn't think it was too
clever...
>
> > + 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;
> > + }
> >
> > ret = drm_fb_helper_init(dev, &ifbdev->helper,
> > INTEL_INFO(dev)->num_pipes,
> > 4);
> > if (ret) {
> > + dev_priv->fbdev = NULL;
> > kfree(ifbdev);
> > return ret;
> > }
> > @@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev)
> > void intel_fbdev_initial_config(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_fbdev *ifbdev = dev_priv->fbdev;
> >
> > /* Due to peculiar init order wrt to hpd handling this is separate. */
> > - drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
> > + drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
> > }
> >
> > void intel_fbdev_fini(struct drm_device *dev)
> > @@ -322,7 +524,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> > * been restored from swap. If the object is stolen however, it will be
> > * full of whatever garbage was left in there.
> > */
> > - if (state == FBINFO_STATE_RUNNING && ifbdev->ifb.obj->stolen)
> > + if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
> > memset_io(info->screen_base, 0, info->screen_size);
> >
> > fb_set_suspend(info, state);
> > @@ -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);
>
> Ok, here's the real reason I've actually replied to this patch. This looks
> like a separate bugfix. And there's not mention of it in the commit
> message. Please split it out and give it the nice commit message
> explanation it deserves (whatever it's doing here).
Oh this hunk may be spurious... I think it hit this case when we were
failing to init dev_priv->fbdev and got an early hotplug event. But
that should no longer be the case.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-12 22:54 ` Daniel Vetter
2013-12-12 23:45 ` Jesse Barnes
@ 2013-12-13 19:09 ` Jesse Barnes
2013-12-13 20:47 ` Daniel Vetter
1 sibling, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-13 19:09 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Thu, 12 Dec 2013 23:54:37 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> > @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> >
> > drm_fb_helper_fini(&ifbdev->helper);
> >
> > - drm_framebuffer_unregister_private(&ifbdev->ifb.base);
> > - intel_framebuffer_fini(&ifbdev->ifb);
> > + drm_framebuffer_unregister_private(&ifbdev->fb->base);
> > + intel_framebuffer_fini(ifbdev->fb);
> > + kfree(ifbdev->fb);
>
> No need to go the private fb route here anymore since now the fb is
> free-standing. Normal refcounting should work. But a separate prep/cleanup
> patch (prep since switching ifbdev->fb from struct to point would look
> neat as a separate patch).
Oh and can you explain this? I wouldn't be surprised if I got the
refcounting wrong, but given how tricky it can be, can you explain
where we'll take the ref here, and show that the right thing will
happen if/when we mode set away from this buffer?
I haven't actually seen a bug here with or without this patch (no
crashes or warns), but I thought I needed this to make sure the obj
didn't get a negative count after a mode set...
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-13 19:09 ` Jesse Barnes
@ 2013-12-13 20:47 ` Daniel Vetter
2013-12-14 0:43 ` Jesse Barnes
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-13 20:47 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 12 Dec 2013 23:54:37 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> > @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>> >
>> > drm_fb_helper_fini(&ifbdev->helper);
>> >
>> > - drm_framebuffer_unregister_private(&ifbdev->ifb.base);
>> > - intel_framebuffer_fini(&ifbdev->ifb);
>> > + drm_framebuffer_unregister_private(&ifbdev->fb->base);
>> > + intel_framebuffer_fini(ifbdev->fb);
>> > + kfree(ifbdev->fb);
>>
>> No need to go the private fb route here anymore since now the fb is
>> free-standing. Normal refcounting should work. But a separate prep/cleanup
>> patch (prep since switching ifbdev->fb from struct to point would look
>> neat as a separate patch).
>
> Oh and can you explain this? I wouldn't be surprised if I got the
> refcounting wrong, but given how tricky it can be, can you explain
> where we'll take the ref here, and show that the right thing will
> happen if/when we mode set away from this buffer?
>
> I haven't actually seen a bug here with or without this patch (no
> crashes or warns), but I thought I needed this to make sure the obj
> didn't get a negative count after a mode set...
There's no bug here, and if you underrun the the refcount the current
logic here won't help. It's just that the explicit call to _fini was
an artifact of the old logic with embedding the framebuffer into the
fbdev structure. But now that the fbdev framebuffer is freestanding
there's no need for that - you exactly duplicate
intel_user_framebuffer_destroy now.
So a simple drm_framebuffer_unreference will do the trick and makes it
clearer that this is now just another fb object with normal lifetime
rules.
I guess I score points for unclear review here ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-13 20:47 ` Daniel Vetter
@ 2013-12-14 0:43 ` Jesse Barnes
2013-12-14 10:44 ` Daniel Vetter
0 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-14 0:43 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Fri, 13 Dec 2013 21:47:45 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Thu, 12 Dec 2013 23:54:37 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> >> > @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> >> >
> >> > drm_fb_helper_fini(&ifbdev->helper);
> >> >
> >> > - drm_framebuffer_unregister_private(&ifbdev->ifb.base);
> >> > - intel_framebuffer_fini(&ifbdev->ifb);
> >> > + drm_framebuffer_unregister_private(&ifbdev->fb->base);
> >> > + intel_framebuffer_fini(ifbdev->fb);
> >> > + kfree(ifbdev->fb);
> >>
> >> No need to go the private fb route here anymore since now the fb is
> >> free-standing. Normal refcounting should work. But a separate prep/cleanup
> >> patch (prep since switching ifbdev->fb from struct to point would look
> >> neat as a separate patch).
> >
> > Oh and can you explain this? I wouldn't be surprised if I got the
> > refcounting wrong, but given how tricky it can be, can you explain
> > where we'll take the ref here, and show that the right thing will
> > happen if/when we mode set away from this buffer?
> >
> > I haven't actually seen a bug here with or without this patch (no
> > crashes or warns), but I thought I needed this to make sure the obj
> > didn't get a negative count after a mode set...
>
> There's no bug here, and if you underrun the the refcount the current
> logic here won't help. It's just that the explicit call to _fini was
> an artifact of the old logic with embedding the framebuffer into the
> fbdev structure. But now that the fbdev framebuffer is freestanding
> there's no need for that - you exactly duplicate
> intel_user_framebuffer_destroy now.
>
> So a simple drm_framebuffer_unreference will do the trick and makes it
> clearer that this is now just another fb object with normal lifetime
> rules.
>
> I guess I score points for unclear review here ;-)
Oh! I had this mixed up with the refs I take in the init_bios code...
I thought you were saying those weren't necessary and I was getting
really confused.
This is just fixing up existing code to use the new field name, so no
functional change. I see what you mean about splitting out the field
change, but now that would be a pain :/
Do you want the above removed as a separate patch regardless of where
the rest of the patches go?
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-14 0:43 ` Jesse Barnes
@ 2013-12-14 10:44 ` Daniel Vetter
2013-12-14 11:33 ` Daniel Vetter
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-14 10:44 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Fri, Dec 13, 2013 at 04:43:50PM -0800, Jesse Barnes wrote:
> On Fri, 13 Dec 2013 21:47:45 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > On Thu, 12 Dec 2013 23:54:37 +0100
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > >> > @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> > >> >
> > >> > drm_fb_helper_fini(&ifbdev->helper);
> > >> >
> > >> > - drm_framebuffer_unregister_private(&ifbdev->ifb.base);
> > >> > - intel_framebuffer_fini(&ifbdev->ifb);
> > >> > + drm_framebuffer_unregister_private(&ifbdev->fb->base);
> > >> > + intel_framebuffer_fini(ifbdev->fb);
> > >> > + kfree(ifbdev->fb);
> > >>
> > >> No need to go the private fb route here anymore since now the fb is
> > >> free-standing. Normal refcounting should work. But a separate prep/cleanup
> > >> patch (prep since switching ifbdev->fb from struct to point would look
> > >> neat as a separate patch).
> > >
> > > Oh and can you explain this? I wouldn't be surprised if I got the
> > > refcounting wrong, but given how tricky it can be, can you explain
> > > where we'll take the ref here, and show that the right thing will
> > > happen if/when we mode set away from this buffer?
> > >
> > > I haven't actually seen a bug here with or without this patch (no
> > > crashes or warns), but I thought I needed this to make sure the obj
> > > didn't get a negative count after a mode set...
> >
> > There's no bug here, and if you underrun the the refcount the current
> > logic here won't help. It's just that the explicit call to _fini was
> > an artifact of the old logic with embedding the framebuffer into the
> > fbdev structure. But now that the fbdev framebuffer is freestanding
> > there's no need for that - you exactly duplicate
> > intel_user_framebuffer_destroy now.
> >
> > So a simple drm_framebuffer_unreference will do the trick and makes it
> > clearer that this is now just another fb object with normal lifetime
> > rules.
> >
> > I guess I score points for unclear review here ;-)
>
> Oh! I had this mixed up with the refs I take in the init_bios code...
> I thought you were saying those weren't necessary and I was getting
> really confused.
>
> This is just fixing up existing code to use the new field name, so no
> functional change. I see what you mean about splitting out the field
> change, but now that would be a pain :/
Yeah, the switch from struct to pointer for ifbdev->fb would be neat as a
separate patch, but also real pain to split out now.
> Do you want the above removed as a separate patch regardless of where
> the rest of the patches go?
Imo it should go with the switch to pointers, so this patch here is fine.
Maybe a quick mention in the commit message about it.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-14 10:44 ` Daniel Vetter
@ 2013-12-14 11:33 ` Daniel Vetter
0 siblings, 0 replies; 36+ messages in thread
From: Daniel Vetter @ 2013-12-14 11:33 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Sat, Dec 14, 2013 at 11:44:06AM +0100, Daniel Vetter wrote:
> On Fri, Dec 13, 2013 at 04:43:50PM -0800, Jesse Barnes wrote:
> > This is just fixing up existing code to use the new field name, so no
> > functional change. I see what you mean about splitting out the field
> > change, but now that would be a pain :/
>
> Yeah, the switch from struct to pointer for ifbdev->fb would be neat as a
> separate patch, but also real pain to split out now.
I'm inclined to say that we actually need the split-out conversion to a
pointer. I might have lost my traces somewhere in this maze, but I think
with the current patches we can have a NULL deref on ifbdev->fb in
intelfb_create. Not sure, but a split-out patch would definitely help ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-12 20:41 ` [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7 Jesse Barnes
2013-12-12 22:54 ` Daniel Vetter
@ 2013-12-14 11:13 ` Daniel Vetter
2013-12-14 11:36 ` Daniel Vetter
1 sibling, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-14 11:13 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote:
> 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 <jbarnes@virtuousgeek.org>
Ok, I've thought through the lifetime rules for the fbcon takeover and I
think we need some more polish for that. I also think we could simplify
the inital_config logic quite a bit. More comments below.
Cheers, Daniel
> ---
> 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 94183af..b868331 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7817,11 +7817,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;
>
> ifbdev->helper.fb = fb;
> ifbdev->helper.fbdev = info;
> @@ -194,14 +203,14 @@ static int intelfb_create(struct drm_fb_helper *helper,
> * If the object is stolen however, it will be full of whatever
> * garbage was left in there.
> */
> - if (ifbdev->ifb.obj->stolen)
> + if (ifbdev->fb->obj->stolen)
> memset_io(info->screen_base, 0, info->screen_size);
>
> /* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
>
> - DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08lx, bo %p\n",
> + DRM_DEBUG_KMS("allocated %dx%d fb: map %p, bo %p, size 0x%lx\n",
> fb->width, fb->height,
> - i915_gem_obj_ggtt_offset(obj), obj);
> + info->screen_base, obj, info->screen_size);
>
> mutex_unlock(&dev->struct_mutex);
> vga_switcheroo_client_fb_set(dev->pdev, info);
> @@ -236,6 +245,96 @@ static void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
> *blue = intel_crtc->lut_b[regno] << 8;
> }
>
> +static struct drm_fb_helper_crtc *
> +intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
> +{
> + int i;
> +
> + for (i = 0; i < fb_helper->crtc_count; i++)
> + if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
> + return &fb_helper->crtc_info[i];
> +
> + return NULL;
> +}
> +
> +/*
> + * Try to read the BIOS display configuration and use it for the initial
> + * fb configuration.
> + *
> + * The BIOS or boot loader will generally create an initial display
> + * configuration for us that includes some set of active pipes and displays.
> + * This routine tries to figure out which pipes and connectors are active
> + * and stuffs them into the crtcs and modes array given to us by the
> + * drm_fb_helper code.
> + *
> + * The overall sequence is:
> + * intel_fbdev_init - from driver load
> + * intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data
> + * drm_fb_helper_init - build fb helper structs
> + * drm_fb_helper_single_add_all_connectors - more fb helper structs
> + * intel_fbdev_initial_config - apply the config
> + * drm_fb_helper_initial_config - call ->probe then register_framebuffer()
> + * drm_setup_crtcs - build crtc config for fbdev
> + * intel_fb_initial_config - find active connectors etc
> + * drm_fb_helper_single_fb_probe - set up fbdev
> + * intelfb_create - re-use or alloc fb, build out fbdev structs
> + *
> + * If the BIOS or boot loader leaves the display in VGA mode, there's not
> + * much we can do; switching out of that mode involves allocating a new,
> + * high res buffer, and also recalculating bandwidth requirements for the
> + * new bpp configuration.
> + */
> +static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> + struct drm_fb_helper_crtc **crtcs,
> + struct drm_display_mode **modes,
> + bool *enabled, int width, int height)
> +{
> + int i;
> +
> + for (i = 0; i < fb_helper->connector_count; i++) {
> + struct drm_connector *connector;
> + struct drm_encoder *encoder;
> +
> + connector = fb_helper->connector_info[i]->connector;
> + if (!enabled[i]) {
> + DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
> + connector->base.id);
> + continue;
> + }
> +
> + encoder = connector->encoder;
> + if (!encoder || !encoder->crtc) {
> + DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
> + connector->base.id);
> + continue;
> + }
> +
> + if (WARN_ON(!encoder->crtc->enabled)) {
> + DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n",
> + drm_get_connector_name(connector),
> + encoder->crtc->base.id);
> + return false;
> + }
> +
> + if (!to_intel_crtc(encoder->crtc)->active) {
> + DRM_DEBUG_KMS("connector %s on inactive crtc %d, borting\n",
> + drm_get_connector_name(connector),
> + encoder->crtc->base.id);
> + return false;
> + }
> +
> + modes[i] = &encoder->crtc->mode;
> + crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> +
> + DRM_DEBUG_KMS("connector %s on crtc %d: %s\n",
> + drm_get_connector_name(connector),
> + encoder->crtc->base.id,
> + modes[i]->name);
> + }
> +
> + return true;
> +}
> +
> static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> .gamma_set = intel_crtc_fb_gamma_set,
> .gamma_get = intel_crtc_fb_gamma_get,
> @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>
> drm_fb_helper_fini(&ifbdev->helper);
>
> - drm_framebuffer_unregister_private(&ifbdev->ifb.base);
> - intel_framebuffer_fini(&ifbdev->ifb);
> + drm_framebuffer_unregister_private(&ifbdev->fb->base);
> + intel_framebuffer_fini(ifbdev->fb);
> + kfree(ifbdev->fb);
> +}
> +
> +/*
> + * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible.
> + * The core display code will have read out the current plane configuration,
> + * so we use that to figure out if there's an object for us to use as the
> + * fb, and if so, we re-use it for the fbdev configuration.
> + *
> + * Note we only support a single fb shared across pipes for boot (mostly for
> + * fbcon), so we just find the biggest and use that.
> + */
> +void intel_fbdev_init_bios(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_fbdev *ifbdev;
> + struct intel_framebuffer *fb = NULL;
> + struct drm_crtc *crtc;
> + struct intel_crtc *intel_crtc;
> + struct intel_plane_config *plane_config = NULL;
> + unsigned int last_size = 0;
> +
> + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> + if (ifbdev == NULL) {
> + DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
> + return;
> + }
> +
> + /* Find the largest framebuffer to use, then free the others */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) {
> + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
> + pipe_name(intel_crtc->pipe));
> + continue;
> + }
> +
> + if (intel_crtc->plane_config.size > last_size) {
> + plane_config = &intel_crtc->plane_config;
> + last_size = plane_config->size;
> + fb = plane_config->fb;
> + }
> + }
> +
> + /* Free unused fbs */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + struct intel_framebuffer *cur_fb;
> +
> + intel_crtc = to_intel_crtc(crtc);
> + cur_fb = intel_crtc->plane_config.fb;
> +
> + if (cur_fb && cur_fb != fb)
> + intel_framebuffer_fini(cur_fb);
> + }
> +
> + if (!fb) {
> + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> + goto out_free;
> + }
> +
> + ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel;
> + ifbdev->helper.funcs = &intel_fb_helper_funcs;
> + ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
This here is a bit surprising - my model of operation here presumed that
if we correctly assign the crtc->fb and the ifbdev->fb pointers we could
fully rely on the fastboot setcrtc logic to eschew the modeset.
Being the ever-vary of special-purpose logic I'd much prefer this implicit
approach - otherwise we have one more special case to care about in the
fastboot=y/n and CONFIG_FB=y/n matrix.
So have you tried to ditch this special initial_config functions
(obviously only looks good with fastboot=1) or what precise corner-case
does this fix?
> + ifbdev->fb = fb;
> +
> + /* Assuming a single fb across all pipes here */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!intel_crtc->active)
> + continue;
> +
> + /*
> + * This should only fail on the first one so we don't need
> + * to cleanup any secondary crtc->fbs
> + */
> + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL))
> + goto out_unref_obj;
> +
> + crtc->fb = &fb->base;
> + drm_gem_object_reference(&fb->obj->base);
> + drm_framebuffer_reference(&fb->base);
As mentioned I think this part here needs to be moved to the earlier
plane_config reconstruction loop.
But checking the fb refcounting now it looks like we leak the initial
references that the plane_config struct holds (or I didn't spot it). And
we miss the reference for ifbdev->fb when stealing the bios framebuffer.
With the current code of using
intel_framebuffer_fini(ifbdev->fb);
kfree(ifbdev->fb);
You get away with this since the kfree will ignore the surplus references
from the plane_config. But if you replace this with the
drm_framebuffer_unreference call like I've suggested we'll have a real
leak. So I think this needs to be fixed.
> + }
> +
> + dev_priv->fbdev = ifbdev;
> +
> + DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> + return;
> +
> +out_unref_obj:
> + intel_framebuffer_fini(fb);
> +out_free:
> + kfree(ifbdev);
> }
>
> 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;
> + }
>
> ret = drm_fb_helper_init(dev, &ifbdev->helper,
> INTEL_INFO(dev)->num_pipes,
> 4);
> if (ret) {
> + dev_priv->fbdev = NULL;
> kfree(ifbdev);
> return ret;
> }
> @@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev)
> void intel_fbdev_initial_config(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_fbdev *ifbdev = dev_priv->fbdev;
>
> /* Due to peculiar init order wrt to hpd handling this is separate. */
> - drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
> + drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
> }
>
> void intel_fbdev_fini(struct drm_device *dev)
> @@ -322,7 +524,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> * been restored from swap. If the object is stolen however, it will be
> * full of whatever garbage was left in there.
> */
> - if (state == FBINFO_STATE_RUNNING && ifbdev->ifb.obj->stolen)
> + if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
> memset_io(info->screen_base, 0, info->screen_size);
>
> fb_set_suspend(info, state);
> @@ -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);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-14 11:13 ` Daniel Vetter
@ 2013-12-14 11:36 ` Daniel Vetter
2014-02-05 14:57 ` Jesse Barnes
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-14 11:36 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Sat, Dec 14, 2013 at 12:13:45PM +0100, Daniel Vetter wrote:
> On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote:
> > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
>
> This here is a bit surprising - my model of operation here presumed that
> if we correctly assign the crtc->fb and the ifbdev->fb pointers we could
> fully rely on the fastboot setcrtc logic to eschew the modeset.
>
> Being the ever-vary of special-purpose logic I'd much prefer this implicit
> approach - otherwise we have one more special case to care about in the
> fastboot=y/n and CONFIG_FB=y/n matrix.
>
> So have you tried to ditch this special initial_config functions
> (obviously only looks good with fastboot=1) or what precise corner-case
> does this fix?
Ok, I've dug out your old patch from almost a year ago which added the
->initial_config hook. I see the point now of copying exactly the bios
config in the hope that we end up with something that has a higher chance
of working.
But imo this is an issue separate from the "take over bios fb" feature
here, so this should be
- split into a separate patch
- used even when we fail to take over the bios fb
The later point will require some mode-from-pipe_config reconstruction to
work outside of the fastboot=1 hack mode.
I really like the idea though.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
2013-12-14 11:36 ` Daniel Vetter
@ 2014-02-05 14:57 ` Jesse Barnes
0 siblings, 0 replies; 36+ messages in thread
From: Jesse Barnes @ 2014-02-05 14:57 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Sat, 14 Dec 2013 12:36:30 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Dec 14, 2013 at 12:13:45PM +0100, Daniel Vetter wrote:
> > On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote:
> > > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
> >
> > This here is a bit surprising - my model of operation here presumed that
> > if we correctly assign the crtc->fb and the ifbdev->fb pointers we could
> > fully rely on the fastboot setcrtc logic to eschew the modeset.
> >
> > Being the ever-vary of special-purpose logic I'd much prefer this implicit
> > approach - otherwise we have one more special case to care about in the
> > fastboot=y/n and CONFIG_FB=y/n matrix.
> >
> > So have you tried to ditch this special initial_config functions
> > (obviously only looks good with fastboot=1) or what precise corner-case
> > does this fix?
>
> Ok, I've dug out your old patch from almost a year ago which added the
> ->initial_config hook. I see the point now of copying exactly the bios
> config in the hope that we end up with something that has a higher chance
> of working.
>
> But imo this is an issue separate from the "take over bios fb" feature
> here, so this should be
> - split into a separate patch
> - used even when we fail to take over the bios fb
> The later point will require some mode-from-pipe_config reconstruction to
> work outside of the fastboot=1 hack mode.
>
> I really like the idea though.
Ok I split this up, made fb a ptr, fixed up the CONFIG_FB bits, and I
think we figured out the crtc timing stuff. I think that's all the
feedback from the last round, so I'll re-post for some (hopefully)
final review.
There are some additional improvements that would be nice:
- compute_mode_changes needs to get smarter in general and look at
pfit state. Eventually we'll probably need a platform specific
callback for this that tells us whether a pipe shutdown is needed
for a given global configuration change.
- pfit disable should be split out into a separate callback from our
mode_set function (which also needs to get smarter after the
compute_mode_changes improvements)
- need to detect audio and infoframe configs and cross-check, though
I don't think these affect fastboot at all since we ought to be
able to leave them alone
All of the above aren't strictly necessary though, they're just
improvements on current code, some of which will overlap with the
atomic mode set work. So we may be able to flip the i915.fastboot=1
default switch once these bits land after some additional HSW and BDW
testing...
Thanks,
Jesse
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 4/6] drm/i915: don't memset the fb buffer if preallocated
2013-12-12 20:41 [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time Jesse Barnes
2013-12-12 20:41 ` [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init Jesse Barnes
2013-12-12 20:41 ` [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7 Jesse Barnes
@ 2013-12-12 20:41 ` Jesse Barnes
2013-12-14 11:28 ` Daniel Vetter
2013-12-12 20:41 ` [PATCH 5/6] drm/i915/vlv: move DPIO init earlier v2 Jesse Barnes
` (2 subsequent siblings)
5 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-12 20:41 UTC (permalink / raw)
To: intel-gfx
We want to preserve the BIOS/bootloader contents for later.
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
drivers/gpu/drm/i915/intel_fbdev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index db75f22..53675d2 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -132,6 +132,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
struct drm_framebuffer *fb;
struct drm_i915_gem_object *obj;
int size, ret;
+ bool prealloc = false;
mutex_lock(&dev->struct_mutex);
@@ -142,6 +143,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
goto out_unlock;
} else {
DRM_DEBUG_KMS("re-using BIOS fb\n");
+ prealloc = true;
sizes->fb_width = intel_fb->base.width;
sizes->fb_height = intel_fb->base.height;
}
@@ -203,7 +205,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
* If the object is stolen however, it will be full of whatever
* garbage was left in there.
*/
- if (ifbdev->fb->obj->stolen)
+ if (ifbdev->fb->obj->stolen && !prealloc)
memset_io(info->screen_base, 0, info->screen_size);
/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
--
1.8.4.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 4/6] drm/i915: don't memset the fb buffer if preallocated
2013-12-12 20:41 ` [PATCH 4/6] drm/i915: don't memset the fb buffer if preallocated Jesse Barnes
@ 2013-12-14 11:28 ` Daniel Vetter
2013-12-17 0:17 ` Jesse Barnes
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-14 11:28 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Dec 12, 2013 at 12:41:55PM -0800, Jesse Barnes wrote:
> We want to preserve the BIOS/bootloader contents for later.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index db75f22..53675d2 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -132,6 +132,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> struct drm_framebuffer *fb;
> struct drm_i915_gem_object *obj;
> int size, ret;
> + bool prealloc = false;
>
> mutex_lock(&dev->struct_mutex);
>
> @@ -142,6 +143,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> goto out_unlock;
> } else {
> DRM_DEBUG_KMS("re-using BIOS fb\n");
> + prealloc = true;
> sizes->fb_width = intel_fb->base.width;
> sizes->fb_height = intel_fb->base.height;
> }
> @@ -203,7 +205,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> * If the object is stolen however, it will be full of whatever
> * garbage was left in there.
> */
> - if (ifbdev->fb->obj->stolen)
> + if (ifbdev->fb->obj->stolen && !prealloc)
> memset_io(info->screen_base, 0, info->screen_size);
Shouldn't we just move this into intelfb_alloc? Now that allocating our
own fb isn't the only way any more to get at one this code + comment above
looks a bit misplaced.
-Daniel
>
> /* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 4/6] drm/i915: don't memset the fb buffer if preallocated
2013-12-14 11:28 ` Daniel Vetter
@ 2013-12-17 0:17 ` Jesse Barnes
2013-12-17 7:56 ` Daniel Vetter
0 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-17 0:17 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Sat, 14 Dec 2013 12:28:49 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Dec 12, 2013 at 12:41:55PM -0800, Jesse Barnes wrote:
> > We want to preserve the BIOS/bootloader contents for later.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> > drivers/gpu/drm/i915/intel_fbdev.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index db75f22..53675d2 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -132,6 +132,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > struct drm_framebuffer *fb;
> > struct drm_i915_gem_object *obj;
> > int size, ret;
> > + bool prealloc = false;
> >
> > mutex_lock(&dev->struct_mutex);
> >
> > @@ -142,6 +143,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > goto out_unlock;
> > } else {
> > DRM_DEBUG_KMS("re-using BIOS fb\n");
> > + prealloc = true;
> > sizes->fb_width = intel_fb->base.width;
> > sizes->fb_height = intel_fb->base.height;
> > }
> > @@ -203,7 +205,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > * If the object is stolen however, it will be full of whatever
> > * garbage was left in there.
> > */
> > - if (ifbdev->fb->obj->stolen)
> > + if (ifbdev->fb->obj->stolen && !prealloc)
> > memset_io(info->screen_base, 0, info->screen_size);
>
> Shouldn't we just move this into intelfb_alloc? Now that allocating our
> own fb isn't the only way any more to get at one this code + comment above
> looks a bit misplaced.
We can't since we haven't ioremapped the buffer and such until after
the alloc or reuse. It's a little awkward, I agree, but we need more
refactoring on the fbdev code to make it look nice.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 4/6] drm/i915: don't memset the fb buffer if preallocated
2013-12-17 0:17 ` Jesse Barnes
@ 2013-12-17 7:56 ` Daniel Vetter
0 siblings, 0 replies; 36+ messages in thread
From: Daniel Vetter @ 2013-12-17 7:56 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Mon, Dec 16, 2013 at 04:17:07PM -0800, Jesse Barnes wrote:
> On Sat, 14 Dec 2013 12:28:49 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Thu, Dec 12, 2013 at 12:41:55PM -0800, Jesse Barnes wrote:
> > > We want to preserve the BIOS/bootloader contents for later.
> > >
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > > drivers/gpu/drm/i915/intel_fbdev.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index db75f22..53675d2 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -132,6 +132,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > > struct drm_framebuffer *fb;
> > > struct drm_i915_gem_object *obj;
> > > int size, ret;
> > > + bool prealloc = false;
> > >
> > > mutex_lock(&dev->struct_mutex);
> > >
> > > @@ -142,6 +143,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > > goto out_unlock;
> > > } else {
> > > DRM_DEBUG_KMS("re-using BIOS fb\n");
> > > + prealloc = true;
> > > sizes->fb_width = intel_fb->base.width;
> > > sizes->fb_height = intel_fb->base.height;
> > > }
> > > @@ -203,7 +205,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > > * If the object is stolen however, it will be full of whatever
> > > * garbage was left in there.
> > > */
> > > - if (ifbdev->fb->obj->stolen)
> > > + if (ifbdev->fb->obj->stolen && !prealloc)
> > > memset_io(info->screen_base, 0, info->screen_size);
> >
> > Shouldn't we just move this into intelfb_alloc? Now that allocating our
> > own fb isn't the only way any more to get at one this code + comment above
> > looks a bit misplaced.
>
> We can't since we haven't ioremapped the buffer and such until after
> the alloc or reuse. It's a little awkward, I agree, but we need more
> refactoring on the fbdev code to make it look nice.
Argh, I've missed that. My idea was to get it more nearby the actual
stolen allocation so that we could move it into the stolen gem bo
allocation itself once we allow userspace such objects. Since then we
definitely need a more generic stolen mem allocator where we can specify
clearing with a flag or so. I just wanted my cake and eat it, too ;-)
So current code looks fine indeed.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 5/6] drm/i915/vlv: move DPIO init earlier v2
2013-12-12 20:41 [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time Jesse Barnes
` (2 preceding siblings ...)
2013-12-12 20:41 ` [PATCH 4/6] drm/i915: don't memset the fb buffer if preallocated Jesse Barnes
@ 2013-12-12 20:41 ` Jesse Barnes
2013-12-14 10:47 ` Daniel Vetter
2013-12-12 20:41 ` [PATCH 6/6] drm/i915: inform drm_fb_helper if we abandoned a connected output Jesse Barnes
2013-12-12 21:21 ` [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time Daniel Vetter
5 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-12 20:41 UTC (permalink / raw)
To: intel-gfx
It's needed for early mode state readout, which is in turn needed to
inherit the BIOS config. So split out the reset, which we need on
resume too, from the DPIO reg init, and do the latter earlier.
v2: split reset and reg init
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b868331..f265110 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1368,6 +1368,15 @@ static void intel_init_dpio(struct drm_device *dev)
return;
DPIO_PHY_IOSF_PORT(DPIO_PHY0) = IOSF_PORT_DPIO;
+}
+
+static void intel_dpio_reset(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ if (!IS_VALLEYVIEW(dev))
+ return;
+
/*
* From VLV2A0_DP_eDP_DPIO_driver_vbios_notes_10.docx -
* 6. De-assert cmn_reset/side_reset. Same as VLV X0.
@@ -10908,7 +10917,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
I915_WRITE(DPLL(PIPE_B), I915_READ(DPLL(PIPE_B)) |
DPLL_INTEGRATED_CRI_CLK_VLV);
- intel_init_dpio(dev);
+ intel_dpio_reset(dev);
mutex_lock(&dev->struct_mutex);
intel_enable_gt_powersave(dev);
@@ -10971,6 +10980,8 @@ void intel_modeset_init(struct drm_device *dev)
}
}
+ intel_init_dpio(dev);
+
intel_cpu_pll_init(dev);
intel_shared_dpll_init(dev);
--
1.8.4.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 5/6] drm/i915/vlv: move DPIO init earlier v2
2013-12-12 20:41 ` [PATCH 5/6] drm/i915/vlv: move DPIO init earlier v2 Jesse Barnes
@ 2013-12-14 10:47 ` Daniel Vetter
2013-12-17 0:02 ` Jesse Barnes
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-14 10:47 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Dec 12, 2013 at 12:41:56PM -0800, Jesse Barnes wrote:
> It's needed for early mode state readout, which is in turn needed to
> inherit the BIOS config. So split out the reset, which we need on
> resume too, from the DPIO reg init, and do the latter earlier.
>
> v2: split reset and reg init
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Isn't this a fixup due to the slight init reordering you do in patch 2? If
so we need to get this one in first, and I'd also prefer if the init
reordering is split out as a separate patch from patch 2 - that stuff
almost always ends up hurting us in some way or the order, so a minimal
patch as an obvious bisect point is imo worth it.
With those prep patches split out I could also start merging to get this
sucker moving finally.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b868331..f265110 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1368,6 +1368,15 @@ static void intel_init_dpio(struct drm_device *dev)
> return;
>
> DPIO_PHY_IOSF_PORT(DPIO_PHY0) = IOSF_PORT_DPIO;
> +}
> +
> +static void intel_dpio_reset(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (!IS_VALLEYVIEW(dev))
> + return;
> +
> /*
> * From VLV2A0_DP_eDP_DPIO_driver_vbios_notes_10.docx -
> * 6. De-assert cmn_reset/side_reset. Same as VLV X0.
> @@ -10908,7 +10917,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
> I915_WRITE(DPLL(PIPE_B), I915_READ(DPLL(PIPE_B)) |
> DPLL_INTEGRATED_CRI_CLK_VLV);
>
> - intel_init_dpio(dev);
> + intel_dpio_reset(dev);
>
> mutex_lock(&dev->struct_mutex);
> intel_enable_gt_powersave(dev);
> @@ -10971,6 +10980,8 @@ void intel_modeset_init(struct drm_device *dev)
> }
> }
>
> + intel_init_dpio(dev);
> +
> intel_cpu_pll_init(dev);
> intel_shared_dpll_init(dev);
>
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 5/6] drm/i915/vlv: move DPIO init earlier v2
2013-12-14 10:47 ` Daniel Vetter
@ 2013-12-17 0:02 ` Jesse Barnes
0 siblings, 0 replies; 36+ messages in thread
From: Jesse Barnes @ 2013-12-17 0:02 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Sat, 14 Dec 2013 11:47:36 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Dec 12, 2013 at 12:41:56PM -0800, Jesse Barnes wrote:
> > It's needed for early mode state readout, which is in turn needed to
> > inherit the BIOS config. So split out the reset, which we need on
> > resume too, from the DPIO reg init, and do the latter earlier.
> >
> > v2: split reset and reg init
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> Isn't this a fixup due to the slight init reordering you do in patch 2? If
> so we need to get this one in first, and I'd also prefer if the init
> reordering is split out as a separate patch from patch 2 - that stuff
> almost always ends up hurting us in some way or the order, so a minimal
> patch as an obvious bisect point is imo worth it.
>
> With those prep patches split out I could also start merging to get this
> sucker moving finally.
Sure, done.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 6/6] drm/i915: inform drm_fb_helper if we abandoned a connected output
2013-12-12 20:41 [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time Jesse Barnes
` (3 preceding siblings ...)
2013-12-12 20:41 ` [PATCH 5/6] drm/i915/vlv: move DPIO init earlier v2 Jesse Barnes
@ 2013-12-12 20:41 ` Jesse Barnes
2013-12-12 22:30 ` Chris Wilson
2013-12-12 21:21 ` [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time Daniel Vetter
5 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-12 20:41 UTC (permalink / raw)
To: intel-gfx
Otherwise subsequent fb activity will try to do blank/unblank on outputs
that were never fully enabled.
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
drivers/gpu/drm/i915/intel_fbdev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 53675d2..dcadd32 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -308,6 +308,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
if (!encoder || !encoder->crtc) {
DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
connector->base.id);
+ enabled[i] = false;
continue;
}
@@ -315,6 +316,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n",
drm_get_connector_name(connector),
encoder->crtc->base.id);
+ enabled[i] = false;
return false;
}
@@ -322,6 +324,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
DRM_DEBUG_KMS("connector %s on inactive crtc %d, borting\n",
drm_get_connector_name(connector),
encoder->crtc->base.id);
+ enabled[i] = false;
return false;
}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 6/6] drm/i915: inform drm_fb_helper if we abandoned a connected output
2013-12-12 20:41 ` [PATCH 6/6] drm/i915: inform drm_fb_helper if we abandoned a connected output Jesse Barnes
@ 2013-12-12 22:30 ` Chris Wilson
2013-12-12 22:34 ` Jesse Barnes
2013-12-17 0:18 ` Jesse Barnes
0 siblings, 2 replies; 36+ messages in thread
From: Chris Wilson @ 2013-12-12 22:30 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Dec 12, 2013 at 12:41:57PM -0800, Jesse Barnes wrote:
> Otherwise subsequent fb activity will try to do blank/unblank on outputs
> that were never fully enabled.
Hmm, actually this highlights a bug in drm_setup_crtcs() that we do not
reconstruct enabled[] after a return false here.
Only the first enabled[i] = false is required where we continue rather
than abort.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 6/6] drm/i915: inform drm_fb_helper if we abandoned a connected output
2013-12-12 22:30 ` Chris Wilson
@ 2013-12-12 22:34 ` Jesse Barnes
2013-12-17 0:18 ` Jesse Barnes
1 sibling, 0 replies; 36+ messages in thread
From: Jesse Barnes @ 2013-12-12 22:34 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Thu, 12 Dec 2013 22:30:41 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Dec 12, 2013 at 12:41:57PM -0800, Jesse Barnes wrote:
> > Otherwise subsequent fb activity will try to do blank/unblank on outputs
> > that were never fully enabled.
>
> Hmm, actually this highlights a bug in drm_setup_crtcs() that we do not
> reconstruct enabled[] after a return false here.
>
> Only the first enabled[i] = false is required where we continue rather
> than abort.
Yeah this one took me awhile to find. Not surprised if there are other
bugs lurking here. This one fixes the issue introduced by these
patches at least in the multihead config.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 6/6] drm/i915: inform drm_fb_helper if we abandoned a connected output
2013-12-12 22:30 ` Chris Wilson
2013-12-12 22:34 ` Jesse Barnes
@ 2013-12-17 0:18 ` Jesse Barnes
1 sibling, 0 replies; 36+ messages in thread
From: Jesse Barnes @ 2013-12-17 0:18 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Thu, 12 Dec 2013 22:30:41 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Dec 12, 2013 at 12:41:57PM -0800, Jesse Barnes wrote:
> > Otherwise subsequent fb activity will try to do blank/unblank on outputs
> > that were never fully enabled.
>
> Hmm, actually this highlights a bug in drm_setup_crtcs() that we do not
> reconstruct enabled[] after a return false here.
>
> Only the first enabled[i] = false is required where we continue rather
> than abort.
Fixed, thanks.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time
2013-12-12 20:41 [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time Jesse Barnes
` (4 preceding siblings ...)
2013-12-12 20:41 ` [PATCH 6/6] drm/i915: inform drm_fb_helper if we abandoned a connected output Jesse Barnes
@ 2013-12-12 21:21 ` Daniel Vetter
2013-12-12 21:29 ` Jesse Barnes
5 siblings, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-12 21:21 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Dec 12, 2013 at 12:41:52PM -0800, Jesse Barnes wrote:
> The BIOS code will need this to properly inherit the mode.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af3717a..1ae3d44 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11175,7 +11175,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> */
> list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> base.head) {
> - if (crtc->active && i915_fastboot) {
> + if (crtc->active) {
That's just enabling half of the fastboot hack, so nacked.
-Daniel
> intel_crtc_mode_from_pipe_config(crtc, &crtc->config);
>
> DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time
2013-12-12 21:21 ` [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time Daniel Vetter
@ 2013-12-12 21:29 ` Jesse Barnes
2013-12-12 22:39 ` Daniel Vetter
0 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-12 21:29 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Thu, 12 Dec 2013 22:21:29 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Dec 12, 2013 at 12:41:52PM -0800, Jesse Barnes wrote:
> > The BIOS code will need this to properly inherit the mode.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index af3717a..1ae3d44 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11175,7 +11175,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> > */
> > list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> > base.head) {
> > - if (crtc->active && i915_fastboot) {
> > + if (crtc->active) {
>
> That's just enabling half of the fastboot hack, so nacked.
This part isn't the hack, but is required for fastboot. Without this,
we'll end up with a bogus mode when we try to inherit from the BIOS.
So if you want to nack this I'll have to put the other BIOS bits under
fastboot as well.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time
2013-12-12 21:29 ` Jesse Barnes
@ 2013-12-12 22:39 ` Daniel Vetter
2013-12-12 22:44 ` Jesse Barnes
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Vetter @ 2013-12-12 22:39 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Dec 12, 2013 at 01:29:39PM -0800, Jesse Barnes wrote:
> On Thu, 12 Dec 2013 22:21:29 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Thu, Dec 12, 2013 at 12:41:52PM -0800, Jesse Barnes wrote:
> > > The BIOS code will need this to properly inherit the mode.
> > >
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index af3717a..1ae3d44 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11175,7 +11175,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> > > */
> > > list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> > > base.head) {
> > > - if (crtc->active && i915_fastboot) {
> > > + if (crtc->active) {
> >
> > That's just enabling half of the fastboot hack, so nacked.
>
> This part isn't the hack, but is required for fastboot. Without this,
> we'll end up with a bogus mode when we try to inherit from the BIOS.
> So if you want to nack this I'll have to put the other BIOS bits under
> fastboot as well.
crtc->config.pipe_src_w/h not good enough? I've thought that's what you've
used in the last iteration, hence my suprise why we suddenly need to
resurrect this hack here. All the information this hack exposes to
crtc->mode is available in the pipe config, so I really don't think you
neeed it.
Count me confused for now, please explain.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time
2013-12-12 22:39 ` Daniel Vetter
@ 2013-12-12 22:44 ` Jesse Barnes
2013-12-12 23:04 ` Daniel Vetter
2013-12-17 0:35 ` Jesse Barnes
0 siblings, 2 replies; 36+ messages in thread
From: Jesse Barnes @ 2013-12-12 22:44 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Thu, 12 Dec 2013 23:39:39 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Dec 12, 2013 at 01:29:39PM -0800, Jesse Barnes wrote:
> > On Thu, 12 Dec 2013 22:21:29 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > On Thu, Dec 12, 2013 at 12:41:52PM -0800, Jesse Barnes wrote:
> > > > The BIOS code will need this to properly inherit the mode.
> > > >
> > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index af3717a..1ae3d44 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -11175,7 +11175,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> > > > */
> > > > list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> > > > base.head) {
> > > > - if (crtc->active && i915_fastboot) {
> > > > + if (crtc->active) {
> > >
> > > That's just enabling half of the fastboot hack, so nacked.
> >
> > This part isn't the hack, but is required for fastboot. Without this,
> > we'll end up with a bogus mode when we try to inherit from the BIOS.
> > So if you want to nack this I'll have to put the other BIOS bits under
> > fastboot as well.
>
> crtc->config.pipe_src_w/h not good enough? I've thought that's what you've
> used in the last iteration, hence my suprise why we suddenly need to
> resurrect this hack here. All the information this hack exposes to
> crtc->mode is available in the pipe config, so I really don't think you
> neeed it.
>
> Count me confused for now, please explain.
Yes, we read out all the data into the pipe config. But if we don't
put that data into the CRTC proper, any subsequent code that uses the
CRTC mode will fail and think there's nothing there (resulting in fail
at fbcon DPMS time for example).
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time
2013-12-12 22:44 ` Jesse Barnes
@ 2013-12-12 23:04 ` Daniel Vetter
2013-12-17 0:35 ` Jesse Barnes
1 sibling, 0 replies; 36+ messages in thread
From: Daniel Vetter @ 2013-12-12 23:04 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Dec 12, 2013 at 02:44:33PM -0800, Jesse Barnes wrote:
> On Thu, 12 Dec 2013 23:39:39 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Thu, Dec 12, 2013 at 01:29:39PM -0800, Jesse Barnes wrote:
> > > On Thu, 12 Dec 2013 22:21:29 +0100
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > > On Thu, Dec 12, 2013 at 12:41:52PM -0800, Jesse Barnes wrote:
> > > > > The BIOS code will need this to properly inherit the mode.
> > > > >
> > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > index af3717a..1ae3d44 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -11175,7 +11175,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> > > > > */
> > > > > list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> > > > > base.head) {
> > > > > - if (crtc->active && i915_fastboot) {
> > > > > + if (crtc->active) {
> > > >
> > > > That's just enabling half of the fastboot hack, so nacked.
> > >
> > > This part isn't the hack, but is required for fastboot. Without this,
> > > we'll end up with a bogus mode when we try to inherit from the BIOS.
> > > So if you want to nack this I'll have to put the other BIOS bits under
> > > fastboot as well.
> >
> > crtc->config.pipe_src_w/h not good enough? I've thought that's what you've
> > used in the last iteration, hence my suprise why we suddenly need to
> > resurrect this hack here. All the information this hack exposes to
> > crtc->mode is available in the pipe config, so I really don't think you
> > neeed it.
> >
> > Count me confused for now, please explain.
>
> Yes, we read out all the data into the pipe config. But if we don't
> put that data into the CRTC proper, any subsequent code that uses the
> CRTC mode will fail and think there's nothing there (resulting in fail
> at fbcon DPMS time for example).
If fbcon dpms is fumbling around in crtc->mode that's a bug (or at least a
pretty blantant layering violation). The one I know of is the crtc->hwmode
fumbling the vblank code does. But iirc Ville has patches for that
somewhere, and it's not really the worse offense in drm_irq.c.
We obviously need crtc->fb for our code to be happy (since we haven't
split off the primary planes yet). But anything beyond that shouldn't be
required. So I'm still confused ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time
2013-12-12 22:44 ` Jesse Barnes
2013-12-12 23:04 ` Daniel Vetter
@ 2013-12-17 0:35 ` Jesse Barnes
2013-12-17 8:00 ` Daniel Vetter
1 sibling, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2013-12-17 0:35 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Thu, 12 Dec 2013 14:44:33 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 12 Dec 2013 23:39:39 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Thu, Dec 12, 2013 at 01:29:39PM -0800, Jesse Barnes wrote:
> > > On Thu, 12 Dec 2013 22:21:29 +0100
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > > On Thu, Dec 12, 2013 at 12:41:52PM -0800, Jesse Barnes wrote:
> > > > > The BIOS code will need this to properly inherit the mode.
> > > > >
> > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > index af3717a..1ae3d44 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -11175,7 +11175,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> > > > > */
> > > > > list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> > > > > base.head) {
> > > > > - if (crtc->active && i915_fastboot) {
> > > > > + if (crtc->active) {
> > > >
> > > > That's just enabling half of the fastboot hack, so nacked.
> > >
> > > This part isn't the hack, but is required for fastboot. Without this,
> > > we'll end up with a bogus mode when we try to inherit from the BIOS.
> > > So if you want to nack this I'll have to put the other BIOS bits under
> > > fastboot as well.
> >
> > crtc->config.pipe_src_w/h not good enough? I've thought that's what you've
> > used in the last iteration, hence my suprise why we suddenly need to
> > resurrect this hack here. All the information this hack exposes to
> > crtc->mode is available in the pipe config, so I really don't think you
> > neeed it.
> >
> > Count me confused for now, please explain.
>
> Yes, we read out all the data into the pipe config. But if we don't
> put that data into the CRTC proper, any subsequent code that uses the
> CRTC mode will fail and think there's nothing there (resulting in fail
> at fbcon DPMS time for example).
The actual warning w/o this change is:
[ 17.088591] [drm:intel_pipe_config_compare] *ERROR* mismatch in pipe_src_w (expected 0, found 4096)
[ 17.088592] ------------[ cut here ]------------
[ 17.088619] WARNING: CPU: 1 PID: 534 at drivers/gpu/drm/i915/intel_display.c:9588 check_crtc_state+0x6bf/0xc90 [i915]()
[ 17.088619] pipe state doesn't match!
...
[ 17.088710] [<ffffffffa02f2caf>] check_crtc_state+0x6bf/0xc90 [i915]
[ 17.088729] [<ffffffffa03007ab>] intel_modeset_check_state+0x2bb/0x7b0 [i915]
[ 17.088745] [<ffffffffa0300d35>] intel_set_mode+0x25/0x30 [i915]
[ 17.088762] [<ffffffffa03015db>] intel_crtc_set_config+0x7ab/0x9a0 [i915]
[ 17.088777] [<ffffffffa0133a4d>] drm_mode_set_config_internal+0x5d/0xe0 [drm]
[ 17.088783] [<ffffffffa0184ea1>] drm_fb_helper_set_par+0x71/0xf0 [drm_kms_helper]
[ 17.088787] [<ffffffff8135b301>] fb_set_var+0x191/0x430
So the first mode set from the fb layer is half baked, because the core
DRM structures didn't have the right mode to pass down, and so we fall
over.
I can fix this either by always copying the mode info into the core
structs, or by putting the call to fbdev_init_bios under i915_fastboot
as well.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time
2013-12-17 0:35 ` Jesse Barnes
@ 2013-12-17 8:00 ` Daniel Vetter
0 siblings, 0 replies; 36+ messages in thread
From: Daniel Vetter @ 2013-12-17 8:00 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Mon, Dec 16, 2013 at 04:35:40PM -0800, Jesse Barnes wrote:
> On Thu, 12 Dec 2013 14:44:33 -0800
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
> > On Thu, 12 Dec 2013 23:39:39 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > On Thu, Dec 12, 2013 at 01:29:39PM -0800, Jesse Barnes wrote:
> > > > On Thu, 12 Dec 2013 22:21:29 +0100
> > > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > > On Thu, Dec 12, 2013 at 12:41:52PM -0800, Jesse Barnes wrote:
> > > > > > The BIOS code will need this to properly inherit the mode.
> > > > > >
> > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index af3717a..1ae3d44 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -11175,7 +11175,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> > > > > > */
> > > > > > list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> > > > > > base.head) {
> > > > > > - if (crtc->active && i915_fastboot) {
> > > > > > + if (crtc->active) {
> > > > >
> > > > > That's just enabling half of the fastboot hack, so nacked.
> > > >
> > > > This part isn't the hack, but is required for fastboot. Without this,
> > > > we'll end up with a bogus mode when we try to inherit from the BIOS.
> > > > So if you want to nack this I'll have to put the other BIOS bits under
> > > > fastboot as well.
> > >
> > > crtc->config.pipe_src_w/h not good enough? I've thought that's what you've
> > > used in the last iteration, hence my suprise why we suddenly need to
> > > resurrect this hack here. All the information this hack exposes to
> > > crtc->mode is available in the pipe config, so I really don't think you
> > > neeed it.
> > >
> > > Count me confused for now, please explain.
> >
> > Yes, we read out all the data into the pipe config. But if we don't
> > put that data into the CRTC proper, any subsequent code that uses the
> > CRTC mode will fail and think there's nothing there (resulting in fail
> > at fbcon DPMS time for example).
>
> The actual warning w/o this change is:
>
> [ 17.088591] [drm:intel_pipe_config_compare] *ERROR* mismatch in pipe_src_w (expected 0, found 4096)
> [ 17.088592] ------------[ cut here ]------------
> [ 17.088619] WARNING: CPU: 1 PID: 534 at drivers/gpu/drm/i915/intel_display.c:9588 check_crtc_state+0x6bf/0xc90 [i915]()
> [ 17.088619] pipe state doesn't match!
> ...
> [ 17.088710] [<ffffffffa02f2caf>] check_crtc_state+0x6bf/0xc90 [i915]
> [ 17.088729] [<ffffffffa03007ab>] intel_modeset_check_state+0x2bb/0x7b0 [i915]
> [ 17.088745] [<ffffffffa0300d35>] intel_set_mode+0x25/0x30 [i915]
> [ 17.088762] [<ffffffffa03015db>] intel_crtc_set_config+0x7ab/0x9a0 [i915]
> [ 17.088777] [<ffffffffa0133a4d>] drm_mode_set_config_internal+0x5d/0xe0 [drm]
> [ 17.088783] [<ffffffffa0184ea1>] drm_fb_helper_set_par+0x71/0xf0 [drm_kms_helper]
> [ 17.088787] [<ffffffff8135b301>] fb_set_var+0x191/0x430
>
> So the first mode set from the fb layer is half baked, because the core
> DRM structures didn't have the right mode to pass down, and so we fall
> over.
>
> I can fix this either by always copying the mode info into the core
> structs, or by putting the call to fbdev_init_bios under i915_fastboot
> as well.
Hm, I think I need to take a closer look here again since I really thought
it'd would Just Work. Hiding more behind the fastboot hack isn't really
what I want either, especially since we want to move from checking the
input mode to checking the pipe config for fastbooting. So having a single
piece which relies on that reconstructed mode would be rather ugly.
Can you please attach the full debug log for this?
Also, could this just be a side effect of the more fancy ->initial_config
logic, i.e. what happens if we disable that one?
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 36+ messages in thread