Re: [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
Date: Thu, 15 Oct 2015 12:30:44 +0100	[thread overview]
Message-ID: <561F8E64.60507@linux.intel.com> (raw)
In-Reply-To: <20151015111730.GD26517@intel.com>

On 15/10/15 12:17, Ville Syrjälä wrote:
> On Thu, Oct 15, 2015 at 12:10:32PM +0100, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> On 14/10/15 17:29, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird
>>> rotation (to find the right GTT view for it), so no need to pass all
>>> kinds of plane stuff.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++--------------------
>>>    drivers/gpu/drm/i915/intel_drv.h     |  5 ++---
>>>    drivers/gpu/drm/i915/intel_fbdev.c   |  2 +-
>>>    3 files changed, 20 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 85e1473..80e9f2e 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height,
>>>    }
>>>
>>>    static int
>>> -intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>>> -			const struct drm_plane_state *plane_state)
>>> +intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
>>> +			const struct drm_framebuffer *fb,
>>> +			unsigned int rotation)
>>>    {
>>>    	struct drm_i915_private *dev_priv = to_i915(fb->dev);
>>>    	struct intel_rotation_info *info = &view->rotation_info;
>>> @@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>>>
>>>    	*view = i915_ggtt_view_normal;
>>>
>>> -	if (!plane_state)
>>> -		return 0;
>>> -
>>> -	if (!intel_rotation_90_or_270(plane_state->rotation))
>>> +	if (!intel_rotation_90_or_270(rotation))
>>>    		return 0;
>>>
>>>    	*view = i915_ggtt_view_rotated;
>>> @@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const struct drm_i915_private *dev_priv
>>>    }
>>>
>>>    int
>>> -intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>> -			   struct drm_framebuffer *fb,
>>> -			   const struct drm_plane_state *plane_state,
>>> +intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
>>> +			   unsigned int rotation,
>>>    			   struct intel_engine_cs *pipelined,
>>>    			   struct drm_i915_gem_request **pipelined_request)
>>>    {
>>
>> It feels like you are losing the benefit of cleaning this up by having
>> to pass in rotation anyway. So I think it makes more sense to keep
>> passing in plane_state and only get rid of the plane. Or vice-versa, not
>> really sure what is conceptually better. Possibly plane and then access
>> the state from it.
>
> The only thing we basically need is "which vma do we want". But just
> passing rotation directly looks nicer I think. The benefit really is
> eliminating the ugly 'if (!plane_state)' mess caused by intel_fbdev.

We'll have to disagree there then because I find it really inelegant to 
express the knowledge of what exact information is needed when preparing 
the frame buffer for display into the function parameters.

It is conceptually much more elegant to say "this fb for this plane - do 
what is right".

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx