Re: [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init

[Daniel Vetter <daniel@ffwll.ch>]

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