* [PATCH] drm/i915: Ensure plane->state->fb stays in sync with plane->fb
@ 2015-03-12 12:16 Xi Ruoyao
2015-03-12 12:45 ` Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Xi Ruoyao @ 2015-03-12 12:16 UTC (permalink / raw)
To: jani.nikula, daniel.vetter; +Cc: Xi Ruoyao, Daniel Vetter, intel-gfx
plane->state->fb and plane->fb should always reference the same FB so
that atomic and legacy codepaths have the same view of display state.
However, there are some places in kernel code that directly set
plane->fb and neglect to update plane->state->fb. If we never do a
successful update through the atomic pipeline, the RmFB cleanup code
will look at the plane->state->fb pointer, which has never actually
been set to a legitimate value, and try to clean it up, leading to
BUG's.
Add a quick helper function to synchronize plane->state->fb with
plane->fb and call it everywhere the driver tries to manually set
plane->fb outside of the atomic pipeline. In this function, use
drm_atomic_set_fb_for_plane instead of writing plane->state->fb
directly to keep the reference count right.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88909
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=93711
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Xi Ruoyao <xry111@outlook.com>
---
This is modified from Matt Roper's patch to drm-intel-nightly with
commit id
afd65eb4cc0578a9c07d621acdb8a570e2782bf7
However this bug exists in mainline kernel too, so I created this to
fix it in mainline kernel.
A minor change is to use drm_atomic_set_fb_for_plane instead of
update reference count manually.
drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e730789..c483f2f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -37,6 +37,7 @@
#include <drm/i915_drm.h>
#include "i915_drv.h"
#include "i915_trace.h"
+#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_dp_helper.h>
#include <drm/drm_crtc_helper.h>
@@ -2416,6 +2417,14 @@ out_unref_obj:
return false;
}
+/* Update plane->state->fb to match plane->fb after driver-internal updates */
+static void
+update_state_fb(struct drm_plane *plane)
+{
+ if (plane->fb != plane->state->fb)
+ drm_atomic_set_fb_for_plane(plane->state, plane->fb);
+}
+
static void
intel_find_plane_obj(struct intel_crtc *intel_crtc,
struct intel_initial_plane_config *plane_config)
@@ -2462,6 +2471,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
break;
}
}
+
+ update_state_fb(intel_crtc->base.primary);
}
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
@@ -6650,6 +6661,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
plane_config->size);
crtc->base.primary->fb = fb;
+ update_state_fb(crtc->base.primary);
}
static void chv_crtc_clock_get(struct intel_crtc *crtc,
@@ -7687,6 +7699,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
plane_config->size);
crtc->base.primary->fb = fb;
+ update_state_fb(crtc->base.primary);
return;
error:
@@ -7778,6 +7791,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
plane_config->size);
crtc->base.primary->fb = fb;
+ update_state_fb(crtc->base.primary);
}
static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
@@ -9816,6 +9830,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
drm_gem_object_reference(&obj->base);
crtc->primary->fb = fb;
+ update_state_fb(crtc->primary);
work->pending_flip_obj = obj;
@@ -9884,6 +9899,7 @@ cleanup_unpin:
cleanup_pending:
atomic_dec(&intel_crtc->unpin_work_count);
crtc->primary->fb = old_fb;
+ update_state_fb(crtc->primary);
drm_gem_object_unreference(&work->old_fb_obj->base);
drm_gem_object_unreference(&obj->base);
mutex_unlock(&dev->struct_mutex);
@@ -13718,6 +13734,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
to_intel_crtc(c)->pipe);
drm_framebuffer_unreference(c->primary->fb);
c->primary->fb = NULL;
+ update_state_fb(c->primary);
}
}
mutex_unlock(&dev->struct_mutex);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: Ensure plane->state->fb stays in sync with plane->fb
2015-03-12 12:16 [PATCH] drm/i915: Ensure plane->state->fb stays in sync with plane->fb Xi Ruoyao
@ 2015-03-12 12:45 ` Jani Nikula
2015-03-12 16:12 ` Matt Roper
0 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2015-03-12 12:45 UTC (permalink / raw)
To: daniel.vetter; +Cc: Xi Ruoyao, Daniel Vetter, intel-gfx
Matt, please review or suggest an alternative for v4.0-rc.
Thanks,
Jani.
On Thu, 12 Mar 2015, Xi Ruoyao <xry111@outlook.com> wrote:
> plane->state->fb and plane->fb should always reference the same FB so
> that atomic and legacy codepaths have the same view of display state.
> However, there are some places in kernel code that directly set
> plane->fb and neglect to update plane->state->fb. If we never do a
> successful update through the atomic pipeline, the RmFB cleanup code
> will look at the plane->state->fb pointer, which has never actually
> been set to a legitimate value, and try to clean it up, leading to
> BUG's.
>
> Add a quick helper function to synchronize plane->state->fb with
> plane->fb and call it everywhere the driver tries to manually set
> plane->fb outside of the atomic pipeline. In this function, use
> drm_atomic_set_fb_for_plane instead of writing plane->state->fb
> directly to keep the reference count right.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88909
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=93711
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Xi Ruoyao <xry111@outlook.com>
> ---
> This is modified from Matt Roper's patch to drm-intel-nightly with
> commit id
>
> afd65eb4cc0578a9c07d621acdb8a570e2782bf7
>
> However this bug exists in mainline kernel too, so I created this to
> fix it in mainline kernel.
>
> A minor change is to use drm_atomic_set_fb_for_plane instead of
> update reference count manually.
>
> drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e730789..c483f2f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -37,6 +37,7 @@
> #include <drm/i915_drm.h>
> #include "i915_drv.h"
> #include "i915_trace.h"
> +#include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_dp_helper.h>
> #include <drm/drm_crtc_helper.h>
> @@ -2416,6 +2417,14 @@ out_unref_obj:
> return false;
> }
>
> +/* Update plane->state->fb to match plane->fb after driver-internal updates */
> +static void
> +update_state_fb(struct drm_plane *plane)
> +{
> + if (plane->fb != plane->state->fb)
> + drm_atomic_set_fb_for_plane(plane->state, plane->fb);
> +}
> +
> static void
> intel_find_plane_obj(struct intel_crtc *intel_crtc,
> struct intel_initial_plane_config *plane_config)
> @@ -2462,6 +2471,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> break;
> }
> }
> +
> + update_state_fb(intel_crtc->base.primary);
> }
>
> static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> @@ -6650,6 +6661,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> plane_config->size);
>
> crtc->base.primary->fb = fb;
> + update_state_fb(crtc->base.primary);
> }
>
> static void chv_crtc_clock_get(struct intel_crtc *crtc,
> @@ -7687,6 +7699,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> plane_config->size);
>
> crtc->base.primary->fb = fb;
> + update_state_fb(crtc->base.primary);
> return;
>
> error:
> @@ -7778,6 +7791,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
> plane_config->size);
>
> crtc->base.primary->fb = fb;
> + update_state_fb(crtc->base.primary);
> }
>
> static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> @@ -9816,6 +9830,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> drm_gem_object_reference(&obj->base);
>
> crtc->primary->fb = fb;
> + update_state_fb(crtc->primary);
>
> work->pending_flip_obj = obj;
>
> @@ -9884,6 +9899,7 @@ cleanup_unpin:
> cleanup_pending:
> atomic_dec(&intel_crtc->unpin_work_count);
> crtc->primary->fb = old_fb;
> + update_state_fb(crtc->primary);
> drm_gem_object_unreference(&work->old_fb_obj->base);
> drm_gem_object_unreference(&obj->base);
> mutex_unlock(&dev->struct_mutex);
> @@ -13718,6 +13734,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
> to_intel_crtc(c)->pipe);
> drm_framebuffer_unreference(c->primary->fb);
> c->primary->fb = NULL;
> + update_state_fb(c->primary);
> }
> }
> mutex_unlock(&dev->struct_mutex);
> --
> 1.9.1
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: Ensure plane->state->fb stays in sync with plane->fb
2015-03-12 12:45 ` Jani Nikula
@ 2015-03-12 16:12 ` Matt Roper
2015-03-13 12:38 ` Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Matt Roper @ 2015-03-12 16:12 UTC (permalink / raw)
To: Jani Nikula; +Cc: Xi Ruoyao, daniel.vetter, intel-gfx, Daniel Vetter
On Thu, Mar 12, 2015 at 02:45:31PM +0200, Jani Nikula wrote:
>
> Matt, please review or suggest an alternative for v4.0-rc.
>
> Thanks,
> Jani.
Yep, this looks good to me. So
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Matt
>
>
> On Thu, 12 Mar 2015, Xi Ruoyao <xry111@outlook.com> wrote:
> > plane->state->fb and plane->fb should always reference the same FB so
> > that atomic and legacy codepaths have the same view of display state.
> > However, there are some places in kernel code that directly set
> > plane->fb and neglect to update plane->state->fb. If we never do a
> > successful update through the atomic pipeline, the RmFB cleanup code
> > will look at the plane->state->fb pointer, which has never actually
> > been set to a legitimate value, and try to clean it up, leading to
> > BUG's.
> >
> > Add a quick helper function to synchronize plane->state->fb with
> > plane->fb and call it everywhere the driver tries to manually set
> > plane->fb outside of the atomic pipeline. In this function, use
> > drm_atomic_set_fb_for_plane instead of writing plane->state->fb
> > directly to keep the reference count right.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88909
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=93711
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Xi Ruoyao <xry111@outlook.com>
> > ---
> > This is modified from Matt Roper's patch to drm-intel-nightly with
> > commit id
> >
> > afd65eb4cc0578a9c07d621acdb8a570e2782bf7
> >
> > However this bug exists in mainline kernel too, so I created this to
> > fix it in mainline kernel.
> >
> > A minor change is to use drm_atomic_set_fb_for_plane instead of
> > update reference count manually.
> >
> > drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e730789..c483f2f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -37,6 +37,7 @@
> > #include <drm/i915_drm.h>
> > #include "i915_drv.h"
> > #include "i915_trace.h"
> > +#include <drm/drm_atomic.h>
> > #include <drm/drm_atomic_helper.h>
> > #include <drm/drm_dp_helper.h>
> > #include <drm/drm_crtc_helper.h>
> > @@ -2416,6 +2417,14 @@ out_unref_obj:
> > return false;
> > }
> >
> > +/* Update plane->state->fb to match plane->fb after driver-internal updates */
> > +static void
> > +update_state_fb(struct drm_plane *plane)
> > +{
> > + if (plane->fb != plane->state->fb)
> > + drm_atomic_set_fb_for_plane(plane->state, plane->fb);
> > +}
> > +
> > static void
> > intel_find_plane_obj(struct intel_crtc *intel_crtc,
> > struct intel_initial_plane_config *plane_config)
> > @@ -2462,6 +2471,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> > break;
> > }
> > }
> > +
> > + update_state_fb(intel_crtc->base.primary);
> > }
> >
> > static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> > @@ -6650,6 +6661,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> > plane_config->size);
> >
> > crtc->base.primary->fb = fb;
> > + update_state_fb(crtc->base.primary);
> > }
> >
> > static void chv_crtc_clock_get(struct intel_crtc *crtc,
> > @@ -7687,6 +7699,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> > plane_config->size);
> >
> > crtc->base.primary->fb = fb;
> > + update_state_fb(crtc->base.primary);
> > return;
> >
> > error:
> > @@ -7778,6 +7791,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
> > plane_config->size);
> >
> > crtc->base.primary->fb = fb;
> > + update_state_fb(crtc->base.primary);
> > }
> >
> > static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> > @@ -9816,6 +9830,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> > drm_gem_object_reference(&obj->base);
> >
> > crtc->primary->fb = fb;
> > + update_state_fb(crtc->primary);
> >
> > work->pending_flip_obj = obj;
> >
> > @@ -9884,6 +9899,7 @@ cleanup_unpin:
> > cleanup_pending:
> > atomic_dec(&intel_crtc->unpin_work_count);
> > crtc->primary->fb = old_fb;
> > + update_state_fb(crtc->primary);
> > drm_gem_object_unreference(&work->old_fb_obj->base);
> > drm_gem_object_unreference(&obj->base);
> > mutex_unlock(&dev->struct_mutex);
> > @@ -13718,6 +13734,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
> > to_intel_crtc(c)->pipe);
> > drm_framebuffer_unreference(c->primary->fb);
> > c->primary->fb = NULL;
> > + update_state_fb(c->primary);
> > }
> > }
> > mutex_unlock(&dev->struct_mutex);
> > --
> > 1.9.1
> >
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: Ensure plane->state->fb stays in sync with plane->fb
2015-03-12 16:12 ` Matt Roper
@ 2015-03-13 12:38 ` Jani Nikula
0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2015-03-13 12:38 UTC (permalink / raw)
To: Matt Roper; +Cc: Xi Ruoyao, daniel.vetter, intel-gfx, Daniel Vetter
On Thu, 12 Mar 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Thu, Mar 12, 2015 at 02:45:31PM +0200, Jani Nikula wrote:
>>
>> Matt, please review or suggest an alternative for v4.0-rc.
>>
>> Thanks,
>> Jani.
>
> Yep, this looks good to me. So
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Pushed to drm-intel-fixes, thanks for the patch and review.
BR,
Jani.
>
>
> Matt
>
>>
>>
>> On Thu, 12 Mar 2015, Xi Ruoyao <xry111@outlook.com> wrote:
>> > plane->state->fb and plane->fb should always reference the same FB so
>> > that atomic and legacy codepaths have the same view of display state.
>> > However, there are some places in kernel code that directly set
>> > plane->fb and neglect to update plane->state->fb. If we never do a
>> > successful update through the atomic pipeline, the RmFB cleanup code
>> > will look at the plane->state->fb pointer, which has never actually
>> > been set to a legitimate value, and try to clean it up, leading to
>> > BUG's.
>> >
>> > Add a quick helper function to synchronize plane->state->fb with
>> > plane->fb and call it everywhere the driver tries to manually set
>> > plane->fb outside of the atomic pipeline. In this function, use
>> > drm_atomic_set_fb_for_plane instead of writing plane->state->fb
>> > directly to keep the reference count right.
>> >
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88909
>> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=93711
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Signed-off-by: Xi Ruoyao <xry111@outlook.com>
>> > ---
>> > This is modified from Matt Roper's patch to drm-intel-nightly with
>> > commit id
>> >
>> > afd65eb4cc0578a9c07d621acdb8a570e2782bf7
>> >
>> > However this bug exists in mainline kernel too, so I created this to
>> > fix it in mainline kernel.
>> >
>> > A minor change is to use drm_atomic_set_fb_for_plane instead of
>> > update reference count manually.
>> >
>> > drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
>> > 1 file changed, 17 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index e730789..c483f2f 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -37,6 +37,7 @@
>> > #include <drm/i915_drm.h>
>> > #include "i915_drv.h"
>> > #include "i915_trace.h"
>> > +#include <drm/drm_atomic.h>
>> > #include <drm/drm_atomic_helper.h>
>> > #include <drm/drm_dp_helper.h>
>> > #include <drm/drm_crtc_helper.h>
>> > @@ -2416,6 +2417,14 @@ out_unref_obj:
>> > return false;
>> > }
>> >
>> > +/* Update plane->state->fb to match plane->fb after driver-internal updates */
>> > +static void
>> > +update_state_fb(struct drm_plane *plane)
>> > +{
>> > + if (plane->fb != plane->state->fb)
>> > + drm_atomic_set_fb_for_plane(plane->state, plane->fb);
>> > +}
>> > +
>> > static void
>> > intel_find_plane_obj(struct intel_crtc *intel_crtc,
>> > struct intel_initial_plane_config *plane_config)
>> > @@ -2462,6 +2471,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
>> > break;
>> > }
>> > }
>> > +
>> > + update_state_fb(intel_crtc->base.primary);
>> > }
>> >
>> > static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>> > @@ -6650,6 +6661,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>> > plane_config->size);
>> >
>> > crtc->base.primary->fb = fb;
>> > + update_state_fb(crtc->base.primary);
>> > }
>> >
>> > static void chv_crtc_clock_get(struct intel_crtc *crtc,
>> > @@ -7687,6 +7699,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>> > plane_config->size);
>> >
>> > crtc->base.primary->fb = fb;
>> > + update_state_fb(crtc->base.primary);
>> > return;
>> >
>> > error:
>> > @@ -7778,6 +7791,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
>> > plane_config->size);
>> >
>> > crtc->base.primary->fb = fb;
>> > + update_state_fb(crtc->base.primary);
>> > }
>> >
>> > static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>> > @@ -9816,6 +9830,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>> > drm_gem_object_reference(&obj->base);
>> >
>> > crtc->primary->fb = fb;
>> > + update_state_fb(crtc->primary);
>> >
>> > work->pending_flip_obj = obj;
>> >
>> > @@ -9884,6 +9899,7 @@ cleanup_unpin:
>> > cleanup_pending:
>> > atomic_dec(&intel_crtc->unpin_work_count);
>> > crtc->primary->fb = old_fb;
>> > + update_state_fb(crtc->primary);
>> > drm_gem_object_unreference(&work->old_fb_obj->base);
>> > drm_gem_object_unreference(&obj->base);
>> > mutex_unlock(&dev->struct_mutex);
>> > @@ -13718,6 +13734,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>> > to_intel_crtc(c)->pipe);
>> > drm_framebuffer_unreference(c->primary->fb);
>> > c->primary->fb = NULL;
>> > + update_state_fb(c->primary);
>> > }
>> > }
>> > mutex_unlock(&dev->struct_mutex);
>> > --
>> > 1.9.1
>> >
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm/i915: Ensure plane->state->fb stays in sync with plane->fb
@ 2015-02-03 21:10 Matt Roper
2015-02-04 9:39 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Matt Roper @ 2015-02-03 21:10 UTC (permalink / raw)
To: intel-gfx
plane->state->fb and plane->fb should always reference the same FB so
that atomic and legacy codepaths have the same view of display state.
In commit
commit db068420560511de80ac59222644f2bdf278c3d5
Author: Matt Roper <matthew.d.roper@intel.com>
Date: Fri Jan 30 16:22:36 2015 -0800
drm/i915: Keep plane->state updated on pageflip
we already fixed one case where these two pointers could get out of
sync. However it turns out there are a few other places (mainly dealing
with initial FB setup at boot) that directly set plane->fb and neglect
to update plane->state->fb. If we never do a successful update through
the atomic pipeline, the RmFB cleanup code will look at the
plane->state->fb pointer, which has never actually been set to a
legitimate value, and try to clean it up, leading to BUG's.
Add a quick helper function to synchronize plane->state->fb with
plane->fb (and update reference counts accordingly) and call it
everywhere the driver tries to manually set plane->fb outside of the
atomic pipeline.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88909
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 213b870..e5c0579 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2410,6 +2410,20 @@ out_unref_obj:
return false;
}
+/* Update plane->state->fb to match plane->fb after driver-internal updates */
+static void
+update_state_fb(struct drm_plane *plane)
+{
+ if (plane->fb == plane->state->fb)
+ return;
+
+ if (plane->state->fb)
+ drm_framebuffer_unreference(plane->state->fb);
+ plane->state->fb = plane->fb;
+ if (plane->state->fb)
+ drm_framebuffer_reference(plane->state->fb);
+}
+
static void
intel_find_plane_obj(struct intel_crtc *intel_crtc,
struct intel_initial_plane_config *plane_config)
@@ -2456,6 +2470,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
break;
}
}
+
+ update_state_fb(intel_crtc->base.primary);
}
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
@@ -6635,6 +6651,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
plane_config->size);
crtc->base.primary->fb = fb;
+ update_state_fb(crtc->base.primary);
}
static void chv_crtc_clock_get(struct intel_crtc *crtc,
@@ -7672,6 +7689,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
plane_config->size);
crtc->base.primary->fb = fb;
+ update_state_fb(crtc->base.primary);
return;
error:
@@ -7763,6 +7781,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
plane_config->size);
crtc->base.primary->fb = fb;
+ update_state_fb(crtc->base.primary);
}
static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
@@ -9800,13 +9819,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
drm_gem_object_reference(&obj->base);
crtc->primary->fb = fb;
-
- /* Keep state structure in sync */
- if (crtc->primary->state->fb)
- drm_framebuffer_unreference(crtc->primary->state->fb);
- crtc->primary->state->fb = fb;
- if (crtc->primary->state->fb)
- drm_framebuffer_reference(crtc->primary->state->fb);
+ update_state_fb(crtc->primary);
work->pending_flip_obj = obj;
@@ -9875,6 +9888,7 @@ cleanup_unpin:
cleanup_pending:
atomic_dec(&intel_crtc->unpin_work_count);
crtc->primary->fb = old_fb;
+ update_state_fb(crtc->primary);
drm_framebuffer_unreference(work->old_fb);
drm_gem_object_unreference(&obj->base);
mutex_unlock(&dev->struct_mutex);
@@ -13709,6 +13723,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
to_intel_crtc(c)->pipe);
drm_framebuffer_unreference(c->primary->fb);
c->primary->fb = NULL;
+ update_state_fb(c->primary);
}
}
mutex_unlock(&dev->struct_mutex);
--
1.8.5.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: Ensure plane->state->fb stays in sync with plane->fb
2015-02-03 21:10 Matt Roper
@ 2015-02-04 9:39 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2015-02-04 9:39 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Tue, Feb 03, 2015 at 01:10:04PM -0800, Matt Roper wrote:
> plane->state->fb and plane->fb should always reference the same FB so
> that atomic and legacy codepaths have the same view of display state.
> In commit
>
> commit db068420560511de80ac59222644f2bdf278c3d5
> Author: Matt Roper <matthew.d.roper@intel.com>
> Date: Fri Jan 30 16:22:36 2015 -0800
>
> drm/i915: Keep plane->state updated on pageflip
>
> we already fixed one case where these two pointers could get out of
> sync. However it turns out there are a few other places (mainly dealing
> with initial FB setup at boot) that directly set plane->fb and neglect
> to update plane->state->fb. If we never do a successful update through
> the atomic pipeline, the RmFB cleanup code will look at the
> plane->state->fb pointer, which has never actually been set to a
> legitimate value, and try to clean it up, leading to BUG's.
>
> Add a quick helper function to synchronize plane->state->fb with
> plane->fb (and update reference counts accordingly) and call it
> everywhere the driver tries to manually set plane->fb outside of the
> atomic pipeline.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88909
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Unrelated thing I've spotted while reviewing this: We still update
plane->fb in our plane commit hooks, but helpers should take care of that.
Do we still have these because our code still looks at plane->fb instead
of plane->state->fb or is there some deeper reason?
I think a large-scale s/plane->fb/plane->state->fb/ and then removing
those and relying upon the fixup code in the helpers/core would be good.
Unfortunately we can't cocci this since we need to keep a few ...
Anyway, patch lgtm and is merged.
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 213b870..e5c0579 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2410,6 +2410,20 @@ out_unref_obj:
> return false;
> }
>
> +/* Update plane->state->fb to match plane->fb after driver-internal updates */
> +static void
> +update_state_fb(struct drm_plane *plane)
> +{
> + if (plane->fb == plane->state->fb)
> + return;
> +
> + if (plane->state->fb)
> + drm_framebuffer_unreference(plane->state->fb);
> + plane->state->fb = plane->fb;
> + if (plane->state->fb)
> + drm_framebuffer_reference(plane->state->fb);
> +}
> +
> static void
> intel_find_plane_obj(struct intel_crtc *intel_crtc,
> struct intel_initial_plane_config *plane_config)
> @@ -2456,6 +2470,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> break;
> }
> }
> +
> + update_state_fb(intel_crtc->base.primary);
> }
>
> static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> @@ -6635,6 +6651,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> plane_config->size);
>
> crtc->base.primary->fb = fb;
> + update_state_fb(crtc->base.primary);
> }
>
> static void chv_crtc_clock_get(struct intel_crtc *crtc,
> @@ -7672,6 +7689,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> plane_config->size);
>
> crtc->base.primary->fb = fb;
> + update_state_fb(crtc->base.primary);
> return;
>
> error:
> @@ -7763,6 +7781,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
> plane_config->size);
>
> crtc->base.primary->fb = fb;
> + update_state_fb(crtc->base.primary);
> }
>
> static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> @@ -9800,13 +9819,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> drm_gem_object_reference(&obj->base);
>
> crtc->primary->fb = fb;
> -
> - /* Keep state structure in sync */
> - if (crtc->primary->state->fb)
> - drm_framebuffer_unreference(crtc->primary->state->fb);
> - crtc->primary->state->fb = fb;
> - if (crtc->primary->state->fb)
> - drm_framebuffer_reference(crtc->primary->state->fb);
> + update_state_fb(crtc->primary);
>
> work->pending_flip_obj = obj;
>
> @@ -9875,6 +9888,7 @@ cleanup_unpin:
> cleanup_pending:
> atomic_dec(&intel_crtc->unpin_work_count);
> crtc->primary->fb = old_fb;
> + update_state_fb(crtc->primary);
> drm_framebuffer_unreference(work->old_fb);
> drm_gem_object_unreference(&obj->base);
> mutex_unlock(&dev->struct_mutex);
> @@ -13709,6 +13723,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
> to_intel_crtc(c)->pipe);
> drm_framebuffer_unreference(c->primary->fb);
> c->primary->fb = NULL;
> + update_state_fb(c->primary);
> }
> }
> mutex_unlock(&dev->struct_mutex);
> --
> 1.8.5.1
>
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-13 12:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-12 12:16 [PATCH] drm/i915: Ensure plane->state->fb stays in sync with plane->fb Xi Ruoyao
2015-03-12 12:45 ` Jani Nikula
2015-03-12 16:12 ` Matt Roper
2015-03-13 12:38 ` Jani Nikula
-- strict thread matches above, loose matches on Subject: below --
2015-02-03 21:10 Matt Roper
2015-02-04 9:39 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox