| Message ID | 20180323134553.15993-2-daniels@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 23, 2018 at 01:45:50PM +0000, Daniel Stone wrote: > Since drm_framebuffer can now store GEM objects directly, place them > there rather than in our own subclass. > > Signed-off-by: Daniel Stone <daniels@collabora.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: intel-gfx@lists.freedesktop.org > --- > drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++--- > drivers/gpu/drm/i915/intel_drv.h | 3 +-- > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 49b0772e9abc..e8100a4fc877 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13916,7 +13916,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb) > drm_framebuffer_cleanup(fb); > > i915_gem_object_lock(obj); > - WARN_ON(!obj->framebuffer_references--); > + WARN_ON(obj->framebuffer_references < fb->format->num_planes); > + obj->framebuffer_references -= fb->format->num_planes; Hmm. I'm thinking we can stick to the single reference per fb. IIRC this counter is there just to prevent changes of the obj tiling mode as long as any fb exists that uses the object. So doesn't actually matter how many planes the fb has. Naturally the story would be slightly difference if we supported fbs using multiple different BOs, as each BO would need to get its framebuffer_references adjusted. > i915_gem_object_unlock(obj); > > i915_gem_object_put(obj); > @@ -14176,9 +14177,9 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, > i, fb->pitches[i], stride_alignment); > goto err; > } > - } > > - intel_fb->obj = obj; > + fb->obj[i] = &obj->base; > + } > > ret = intel_fill_fb_info(dev_priv, fb); > if (ret) > @@ -14190,6 +14191,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, > goto err; > } > > + /* We already hold one reference to the fb, but in case it's > + * multi-planar and we've placed multiple pointers in > + * drm_framebuffer, hold extra refs. > + */ > + i915_gem_object_lock(obj); > + obj->framebuffer_references += fb->format->num_planes - 1; > + i915_gem_object_unlock(obj); > + > return 0; > > err: > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 570f89b31544..d93ed9e59867 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -186,7 +186,6 @@ enum intel_output_type { > > struct intel_framebuffer { > struct drm_framebuffer base; > - struct drm_i915_gem_object *obj; > struct intel_rotation_info rot_info; > > /* for each plane in the normal GTT view */ > @@ -985,7 +984,7 @@ struct cxsr_latency { > #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base) > #define to_intel_plane(x) container_of(x, struct intel_plane, base) > #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base) > -#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL) > +#define intel_fb_obj(x) (((x) && (x)->obj[0]) ? to_intel_bo((x)->obj[0]) : NULL) Ah, I've had that "(x)" part coded up so many times, but never sent it out :) > > struct intel_hdmi { > i915_reg_t hdmi_reg; > -- > 2.16.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Ville, On 23 March 2018 at 14:42, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Mar 23, 2018 at 01:45:50PM +0000, Daniel Stone wrote: >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -13916,7 +13916,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb) >> drm_framebuffer_cleanup(fb); >> >> i915_gem_object_lock(obj); >> - WARN_ON(!obj->framebuffer_references--); >> + WARN_ON(obj->framebuffer_references < fb->format->num_planes); >> + obj->framebuffer_references -= fb->format->num_planes; > > Hmm. I'm thinking we can stick to the single reference per fb. > IIRC this counter is there just to prevent changes of the obj > tiling mode as long as any fb exists that uses the object. So > doesn't actually matter how many planes the fb has. > > Naturally the story would be slightly difference if we supported > fbs using multiple different BOs, as each BO would need to get its > framebuffer_references adjusted. Yeah, fair enough. It looks a little bit weird (perhaps deserving of a comment) there. The reason to do that was just the general principle of having one reference per object pointer, especially when other drivers (ones which can have separate BOs in a single logical image) will and do refcount them separately. Having different refcounting semantics in shared structures depending on which driver is in use makes me itchy. Cheers, Daniel
Hi Ville, On 23 March 2018 at 14:49, Daniel Stone <daniel@fooishbar.org> wrote: > On 23 March 2018 at 14:42, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> Hmm. I'm thinking we can stick to the single reference per fb. >> IIRC this counter is there just to prevent changes of the obj >> tiling mode as long as any fb exists that uses the object. So >> doesn't actually matter how many planes the fb has. >> >> Naturally the story would be slightly difference if we supported >> fbs using multiple different BOs, as each BO would need to get its >> framebuffer_references adjusted. > > Yeah, fair enough. It looks a little bit weird (perhaps deserving of a > comment) there. The reason to do that was just the general principle > of having one reference per object pointer, especially when other > drivers (ones which can have separate BOs in a single logical image) > will and do refcount them separately. Having different refcounting > semantics in shared structures depending on which driver is in use > makes me itchy. Absent any other comment, I've dropped this change and will keep a single framebuffer_reference[s] for v2. Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 49b0772e9abc..e8100a4fc877 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13916,7 +13916,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb) drm_framebuffer_cleanup(fb); i915_gem_object_lock(obj); - WARN_ON(!obj->framebuffer_references--); + WARN_ON(obj->framebuffer_references < fb->format->num_planes); + obj->framebuffer_references -= fb->format->num_planes; i915_gem_object_unlock(obj); i915_gem_object_put(obj); @@ -14176,9 +14177,9 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, i, fb->pitches[i], stride_alignment); goto err; } - } - intel_fb->obj = obj; + fb->obj[i] = &obj->base; + } ret = intel_fill_fb_info(dev_priv, fb); if (ret) @@ -14190,6 +14191,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, goto err; } + /* We already hold one reference to the fb, but in case it's + * multi-planar and we've placed multiple pointers in + * drm_framebuffer, hold extra refs. + */ + i915_gem_object_lock(obj); + obj->framebuffer_references += fb->format->num_planes - 1; + i915_gem_object_unlock(obj); + return 0; err: diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 570f89b31544..d93ed9e59867 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -186,7 +186,6 @@ enum intel_output_type { struct intel_framebuffer { struct drm_framebuffer base; - struct drm_i915_gem_object *obj; struct intel_rotation_info rot_info; /* for each plane in the normal GTT view */ @@ -985,7 +984,7 @@ struct cxsr_latency { #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base) #define to_intel_plane(x) container_of(x, struct intel_plane, base) #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base) -#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL) +#define intel_fb_obj(x) (((x) && (x)->obj[0]) ? to_intel_bo((x)->obj[0]) : NULL) struct intel_hdmi { i915_reg_t hdmi_reg;
Since drm_framebuffer can now store GEM objects directly, place them there rather than in our own subclass. Signed-off-by: Daniel Stone <daniels@collabora.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: intel-gfx@lists.freedesktop.org --- drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++--- drivers/gpu/drm/i915/intel_drv.h | 3 +-- 2 files changed, 13 insertions(+), 5 deletions(-)