[Intel-gfx] [PATCH 1/1] drm/i915: Fix fb object's frontbuffer-bits
Daniel Vetter
daniel at ffwll.ch
Mon Sep 7 09:41:36 PDT 2015
On Mon, Sep 07, 2015 at 01:33:51PM +0530, Sagar Arun Kamble wrote:
> Shared frontbuffer bits are causing warnings when same FB is displayed
> in another plane without clearing the bits from previous plane.
This commit message is very confusing, since clearing FB bits isn't
broken. What you actually seem to be doing is extend this to u64 and
increase the per-pipe bits to 8. Not that even with 8 bits things will
still easily fit into 32 bits. And last time I checked we don't chip hw
with more than 4 planes per pipe in total.
Anyway please explain a bit more what's actually going on here, this looks
suspiciously like duct-tape.
-Daniel
>
> Change-Id: Ic2df80747f314b82afd22f8326297c57d1e652c6
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Signed-off-by: Kumar, Mahesh <mahesh1.kumar at intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 6 ++--
> drivers/gpu/drm/i915/i915_drv.h | 61 ++++++++++++++++----------------
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/intel_dp.c | 4 +--
> drivers/gpu/drm/i915/intel_drv.h | 24 ++++++-------
> drivers/gpu/drm/i915/intel_fbc.c | 6 ++--
> drivers/gpu/drm/i915/intel_frontbuffer.c | 10 +++---
> drivers/gpu/drm/i915/intel_psr.c | 6 ++--
> drivers/gpu/drm/i915/intel_sprite.c | 2 +-
> 9 files changed, 61 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 41629fa..b6082f0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -188,7 +188,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> seq_printf(m, " (%s)",
> i915_gem_request_get_ring(obj->last_write_req)->name);
> if (obj->frontbuffer_bits)
> - seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
> + seq_printf(m, " (frontbuffer: 0x%llx)", obj->frontbuffer_bits);
> }
>
> static void describe_ctx(struct seq_file *m, struct intel_context *ctx)
> @@ -2531,7 +2531,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
> seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
> seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
> - seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> + seq_printf(m, "Busy frontbuffer bits: 0x%llx\n",
> dev_priv->psr.busy_frontbuffer_bits);
> seq_printf(m, "Re-enable work scheduled: %s\n",
> yesno(work_busy(&dev_priv->psr.work.work)));
> @@ -3208,7 +3208,7 @@ static void drrs_status_per_crtc(struct seq_file *m,
> }
>
> panel = &drrs->dp->attached_connector->panel;
> - seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
> + seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%llx",
> drrs->busy_frontbuffer_bits);
>
> seq_puts(m, "\n\t\t");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1287007..285de49 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -906,6 +906,29 @@ enum fb_op_origin {
> ORIGIN_DIRTYFB,
> };
>
> +/*
> + * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
> + * considered to be the frontbuffer for the given plane interface-wise. This
> + * doesn't mean that the hw necessarily already scans it out, but that any
> + * rendering (by the cpu or gpu) will land in the frontbuffer eventually.
> + *
> + * We have one bit per pipe and per scanout plane type.
> + */
> +#define INTEL_MAX_SPRITE_BITS_PER_PIPE 5
> +#define INTEL_FRONTBUFFER_BITS_PER_PIPE 8
> +#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
> + (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> +#define INTEL_FRONTBUFFER_CURSOR(pipe) \
> + (1 << (1 + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
> +#define INTEL_FRONTBUFFER_SPRITE(pipe, plane) \
> + (1 << (2 + plane + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
> +#define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> + (1 << (2 + INTEL_MAX_SPRITE_BITS_PER_PIPE + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
> +#define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> + (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> +#define INTEL_FRONTBUFFER_SPRITE_MASK(pipe) \
> + (0x7C << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> +
> struct i915_fbc {
> /* This is always the inner lock when overlapping with struct_mutex and
> * it's the outer lock when overlapping with stolen_lock. */
> @@ -913,8 +936,8 @@ struct i915_fbc {
> unsigned long uncompressed_size;
> unsigned threshold;
> unsigned int fb_id;
> - unsigned int possible_framebuffer_bits;
> - unsigned int busy_bits;
> + u64 possible_framebuffer_bits;
> + u64 busy_bits;
> struct intel_crtc *crtc;
> int y;
>
> @@ -976,7 +999,7 @@ struct i915_drrs {
> struct mutex mutex;
> struct delayed_work work;
> struct intel_dp *dp;
> - unsigned busy_frontbuffer_bits;
> + u64 busy_frontbuffer_bits;
> enum drrs_refresh_rate_type refresh_rate_type;
> enum drrs_support_type type;
> };
> @@ -988,7 +1011,7 @@ struct i915_psr {
> struct intel_dp *enabled;
> bool active;
> struct delayed_work work;
> - unsigned busy_frontbuffer_bits;
> + u64 busy_frontbuffer_bits;
> bool psr2_support;
> bool aux_frame_sync;
> };
> @@ -1672,8 +1695,8 @@ struct i915_frontbuffer_tracking {
> * Tracking bits for delayed frontbuffer flushing du to gpu activity or
> * scheduled flips.
> */
> - unsigned busy_bits;
> - unsigned flip_bits;
> + u64 busy_bits;
> + u64 flip_bits;
> };
>
> struct i915_wa_reg {
> @@ -2009,28 +2032,6 @@ struct drm_i915_gem_object_ops {
> void (*release)(struct drm_i915_gem_object *);
> };
>
> -/*
> - * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
> - * considered to be the frontbuffer for the given plane interface-vise. This
> - * doesn't mean that the hw necessarily already scans it out, but that any
> - * rendering (by the cpu or gpu) will land in the frontbuffer eventually.
> - *
> - * We have one bit per pipe and per scanout plane type.
> - */
> -#define INTEL_FRONTBUFFER_BITS_PER_PIPE 4
> -#define INTEL_FRONTBUFFER_BITS \
> - (INTEL_FRONTBUFFER_BITS_PER_PIPE * I915_MAX_PIPES)
> -#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
> - (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> -#define INTEL_FRONTBUFFER_CURSOR(pipe) \
> - (1 << (1 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
> -#define INTEL_FRONTBUFFER_SPRITE(pipe) \
> - (1 << (2 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
> -#define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> - (1 << (3 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
> -#define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> - (0xf << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> -
> struct drm_i915_gem_object {
> struct drm_gem_object base;
>
> @@ -2108,7 +2109,7 @@ struct drm_i915_gem_object {
> unsigned int cache_level:3;
> unsigned int cache_dirty:1;
>
> - unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> + u64 frontbuffer_bits;
>
> unsigned int pin_display;
>
> @@ -2167,7 +2168,7 @@ struct drm_i915_gem_object {
>
> void i915_gem_track_fb(struct drm_i915_gem_object *old,
> struct drm_i915_gem_object *new,
> - unsigned frontbuffer_bits);
> + u64 frontbuffer_bits);
>
> /**
> * Request queue structure.
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 41263cd..2e2d873 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4985,7 +4985,7 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file)
> */
> void i915_gem_track_fb(struct drm_i915_gem_object *old,
> struct drm_i915_gem_object *new,
> - unsigned frontbuffer_bits)
> + u64 frontbuffer_bits)
> {
> if (old) {
> WARN_ON(!mutex_is_locked(&old->base.dev->struct_mutex));
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 45ab25e..212d4cf 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5714,7 +5714,7 @@ unlock:
> * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
> */
> void intel_edp_drrs_invalidate(struct drm_device *dev,
> - unsigned frontbuffer_bits)
> + u64 frontbuffer_bits)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> @@ -5759,7 +5759,7 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
> * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
> */
> void intel_edp_drrs_flush(struct drm_device *dev,
> - unsigned frontbuffer_bits)
> + u64 frontbuffer_bits)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 46484e4..1398ed4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -506,7 +506,7 @@ struct intel_crtc_atomic_commit {
> unsigned disabled_planes;
>
> /* Sleepable operations to perform after commit */
> - unsigned fb_bits;
> + u64 fb_bits;
> bool wait_vblank;
> bool update_fbc;
> bool post_enable_primary;
> @@ -599,7 +599,7 @@ struct intel_plane {
> enum pipe pipe;
> bool can_scale;
> int max_downscale;
> - uint32_t frontbuffer_bit;
> + u64 frontbuffer_bit;
>
> /* Since we need to change the watermarks before/after
> * enabling/disabling the planes, we need to store the parameters here
> @@ -987,11 +987,11 @@ uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> enum fb_op_origin origin);
> void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> - unsigned frontbuffer_bits);
> + u64 frontbuffer_bits);
> void intel_frontbuffer_flip_complete(struct drm_device *dev,
> - unsigned frontbuffer_bits);
> + u64 frontbuffer_bits);
> void intel_frontbuffer_flip(struct drm_device *dev,
> - unsigned frontbuffer_bits);
> + u64 frontbuffer_bits);
> unsigned int intel_fb_align_height(struct drm_device *dev,
> unsigned int height,
> uint32_t pixel_format,
> @@ -1207,8 +1207,8 @@ void intel_plane_destroy(struct drm_plane *plane);
> void intel_edp_drrs_enable(struct intel_dp *intel_dp);
> void intel_edp_drrs_disable(struct intel_dp *intel_dp);
> void intel_edp_drrs_invalidate(struct drm_device *dev,
> - unsigned frontbuffer_bits);
> -void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
> + u64 frontbuffer_bits);
> +void intel_edp_drrs_flush(struct drm_device *dev, u64 frontbuffer_bits);
> void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config);
>
> /* intel_dp_mst.c */
> @@ -1260,10 +1260,10 @@ void intel_fbc_init(struct drm_i915_private *dev_priv);
> void intel_fbc_disable(struct drm_i915_private *dev_priv);
> void intel_fbc_disable_crtc(struct intel_crtc *crtc);
> void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> - unsigned int frontbuffer_bits,
> + u64 frontbuffer_bits,
> enum fb_op_origin origin);
> void intel_fbc_flush(struct drm_i915_private *dev_priv,
> - unsigned int frontbuffer_bits, enum fb_op_origin origin);
> + u64 frontbuffer_bits, enum fb_op_origin origin);
> const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
> void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
>
> @@ -1333,13 +1333,13 @@ void intel_backlight_unregister(struct drm_device *dev);
> void intel_psr_enable(struct intel_dp *intel_dp);
> void intel_psr_disable(struct intel_dp *intel_dp);
> void intel_psr_invalidate(struct drm_device *dev,
> - unsigned frontbuffer_bits);
> + u64 frontbuffer_bits);
> void intel_psr_flush(struct drm_device *dev,
> - unsigned frontbuffer_bits,
> + u64 frontbuffer_bits,
> enum fb_op_origin origin);
> void intel_psr_init(struct drm_device *dev);
> void intel_psr_single_frame_update(struct drm_device *dev,
> - unsigned frontbuffer_bits);
> + u64 frontbuffer_bits);
>
> /* intel_runtime_pm.c */
> int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 1f97fb5..cdcc284 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -854,10 +854,10 @@ void intel_fbc_update(struct drm_i915_private *dev_priv)
> }
>
> void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> - unsigned int frontbuffer_bits,
> + u64 frontbuffer_bits,
> enum fb_op_origin origin)
> {
> - unsigned int fbc_bits;
> + u64 fbc_bits;
>
> if (!dev_priv->fbc.enable_fbc)
> return;
> @@ -884,7 +884,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> }
>
> void intel_fbc_flush(struct drm_i915_private *dev_priv,
> - unsigned int frontbuffer_bits, enum fb_op_origin origin)
> + u64 frontbuffer_bits, enum fb_op_origin origin)
> {
> if (!dev_priv->fbc.enable_fbc)
> return;
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index ac85357..3115d8b 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -114,7 +114,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> * Can be called without any locks held.
> */
> static void intel_frontbuffer_flush(struct drm_device *dev,
> - unsigned frontbuffer_bits,
> + u64 frontbuffer_bits,
> enum fb_op_origin origin)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -147,7 +147,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> {
> struct drm_device *dev = obj->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - unsigned frontbuffer_bits;
> + u64 frontbuffer_bits;
>
> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> @@ -181,7 +181,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> * Can be called without any locks held.
> */
> void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> - unsigned frontbuffer_bits)
> + u64 frontbuffer_bits)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> @@ -205,7 +205,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> * Can be called without any locks held.
> */
> void intel_frontbuffer_flip_complete(struct drm_device *dev,
> - unsigned frontbuffer_bits)
> + u64 frontbuffer_bits)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> @@ -230,7 +230,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> * Can be called without any locks held.
> */
> void intel_frontbuffer_flip(struct drm_device *dev,
> - unsigned frontbuffer_bits)
> + u64 frontbuffer_bits)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index a04b4dc..a8a236c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -607,7 +607,7 @@ static void intel_psr_exit(struct drm_device *dev)
> * hardware requires this to be done before a page flip.
> */
> void intel_psr_single_frame_update(struct drm_device *dev,
> - unsigned frontbuffer_bits)
> + u64 frontbuffer_bits)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> @@ -655,7 +655,7 @@ void intel_psr_single_frame_update(struct drm_device *dev,
> * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits."
> */
> void intel_psr_invalidate(struct drm_device *dev,
> - unsigned frontbuffer_bits)
> + u64 frontbuffer_bits)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> @@ -693,7 +693,7 @@ void intel_psr_invalidate(struct drm_device *dev,
> * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits.
> */
> void intel_psr_flush(struct drm_device *dev,
> - unsigned frontbuffer_bits, enum fb_op_origin origin)
> + u64 frontbuffer_bits, enum fb_op_origin origin)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ca7e264..5b9bcf9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1125,7 +1125,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>
> intel_plane->pipe = pipe;
> intel_plane->plane = plane;
> - intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe);
> + intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> intel_plane->check_plane = intel_check_sprite_plane;
> intel_plane->commit_plane = intel_commit_sprite_plane;
> possible_crtcs = (1 << pipe);
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list