| Message ID | 1444840154-7804-12-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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. imho this is a mistep, I think using the plane-state to not only pass the full description of the plane being bound (which may have additional information like the need for fencing for fbc as well as alternative views, i.e. it is a lot more versatile) but also allows us to track the binding for the plane-state and tie the VMA to lifetime of the plane. i.e. I think intel_pin_and_fence_fb_obj would be better described as intel_plane_state_pin_vma (and correspondingly intel_plane_state_unpin_vma). Yes, intel_fbdev.c is a wart to any proposed interface. -Chris
On Thu, Oct 15, 2015 at 10:08:53AM +0100, Chris Wilson wrote: > On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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. > > imho this is a mistep, I think using the plane-state to not only pass > the full description of the plane being bound (which may have additional > information like the need for fencing for fbc as well as alternative > views, i.e. it is a lot more versatile) but also allows us to track the > binding for the plane-state and tie the VMA to lifetime of the plane. > > i.e. I think intel_pin_and_fence_fb_obj would be better described as > intel_plane_state_pin_vma (and correspondingly > intel_plane_state_unpin_vma). > > Yes, intel_fbdev.c is a wart to any proposed interface. The current code is just too ugly to live IMO (due to fbdev, yes), so I think we want this for now. We can always wrap it up in fancier clothing for users that actually have a plane state once someone comes up with some real code that needs it.
On Thu, Oct 15, 2015 at 12:36:43PM +0300, Ville Syrjälä wrote: > On Thu, Oct 15, 2015 at 10:08:53AM +0100, Chris Wilson wrote: > > On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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. > > > > imho this is a mistep, I think using the plane-state to not only pass > > the full description of the plane being bound (which may have additional > > information like the need for fencing for fbc as well as alternative > > views, i.e. it is a lot more versatile) but also allows us to track the > > binding for the plane-state and tie the VMA to lifetime of the plane. > > > > i.e. I think intel_pin_and_fence_fb_obj would be better described as > > intel_plane_state_pin_vma (and correspondingly > > intel_plane_state_unpin_vma). > > > > Yes, intel_fbdev.c is a wart to any proposed interface. > > The current code is just too ugly to live IMO (due to fbdev, yes), so > I think we want this for now. We can always wrap it up in fancier > clothing for users that actually have a plane state once someone comes > up with some real code that needs it. To handle this we could make intel_pin_and_fence_fb return the vma for callers to store in the plane state eventually, with errors encoded using ERR_PTR. That way we can keep intel_fbdev.c as is and still store the vma in the plane state. -Daniel
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. Regards, Tvrtko
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.
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
On Thu, Oct 15, 2015 at 12:30:44PM +0100, Tvrtko Ursulin wrote: > > 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". Only if the function is actually used for that. With fbdev it's not. Chris did sell the "vma in plane state" idea to me last night on irc, so when we get that we probably want a function that accepts the plane state. But that can basically just wrap the current thing, which means fbdev can keep using the lower level function that doesn't need the plane state.
On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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> Feels indeed a bit like a bikeshed and just churn without resolving the "pass vma in plane_state around" idea for real. But meh, it's imo not worse than the existing code, and looks correct. Less churn would make me a happier reviewer thought (there's a pile of whitespace in here too). Grumpily-Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > 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) > { > @@ -2371,7 +2368,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > > alignment = intel_surf_alignment(dev_priv, fb->modifier[0]); > > - ret = intel_fill_fb_ggtt_view(&view, fb, plane_state); > + ret = intel_fill_fb_ggtt_view(&view, fb, rotation); > if (ret) > return ret; > > @@ -2432,8 +2429,7 @@ err_interruptible: > return ret; > } > > -static void intel_unpin_fb_obj(struct drm_framebuffer *fb, > - const struct drm_plane_state *plane_state) > +static void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation) > { > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > struct i915_ggtt_view view; > @@ -2441,7 +2437,7 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb, > > WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex)); > > - ret = intel_fill_fb_ggtt_view(&view, fb, plane_state); > + ret = intel_fill_fb_ggtt_view(&view, fb, rotation); > WARN_ONCE(ret, "Couldn't get view from plane state!"); > > i915_gem_object_unpin_fence(obj); > @@ -10780,7 +10776,7 @@ static void intel_unpin_work_fn(struct work_struct *__work) > struct drm_plane *primary = crtc->base.primary; > > mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(work->old_fb, primary->state); > + intel_unpin_fb_obj(work->old_fb, primary->state->rotation); > drm_gem_object_unreference(&work->pending_flip_obj->base); > > if (work->flip_queued_req) > @@ -11521,8 +11517,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > * synchronisation, so all we want here is to pin the framebuffer > * into the display plane and skip any waits. > */ > - ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, > - crtc->primary->state, > + ret = intel_pin_and_fence_fb_obj(fb, primary->state->rotation, > mmio_flip ? i915_gem_request_get_ring(obj->last_write_req) : ring, &request); > if (ret) > goto cleanup_pending; > @@ -11573,7 +11568,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > return 0; > > cleanup_unpin: > - intel_unpin_fb_obj(fb, crtc->primary->state); > + intel_unpin_fb_obj(fb, crtc->primary->state->rotation); > cleanup_pending: > if (request) > i915_gem_request_cancel(request); > @@ -13457,7 +13452,8 @@ intel_prepare_plane_fb(struct drm_plane *plane, > if (ret) > DRM_DEBUG_KMS("failed to attach phys object\n"); > } else { > - ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL); > + ret = intel_pin_and_fence_fb_obj(fb, new_state->rotation, > + NULL, NULL); > } > > if (ret == 0) > @@ -13488,7 +13484,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane, > if (plane->type != DRM_PLANE_TYPE_CURSOR || > !INTEL_INFO(dev)->cursor_needs_physical) { > mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(old_state->fb, old_state); > + intel_unpin_fb_obj(old_state->fb, old_state->rotation); > mutex_unlock(&dev->struct_mutex); > } > } > @@ -15474,9 +15470,8 @@ void intel_modeset_gem_init(struct drm_device *dev) > continue; > > mutex_lock(&dev->struct_mutex); > - ret = intel_pin_and_fence_fb_obj(c->primary, > - c->primary->fb, > - c->primary->state, > + ret = intel_pin_and_fence_fb_obj(c->primary->fb, > + c->primary->state->rotation, > NULL, NULL); > mutex_unlock(&dev->struct_mutex); > if (ret) { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index ed47ca3..b0d92e0 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1067,9 +1067,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, > void intel_release_load_detect_pipe(struct drm_connector *connector, > struct intel_load_detect_pipe *old, > struct drm_modeset_acquire_ctx *ctx); > -int intel_pin_and_fence_fb_obj(struct drm_plane *plane, > - struct drm_framebuffer *fb, > - const struct drm_plane_state *plane_state, > +int 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); > struct drm_framebuffer * > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 4fd5fdf..6bef820 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -161,7 +161,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > } > > /* Flush everything out, we'll be doing GTT only from now on */ > - ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL); > + ret = intel_pin_and_fence_fb_obj(fb, BIT(DRM_ROTATE_0), NULL, NULL); > if (ret) { > DRM_ERROR("failed to pin obj: %d\n", ret); > goto out_fb; > -- > 2.4.9 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 21/10/15 12:28, Daniel Vetter wrote: > On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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> > > Feels indeed a bit like a bikeshed and just churn without resolving the > "pass vma in plane_state around" idea for real. But meh, it's imo not > worse than the existing code, and looks correct. Less churn would make me > a happier reviewer thought (there's a pile of whitespace in here too). > > Grumpily-Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> It is worse, and is definitely not better. :( Parameter reduction can be still accomplished without exposing one random piece of plane state in the function signature. Regards, Tvrtko
On Wed, Oct 21, 2015 at 01:17:10PM +0100, Tvrtko Ursulin wrote: > > On 21/10/15 12:28, Daniel Vetter wrote: > > On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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> > > > > Feels indeed a bit like a bikeshed and just churn without resolving the > > "pass vma in plane_state around" idea for real. But meh, it's imo not > > worse than the existing code, and looks correct. Less churn would make me > > a happier reviewer thought (there's a pile of whitespace in here too). > > > > Grumpily-Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > It is worse, and is definitely not better. :( I might accept that if it wasn't for the ugly 'if (!plane_state)' mess.
On 21/10/15 14:09, Ville Syrjälä wrote: > On Wed, Oct 21, 2015 at 01:17:10PM +0100, Tvrtko Ursulin wrote: >> >> On 21/10/15 12:28, Daniel Vetter wrote: >>> On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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> >>> >>> Feels indeed a bit like a bikeshed and just churn without resolving the >>> "pass vma in plane_state around" idea for real. But meh, it's imo not >>> worse than the existing code, and looks correct. Less churn would make me >>> a happier reviewer thought (there's a pile of whitespace in here too). >>> >>> Grumpily-Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> It is worse, and is definitely not better. :( > > I might accept that if it wasn't for the ugly 'if (!plane_state)' mess. Ok you have a point there. Maybe there should be a fake default state available, if somehow possible? Would all zero be a valid state? Tvrtko
On Wed, Oct 21, 2015 at 02:22:10PM +0100, Tvrtko Ursulin wrote: > > On 21/10/15 14:09, Ville Syrjälä wrote: > > On Wed, Oct 21, 2015 at 01:17:10PM +0100, Tvrtko Ursulin wrote: > >> > >> On 21/10/15 12:28, Daniel Vetter wrote: > >>> On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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> > >>> > >>> Feels indeed a bit like a bikeshed and just churn without resolving the > >>> "pass vma in plane_state around" idea for real. But meh, it's imo not > >>> worse than the existing code, and looks correct. Less churn would make me > >>> a happier reviewer thought (there's a pile of whitespace in here too). > >>> > >>> Grumpily-Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > >> It is worse, and is definitely not better. :( > > > > I might accept that if it wasn't for the ugly 'if (!plane_state)' mess. > > Ok you have a point there. Maybe there should be a fake default state > available, if somehow possible? Would all zero be a valid state? Let's not start adding fake stuff, that's going to be even more confusing. IMO the correct approach is to have a low level function that doesn't depend on the state (which is what we have after my patch more or less), and then we can just wrap it up in nicer clothing for most normal users.
On Wed, Oct 21, 2015 at 05:22:40PM +0300, Ville Syrjälä wrote: > On Wed, Oct 21, 2015 at 02:22:10PM +0100, Tvrtko Ursulin wrote: > > > > On 21/10/15 14:09, Ville Syrjälä wrote: > > > On Wed, Oct 21, 2015 at 01:17:10PM +0100, Tvrtko Ursulin wrote: > > >> > > >> On 21/10/15 12:28, Daniel Vetter wrote: > > >>> On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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> > > >>> > > >>> Feels indeed a bit like a bikeshed and just churn without resolving the > > >>> "pass vma in plane_state around" idea for real. But meh, it's imo not > > >>> worse than the existing code, and looks correct. Less churn would make me > > >>> a happier reviewer thought (there's a pile of whitespace in here too). > > >>> > > >>> Grumpily-Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > >> > > >> It is worse, and is definitely not better. :( > > > > > > I might accept that if it wasn't for the ugly 'if (!plane_state)' mess. > > > > Ok you have a point there. Maybe there should be a fake default state > > available, if somehow possible? Would all zero be a valid state? > > Let's not start adding fake stuff, that's going to be even more > confusing. IMO the correct approach is to have a low level function that > doesn't depend on the state (which is what we have after my patch more or > less), and then we can just wrap it up in nicer clothing for most normal > users. I agree, my only complaint is that it's hard to judge the low-level polish without the pretty clothes on top ;-) Cheers, Daniel
On Wed, Oct 21, 2015 at 05:20:02PM +0200, Daniel Vetter wrote: > On Wed, Oct 21, 2015 at 05:22:40PM +0300, Ville Syrjälä wrote: > > On Wed, Oct 21, 2015 at 02:22:10PM +0100, Tvrtko Ursulin wrote: > > > > > > On 21/10/15 14:09, Ville Syrjälä wrote: > > > > On Wed, Oct 21, 2015 at 01:17:10PM +0100, Tvrtko Ursulin wrote: > > > >> > > > >> On 21/10/15 12:28, Daniel Vetter wrote: > > > >>> On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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> > > > >>> > > > >>> Feels indeed a bit like a bikeshed and just churn without resolving the > > > >>> "pass vma in plane_state around" idea for real. But meh, it's imo not > > > >>> worse than the existing code, and looks correct. Less churn would make me > > > >>> a happier reviewer thought (there's a pile of whitespace in here too). > > > >>> > > > >>> Grumpily-Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > >> > > > >> It is worse, and is definitely not better. :( > > > > > > > > I might accept that if it wasn't for the ugly 'if (!plane_state)' mess. > > > > > > Ok you have a point there. Maybe there should be a fake default state > > > available, if somehow possible? Would all zero be a valid state? > > > > Let's not start adding fake stuff, that's going to be even more > > confusing. IMO the correct approach is to have a low level function that > > doesn't depend on the state (which is what we have after my patch more or > > less), and then we can just wrap it up in nicer clothing for most normal > > users. > > I agree, my only complaint is that it's hard to judge the low-level polish > without the pretty clothes on top ;-) Quality tailoring requires decent measurements, or we might end up with a clown costume instead of a nice suit. At least I haven't really thought through what we really want here.
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) { @@ -2371,7 +2368,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, alignment = intel_surf_alignment(dev_priv, fb->modifier[0]); - ret = intel_fill_fb_ggtt_view(&view, fb, plane_state); + ret = intel_fill_fb_ggtt_view(&view, fb, rotation); if (ret) return ret; @@ -2432,8 +2429,7 @@ err_interruptible: return ret; } -static void intel_unpin_fb_obj(struct drm_framebuffer *fb, - const struct drm_plane_state *plane_state) +static void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation) { struct drm_i915_gem_object *obj = intel_fb_obj(fb); struct i915_ggtt_view view; @@ -2441,7 +2437,7 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb, WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex)); - ret = intel_fill_fb_ggtt_view(&view, fb, plane_state); + ret = intel_fill_fb_ggtt_view(&view, fb, rotation); WARN_ONCE(ret, "Couldn't get view from plane state!"); i915_gem_object_unpin_fence(obj); @@ -10780,7 +10776,7 @@ static void intel_unpin_work_fn(struct work_struct *__work) struct drm_plane *primary = crtc->base.primary; mutex_lock(&dev->struct_mutex); - intel_unpin_fb_obj(work->old_fb, primary->state); + intel_unpin_fb_obj(work->old_fb, primary->state->rotation); drm_gem_object_unreference(&work->pending_flip_obj->base); if (work->flip_queued_req) @@ -11521,8 +11517,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, * synchronisation, so all we want here is to pin the framebuffer * into the display plane and skip any waits. */ - ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, - crtc->primary->state, + ret = intel_pin_and_fence_fb_obj(fb, primary->state->rotation, mmio_flip ? i915_gem_request_get_ring(obj->last_write_req) : ring, &request); if (ret) goto cleanup_pending; @@ -11573,7 +11568,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, return 0; cleanup_unpin: - intel_unpin_fb_obj(fb, crtc->primary->state); + intel_unpin_fb_obj(fb, crtc->primary->state->rotation); cleanup_pending: if (request) i915_gem_request_cancel(request); @@ -13457,7 +13452,8 @@ intel_prepare_plane_fb(struct drm_plane *plane, if (ret) DRM_DEBUG_KMS("failed to attach phys object\n"); } else { - ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL); + ret = intel_pin_and_fence_fb_obj(fb, new_state->rotation, + NULL, NULL); } if (ret == 0) @@ -13488,7 +13484,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane, if (plane->type != DRM_PLANE_TYPE_CURSOR || !INTEL_INFO(dev)->cursor_needs_physical) { mutex_lock(&dev->struct_mutex); - intel_unpin_fb_obj(old_state->fb, old_state); + intel_unpin_fb_obj(old_state->fb, old_state->rotation); mutex_unlock(&dev->struct_mutex); } } @@ -15474,9 +15470,8 @@ void intel_modeset_gem_init(struct drm_device *dev) continue; mutex_lock(&dev->struct_mutex); - ret = intel_pin_and_fence_fb_obj(c->primary, - c->primary->fb, - c->primary->state, + ret = intel_pin_and_fence_fb_obj(c->primary->fb, + c->primary->state->rotation, NULL, NULL); mutex_unlock(&dev->struct_mutex); if (ret) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index ed47ca3..b0d92e0 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1067,9 +1067,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, void intel_release_load_detect_pipe(struct drm_connector *connector, struct intel_load_detect_pipe *old, struct drm_modeset_acquire_ctx *ctx); -int intel_pin_and_fence_fb_obj(struct drm_plane *plane, - struct drm_framebuffer *fb, - const struct drm_plane_state *plane_state, +int 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); struct drm_framebuffer * diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 4fd5fdf..6bef820 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -161,7 +161,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, } /* Flush everything out, we'll be doing GTT only from now on */ - ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL); + ret = intel_pin_and_fence_fb_obj(fb, BIT(DRM_ROTATE_0), NULL, NULL); if (ret) { DRM_ERROR("failed to pin obj: %d\n", ret); goto out_fb;