Re: [Intel-gfx] [PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer

2018-05-17 Thread Daniel Stone
Hi Ville,

On 23 March 2018 at 14:49, Daniel Stone  wrote:
> On 23 March 2018 at 14:42, Ville Syrjälä  
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer

2018-03-23 Thread Daniel Stone
Hi Ville,

On 23 March 2018 at 14:42, Ville Syrjälä  wrote:
> On Fri, Mar 23, 2018 at 01:45:50PM +, 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer

2018-03-23 Thread Ville Syrjälä
On Fri, Mar 23, 2018 at 01:45:50PM +, 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 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> 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

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx