Re: [Intel-gfx] [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
On Mon, Dec 16, 2013 at 04:01:41PM -0800, Jesse Barnes wrote: On Sat, 14 Dec 2013 12:01:47 +0100 Daniel Vetter dan...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
On Tue, 17 Dec 2013 09:38:36 +0100 Daniel Vetter dan...@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 dan...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
On Tue, Dec 17, 2013 at 10:04 PM, Jesse Barnes jbar...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
On Sat, 14 Dec 2013 12:01:47 +0100 Daniel Vetter dan...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
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 jbar...@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_XRGB;
+ case DISPPLANE_RGBX888:
+ return DRM_FORMAT_XBGR;
+ 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)) 0xf000;
+ } 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 0xff80;
+
+ 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,
+
[Intel-gfx] [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init
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 jbar...@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_XRGB;
+ case DISPPLANE_RGBX888:
+ return DRM_FORMAT_XBGR;
+ 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)) 0xf000;
+ } 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 0xff80;
+
+ 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,
+
