* [PATCH 1/2] drm/i915/skl: Allow universal planes to position @ 2015-03-30 8:34 Sonika Jindal 2015-03-30 8:34 ` [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation Sonika Jindal 2015-04-01 18:21 ` [PATCH 1/2] drm/i915/skl: Allow universal planes to position Matt Roper 0 siblings, 2 replies; 28+ messages in thread From: Sonika Jindal @ 2015-03-30 8:34 UTC (permalink / raw) To: intel-gfx Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ceb2e61..f0bbc22 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; + bool can_position = false; int ret; crtc = crtc ? crtc : plane->crtc; intel_crtc = to_intel_crtc(crtc); + if (INTEL_INFO(dev)->gen >= 9) + can_position = true; + ret = drm_plane_helper_check_update(plane, crtc, fb, src, dest, clip, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, - false, true, &state->visible); + can_position, true, + &state->visible); if (ret) return ret; -- 1.7.10.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation 2015-03-30 8:34 [PATCH 1/2] drm/i915/skl: Allow universal planes to position Sonika Jindal @ 2015-03-30 8:34 ` Sonika Jindal 2015-03-30 12:15 ` shuang.he 2015-04-01 18:22 ` Matt Roper 2015-04-01 18:21 ` [PATCH 1/2] drm/i915/skl: Allow universal planes to position Matt Roper 1 sibling, 2 replies; 28+ messages in thread From: Sonika Jindal @ 2015-03-30 8:34 UTC (permalink / raw) To: intel-gfx v2: Moving creation of property in a function, checking for 90/270 rotation simultaneously (Chris) Letting primary plane to be positioned v3: Adding if/else for 90/270 and rest params programming, adding check for pixel_format, some cleanup (review comments) v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset and size programming (Ville) v5: Rebased on -nightly and Tvrtko's series for gtt remapping. v6: Rebased on -nightly (Tvrtko's series merged) Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_display.c | 103 +++++++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_drv.h | 6 ++ drivers/gpu/drm/i915/intel_sprite.c | 52 ++++++++++++----- 4 files changed, 129 insertions(+), 34 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b522eb6..564bbd5 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells { #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4) #define PLANE_CTL_ROTATE_MASK 0x3 #define PLANE_CTL_ROTATE_0 0x0 +#define PLANE_CTL_ROTATE_90 0x1 #define PLANE_CTL_ROTATE_180 0x2 +#define PLANE_CTL_ROTATE_270 0x3 #define _PLANE_STRIDE_1_A 0x70188 #define _PLANE_STRIDE_2_A 0x70288 #define _PLANE_STRIDE_3_A 0x70388 diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f0bbc22..86ee0f0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, return -EINVAL; } + switch (fb->pixel_format) { + case DRM_FORMAT_XRGB8888: + case DRM_FORMAT_ARGB8888: + case DRM_FORMAT_XBGR8888: + case DRM_FORMAT_ABGR8888: + case DRM_FORMAT_XRGB2101010: + case DRM_FORMAT_ARGB2101010: + case DRM_FORMAT_XBGR2101010: + case DRM_FORMAT_ABGR2101010: + case DRM_FORMAT_YUYV: + case DRM_FORMAT_YVYU: + case DRM_FORMAT_UYVY: + case DRM_FORMAT_VYUY: + case DRM_FORMAT_NV12: + break; + + default: + DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 rotation!\n", + fb->pixel_format); + return -EINVAL; + } + return 0; } @@ -2919,8 +2941,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_i915_gem_object *obj; int pipe = intel_crtc->pipe; - u32 plane_ctl, stride_div; + u32 plane_ctl, stride_div, stride; + u32 tile_height, plane_offset, plane_size; + unsigned int rotation; + int x_offset, y_offset; unsigned long surf_addr; + struct drm_plane *plane; if (!intel_crtc->primary_enabled) { I915_WRITE(PLANE_CTL(pipe, 0), 0); @@ -2981,21 +3007,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, } plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE; - if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) + + plane = crtc->primary; + rotation = plane->state->rotation; + switch (rotation) { + case BIT(DRM_ROTATE_90): + plane_ctl |= PLANE_CTL_ROTATE_90; + break; + + case BIT(DRM_ROTATE_180): plane_ctl |= PLANE_CTL_ROTATE_180; + break; + + case BIT(DRM_ROTATE_270): + plane_ctl |= PLANE_CTL_ROTATE_270; + break; + } obj = intel_fb_obj(fb); stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], fb->pixel_format); - surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj); + surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj); + + if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { + /* stride = Surface height in tiles */ + tile_height = intel_tile_height(dev, fb->bits_per_pixel, + fb->modifier[0]); + stride = DIV_ROUND_UP(fb->height, tile_height); + x_offset = stride * tile_height - y - (plane->state->src_h >> 16); + y_offset = x; + plane_size = ((plane->state->src_w >> 16) - 1) << 16 | + ((plane->state->src_h >> 16) - 1); + } else { + stride = fb->pitches[0] / stride_div; + x_offset = x; + y_offset = y; + plane_size = ((plane->state->src_h >> 16) - 1) << 16 | + ((plane->state->src_w >> 16) - 1); + } + plane_offset = y_offset << 16 | x_offset; I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); I915_WRITE(PLANE_POS(pipe, 0), 0); - I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x); - I915_WRITE(PLANE_SIZE(pipe, 0), - (intel_crtc->config->pipe_src_h - 1) << 16 | - (intel_crtc->config->pipe_src_w - 1)); - I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div); + I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); + I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); + I915_WRITE(PLANE_STRIDE(pipe, 0), stride); I915_WRITE(PLANE_SURF(pipe, 0), surf_addr); POSTING_READ(PLANE_SURF(pipe, 0)); @@ -12406,23 +12462,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, intel_primary_formats, num_formats, DRM_PLANE_TYPE_PRIMARY); - if (INTEL_INFO(dev)->gen >= 4) { - if (!dev->mode_config.rotation_property) - dev->mode_config.rotation_property = - drm_mode_create_rotation_property(dev, - BIT(DRM_ROTATE_0) | - BIT(DRM_ROTATE_180)); - if (dev->mode_config.rotation_property) - drm_object_attach_property(&primary->base.base, - dev->mode_config.rotation_property, - state->base.rotation); - } + if (INTEL_INFO(dev)->gen >= 4) + intel_create_rotation_property(dev, primary); drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs); return &primary->base; } +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane) +{ + if (!dev->mode_config.rotation_property) { + unsigned long flags = BIT(DRM_ROTATE_0) | + BIT(DRM_ROTATE_180); + + if (INTEL_INFO(dev)->gen >= 9) + flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270); + + dev->mode_config.rotation_property = + drm_mode_create_rotation_property(dev, flags); + } + if (dev->mode_config.rotation_property) + drm_object_attach_property(&plane->base.base, + dev->mode_config.rotation_property, + plane->base.state->rotation); +} + static int intel_check_cursor_plane(struct drm_plane *plane, struct intel_plane_state *state) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 811a1db..d32025a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation) return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)); } +unsigned int +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel, + uint64_t fb_modifier); +void intel_create_rotation_property(struct drm_device *dev, + struct intel_plane *plane); + bool intel_wm_need_update(struct drm_plane *plane, struct drm_plane_state *state); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index f41e872..65eb147 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, struct drm_i915_gem_object *obj = intel_fb_obj(fb); const int pipe = intel_plane->pipe; const int plane = intel_plane->plane + 1; - u32 plane_ctl, stride_div; + u32 plane_ctl, stride_div, stride; int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey; unsigned long surf_addr; + u32 tile_height, plane_offset, plane_size; + unsigned int rotation; + int x_offset, y_offset; plane_ctl = PLANE_CTL_ENABLE | PLANE_CTL_PIPE_CSC_ENABLE; @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, MISSING_CASE(fb->modifier[0]); } - if (drm_plane->state->rotation == BIT(DRM_ROTATE_180)) + rotation = drm_plane->state->rotation; + switch (rotation) { + case BIT(DRM_ROTATE_90): + plane_ctl |= PLANE_CTL_ROTATE_90; + break; + + case BIT(DRM_ROTATE_180): plane_ctl |= PLANE_CTL_ROTATE_180; + break; + + case BIT(DRM_ROTATE_270): + plane_ctl |= PLANE_CTL_ROTATE_270; + break; + } intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h, pixel_size, true, @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, surf_addr = intel_plane_obj_offset(intel_plane, obj); - I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x); - I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div); + if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { + /* stride: Surface height in tiles */ + tile_height = intel_tile_height(dev, fb->bits_per_pixel, + fb->modifier[0]); + stride = DIV_ROUND_UP(fb->height, tile_height); + plane_size = (src_w << 16) | src_h; + x_offset = stride * tile_height - y - (src_h + 1); + y_offset = x; + } else { + stride = fb->pitches[0] / stride_div; + plane_size = (src_h << 16) | src_w; + x_offset = x; + y_offset = y; + } + plane_offset = y_offset << 16 | x_offset; + + I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset); + I915_WRITE(PLANE_STRIDE(pipe, plane), stride); I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x); - I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w); + I915_WRITE(PLANE_SIZE(pipe, plane), plane_size); I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); I915_WRITE(PLANE_SURF(pipe, plane), surf_addr); POSTING_READ(PLANE_SURF(pipe, plane)); @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) goto out; } - if (!dev->mode_config.rotation_property) - dev->mode_config.rotation_property = - drm_mode_create_rotation_property(dev, - BIT(DRM_ROTATE_0) | - BIT(DRM_ROTATE_180)); - - if (dev->mode_config.rotation_property) - drm_object_attach_property(&intel_plane->base.base, - dev->mode_config.rotation_property, - state->base.rotation); + intel_create_rotation_property(dev, intel_plane); drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); -- 1.7.10.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation 2015-03-30 8:34 ` [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation Sonika Jindal @ 2015-03-30 12:15 ` shuang.he 2015-04-01 18:22 ` Matt Roper 1 sibling, 0 replies; 28+ messages in thread From: shuang.he @ 2015-03-30 12:15 UTC (permalink / raw) To: shuang.he, ethan.gao, intel-gfx, sonika.jindal Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) Task id: 6091 -------------------------------------Summary------------------------------------- Platform Delta drm-intel-nightly Series Applied PNV -4 270/270 266/270 ILK 303/303 303/303 SNB -1 304/304 303/304 IVB 337/337 337/337 BYT 287/287 287/287 HSW 361/361 361/361 BDW 309/309 309/309 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied PNV igt@gem_userptr_blits@coherency-sync CRASH(2)PASS(2) CRASH(2) *PNV igt@gem_fence_thrash@bo-write-verify-threaded-none PASS(2) FAIL(1)PASS(1) *PNV igt@gem_fence_thrash@bo-write-verify-x PASS(2) FAIL(1)PASS(1) *PNV igt@gem_fence_thrash@bo-write-verify-y PASS(2) FAIL(1)PASS(1) *SNB igt@kms_rotation_crc@sprite-rotation PASS(3) FAIL(1)PASS(1) Note: You need to pay more attention to line start with '*' _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation 2015-03-30 8:34 ` [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation Sonika Jindal 2015-03-30 12:15 ` shuang.he @ 2015-04-01 18:22 ` Matt Roper 2015-04-02 4:54 ` Jindal, Sonika 1 sibling, 1 reply; 28+ messages in thread From: Matt Roper @ 2015-04-01 18:22 UTC (permalink / raw) To: Sonika Jindal; +Cc: intel-gfx On Mon, Mar 30, 2015 at 02:04:57PM +0530, Sonika Jindal wrote: > v2: Moving creation of property in a function, checking for 90/270 > rotation simultaneously (Chris) > Letting primary plane to be positioned > v3: Adding if/else for 90/270 and rest params programming, adding check for > pixel_format, some cleanup (review comments) > v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset > and size programming (Ville) > v5: Rebased on -nightly and Tvrtko's series for gtt remapping. > v6: Rebased on -nightly (Tvrtko's series merged) > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 2 + > drivers/gpu/drm/i915/intel_display.c | 103 +++++++++++++++++++++++++++------- > drivers/gpu/drm/i915/intel_drv.h | 6 ++ > drivers/gpu/drm/i915/intel_sprite.c | 52 ++++++++++++----- > 4 files changed, 129 insertions(+), 34 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index b522eb6..564bbd5 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells { > #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4) > #define PLANE_CTL_ROTATE_MASK 0x3 > #define PLANE_CTL_ROTATE_0 0x0 > +#define PLANE_CTL_ROTATE_90 0x1 > #define PLANE_CTL_ROTATE_180 0x2 > +#define PLANE_CTL_ROTATE_270 0x3 > #define _PLANE_STRIDE_1_A 0x70188 > #define _PLANE_STRIDE_2_A 0x70288 > #define _PLANE_STRIDE_3_A 0x70388 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f0bbc22..86ee0f0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, > return -EINVAL; > } > > + switch (fb->pixel_format) { > + case DRM_FORMAT_XRGB8888: > + case DRM_FORMAT_ARGB8888: > + case DRM_FORMAT_XBGR8888: > + case DRM_FORMAT_ABGR8888: > + case DRM_FORMAT_XRGB2101010: > + case DRM_FORMAT_ARGB2101010: > + case DRM_FORMAT_XBGR2101010: > + case DRM_FORMAT_ABGR2101010: > + case DRM_FORMAT_YUYV: > + case DRM_FORMAT_YVYU: > + case DRM_FORMAT_UYVY: > + case DRM_FORMAT_VYUY: > + case DRM_FORMAT_NV12: > + break; > + > + default: > + DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 rotation!\n", > + fb->pixel_format); > + return -EINVAL; > + } Shouldn't we be matching against the list of formats the plane supports (which may vary by platform, or by specific plane) rather than this generic list? We already specified what formats the plane can handle at plane init time, so it seems like what you'd really want is just a call to drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format) then follow that up with explicit checks to exclude any formats that we can handle in 0/180, but not in 90/270. I'd also move this check to intel_plane_atomic_check(), since the 'check' code path is where I'd usually go looking for these types of checks; the function you've got it in at the moment gets called from the 'prepare' step which works as well, but seems a bit less obvious. > + > return 0; > } > > @@ -2919,8 +2941,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct drm_i915_gem_object *obj; > int pipe = intel_crtc->pipe; > - u32 plane_ctl, stride_div; > + u32 plane_ctl, stride_div, stride; > + u32 tile_height, plane_offset, plane_size; > + unsigned int rotation; > + int x_offset, y_offset; > unsigned long surf_addr; > + struct drm_plane *plane; > > if (!intel_crtc->primary_enabled) { > I915_WRITE(PLANE_CTL(pipe, 0), 0); > @@ -2981,21 +3007,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > } > > plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE; > - if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) > + > + plane = crtc->primary; > + rotation = plane->state->rotation; > + switch (rotation) { > + case BIT(DRM_ROTATE_90): > + plane_ctl |= PLANE_CTL_ROTATE_90; > + break; > + > + case BIT(DRM_ROTATE_180): > plane_ctl |= PLANE_CTL_ROTATE_180; > + break; > + > + case BIT(DRM_ROTATE_270): > + plane_ctl |= PLANE_CTL_ROTATE_270; > + break; > + } > > obj = intel_fb_obj(fb); > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > fb->pixel_format); Minor nit; can we move this down inside the 'else' branch to make it apparent how this serves a parallel role to 'tile_height' from the 'if' branch. > - surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj); > + surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj); > + > + if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { Might as well use the intel_rotation_90_or_270() function since we have it. Same comment for the sprite path farther down. > + /* stride = Surface height in tiles */ > + tile_height = intel_tile_height(dev, fb->bits_per_pixel, > + fb->modifier[0]); > + stride = DIV_ROUND_UP(fb->height, tile_height); > + x_offset = stride * tile_height - y - (plane->state->src_h >> 16); > + y_offset = x; > + plane_size = ((plane->state->src_w >> 16) - 1) << 16 | > + ((plane->state->src_h >> 16) - 1); > + } else { > + stride = fb->pitches[0] / stride_div; > + x_offset = x; > + y_offset = y; > + plane_size = ((plane->state->src_h >> 16) - 1) << 16 | > + ((plane->state->src_w >> 16) - 1); > + } > + plane_offset = y_offset << 16 | x_offset; > > I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); > I915_WRITE(PLANE_POS(pipe, 0), 0); > - I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x); > - I915_WRITE(PLANE_SIZE(pipe, 0), > - (intel_crtc->config->pipe_src_h - 1) << 16 | > - (intel_crtc->config->pipe_src_w - 1)); > - I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div); > + I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); > + I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); > + I915_WRITE(PLANE_STRIDE(pipe, 0), stride); > I915_WRITE(PLANE_SURF(pipe, 0), surf_addr); > > POSTING_READ(PLANE_SURF(pipe, 0)); > @@ -12406,23 +12462,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > intel_primary_formats, num_formats, > DRM_PLANE_TYPE_PRIMARY); > > - if (INTEL_INFO(dev)->gen >= 4) { > - if (!dev->mode_config.rotation_property) > - dev->mode_config.rotation_property = > - drm_mode_create_rotation_property(dev, > - BIT(DRM_ROTATE_0) | > - BIT(DRM_ROTATE_180)); > - if (dev->mode_config.rotation_property) > - drm_object_attach_property(&primary->base.base, > - dev->mode_config.rotation_property, > - state->base.rotation); > - } > + if (INTEL_INFO(dev)->gen >= 4) > + intel_create_rotation_property(dev, primary); > > drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs); > > return &primary->base; > } > > +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane) > +{ > + if (!dev->mode_config.rotation_property) { > + unsigned long flags = BIT(DRM_ROTATE_0) | > + BIT(DRM_ROTATE_180); > + > + if (INTEL_INFO(dev)->gen >= 9) > + flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270); > + > + dev->mode_config.rotation_property = > + drm_mode_create_rotation_property(dev, flags); > + } > + if (dev->mode_config.rotation_property) > + drm_object_attach_property(&plane->base.base, > + dev->mode_config.rotation_property, > + plane->base.state->rotation); > +} > + > static int > intel_check_cursor_plane(struct drm_plane *plane, > struct intel_plane_state *state) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 811a1db..d32025a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation) > return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)); > } > > +unsigned int > +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel, > + uint64_t fb_modifier); > +void intel_create_rotation_property(struct drm_device *dev, > + struct intel_plane *plane); > + > bool intel_wm_need_update(struct drm_plane *plane, > struct drm_plane_state *state); > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index f41e872..65eb147 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > const int pipe = intel_plane->pipe; > const int plane = intel_plane->plane + 1; > - u32 plane_ctl, stride_div; > + u32 plane_ctl, stride_div, stride; > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey; > unsigned long surf_addr; > + u32 tile_height, plane_offset, plane_size; > + unsigned int rotation; > + int x_offset, y_offset; > > plane_ctl = PLANE_CTL_ENABLE | > PLANE_CTL_PIPE_CSC_ENABLE; > @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > MISSING_CASE(fb->modifier[0]); > } > > - if (drm_plane->state->rotation == BIT(DRM_ROTATE_180)) > + rotation = drm_plane->state->rotation; > + switch (rotation) { > + case BIT(DRM_ROTATE_90): > + plane_ctl |= PLANE_CTL_ROTATE_90; > + break; > + > + case BIT(DRM_ROTATE_180): > plane_ctl |= PLANE_CTL_ROTATE_180; > + break; > + > + case BIT(DRM_ROTATE_270): > + plane_ctl |= PLANE_CTL_ROTATE_270; > + break; > + } > > intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h, > pixel_size, true, > @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > > surf_addr = intel_plane_obj_offset(intel_plane, obj); > > - I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x); > - I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div); > + if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { > + /* stride: Surface height in tiles */ > + tile_height = intel_tile_height(dev, fb->bits_per_pixel, > + fb->modifier[0]); > + stride = DIV_ROUND_UP(fb->height, tile_height); > + plane_size = (src_w << 16) | src_h; > + x_offset = stride * tile_height - y - (src_h + 1); > + y_offset = x; > + } else { > + stride = fb->pitches[0] / stride_div; > + plane_size = (src_h << 16) | src_w; > + x_offset = x; > + y_offset = y; > + } > + plane_offset = y_offset << 16 | x_offset; > + > + I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset); > + I915_WRITE(PLANE_STRIDE(pipe, plane), stride); > I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x); > - I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w); > + I915_WRITE(PLANE_SIZE(pipe, plane), plane_size); > I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); > I915_WRITE(PLANE_SURF(pipe, plane), surf_addr); > POSTING_READ(PLANE_SURF(pipe, plane)); > @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > goto out; > } > > - if (!dev->mode_config.rotation_property) > - dev->mode_config.rotation_property = > - drm_mode_create_rotation_property(dev, > - BIT(DRM_ROTATE_0) | > - BIT(DRM_ROTATE_180)); > - > - if (dev->mode_config.rotation_property) > - drm_object_attach_property(&intel_plane->base.base, > - dev->mode_config.rotation_property, > - state->base.rotation); > + intel_create_rotation_property(dev, intel_plane); > > drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); > > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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] 28+ messages in thread
* Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation 2015-04-01 18:22 ` Matt Roper @ 2015-04-02 4:54 ` Jindal, Sonika 2015-04-02 8:55 ` Tvrtko Ursulin 2015-04-02 15:59 ` Matt Roper 0 siblings, 2 replies; 28+ messages in thread From: Jindal, Sonika @ 2015-04-02 4:54 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On 4/1/2015 11:52 PM, Matt Roper wrote: > On Mon, Mar 30, 2015 at 02:04:57PM +0530, Sonika Jindal wrote: >> v2: Moving creation of property in a function, checking for 90/270 >> rotation simultaneously (Chris) >> Letting primary plane to be positioned >> v3: Adding if/else for 90/270 and rest params programming, adding check for >> pixel_format, some cleanup (review comments) >> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset >> and size programming (Ville) >> v5: Rebased on -nightly and Tvrtko's series for gtt remapping. >> v6: Rebased on -nightly (Tvrtko's series merged) >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 2 + >> drivers/gpu/drm/i915/intel_display.c | 103 +++++++++++++++++++++++++++------- >> drivers/gpu/drm/i915/intel_drv.h | 6 ++ >> drivers/gpu/drm/i915/intel_sprite.c | 52 ++++++++++++----- >> 4 files changed, 129 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index b522eb6..564bbd5 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells { >> #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4) >> #define PLANE_CTL_ROTATE_MASK 0x3 >> #define PLANE_CTL_ROTATE_0 0x0 >> +#define PLANE_CTL_ROTATE_90 0x1 >> #define PLANE_CTL_ROTATE_180 0x2 >> +#define PLANE_CTL_ROTATE_270 0x3 >> #define _PLANE_STRIDE_1_A 0x70188 >> #define _PLANE_STRIDE_2_A 0x70288 >> #define _PLANE_STRIDE_3_A 0x70388 >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index f0bbc22..86ee0f0 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, >> return -EINVAL; >> } >> >> + switch (fb->pixel_format) { >> + case DRM_FORMAT_XRGB8888: >> + case DRM_FORMAT_ARGB8888: >> + case DRM_FORMAT_XBGR8888: >> + case DRM_FORMAT_ABGR8888: >> + case DRM_FORMAT_XRGB2101010: >> + case DRM_FORMAT_ARGB2101010: >> + case DRM_FORMAT_XBGR2101010: >> + case DRM_FORMAT_ABGR2101010: >> + case DRM_FORMAT_YUYV: >> + case DRM_FORMAT_YVYU: >> + case DRM_FORMAT_UYVY: >> + case DRM_FORMAT_VYUY: >> + case DRM_FORMAT_NV12: >> + break; >> + >> + default: >> + DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 rotation!\n", >> + fb->pixel_format); >> + return -EINVAL; >> + } > > Shouldn't we be matching against the list of formats the plane supports > (which may vary by platform, or by specific plane) rather than this > generic list? We already specified what formats the plane can handle at > plane init time, so it seems like what you'd really want is just a call > to > > drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format) > > then follow that up with explicit checks to exclude any formats that we > can handle in 0/180, but not in 90/270. > I am not sure how it will help. drm_plane_check_pixel_format should be used to check the pixel format of the fb which we should be doing in some -check functions (I don't think we do that right now?) against what is supported by the plane. But to check for the formats which are allowed for 90/270, we would need this kind of explicit check. > I'd also move this check to intel_plane_atomic_check(), since the > 'check' code path is where I'd usually go looking for these types of > checks; the function you've got it in at the moment gets called from the > 'prepare' step which works as well, but seems a bit less obvious. > Yes, I agree, but this is on top of Tvrtko's patch for secondary buffer mapping where based upon tiling and pixel format we are allowing the rotated gtt. Tvrtko, Can these be moved to the intel_plane_atomic_check() >> + >> return 0; >> } >> >> @@ -2919,8 +2941,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> struct drm_i915_gem_object *obj; >> int pipe = intel_crtc->pipe; >> - u32 plane_ctl, stride_div; >> + u32 plane_ctl, stride_div, stride; >> + u32 tile_height, plane_offset, plane_size; >> + unsigned int rotation; >> + int x_offset, y_offset; >> unsigned long surf_addr; >> + struct drm_plane *plane; >> >> if (!intel_crtc->primary_enabled) { >> I915_WRITE(PLANE_CTL(pipe, 0), 0); >> @@ -2981,21 +3007,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, >> } >> >> plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE; >> - if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) >> + >> + plane = crtc->primary; >> + rotation = plane->state->rotation; >> + switch (rotation) { >> + case BIT(DRM_ROTATE_90): >> + plane_ctl |= PLANE_CTL_ROTATE_90; >> + break; >> + >> + case BIT(DRM_ROTATE_180): >> plane_ctl |= PLANE_CTL_ROTATE_180; >> + break; >> + >> + case BIT(DRM_ROTATE_270): >> + plane_ctl |= PLANE_CTL_ROTATE_270; >> + break; >> + } >> >> obj = intel_fb_obj(fb); >> stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], >> fb->pixel_format); > > Minor nit; can we move this down inside the 'else' branch to make it > apparent how this serves a parallel role to 'tile_height' from the 'if' > branch. > Yes sure. > >> - surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj); >> + surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj); >> + >> + if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { > > Might as well use the intel_rotation_90_or_270() function since we have > it. Same comment for the sprite path farther down. > Sure. Regards, Sonika >> + /* stride = Surface height in tiles */ >> + tile_height = intel_tile_height(dev, fb->bits_per_pixel, >> + fb->modifier[0]); >> + stride = DIV_ROUND_UP(fb->height, tile_height); >> + x_offset = stride * tile_height - y - (plane->state->src_h >> 16); >> + y_offset = x; >> + plane_size = ((plane->state->src_w >> 16) - 1) << 16 | >> + ((plane->state->src_h >> 16) - 1); >> + } else { >> + stride = fb->pitches[0] / stride_div; >> + x_offset = x; >> + y_offset = y; >> + plane_size = ((plane->state->src_h >> 16) - 1) << 16 | >> + ((plane->state->src_w >> 16) - 1); >> + } >> + plane_offset = y_offset << 16 | x_offset; >> >> I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); >> I915_WRITE(PLANE_POS(pipe, 0), 0); >> - I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x); >> - I915_WRITE(PLANE_SIZE(pipe, 0), >> - (intel_crtc->config->pipe_src_h - 1) << 16 | >> - (intel_crtc->config->pipe_src_w - 1)); >> - I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div); >> + I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); >> + I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); >> + I915_WRITE(PLANE_STRIDE(pipe, 0), stride); >> I915_WRITE(PLANE_SURF(pipe, 0), surf_addr); >> >> POSTING_READ(PLANE_SURF(pipe, 0)); >> @@ -12406,23 +12462,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, >> intel_primary_formats, num_formats, >> DRM_PLANE_TYPE_PRIMARY); >> >> - if (INTEL_INFO(dev)->gen >= 4) { >> - if (!dev->mode_config.rotation_property) >> - dev->mode_config.rotation_property = >> - drm_mode_create_rotation_property(dev, >> - BIT(DRM_ROTATE_0) | >> - BIT(DRM_ROTATE_180)); >> - if (dev->mode_config.rotation_property) >> - drm_object_attach_property(&primary->base.base, >> - dev->mode_config.rotation_property, >> - state->base.rotation); >> - } >> + if (INTEL_INFO(dev)->gen >= 4) >> + intel_create_rotation_property(dev, primary); >> >> drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs); >> >> return &primary->base; >> } >> >> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane) >> +{ >> + if (!dev->mode_config.rotation_property) { >> + unsigned long flags = BIT(DRM_ROTATE_0) | >> + BIT(DRM_ROTATE_180); >> + >> + if (INTEL_INFO(dev)->gen >= 9) >> + flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270); >> + >> + dev->mode_config.rotation_property = >> + drm_mode_create_rotation_property(dev, flags); >> + } >> + if (dev->mode_config.rotation_property) >> + drm_object_attach_property(&plane->base.base, >> + dev->mode_config.rotation_property, >> + plane->base.state->rotation); >> +} >> + >> static int >> intel_check_cursor_plane(struct drm_plane *plane, >> struct intel_plane_state *state) >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 811a1db..d32025a 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation) >> return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)); >> } >> >> +unsigned int >> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel, >> + uint64_t fb_modifier); >> +void intel_create_rotation_property(struct drm_device *dev, >> + struct intel_plane *plane); >> + >> bool intel_wm_need_update(struct drm_plane *plane, >> struct drm_plane_state *state); >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index f41e872..65eb147 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, >> struct drm_i915_gem_object *obj = intel_fb_obj(fb); >> const int pipe = intel_plane->pipe; >> const int plane = intel_plane->plane + 1; >> - u32 plane_ctl, stride_div; >> + u32 plane_ctl, stride_div, stride; >> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); >> const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey; >> unsigned long surf_addr; >> + u32 tile_height, plane_offset, plane_size; >> + unsigned int rotation; >> + int x_offset, y_offset; >> >> plane_ctl = PLANE_CTL_ENABLE | >> PLANE_CTL_PIPE_CSC_ENABLE; >> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, >> MISSING_CASE(fb->modifier[0]); >> } >> >> - if (drm_plane->state->rotation == BIT(DRM_ROTATE_180)) >> + rotation = drm_plane->state->rotation; >> + switch (rotation) { >> + case BIT(DRM_ROTATE_90): >> + plane_ctl |= PLANE_CTL_ROTATE_90; >> + break; >> + >> + case BIT(DRM_ROTATE_180): >> plane_ctl |= PLANE_CTL_ROTATE_180; >> + break; >> + >> + case BIT(DRM_ROTATE_270): >> + plane_ctl |= PLANE_CTL_ROTATE_270; >> + break; >> + } >> >> intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h, >> pixel_size, true, >> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, >> >> surf_addr = intel_plane_obj_offset(intel_plane, obj); >> >> - I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x); >> - I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div); >> + if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { >> + /* stride: Surface height in tiles */ >> + tile_height = intel_tile_height(dev, fb->bits_per_pixel, >> + fb->modifier[0]); >> + stride = DIV_ROUND_UP(fb->height, tile_height); >> + plane_size = (src_w << 16) | src_h; >> + x_offset = stride * tile_height - y - (src_h + 1); >> + y_offset = x; >> + } else { >> + stride = fb->pitches[0] / stride_div; >> + plane_size = (src_h << 16) | src_w; >> + x_offset = x; >> + y_offset = y; >> + } >> + plane_offset = y_offset << 16 | x_offset; >> + >> + I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset); >> + I915_WRITE(PLANE_STRIDE(pipe, plane), stride); >> I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x); >> - I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w); >> + I915_WRITE(PLANE_SIZE(pipe, plane), plane_size); >> I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); >> I915_WRITE(PLANE_SURF(pipe, plane), surf_addr); >> POSTING_READ(PLANE_SURF(pipe, plane)); >> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) >> goto out; >> } >> >> - if (!dev->mode_config.rotation_property) >> - dev->mode_config.rotation_property = >> - drm_mode_create_rotation_property(dev, >> - BIT(DRM_ROTATE_0) | >> - BIT(DRM_ROTATE_180)); >> - >> - if (dev->mode_config.rotation_property) >> - drm_object_attach_property(&intel_plane->base.base, >> - dev->mode_config.rotation_property, >> - state->base.rotation); >> + intel_create_rotation_property(dev, intel_plane); >> >> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); >> >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation 2015-04-02 4:54 ` Jindal, Sonika @ 2015-04-02 8:55 ` Tvrtko Ursulin 2015-04-02 15:59 ` Matt Roper 1 sibling, 0 replies; 28+ messages in thread From: Tvrtko Ursulin @ 2015-04-02 8:55 UTC (permalink / raw) To: Jindal, Sonika, Matt Roper; +Cc: intel-gfx Hi, On 04/02/2015 05:54 AM, Jindal, Sonika wrote: >>> diff --git a/drivers/gpu/drm/i915/intel_display.c >>> b/drivers/gpu/drm/i915/intel_display.c >>> index f0bbc22..86ee0f0 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view >>> *view, struct drm_framebuffer *fb, >>> return -EINVAL; >>> } >>> >>> + switch (fb->pixel_format) { >>> + case DRM_FORMAT_XRGB8888: >>> + case DRM_FORMAT_ARGB8888: >>> + case DRM_FORMAT_XBGR8888: >>> + case DRM_FORMAT_ABGR8888: >>> + case DRM_FORMAT_XRGB2101010: >>> + case DRM_FORMAT_ARGB2101010: >>> + case DRM_FORMAT_XBGR2101010: >>> + case DRM_FORMAT_ABGR2101010: >>> + case DRM_FORMAT_YUYV: >>> + case DRM_FORMAT_YVYU: >>> + case DRM_FORMAT_UYVY: >>> + case DRM_FORMAT_VYUY: >>> + case DRM_FORMAT_NV12: >>> + break; >>> + >>> + default: >>> + DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 >>> rotation!\n", >>> + fb->pixel_format); >>> + return -EINVAL; >>> + } >> >> Shouldn't we be matching against the list of formats the plane supports >> (which may vary by platform, or by specific plane) rather than this >> generic list? We already specified what formats the plane can handle at >> plane init time, so it seems like what you'd really want is just a call >> to >> >> drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format) >> >> then follow that up with explicit checks to exclude any formats that we >> can handle in 0/180, but not in 90/270. >> > I am not sure how it will help. drm_plane_check_pixel_format should be > used to check the pixel format of the fb which we should be doing in > some -check functions (I don't think we do that right now?) against what > is supported by the plane. > But to check for the formats which are allowed for 90/270, we would need > this kind of explicit check. > >> I'd also move this check to intel_plane_atomic_check(), since the >> 'check' code path is where I'd usually go looking for these types of >> checks; the function you've got it in at the moment gets called from the >> 'prepare' step which works as well, but seems a bit less obvious. >> > Yes, I agree, but this is on top of Tvrtko's patch for secondary buffer > mapping where based upon tiling and pixel format we are allowing the > rotated gtt. > > Tvrtko, > Can these be moved to the intel_plane_atomic_check() Good point, I think it can and should. I suppose it was just an oversight during endless rebasing, that I put the Y tiling check in there. So you can move that part as well while you are doing it. Also highlights the fact we have no negative testing in kms_rotation_crc for this. I mean, trying to rotate by 90/270 linear or X tiled, or wrong pixel format. Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation 2015-04-02 4:54 ` Jindal, Sonika 2015-04-02 8:55 ` Tvrtko Ursulin @ 2015-04-02 15:59 ` Matt Roper 2015-04-06 12:27 ` Jindal, Sonika 2015-04-07 8:13 ` Daniel Vetter 1 sibling, 2 replies; 28+ messages in thread From: Matt Roper @ 2015-04-02 15:59 UTC (permalink / raw) To: Jindal, Sonika; +Cc: intel-gfx On Thu, Apr 02, 2015 at 10:24:02AM +0530, Jindal, Sonika wrote: > > > On 4/1/2015 11:52 PM, Matt Roper wrote: > >On Mon, Mar 30, 2015 at 02:04:57PM +0530, Sonika Jindal wrote: > >>v2: Moving creation of property in a function, checking for 90/270 > >>rotation simultaneously (Chris) > >>Letting primary plane to be positioned > >>v3: Adding if/else for 90/270 and rest params programming, adding check for > >>pixel_format, some cleanup (review comments) > >>v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset > >>and size programming (Ville) > >>v5: Rebased on -nightly and Tvrtko's series for gtt remapping. > >>v6: Rebased on -nightly (Tvrtko's series merged) > >> > >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >>--- > >> drivers/gpu/drm/i915/i915_reg.h | 2 + > >> drivers/gpu/drm/i915/intel_display.c | 103 +++++++++++++++++++++++++++------- > >> drivers/gpu/drm/i915/intel_drv.h | 6 ++ > >> drivers/gpu/drm/i915/intel_sprite.c | 52 ++++++++++++----- > >> 4 files changed, 129 insertions(+), 34 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >>index b522eb6..564bbd5 100644 > >>--- a/drivers/gpu/drm/i915/i915_reg.h > >>+++ b/drivers/gpu/drm/i915/i915_reg.h > >>@@ -4854,7 +4854,9 @@ enum skl_disp_power_wells { > >> #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4) > >> #define PLANE_CTL_ROTATE_MASK 0x3 > >> #define PLANE_CTL_ROTATE_0 0x0 > >>+#define PLANE_CTL_ROTATE_90 0x1 > >> #define PLANE_CTL_ROTATE_180 0x2 > >>+#define PLANE_CTL_ROTATE_270 0x3 > >> #define _PLANE_STRIDE_1_A 0x70188 > >> #define _PLANE_STRIDE_2_A 0x70288 > >> #define _PLANE_STRIDE_3_A 0x70388 > >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>index f0bbc22..86ee0f0 100644 > >>--- a/drivers/gpu/drm/i915/intel_display.c > >>+++ b/drivers/gpu/drm/i915/intel_display.c > >>@@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, > >> return -EINVAL; > >> } > >> > >>+ switch (fb->pixel_format) { > >>+ case DRM_FORMAT_XRGB8888: > >>+ case DRM_FORMAT_ARGB8888: > >>+ case DRM_FORMAT_XBGR8888: > >>+ case DRM_FORMAT_ABGR8888: > >>+ case DRM_FORMAT_XRGB2101010: > >>+ case DRM_FORMAT_ARGB2101010: > >>+ case DRM_FORMAT_XBGR2101010: > >>+ case DRM_FORMAT_ABGR2101010: > >>+ case DRM_FORMAT_YUYV: > >>+ case DRM_FORMAT_YVYU: > >>+ case DRM_FORMAT_UYVY: > >>+ case DRM_FORMAT_VYUY: > >>+ case DRM_FORMAT_NV12: > >>+ break; > >>+ > >>+ default: > >>+ DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 rotation!\n", > >>+ fb->pixel_format); > >>+ return -EINVAL; > >>+ } > > > >Shouldn't we be matching against the list of formats the plane supports > >(which may vary by platform, or by specific plane) rather than this > >generic list? We already specified what formats the plane can handle at > >plane init time, so it seems like what you'd really want is just a call > >to > > > > drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format) > > > >then follow that up with explicit checks to exclude any formats that we > >can handle in 0/180, but not in 90/270. > > > I am not sure how it will help. drm_plane_check_pixel_format should > be used to check the pixel format of the fb which we should be doing > in some -check functions (I don't think we do that right now?) > against what is supported by the plane. > But to check for the formats which are allowed for 90/270, we would > need this kind of explicit check. Right, I guess there are two aspects here. First, we need to properly test for acceptable pixel formats for the plane in general; at the moment the DRM core setplane() tests this, but if we use the atomic ioctl it never gets checked (which is a bug). So as you say, we need a test in a _check() function to verify this. We probably also need to add an i-g-t test for it too. Once we know that the pixel format is valid in general, it makes sense to have a simpler test to reject some subset of those formats iff we notice we're doing 90/270 rotation. Maybe it's not really a big deal, but it seems like that's a little easier to understand and verify than having two completely separate lists, especially when future platforms may support different formats, or even different planes of the same platform have varying pixel format capabilities. Matt > >I'd also move this check to intel_plane_atomic_check(), since the > >'check' code path is where I'd usually go looking for these types of > >checks; the function you've got it in at the moment gets called from the > >'prepare' step which works as well, but seems a bit less obvious. > > > Yes, I agree, but this is on top of Tvrtko's patch for secondary > buffer mapping where based upon tiling and pixel format we are > allowing the rotated gtt. > > Tvrtko, > Can these be moved to the intel_plane_atomic_check() > > >>+ > >> return 0; > >> } > >> > >>@@ -2919,8 +2941,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > >> struct drm_i915_gem_object *obj; > >> int pipe = intel_crtc->pipe; > >>- u32 plane_ctl, stride_div; > >>+ u32 plane_ctl, stride_div, stride; > >>+ u32 tile_height, plane_offset, plane_size; > >>+ unsigned int rotation; > >>+ int x_offset, y_offset; > >> unsigned long surf_addr; > >>+ struct drm_plane *plane; > >> > >> if (!intel_crtc->primary_enabled) { > >> I915_WRITE(PLANE_CTL(pipe, 0), 0); > >>@@ -2981,21 +3007,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > >> } > >> > >> plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE; > >>- if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) > >>+ > >>+ plane = crtc->primary; > >>+ rotation = plane->state->rotation; > >>+ switch (rotation) { > >>+ case BIT(DRM_ROTATE_90): > >>+ plane_ctl |= PLANE_CTL_ROTATE_90; > >>+ break; > >>+ > >>+ case BIT(DRM_ROTATE_180): > >> plane_ctl |= PLANE_CTL_ROTATE_180; > >>+ break; > >>+ > >>+ case BIT(DRM_ROTATE_270): > >>+ plane_ctl |= PLANE_CTL_ROTATE_270; > >>+ break; > >>+ } > >> > >> obj = intel_fb_obj(fb); > >> stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > >> fb->pixel_format); > > > >Minor nit; can we move this down inside the 'else' branch to make it > >apparent how this serves a parallel role to 'tile_height' from the 'if' > >branch. > > > Yes sure. > > > >>- surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj); > >>+ surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj); > >>+ > >>+ if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { > > > >Might as well use the intel_rotation_90_or_270() function since we have > >it. Same comment for the sprite path farther down. > > > Sure. > > Regards, > Sonika > >>+ /* stride = Surface height in tiles */ > >>+ tile_height = intel_tile_height(dev, fb->bits_per_pixel, > >>+ fb->modifier[0]); > >>+ stride = DIV_ROUND_UP(fb->height, tile_height); > >>+ x_offset = stride * tile_height - y - (plane->state->src_h >> 16); > >>+ y_offset = x; > >>+ plane_size = ((plane->state->src_w >> 16) - 1) << 16 | > >>+ ((plane->state->src_h >> 16) - 1); > >>+ } else { > >>+ stride = fb->pitches[0] / stride_div; > >>+ x_offset = x; > >>+ y_offset = y; > >>+ plane_size = ((plane->state->src_h >> 16) - 1) << 16 | > >>+ ((plane->state->src_w >> 16) - 1); > >>+ } > >>+ plane_offset = y_offset << 16 | x_offset; > >> > >> I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); > >> I915_WRITE(PLANE_POS(pipe, 0), 0); > >>- I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x); > >>- I915_WRITE(PLANE_SIZE(pipe, 0), > >>- (intel_crtc->config->pipe_src_h - 1) << 16 | > >>- (intel_crtc->config->pipe_src_w - 1)); > >>- I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div); > >>+ I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); > >>+ I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); > >>+ I915_WRITE(PLANE_STRIDE(pipe, 0), stride); > >> I915_WRITE(PLANE_SURF(pipe, 0), surf_addr); > >> > >> POSTING_READ(PLANE_SURF(pipe, 0)); > >>@@ -12406,23 +12462,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > >> intel_primary_formats, num_formats, > >> DRM_PLANE_TYPE_PRIMARY); > >> > >>- if (INTEL_INFO(dev)->gen >= 4) { > >>- if (!dev->mode_config.rotation_property) > >>- dev->mode_config.rotation_property = > >>- drm_mode_create_rotation_property(dev, > >>- BIT(DRM_ROTATE_0) | > >>- BIT(DRM_ROTATE_180)); > >>- if (dev->mode_config.rotation_property) > >>- drm_object_attach_property(&primary->base.base, > >>- dev->mode_config.rotation_property, > >>- state->base.rotation); > >>- } > >>+ if (INTEL_INFO(dev)->gen >= 4) > >>+ intel_create_rotation_property(dev, primary); > >> > >> drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs); > >> > >> return &primary->base; > >> } > >> > >>+void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane) > >>+{ > >>+ if (!dev->mode_config.rotation_property) { > >>+ unsigned long flags = BIT(DRM_ROTATE_0) | > >>+ BIT(DRM_ROTATE_180); > >>+ > >>+ if (INTEL_INFO(dev)->gen >= 9) > >>+ flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270); > >>+ > >>+ dev->mode_config.rotation_property = > >>+ drm_mode_create_rotation_property(dev, flags); > >>+ } > >>+ if (dev->mode_config.rotation_property) > >>+ drm_object_attach_property(&plane->base.base, > >>+ dev->mode_config.rotation_property, > >>+ plane->base.state->rotation); > >>+} > >>+ > >> static int > >> intel_check_cursor_plane(struct drm_plane *plane, > >> struct intel_plane_state *state) > >>diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >>index 811a1db..d32025a 100644 > >>--- a/drivers/gpu/drm/i915/intel_drv.h > >>+++ b/drivers/gpu/drm/i915/intel_drv.h > >>@@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation) > >> return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)); > >> } > >> > >>+unsigned int > >>+intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel, > >>+ uint64_t fb_modifier); > >>+void intel_create_rotation_property(struct drm_device *dev, > >>+ struct intel_plane *plane); > >>+ > >> bool intel_wm_need_update(struct drm_plane *plane, > >> struct drm_plane_state *state); > >> > >>diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > >>index f41e872..65eb147 100644 > >>--- a/drivers/gpu/drm/i915/intel_sprite.c > >>+++ b/drivers/gpu/drm/i915/intel_sprite.c > >>@@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > >> struct drm_i915_gem_object *obj = intel_fb_obj(fb); > >> const int pipe = intel_plane->pipe; > >> const int plane = intel_plane->plane + 1; > >>- u32 plane_ctl, stride_div; > >>+ u32 plane_ctl, stride_div, stride; > >> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > >> const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey; > >> unsigned long surf_addr; > >>+ u32 tile_height, plane_offset, plane_size; > >>+ unsigned int rotation; > >>+ int x_offset, y_offset; > >> > >> plane_ctl = PLANE_CTL_ENABLE | > >> PLANE_CTL_PIPE_CSC_ENABLE; > >>@@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > >> MISSING_CASE(fb->modifier[0]); > >> } > >> > >>- if (drm_plane->state->rotation == BIT(DRM_ROTATE_180)) > >>+ rotation = drm_plane->state->rotation; > >>+ switch (rotation) { > >>+ case BIT(DRM_ROTATE_90): > >>+ plane_ctl |= PLANE_CTL_ROTATE_90; > >>+ break; > >>+ > >>+ case BIT(DRM_ROTATE_180): > >> plane_ctl |= PLANE_CTL_ROTATE_180; > >>+ break; > >>+ > >>+ case BIT(DRM_ROTATE_270): > >>+ plane_ctl |= PLANE_CTL_ROTATE_270; > >>+ break; > >>+ } > >> > >> intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h, > >> pixel_size, true, > >>@@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > >> > >> surf_addr = intel_plane_obj_offset(intel_plane, obj); > >> > >>- I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x); > >>- I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div); > >>+ if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { > >>+ /* stride: Surface height in tiles */ > >>+ tile_height = intel_tile_height(dev, fb->bits_per_pixel, > >>+ fb->modifier[0]); > >>+ stride = DIV_ROUND_UP(fb->height, tile_height); > >>+ plane_size = (src_w << 16) | src_h; > >>+ x_offset = stride * tile_height - y - (src_h + 1); > >>+ y_offset = x; > >>+ } else { > >>+ stride = fb->pitches[0] / stride_div; > >>+ plane_size = (src_h << 16) | src_w; > >>+ x_offset = x; > >>+ y_offset = y; > >>+ } > >>+ plane_offset = y_offset << 16 | x_offset; > >>+ > >>+ I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset); > >>+ I915_WRITE(PLANE_STRIDE(pipe, plane), stride); > >> I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x); > >>- I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w); > >>+ I915_WRITE(PLANE_SIZE(pipe, plane), plane_size); > >> I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); > >> I915_WRITE(PLANE_SURF(pipe, plane), surf_addr); > >> POSTING_READ(PLANE_SURF(pipe, plane)); > >>@@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > >> goto out; > >> } > >> > >>- if (!dev->mode_config.rotation_property) > >>- dev->mode_config.rotation_property = > >>- drm_mode_create_rotation_property(dev, > >>- BIT(DRM_ROTATE_0) | > >>- BIT(DRM_ROTATE_180)); > >>- > >>- if (dev->mode_config.rotation_property) > >>- drm_object_attach_property(&intel_plane->base.base, > >>- dev->mode_config.rotation_property, > >>- state->base.rotation); > >>+ intel_create_rotation_property(dev, intel_plane); > >> > >> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); > >> > >>-- > >>1.7.10.4 > >> > >>_______________________________________________ > >>Intel-gfx mailing list > >>Intel-gfx@lists.freedesktop.org > >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- 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] 28+ messages in thread
* Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation 2015-04-02 15:59 ` Matt Roper @ 2015-04-06 12:27 ` Jindal, Sonika 2015-04-07 8:13 ` Daniel Vetter 1 sibling, 0 replies; 28+ messages in thread From: Jindal, Sonika @ 2015-04-06 12:27 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On 4/2/2015 9:29 PM, Matt Roper wrote: > On Thu, Apr 02, 2015 at 10:24:02AM +0530, Jindal, Sonika wrote: >> >> >> On 4/1/2015 11:52 PM, Matt Roper wrote: >>> On Mon, Mar 30, 2015 at 02:04:57PM +0530, Sonika Jindal wrote: >>>> v2: Moving creation of property in a function, checking for 90/270 >>>> rotation simultaneously (Chris) >>>> Letting primary plane to be positioned >>>> v3: Adding if/else for 90/270 and rest params programming, adding check for >>>> pixel_format, some cleanup (review comments) >>>> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset >>>> and size programming (Ville) >>>> v5: Rebased on -nightly and Tvrtko's series for gtt remapping. >>>> v6: Rebased on -nightly (Tvrtko's series merged) >>>> >>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_reg.h | 2 + >>>> drivers/gpu/drm/i915/intel_display.c | 103 +++++++++++++++++++++++++++------- >>>> drivers/gpu/drm/i915/intel_drv.h | 6 ++ >>>> drivers/gpu/drm/i915/intel_sprite.c | 52 ++++++++++++----- >>>> 4 files changed, 129 insertions(+), 34 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>> index b522eb6..564bbd5 100644 >>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells { >>>> #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4) >>>> #define PLANE_CTL_ROTATE_MASK 0x3 >>>> #define PLANE_CTL_ROTATE_0 0x0 >>>> +#define PLANE_CTL_ROTATE_90 0x1 >>>> #define PLANE_CTL_ROTATE_180 0x2 >>>> +#define PLANE_CTL_ROTATE_270 0x3 >>>> #define _PLANE_STRIDE_1_A 0x70188 >>>> #define _PLANE_STRIDE_2_A 0x70288 >>>> #define _PLANE_STRIDE_3_A 0x70388 >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index f0bbc22..86ee0f0 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, >>>> return -EINVAL; >>>> } >>>> >>>> + switch (fb->pixel_format) { >>>> + case DRM_FORMAT_XRGB8888: >>>> + case DRM_FORMAT_ARGB8888: >>>> + case DRM_FORMAT_XBGR8888: >>>> + case DRM_FORMAT_ABGR8888: >>>> + case DRM_FORMAT_XRGB2101010: >>>> + case DRM_FORMAT_ARGB2101010: >>>> + case DRM_FORMAT_XBGR2101010: >>>> + case DRM_FORMAT_ABGR2101010: >>>> + case DRM_FORMAT_YUYV: >>>> + case DRM_FORMAT_YVYU: >>>> + case DRM_FORMAT_UYVY: >>>> + case DRM_FORMAT_VYUY: >>>> + case DRM_FORMAT_NV12: >>>> + break; >>>> + >>>> + default: >>>> + DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 rotation!\n", >>>> + fb->pixel_format); >>>> + return -EINVAL; >>>> + } >>> >>> Shouldn't we be matching against the list of formats the plane supports >>> (which may vary by platform, or by specific plane) rather than this >>> generic list? We already specified what formats the plane can handle at >>> plane init time, so it seems like what you'd really want is just a call >>> to >>> >>> drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format) >>> >>> then follow that up with explicit checks to exclude any formats that we >>> can handle in 0/180, but not in 90/270. >>> >> I am not sure how it will help. drm_plane_check_pixel_format should >> be used to check the pixel format of the fb which we should be doing >> in some -check functions (I don't think we do that right now?) >> against what is supported by the plane. >> But to check for the formats which are allowed for 90/270, we would >> need this kind of explicit check. > > Right, I guess there are two aspects here. First, we need to properly > test for acceptable pixel formats for the plane in general; at the > moment the DRM core setplane() tests this, but if we use the atomic > ioctl it never gets checked (which is a bug). So as you say, we need a > test in a _check() function to verify this. We probably also need to > add an i-g-t test for it too. > OK, I'l add that check in intel_atomic_plane_check() > Once we know that the pixel format is valid in general, it makes sense > to have a simpler test to reject some subset of those formats iff we > notice we're doing 90/270 rotation. Maybe it's not really a big deal, > but it seems like that's a little easier to understand and verify than > having two completely separate lists, especially when future platforms > may support different formats, or even different planes of the same > platform have varying pixel format capabilities. > I will send out a patch with the changes and add negative test case in kms_rotation_crc. Regards, Sonika > > Matt > >>> I'd also move this check to intel_plane_atomic_check(), since the >>> 'check' code path is where I'd usually go looking for these types of >>> checks; the function you've got it in at the moment gets called from the >>> 'prepare' step which works as well, but seems a bit less obvious. >>> >> Yes, I agree, but this is on top of Tvrtko's patch for secondary >> buffer mapping where based upon tiling and pixel format we are >> allowing the rotated gtt. >> >> Tvrtko, >> Can these be moved to the intel_plane_atomic_check() >> >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -2919,8 +2941,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, >>>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>>> struct drm_i915_gem_object *obj; >>>> int pipe = intel_crtc->pipe; >>>> - u32 plane_ctl, stride_div; >>>> + u32 plane_ctl, stride_div, stride; >>>> + u32 tile_height, plane_offset, plane_size; >>>> + unsigned int rotation; >>>> + int x_offset, y_offset; >>>> unsigned long surf_addr; >>>> + struct drm_plane *plane; >>>> >>>> if (!intel_crtc->primary_enabled) { >>>> I915_WRITE(PLANE_CTL(pipe, 0), 0); >>>> @@ -2981,21 +3007,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, >>>> } >>>> >>>> plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE; >>>> - if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) >>>> + >>>> + plane = crtc->primary; >>>> + rotation = plane->state->rotation; >>>> + switch (rotation) { >>>> + case BIT(DRM_ROTATE_90): >>>> + plane_ctl |= PLANE_CTL_ROTATE_90; >>>> + break; >>>> + >>>> + case BIT(DRM_ROTATE_180): >>>> plane_ctl |= PLANE_CTL_ROTATE_180; >>>> + break; >>>> + >>>> + case BIT(DRM_ROTATE_270): >>>> + plane_ctl |= PLANE_CTL_ROTATE_270; >>>> + break; >>>> + } >>>> >>>> obj = intel_fb_obj(fb); >>>> stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], >>>> fb->pixel_format); >>> >>> Minor nit; can we move this down inside the 'else' branch to make it >>> apparent how this serves a parallel role to 'tile_height' from the 'if' >>> branch. >>> >> Yes sure. >>> >>>> - surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj); >>>> + surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj); >>>> + >>>> + if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { >>> >>> Might as well use the intel_rotation_90_or_270() function since we have >>> it. Same comment for the sprite path farther down. >>> >> Sure. >> >> Regards, >> Sonika >>>> + /* stride = Surface height in tiles */ >>>> + tile_height = intel_tile_height(dev, fb->bits_per_pixel, >>>> + fb->modifier[0]); >>>> + stride = DIV_ROUND_UP(fb->height, tile_height); >>>> + x_offset = stride * tile_height - y - (plane->state->src_h >> 16); >>>> + y_offset = x; >>>> + plane_size = ((plane->state->src_w >> 16) - 1) << 16 | >>>> + ((plane->state->src_h >> 16) - 1); >>>> + } else { >>>> + stride = fb->pitches[0] / stride_div; >>>> + x_offset = x; >>>> + y_offset = y; >>>> + plane_size = ((plane->state->src_h >> 16) - 1) << 16 | >>>> + ((plane->state->src_w >> 16) - 1); >>>> + } >>>> + plane_offset = y_offset << 16 | x_offset; >>>> >>>> I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); >>>> I915_WRITE(PLANE_POS(pipe, 0), 0); >>>> - I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x); >>>> - I915_WRITE(PLANE_SIZE(pipe, 0), >>>> - (intel_crtc->config->pipe_src_h - 1) << 16 | >>>> - (intel_crtc->config->pipe_src_w - 1)); >>>> - I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div); >>>> + I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); >>>> + I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); >>>> + I915_WRITE(PLANE_STRIDE(pipe, 0), stride); >>>> I915_WRITE(PLANE_SURF(pipe, 0), surf_addr); >>>> >>>> POSTING_READ(PLANE_SURF(pipe, 0)); >>>> @@ -12406,23 +12462,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, >>>> intel_primary_formats, num_formats, >>>> DRM_PLANE_TYPE_PRIMARY); >>>> >>>> - if (INTEL_INFO(dev)->gen >= 4) { >>>> - if (!dev->mode_config.rotation_property) >>>> - dev->mode_config.rotation_property = >>>> - drm_mode_create_rotation_property(dev, >>>> - BIT(DRM_ROTATE_0) | >>>> - BIT(DRM_ROTATE_180)); >>>> - if (dev->mode_config.rotation_property) >>>> - drm_object_attach_property(&primary->base.base, >>>> - dev->mode_config.rotation_property, >>>> - state->base.rotation); >>>> - } >>>> + if (INTEL_INFO(dev)->gen >= 4) >>>> + intel_create_rotation_property(dev, primary); >>>> >>>> drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs); >>>> >>>> return &primary->base; >>>> } >>>> >>>> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane) >>>> +{ >>>> + if (!dev->mode_config.rotation_property) { >>>> + unsigned long flags = BIT(DRM_ROTATE_0) | >>>> + BIT(DRM_ROTATE_180); >>>> + >>>> + if (INTEL_INFO(dev)->gen >= 9) >>>> + flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270); >>>> + >>>> + dev->mode_config.rotation_property = >>>> + drm_mode_create_rotation_property(dev, flags); >>>> + } >>>> + if (dev->mode_config.rotation_property) >>>> + drm_object_attach_property(&plane->base.base, >>>> + dev->mode_config.rotation_property, >>>> + plane->base.state->rotation); >>>> +} >>>> + >>>> static int >>>> intel_check_cursor_plane(struct drm_plane *plane, >>>> struct intel_plane_state *state) >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>> index 811a1db..d32025a 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation) >>>> return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)); >>>> } >>>> >>>> +unsigned int >>>> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel, >>>> + uint64_t fb_modifier); >>>> +void intel_create_rotation_property(struct drm_device *dev, >>>> + struct intel_plane *plane); >>>> + >>>> bool intel_wm_need_update(struct drm_plane *plane, >>>> struct drm_plane_state *state); >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >>>> index f41e872..65eb147 100644 >>>> --- a/drivers/gpu/drm/i915/intel_sprite.c >>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c >>>> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, >>>> struct drm_i915_gem_object *obj = intel_fb_obj(fb); >>>> const int pipe = intel_plane->pipe; >>>> const int plane = intel_plane->plane + 1; >>>> - u32 plane_ctl, stride_div; >>>> + u32 plane_ctl, stride_div, stride; >>>> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); >>>> const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey; >>>> unsigned long surf_addr; >>>> + u32 tile_height, plane_offset, plane_size; >>>> + unsigned int rotation; >>>> + int x_offset, y_offset; >>>> >>>> plane_ctl = PLANE_CTL_ENABLE | >>>> PLANE_CTL_PIPE_CSC_ENABLE; >>>> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, >>>> MISSING_CASE(fb->modifier[0]); >>>> } >>>> >>>> - if (drm_plane->state->rotation == BIT(DRM_ROTATE_180)) >>>> + rotation = drm_plane->state->rotation; >>>> + switch (rotation) { >>>> + case BIT(DRM_ROTATE_90): >>>> + plane_ctl |= PLANE_CTL_ROTATE_90; >>>> + break; >>>> + >>>> + case BIT(DRM_ROTATE_180): >>>> plane_ctl |= PLANE_CTL_ROTATE_180; >>>> + break; >>>> + >>>> + case BIT(DRM_ROTATE_270): >>>> + plane_ctl |= PLANE_CTL_ROTATE_270; >>>> + break; >>>> + } >>>> >>>> intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h, >>>> pixel_size, true, >>>> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, >>>> >>>> surf_addr = intel_plane_obj_offset(intel_plane, obj); >>>> >>>> - I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x); >>>> - I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div); >>>> + if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) { >>>> + /* stride: Surface height in tiles */ >>>> + tile_height = intel_tile_height(dev, fb->bits_per_pixel, >>>> + fb->modifier[0]); >>>> + stride = DIV_ROUND_UP(fb->height, tile_height); >>>> + plane_size = (src_w << 16) | src_h; >>>> + x_offset = stride * tile_height - y - (src_h + 1); >>>> + y_offset = x; >>>> + } else { >>>> + stride = fb->pitches[0] / stride_div; >>>> + plane_size = (src_h << 16) | src_w; >>>> + x_offset = x; >>>> + y_offset = y; >>>> + } >>>> + plane_offset = y_offset << 16 | x_offset; >>>> + >>>> + I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset); >>>> + I915_WRITE(PLANE_STRIDE(pipe, plane), stride); >>>> I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x); >>>> - I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w); >>>> + I915_WRITE(PLANE_SIZE(pipe, plane), plane_size); >>>> I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); >>>> I915_WRITE(PLANE_SURF(pipe, plane), surf_addr); >>>> POSTING_READ(PLANE_SURF(pipe, plane)); >>>> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) >>>> goto out; >>>> } >>>> >>>> - if (!dev->mode_config.rotation_property) >>>> - dev->mode_config.rotation_property = >>>> - drm_mode_create_rotation_property(dev, >>>> - BIT(DRM_ROTATE_0) | >>>> - BIT(DRM_ROTATE_180)); >>>> - >>>> - if (dev->mode_config.rotation_property) >>>> - drm_object_attach_property(&intel_plane->base.base, >>>> - dev->mode_config.rotation_property, >>>> - state->base.rotation); >>>> + intel_create_rotation_property(dev, intel_plane); >>>> >>>> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); >>>> >>>> -- >>>> 1.7.10.4 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation 2015-04-02 15:59 ` Matt Roper 2015-04-06 12:27 ` Jindal, Sonika @ 2015-04-07 8:13 ` Daniel Vetter 2015-04-07 8:22 ` Jindal, Sonika 1 sibling, 1 reply; 28+ messages in thread From: Daniel Vetter @ 2015-04-07 8:13 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On Thu, Apr 02, 2015 at 08:59:33AM -0700, Matt Roper wrote: > On Thu, Apr 02, 2015 at 10:24:02AM +0530, Jindal, Sonika wrote: > > I am not sure how it will help. drm_plane_check_pixel_format should > > be used to check the pixel format of the fb which we should be doing > > in some -check functions (I don't think we do that right now?) > > against what is supported by the plane. > > But to check for the formats which are allowed for 90/270, we would > > need this kind of explicit check. > > Right, I guess there are two aspects here. First, we need to properly > test for acceptable pixel formats for the plane in general; at the > moment the DRM core setplane() tests this, but if we use the atomic > ioctl it never gets checked (which is a bug). So as you say, we need a > test in a _check() function to verify this. We probably also need to > add an i-g-t test for it too. The core atomic ioctl does check for valid plane pixel formats for you. And there shouldn't be any other entrypoint (except internal ones, but that's ok). Is there a bug left? Definitely agree that testcases would be nice. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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] 28+ messages in thread
* Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation 2015-04-07 8:13 ` Daniel Vetter @ 2015-04-07 8:22 ` Jindal, Sonika 2015-04-07 8:26 ` [PATCH] " Sonika Jindal 0 siblings, 1 reply; 28+ messages in thread From: Jindal, Sonika @ 2015-04-07 8:22 UTC (permalink / raw) To: Daniel Vetter, Matt Roper; +Cc: intel-gfx On 4/7/2015 1:43 PM, Daniel Vetter wrote: > On Thu, Apr 02, 2015 at 08:59:33AM -0700, Matt Roper wrote: >> On Thu, Apr 02, 2015 at 10:24:02AM +0530, Jindal, Sonika wrote: >>> I am not sure how it will help. drm_plane_check_pixel_format should >>> be used to check the pixel format of the fb which we should be doing >>> in some -check functions (I don't think we do that right now?) >>> against what is supported by the plane. >>> But to check for the formats which are allowed for 90/270, we would >>> need this kind of explicit check. >> >> Right, I guess there are two aspects here. First, we need to properly >> test for acceptable pixel formats for the plane in general; at the >> moment the DRM core setplane() tests this, but if we use the atomic >> ioctl it never gets checked (which is a bug). So as you say, we need a >> test in a _check() function to verify this. We probably also need to >> add an i-g-t test for it too. > > The core atomic ioctl does check for valid plane pixel formats for you. > And there shouldn't be any other entrypoint (except internal ones, but > that's ok). Is there a bug left? > Oh, yes the check is there. Will abandon that check then and just move the 90/270 pixel format check to intel_plane_atomic_check. Regards, Sonika > Definitely agree that testcases would be nice. > -Daniel > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] drm/i915/skl: Support for 90/270 rotation 2015-04-07 8:22 ` Jindal, Sonika @ 2015-04-07 8:26 ` Sonika Jindal 2015-04-07 10:59 ` shuang.he 2015-04-09 22:54 ` Matt Roper 0 siblings, 2 replies; 28+ messages in thread From: Sonika Jindal @ 2015-04-07 8:26 UTC (permalink / raw) To: intel-gfx v2: Moving creation of property in a function, checking for 90/270 rotation simultaneously (Chris) Letting primary plane to be positioned v3: Adding if/else for 90/270 and rest params programming, adding check for pixel_format, some cleanup (review comments) v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset and size programming (Ville) v5: Rebased on -nightly and Tvrtko's series for gtt remapping. v6: Rebased on -nightly (Tvrtko's series merged) v7: Moving pixel_format check to intel_atomic_plane_check (Matt) Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_atomic_plane.c | 23 ++++++++ drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++--------- drivers/gpu/drm/i915/intel_drv.h | 6 ++ drivers/gpu/drm/i915/intel_sprite.c | 52 ++++++++++++----- 5 files changed, 130 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b522eb6..564bbd5 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells { #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4) #define PLANE_CTL_ROTATE_MASK 0x3 #define PLANE_CTL_ROTATE_0 0x0 +#define PLANE_CTL_ROTATE_90 0x1 #define PLANE_CTL_ROTATE_180 0x2 +#define PLANE_CTL_ROTATE_270 0x3 #define _PLANE_STRIDE_1_A 0x70188 #define _PLANE_STRIDE_2_A 0x70288 #define _PLANE_STRIDE_3_A 0x70388 diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index 976b891..ef8c291 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -162,6 +162,29 @@ static int intel_plane_atomic_check(struct drm_plane *plane, (1 << drm_plane_index(plane)); } + if (state->fb && intel_rotation_90_or_270(state->rotation)) { + if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || + state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) { + DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n"); + return -EINVAL; + } + + /* + * 90/270 is not allowed with RGB64 16:16:16:16, + * RGB 16-bit 5:6:5, and Indexed 8-bit. + */ + switch (state->fb->pixel_format) { + case DRM_FORMAT_C8: + case DRM_FORMAT_RGB565: + DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", + drm_get_format_name(state->fb->pixel_format)); + return -EINVAL; + + default: + break; + } + } + return intel_plane->check_plane(plane, intel_state); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f0bbc22..4de544c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, info->pitch = fb->pitches[0]; info->fb_modifier = fb->modifier[0]; - if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED || - info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) { - DRM_DEBUG_KMS( - "Y or Yf tiling is needed for 90/270 rotation!\n"); - return -EINVAL; - } - return 0; } @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_i915_gem_object *obj; int pipe = intel_crtc->pipe; - u32 plane_ctl, stride_div; + u32 plane_ctl, stride_div, stride; + u32 tile_height, plane_offset, plane_size; + unsigned int rotation; + int x_offset, y_offset; unsigned long surf_addr; + struct drm_plane *plane; if (!intel_crtc->primary_enabled) { I915_WRITE(PLANE_CTL(pipe, 0), 0); @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, } plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE; - if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) + + plane = crtc->primary; + rotation = plane->state->rotation; + switch (rotation) { + case BIT(DRM_ROTATE_90): + plane_ctl |= PLANE_CTL_ROTATE_90; + break; + + case BIT(DRM_ROTATE_180): plane_ctl |= PLANE_CTL_ROTATE_180; + break; + + case BIT(DRM_ROTATE_270): + plane_ctl |= PLANE_CTL_ROTATE_270; + break; + } obj = intel_fb_obj(fb); stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], fb->pixel_format); - surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj); + surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj); + + if (intel_rotation_90_or_270(rotation)) { + /* stride = Surface height in tiles */ + tile_height = intel_tile_height(dev, fb->bits_per_pixel, + fb->modifier[0]); + stride = DIV_ROUND_UP(fb->height, tile_height); + x_offset = stride * tile_height - y - (plane->state->src_h >> 16); + y_offset = x; + plane_size = ((plane->state->src_w >> 16) - 1) << 16 | + ((plane->state->src_h >> 16) - 1); + } else { + stride = fb->pitches[0] / stride_div; + x_offset = x; + y_offset = y; + plane_size = ((plane->state->src_h >> 16) - 1) << 16 | + ((plane->state->src_w >> 16) - 1); + } + plane_offset = y_offset << 16 | x_offset; I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); I915_WRITE(PLANE_POS(pipe, 0), 0); - I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x); - I915_WRITE(PLANE_SIZE(pipe, 0), - (intel_crtc->config->pipe_src_h - 1) << 16 | - (intel_crtc->config->pipe_src_w - 1)); - I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div); + I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); + I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); + I915_WRITE(PLANE_STRIDE(pipe, 0), stride); I915_WRITE(PLANE_SURF(pipe, 0), surf_addr); POSTING_READ(PLANE_SURF(pipe, 0)); @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, intel_primary_formats, num_formats, DRM_PLANE_TYPE_PRIMARY); - if (INTEL_INFO(dev)->gen >= 4) { - if (!dev->mode_config.rotation_property) - dev->mode_config.rotation_property = - drm_mode_create_rotation_property(dev, - BIT(DRM_ROTATE_0) | - BIT(DRM_ROTATE_180)); - if (dev->mode_config.rotation_property) - drm_object_attach_property(&primary->base.base, - dev->mode_config.rotation_property, - state->base.rotation); - } + if (INTEL_INFO(dev)->gen >= 4) + intel_create_rotation_property(dev, primary); drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs); return &primary->base; } +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane) +{ + if (!dev->mode_config.rotation_property) { + unsigned long flags = BIT(DRM_ROTATE_0) | + BIT(DRM_ROTATE_180); + + if (INTEL_INFO(dev)->gen >= 9) + flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270); + + dev->mode_config.rotation_property = + drm_mode_create_rotation_property(dev, flags); + } + if (dev->mode_config.rotation_property) + drm_object_attach_property(&plane->base.base, + dev->mode_config.rotation_property, + plane->base.state->rotation); +} + static int intel_check_cursor_plane(struct drm_plane *plane, struct intel_plane_state *state) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 811a1db..d32025a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation) return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)); } +unsigned int +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel, + uint64_t fb_modifier); +void intel_create_rotation_property(struct drm_device *dev, + struct intel_plane *plane); + bool intel_wm_need_update(struct drm_plane *plane, struct drm_plane_state *state); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index f41e872..83adc9b 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, struct drm_i915_gem_object *obj = intel_fb_obj(fb); const int pipe = intel_plane->pipe; const int plane = intel_plane->plane + 1; - u32 plane_ctl, stride_div; + u32 plane_ctl, stride_div, stride; int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey; unsigned long surf_addr; + u32 tile_height, plane_offset, plane_size; + unsigned int rotation; + int x_offset, y_offset; plane_ctl = PLANE_CTL_ENABLE | PLANE_CTL_PIPE_CSC_ENABLE; @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, MISSING_CASE(fb->modifier[0]); } - if (drm_plane->state->rotation == BIT(DRM_ROTATE_180)) + rotation = drm_plane->state->rotation; + switch (rotation) { + case BIT(DRM_ROTATE_90): + plane_ctl |= PLANE_CTL_ROTATE_90; + break; + + case BIT(DRM_ROTATE_180): plane_ctl |= PLANE_CTL_ROTATE_180; + break; + + case BIT(DRM_ROTATE_270): + plane_ctl |= PLANE_CTL_ROTATE_270; + break; + } intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h, pixel_size, true, @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, surf_addr = intel_plane_obj_offset(intel_plane, obj); - I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x); - I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div); + if (intel_rotation_90_or_270(rotation)) { + /* stride: Surface height in tiles */ + tile_height = intel_tile_height(dev, fb->bits_per_pixel, + fb->modifier[0]); + stride = DIV_ROUND_UP(fb->height, tile_height); + plane_size = (src_w << 16) | src_h; + x_offset = stride * tile_height - y - (src_h + 1); + y_offset = x; + } else { + stride = fb->pitches[0] / stride_div; + plane_size = (src_h << 16) | src_w; + x_offset = x; + y_offset = y; + } + plane_offset = y_offset << 16 | x_offset; + + I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset); + I915_WRITE(PLANE_STRIDE(pipe, plane), stride); I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x); - I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w); + I915_WRITE(PLANE_SIZE(pipe, plane), plane_size); I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); I915_WRITE(PLANE_SURF(pipe, plane), surf_addr); POSTING_READ(PLANE_SURF(pipe, plane)); @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) goto out; } - if (!dev->mode_config.rotation_property) - dev->mode_config.rotation_property = - drm_mode_create_rotation_property(dev, - BIT(DRM_ROTATE_0) | - BIT(DRM_ROTATE_180)); - - if (dev->mode_config.rotation_property) - drm_object_attach_property(&intel_plane->base.base, - dev->mode_config.rotation_property, - state->base.rotation); + intel_create_rotation_property(dev, intel_plane); drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); -- 1.7.10.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] drm/i915/skl: Support for 90/270 rotation 2015-04-07 8:26 ` [PATCH] " Sonika Jindal @ 2015-04-07 10:59 ` shuang.he 2015-04-09 22:54 ` Matt Roper 1 sibling, 0 replies; 28+ messages in thread From: shuang.he @ 2015-04-07 10:59 UTC (permalink / raw) To: shuang.he, ethan.gao, intel-gfx, sonika.jindal Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) Task id: 6135 -------------------------------------Summary------------------------------------- Platform Delta drm-intel-nightly Series Applied PNV -1 272/272 271/272 ILK 302/302 302/302 SNB 303/303 303/303 IVB -1 338/338 337/338 BYT -1 287/287 286/287 HSW 361/361 361/361 BDW 308/308 308/308 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied PNV igt@gen3_render_tiledx_blits FAIL(7)PASS(6) FAIL(2) *IVB igt@gem_storedw_batches_loop@secure-dispatch PASS(2) DMESG_WARN(1)PASS(1) (dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(20) FAIL(1)PASS(1) Note: You need to pay more attention to line start with '*' _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] drm/i915/skl: Support for 90/270 rotation 2015-04-07 8:26 ` [PATCH] " Sonika Jindal 2015-04-07 10:59 ` shuang.he @ 2015-04-09 22:54 ` Matt Roper 1 sibling, 0 replies; 28+ messages in thread From: Matt Roper @ 2015-04-09 22:54 UTC (permalink / raw) To: Sonika Jindal; +Cc: intel-gfx On Tue, Apr 07, 2015 at 01:56:28PM +0530, Sonika Jindal wrote: > v2: Moving creation of property in a function, checking for 90/270 > rotation simultaneously (Chris) > Letting primary plane to be positioned > v3: Adding if/else for 90/270 and rest params programming, adding check for > pixel_format, some cleanup (review comments) > v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset > and size programming (Ville) > v5: Rebased on -nightly and Tvrtko's series for gtt remapping. > v6: Rebased on -nightly (Tvrtko's series merged) > v7: Moving pixel_format check to intel_atomic_plane_check (Matt) > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 2 + > drivers/gpu/drm/i915/intel_atomic_plane.c | 23 ++++++++ > drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++--------- > drivers/gpu/drm/i915/intel_drv.h | 6 ++ > drivers/gpu/drm/i915/intel_sprite.c | 52 ++++++++++++----- > 5 files changed, 130 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index b522eb6..564bbd5 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells { > #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4) > #define PLANE_CTL_ROTATE_MASK 0x3 > #define PLANE_CTL_ROTATE_0 0x0 > +#define PLANE_CTL_ROTATE_90 0x1 > #define PLANE_CTL_ROTATE_180 0x2 > +#define PLANE_CTL_ROTATE_270 0x3 > #define _PLANE_STRIDE_1_A 0x70188 > #define _PLANE_STRIDE_2_A 0x70288 > #define _PLANE_STRIDE_3_A 0x70388 > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > index 976b891..ef8c291 100644 > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > @@ -162,6 +162,29 @@ static int intel_plane_atomic_check(struct drm_plane *plane, > (1 << drm_plane_index(plane)); > } > > + if (state->fb && intel_rotation_90_or_270(state->rotation)) { > + if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || > + state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) { > + DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n"); > + return -EINVAL; > + } > + > + /* > + * 90/270 is not allowed with RGB64 16:16:16:16, > + * RGB 16-bit 5:6:5, and Indexed 8-bit. > + */ > + switch (state->fb->pixel_format) { > + case DRM_FORMAT_C8: > + case DRM_FORMAT_RGB565: Your comment mentions RGB64, which isn't handled in the case statement here. But we don't support RGB64 at all today (in skl_plane_formats[]), so I'd just remove it from the comment to avoid confusion. Aside from that, you can consider this Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > + DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", > + drm_get_format_name(state->fb->pixel_format)); > + return -EINVAL; > + > + default: > + break; > + } > + } > + > return intel_plane->check_plane(plane, intel_state); > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f0bbc22..4de544c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, > info->pitch = fb->pitches[0]; > info->fb_modifier = fb->modifier[0]; > > - if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED || > - info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) { > - DRM_DEBUG_KMS( > - "Y or Yf tiling is needed for 90/270 rotation!\n"); > - return -EINVAL; > - } > - > return 0; > } > > @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct drm_i915_gem_object *obj; > int pipe = intel_crtc->pipe; > - u32 plane_ctl, stride_div; > + u32 plane_ctl, stride_div, stride; > + u32 tile_height, plane_offset, plane_size; > + unsigned int rotation; > + int x_offset, y_offset; > unsigned long surf_addr; > + struct drm_plane *plane; > > if (!intel_crtc->primary_enabled) { > I915_WRITE(PLANE_CTL(pipe, 0), 0); > @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > } > > plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE; > - if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) > + > + plane = crtc->primary; > + rotation = plane->state->rotation; > + switch (rotation) { > + case BIT(DRM_ROTATE_90): > + plane_ctl |= PLANE_CTL_ROTATE_90; > + break; > + > + case BIT(DRM_ROTATE_180): > plane_ctl |= PLANE_CTL_ROTATE_180; > + break; > + > + case BIT(DRM_ROTATE_270): > + plane_ctl |= PLANE_CTL_ROTATE_270; > + break; > + } > > obj = intel_fb_obj(fb); > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > fb->pixel_format); > - surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj); > + surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj); > + > + if (intel_rotation_90_or_270(rotation)) { > + /* stride = Surface height in tiles */ > + tile_height = intel_tile_height(dev, fb->bits_per_pixel, > + fb->modifier[0]); > + stride = DIV_ROUND_UP(fb->height, tile_height); > + x_offset = stride * tile_height - y - (plane->state->src_h >> 16); > + y_offset = x; > + plane_size = ((plane->state->src_w >> 16) - 1) << 16 | > + ((plane->state->src_h >> 16) - 1); > + } else { > + stride = fb->pitches[0] / stride_div; > + x_offset = x; > + y_offset = y; > + plane_size = ((plane->state->src_h >> 16) - 1) << 16 | > + ((plane->state->src_w >> 16) - 1); > + } > + plane_offset = y_offset << 16 | x_offset; > > I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); > I915_WRITE(PLANE_POS(pipe, 0), 0); > - I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x); > - I915_WRITE(PLANE_SIZE(pipe, 0), > - (intel_crtc->config->pipe_src_h - 1) << 16 | > - (intel_crtc->config->pipe_src_w - 1)); > - I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div); > + I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); > + I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); > + I915_WRITE(PLANE_STRIDE(pipe, 0), stride); > I915_WRITE(PLANE_SURF(pipe, 0), surf_addr); > > POSTING_READ(PLANE_SURF(pipe, 0)); > @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > intel_primary_formats, num_formats, > DRM_PLANE_TYPE_PRIMARY); > > - if (INTEL_INFO(dev)->gen >= 4) { > - if (!dev->mode_config.rotation_property) > - dev->mode_config.rotation_property = > - drm_mode_create_rotation_property(dev, > - BIT(DRM_ROTATE_0) | > - BIT(DRM_ROTATE_180)); > - if (dev->mode_config.rotation_property) > - drm_object_attach_property(&primary->base.base, > - dev->mode_config.rotation_property, > - state->base.rotation); > - } > + if (INTEL_INFO(dev)->gen >= 4) > + intel_create_rotation_property(dev, primary); > > drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs); > > return &primary->base; > } > > +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane) > +{ > + if (!dev->mode_config.rotation_property) { > + unsigned long flags = BIT(DRM_ROTATE_0) | > + BIT(DRM_ROTATE_180); > + > + if (INTEL_INFO(dev)->gen >= 9) > + flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270); > + > + dev->mode_config.rotation_property = > + drm_mode_create_rotation_property(dev, flags); > + } > + if (dev->mode_config.rotation_property) > + drm_object_attach_property(&plane->base.base, > + dev->mode_config.rotation_property, > + plane->base.state->rotation); > +} > + > static int > intel_check_cursor_plane(struct drm_plane *plane, > struct intel_plane_state *state) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 811a1db..d32025a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation) > return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)); > } > > +unsigned int > +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel, > + uint64_t fb_modifier); > +void intel_create_rotation_property(struct drm_device *dev, > + struct intel_plane *plane); > + > bool intel_wm_need_update(struct drm_plane *plane, > struct drm_plane_state *state); > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index f41e872..83adc9b 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > const int pipe = intel_plane->pipe; > const int plane = intel_plane->plane + 1; > - u32 plane_ctl, stride_div; > + u32 plane_ctl, stride_div, stride; > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey; > unsigned long surf_addr; > + u32 tile_height, plane_offset, plane_size; > + unsigned int rotation; > + int x_offset, y_offset; > > plane_ctl = PLANE_CTL_ENABLE | > PLANE_CTL_PIPE_CSC_ENABLE; > @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > MISSING_CASE(fb->modifier[0]); > } > > - if (drm_plane->state->rotation == BIT(DRM_ROTATE_180)) > + rotation = drm_plane->state->rotation; > + switch (rotation) { > + case BIT(DRM_ROTATE_90): > + plane_ctl |= PLANE_CTL_ROTATE_90; > + break; > + > + case BIT(DRM_ROTATE_180): > plane_ctl |= PLANE_CTL_ROTATE_180; > + break; > + > + case BIT(DRM_ROTATE_270): > + plane_ctl |= PLANE_CTL_ROTATE_270; > + break; > + } > > intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h, > pixel_size, true, > @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > > surf_addr = intel_plane_obj_offset(intel_plane, obj); > > - I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x); > - I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div); > + if (intel_rotation_90_or_270(rotation)) { > + /* stride: Surface height in tiles */ > + tile_height = intel_tile_height(dev, fb->bits_per_pixel, > + fb->modifier[0]); > + stride = DIV_ROUND_UP(fb->height, tile_height); > + plane_size = (src_w << 16) | src_h; > + x_offset = stride * tile_height - y - (src_h + 1); > + y_offset = x; > + } else { > + stride = fb->pitches[0] / stride_div; > + plane_size = (src_h << 16) | src_w; > + x_offset = x; > + y_offset = y; > + } > + plane_offset = y_offset << 16 | x_offset; > + > + I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset); > + I915_WRITE(PLANE_STRIDE(pipe, plane), stride); > I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x); > - I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w); > + I915_WRITE(PLANE_SIZE(pipe, plane), plane_size); > I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); > I915_WRITE(PLANE_SURF(pipe, plane), surf_addr); > POSTING_READ(PLANE_SURF(pipe, plane)); > @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > goto out; > } > > - if (!dev->mode_config.rotation_property) > - dev->mode_config.rotation_property = > - drm_mode_create_rotation_property(dev, > - BIT(DRM_ROTATE_0) | > - BIT(DRM_ROTATE_180)); > - > - if (dev->mode_config.rotation_property) > - drm_object_attach_property(&intel_plane->base.base, > - dev->mode_config.rotation_property, > - state->base.rotation); > + intel_create_rotation_property(dev, intel_plane); > > drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); > > -- > 1.7.10.4 > -- 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] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-03-30 8:34 [PATCH 1/2] drm/i915/skl: Allow universal planes to position Sonika Jindal 2015-03-30 8:34 ` [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation Sonika Jindal @ 2015-04-01 18:21 ` Matt Roper 2015-04-02 4:38 ` Jindal, Sonika 1 sibling, 1 reply; 28+ messages in thread From: Matt Roper @ 2015-04-01 18:21 UTC (permalink / raw) To: Sonika Jindal; +Cc: intel-gfx On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote: > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> It looks like this is dependent on Ville's patch [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane() to actually let us do something sensible with the destination rectangle at the hardware level. Looks like that patch has a r-b, but hasn't made it into di-nightly yet. Assuming Ville's patch lands first, this is Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Matt > --- > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index ceb2e61..f0bbc22 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > struct drm_rect *dest = &state->dst; > struct drm_rect *src = &state->src; > const struct drm_rect *clip = &state->clip; > + bool can_position = false; > int ret; > > crtc = crtc ? crtc : plane->crtc; > intel_crtc = to_intel_crtc(crtc); > > + if (INTEL_INFO(dev)->gen >= 9) > + can_position = true; > + > ret = drm_plane_helper_check_update(plane, crtc, fb, > src, dest, clip, > DRM_PLANE_HELPER_NO_SCALING, > DRM_PLANE_HELPER_NO_SCALING, > - false, true, &state->visible); > + can_position, true, > + &state->visible); > if (ret) > return ret; > > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-04-01 18:21 ` [PATCH 1/2] drm/i915/skl: Allow universal planes to position Matt Roper @ 2015-04-02 4:38 ` Jindal, Sonika 2015-04-02 15:48 ` Matt Roper 0 siblings, 1 reply; 28+ messages in thread From: Jindal, Sonika @ 2015-04-02 4:38 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On 4/1/2015 11:51 PM, Matt Roper wrote: > On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote: >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > It looks like this is dependent on Ville's patch > > [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane() > > to actually let us do something sensible with the destination rectangle > at the hardware level. Looks like that patch has a r-b, but hasn't made > it into di-nightly yet. > Right now, can_position is used to check for the scenarios where the primary plane is not covering the complete crtc. This could be due to positioning or a smaller fb on primary plane. With Ville's patch, we would be able to allow positioning to happen. But I need it here, to create a smaller fb for 90/270 rotation. I agree that, until Ville's patch is there, we won't be entertaining any positioning requests on the primary plane and we will not be throwing any error also. But for the 90/270 testcase in kms_rotation_crc to go through, we would need this to create a smaller fb so that we can rotate it. > Assuming Ville's patch lands first, this is > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > Matt > >> --- >> drivers/gpu/drm/i915/intel_display.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index ceb2e61..f0bbc22 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, >> struct drm_rect *dest = &state->dst; >> struct drm_rect *src = &state->src; >> const struct drm_rect *clip = &state->clip; >> + bool can_position = false; >> int ret; >> >> crtc = crtc ? crtc : plane->crtc; >> intel_crtc = to_intel_crtc(crtc); >> >> + if (INTEL_INFO(dev)->gen >= 9) >> + can_position = true; >> + >> ret = drm_plane_helper_check_update(plane, crtc, fb, >> src, dest, clip, >> DRM_PLANE_HELPER_NO_SCALING, >> DRM_PLANE_HELPER_NO_SCALING, >> - false, true, &state->visible); >> + can_position, true, >> + &state->visible); >> if (ret) >> return ret; >> >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-04-02 4:38 ` Jindal, Sonika @ 2015-04-02 15:48 ` Matt Roper 2015-04-06 5:20 ` Jindal, Sonika 0 siblings, 1 reply; 28+ messages in thread From: Matt Roper @ 2015-04-02 15:48 UTC (permalink / raw) To: Jindal, Sonika; +Cc: intel-gfx On Thu, Apr 02, 2015 at 10:08:27AM +0530, Jindal, Sonika wrote: > > > On 4/1/2015 11:51 PM, Matt Roper wrote: > >On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote: > >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > >It looks like this is dependent on Ville's patch > > > > [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane() > > > >to actually let us do something sensible with the destination rectangle > >at the hardware level. Looks like that patch has a r-b, but hasn't made > >it into di-nightly yet. > > > Right now, can_position is used to check for the scenarios where the > primary plane is not covering the complete crtc. This could be due > to positioning or a smaller fb on primary plane. > With Ville's patch, we would be able to allow positioning to happen. > But I need it here, to create a smaller fb for 90/270 rotation. > > I agree that, until Ville's patch is there, we won't be entertaining > any positioning requests on the primary plane and we will not be > throwing any error also. Right...and I think failing to throw an error would be seen as a bug, which is why I think Ville's patch needs to go in first. Since it's already been reviewed, I'm not aware of anything holding that up from happening. > But for the 90/270 testcase in kms_rotation_crc to go through, we > would need this to create a smaller fb so that we can rotate it. So is your worry here that drm_plane_helper_check_update() doesn't understand rotation and winds up mixing up width/height? If so, I think the proper course of action is to write a patch for the helper function that makes it rotation-aware. Matt > > >Assuming Ville's patch lands first, this is > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > >Matt > > > >>--- > >> drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>index ceb2e61..f0bbc22 100644 > >>--- a/drivers/gpu/drm/i915/intel_display.c > >>+++ b/drivers/gpu/drm/i915/intel_display.c > >>@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > >> struct drm_rect *dest = &state->dst; > >> struct drm_rect *src = &state->src; > >> const struct drm_rect *clip = &state->clip; > >>+ bool can_position = false; > >> int ret; > >> > >> crtc = crtc ? crtc : plane->crtc; > >> intel_crtc = to_intel_crtc(crtc); > >> > >>+ if (INTEL_INFO(dev)->gen >= 9) > >>+ can_position = true; > >>+ > >> ret = drm_plane_helper_check_update(plane, crtc, fb, > >> src, dest, clip, > >> DRM_PLANE_HELPER_NO_SCALING, > >> DRM_PLANE_HELPER_NO_SCALING, > >>- false, true, &state->visible); > >>+ can_position, true, > >>+ &state->visible); > >> if (ret) > >> return ret; > >> > >>-- > >>1.7.10.4 > >> > >>_______________________________________________ > >>Intel-gfx mailing list > >>Intel-gfx@lists.freedesktop.org > >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- 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] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-04-02 15:48 ` Matt Roper @ 2015-04-06 5:20 ` Jindal, Sonika 2015-04-07 8:16 ` Daniel Vetter 0 siblings, 1 reply; 28+ messages in thread From: Jindal, Sonika @ 2015-04-06 5:20 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On 4/2/2015 9:18 PM, Matt Roper wrote: > On Thu, Apr 02, 2015 at 10:08:27AM +0530, Jindal, Sonika wrote: >> >> >> On 4/1/2015 11:51 PM, Matt Roper wrote: >>> On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote: >>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >>> >>> It looks like this is dependent on Ville's patch >>> >>> [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane() >>> >>> to actually let us do something sensible with the destination rectangle >>> at the hardware level. Looks like that patch has a r-b, but hasn't made >>> it into di-nightly yet. >>> >> Right now, can_position is used to check for the scenarios where the >> primary plane is not covering the complete crtc. This could be due >> to positioning or a smaller fb on primary plane. >> With Ville's patch, we would be able to allow positioning to happen. >> But I need it here, to create a smaller fb for 90/270 rotation. >> >> I agree that, until Ville's patch is there, we won't be entertaining >> any positioning requests on the primary plane and we will not be >> throwing any error also. > > Right...and I think failing to throw an error would be seen as a bug, > which is why I think Ville's patch needs to go in first. Since it's > already been reviewed, I'm not aware of anything holding that up from > happening. > Agree, will check with Daniel on this. >> But for the 90/270 testcase in kms_rotation_crc to go through, we >> would need this to create a smaller fb so that we can rotate it. > > So is your worry here that drm_plane_helper_check_update() doesn't > understand rotation and winds up mixing up width/height? If so, I think > the proper course of action is to write a patch for the helper function > that makes it rotation-aware. > No, the worry is that it rejects a smaller fb for primary plane for all the platforms. I mentioned 90/270 rotation, because I create a smaller fb (rather than the full screen fb), so that the rotated plane fits into the screen. If it is lets say 1920x1080, and the pipe is set at 1920x1080, after rotation the plane becomes 1080x1920 and the height of the plane surpasses that of crtc. For gen >=9 , we can have smaller fb for primary plane which might not cover the entire crtc. Regards, Sonika > > Matt > >> >>> Assuming Ville's patch lands first, this is >>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >>> >>> >>> Matt >>> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 7 ++++++- >>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index ceb2e61..f0bbc22 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, >>>> struct drm_rect *dest = &state->dst; >>>> struct drm_rect *src = &state->src; >>>> const struct drm_rect *clip = &state->clip; >>>> + bool can_position = false; >>>> int ret; >>>> >>>> crtc = crtc ? crtc : plane->crtc; >>>> intel_crtc = to_intel_crtc(crtc); >>>> >>>> + if (INTEL_INFO(dev)->gen >= 9) >>>> + can_position = true; >>>> + >>>> ret = drm_plane_helper_check_update(plane, crtc, fb, >>>> src, dest, clip, >>>> DRM_PLANE_HELPER_NO_SCALING, >>>> DRM_PLANE_HELPER_NO_SCALING, >>>> - false, true, &state->visible); >>>> + can_position, true, >>>> + &state->visible); >>>> if (ret) >>>> return ret; >>>> >>>> -- >>>> 1.7.10.4 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-04-06 5:20 ` Jindal, Sonika @ 2015-04-07 8:16 ` Daniel Vetter 2015-04-07 8:21 ` Jindal, Sonika 0 siblings, 1 reply; 28+ messages in thread From: Daniel Vetter @ 2015-04-07 8:16 UTC (permalink / raw) To: Jindal, Sonika; +Cc: intel-gfx On Mon, Apr 06, 2015 at 10:50:51AM +0530, Jindal, Sonika wrote: > > > On 4/2/2015 9:18 PM, Matt Roper wrote: > >On Thu, Apr 02, 2015 at 10:08:27AM +0530, Jindal, Sonika wrote: > >> > >> > >>On 4/1/2015 11:51 PM, Matt Roper wrote: > >>>On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote: > >>>>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >>> > >>>It looks like this is dependent on Ville's patch > >>> > >>> [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane() > >>> > >>>to actually let us do something sensible with the destination rectangle > >>>at the hardware level. Looks like that patch has a r-b, but hasn't made > >>>it into di-nightly yet. > >>> > >>Right now, can_position is used to check for the scenarios where the > >>primary plane is not covering the complete crtc. This could be due > >>to positioning or a smaller fb on primary plane. > >>With Ville's patch, we would be able to allow positioning to happen. > >>But I need it here, to create a smaller fb for 90/270 rotation. > >> > >>I agree that, until Ville's patch is there, we won't be entertaining > >>any positioning requests on the primary plane and we will not be > >>throwing any error also. > > > >Right...and I think failing to throw an error would be seen as a bug, > >which is why I think Ville's patch needs to go in first. Since it's > >already been reviewed, I'm not aware of anything holding that up from > >happening. > > > Agree, will check with Daniel on this. > > >>But for the 90/270 testcase in kms_rotation_crc to go through, we > >>would need this to create a smaller fb so that we can rotate it. > > > >So is your worry here that drm_plane_helper_check_update() doesn't > >understand rotation and winds up mixing up width/height? If so, I think > >the proper course of action is to write a patch for the helper function > >that makes it rotation-aware. > > > No, the worry is that it rejects a smaller fb for primary plane for all the > platforms. I mentioned 90/270 rotation, because I create a smaller fb > (rather than the full screen fb), so that the rotated plane fits into the > screen. If it is lets say 1920x1080, and the pipe is set at 1920x1080, after > rotation the plane becomes 1080x1920 and the height of the plane surpasses > that of crtc. > For gen >=9 , we can have smaller fb for primary plane which might not cover > the entire crtc. That sounds like a bug in the helper though if it rejects such a framebuffer. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-04-07 8:16 ` Daniel Vetter @ 2015-04-07 8:21 ` Jindal, Sonika 0 siblings, 0 replies; 28+ messages in thread From: Jindal, Sonika @ 2015-04-07 8:21 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On 4/7/2015 1:46 PM, Daniel Vetter wrote: > On Mon, Apr 06, 2015 at 10:50:51AM +0530, Jindal, Sonika wrote: >> >> >> On 4/2/2015 9:18 PM, Matt Roper wrote: >>> On Thu, Apr 02, 2015 at 10:08:27AM +0530, Jindal, Sonika wrote: >>>> >>>> >>>> On 4/1/2015 11:51 PM, Matt Roper wrote: >>>>> On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote: >>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >>>>> >>>>> It looks like this is dependent on Ville's patch >>>>> >>>>> [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane() >>>>> >>>>> to actually let us do something sensible with the destination rectangle >>>>> at the hardware level. Looks like that patch has a r-b, but hasn't made >>>>> it into di-nightly yet. >>>>> >>>> Right now, can_position is used to check for the scenarios where the >>>> primary plane is not covering the complete crtc. This could be due >>>> to positioning or a smaller fb on primary plane. >>>> With Ville's patch, we would be able to allow positioning to happen. >>>> But I need it here, to create a smaller fb for 90/270 rotation. >>>> >>>> I agree that, until Ville's patch is there, we won't be entertaining >>>> any positioning requests on the primary plane and we will not be >>>> throwing any error also. >>> >>> Right...and I think failing to throw an error would be seen as a bug, >>> which is why I think Ville's patch needs to go in first. Since it's >>> already been reviewed, I'm not aware of anything holding that up from >>> happening. >>> >> Agree, will check with Daniel on this. >> >>>> But for the 90/270 testcase in kms_rotation_crc to go through, we >>>> would need this to create a smaller fb so that we can rotate it. >>> >>> So is your worry here that drm_plane_helper_check_update() doesn't >>> understand rotation and winds up mixing up width/height? If so, I think >>> the proper course of action is to write a patch for the helper function >>> that makes it rotation-aware. >>> >> No, the worry is that it rejects a smaller fb for primary plane for all the >> platforms. I mentioned 90/270 rotation, because I create a smaller fb >> (rather than the full screen fb), so that the rotated plane fits into the >> screen. If it is lets say 1920x1080, and the pipe is set at 1920x1080, after >> rotation the plane becomes 1080x1920 and the height of the plane surpasses >> that of crtc. >> For gen >=9 , we can have smaller fb for primary plane which might not cover >> the entire crtc. > > That sounds like a bug in the helper though if it rejects such a > framebuffer. > -Daniel Till now we used to have primary plane covering the crtc, so its not a bug. For gen > 9 we can have smaller primary planes (or with other platforms as well?) Anyways, we need to add this patch to get past that restriction. Regards, Sonika > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] drm/i915/skl: Allow universal planes to position @ 2015-04-10 9:07 Sonika Jindal 2015-10-06 13:32 ` Tvrtko Ursulin 0 siblings, 1 reply; 28+ messages in thread From: Sonika Jindal @ 2015-04-10 9:07 UTC (permalink / raw) To: intel-gfx Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ceb2e61..f0bbc22 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; + bool can_position = false; int ret; crtc = crtc ? crtc : plane->crtc; intel_crtc = to_intel_crtc(crtc); + if (INTEL_INFO(dev)->gen >= 9) + can_position = true; + ret = drm_plane_helper_check_update(plane, crtc, fb, src, dest, clip, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, - false, true, &state->visible); + can_position, true, + &state->visible); if (ret) return ret; -- 1.7.10.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-04-10 9:07 Sonika Jindal @ 2015-10-06 13:32 ` Tvrtko Ursulin 2015-10-06 14:29 ` Matt Roper 0 siblings, 1 reply; 28+ messages in thread From: Tvrtko Ursulin @ 2015-10-06 13:32 UTC (permalink / raw) To: intel-gfx On 10/04/15 10:07, Sonika Jindal wrote: > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index ceb2e61..f0bbc22 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > struct drm_rect *dest = &state->dst; > struct drm_rect *src = &state->src; > const struct drm_rect *clip = &state->clip; > + bool can_position = false; > int ret; > > crtc = crtc ? crtc : plane->crtc; > intel_crtc = to_intel_crtc(crtc); > > + if (INTEL_INFO(dev)->gen >= 9) > + can_position = true; > + > ret = drm_plane_helper_check_update(plane, crtc, fb, > src, dest, clip, > DRM_PLANE_HELPER_NO_SCALING, > DRM_PLANE_HELPER_NO_SCALING, > - false, true, &state->visible); > + can_position, true, > + &state->visible); > if (ret) > return ret; > > I have discovered today that, while this allows SetCrtc and SetPlane ioctls to work with frame buffers which do not cover the plane, page flips are not that lucky and fail roughly with: [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC viewport 1920x1080+0+0. I have posted a quick IGT exerciser for this as "kms_rotation_crc: Excercise page flips with 90 degree rotation". May not be that great but shows the failure. I am not that hot on meddling with this code, nor do I feel competent to even try on my own at least. :/ Maybe just because the atomic and plane related rewrites have been going on for so long, and have multiple people involved, it all sounds pretty scary and fragile. But I think some sort of plan on how to fix this could be in order? Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-10-06 13:32 ` Tvrtko Ursulin @ 2015-10-06 14:29 ` Matt Roper 2015-10-06 14:42 ` Ville Syrjälä 0 siblings, 1 reply; 28+ messages in thread From: Matt Roper @ 2015-10-06 14:29 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: intel-gfx On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > On 10/04/15 10:07, Sonika Jindal wrote: > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > >--- > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >index ceb2e61..f0bbc22 100644 > >--- a/drivers/gpu/drm/i915/intel_display.c > >+++ b/drivers/gpu/drm/i915/intel_display.c > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > struct drm_rect *dest = &state->dst; > > struct drm_rect *src = &state->src; > > const struct drm_rect *clip = &state->clip; > >+ bool can_position = false; > > int ret; > > > > crtc = crtc ? crtc : plane->crtc; > > intel_crtc = to_intel_crtc(crtc); > > > >+ if (INTEL_INFO(dev)->gen >= 9) > >+ can_position = true; > >+ > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > src, dest, clip, > > DRM_PLANE_HELPER_NO_SCALING, > > DRM_PLANE_HELPER_NO_SCALING, > >- false, true, &state->visible); > >+ can_position, true, > >+ &state->visible); > > if (ret) > > return ret; > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > ioctls to work with frame buffers which do not cover the plane, page > flips are not that lucky and fail roughly with: > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > viewport 1920x1080+0+0. Maybe I'm misunderstanding your explanation, but a framebuffer is always required to fill/cover the plane scanning out of it. What this patch is supposed to be allowing is for the primary plane to not cover the entire CRTC (since that's something that only became possible for Intel hardware on the gen9+ platforms). I.e., the primary plane is now allowed to positioned and resized to cover a subset of the CRTC area, just like "sprite" planes have always been able to. If you've got a 1080x1080 framebuffer, then it's legal to have a 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. However it is not legal to size the primary plane as 1920x1080 and use this same 1080x1080 framebuffer with any of our interfaces (setplane, setcrtc, pageflip, or atomic). Are you using ioctls/libdrm directly or are you using igt_kms helpers? IIRC, the IGT helpers will try to be extra helpful and automatically size the plane to match the framebuffer (unless you override that behavior), so that might be what's causing the confusion here. Matt > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > Excercise page flips with 90 degree rotation". May not be that great > but shows the failure. > > I am not that hot on meddling with this code, nor do I feel > competent to even try on my own at least. :/ Maybe just because the > atomic and plane related rewrites have been going on for so long, > and have multiple people involved, it all sounds pretty scary and > fragile. > > But I think some sort of plan on how to fix this could be in order? > > Regards, > > Tvrtko -- 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] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-10-06 14:29 ` Matt Roper @ 2015-10-06 14:42 ` Ville Syrjälä 2015-10-06 15:16 ` Matt Roper 0 siblings, 1 reply; 28+ messages in thread From: Ville Syrjälä @ 2015-10-06 14:42 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > > > On 10/04/15 10:07, Sonika Jindal wrote: > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > >--- > > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > >index ceb2e61..f0bbc22 100644 > > >--- a/drivers/gpu/drm/i915/intel_display.c > > >+++ b/drivers/gpu/drm/i915/intel_display.c > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > > struct drm_rect *dest = &state->dst; > > > struct drm_rect *src = &state->src; > > > const struct drm_rect *clip = &state->clip; > > >+ bool can_position = false; > > > int ret; > > > > > > crtc = crtc ? crtc : plane->crtc; > > > intel_crtc = to_intel_crtc(crtc); > > > > > >+ if (INTEL_INFO(dev)->gen >= 9) > > >+ can_position = true; > > >+ > > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > > src, dest, clip, > > > DRM_PLANE_HELPER_NO_SCALING, > > > DRM_PLANE_HELPER_NO_SCALING, > > >- false, true, &state->visible); > > >+ can_position, true, > > >+ &state->visible); > > > if (ret) > > > return ret; > > > > > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > > ioctls to work with frame buffers which do not cover the plane, page > > flips are not that lucky and fail roughly with: > > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > > viewport 1920x1080+0+0. > > Maybe I'm misunderstanding your explanation, but a framebuffer is always > required to fill/cover the plane scanning out of it. What this patch is > supposed to be allowing is for the primary plane to not cover the entire > CRTC (since that's something that only became possible for Intel > hardware on the gen9+ platforms). I.e., the primary plane is now > allowed to positioned and resized to cover a subset of the CRTC area, > just like "sprite" planes have always been able to. > > If you've got a 1080x1080 framebuffer, then it's legal to have a > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. > However it is not legal to size the primary plane as 1920x1080 and use > this same 1080x1080 framebuffer with any of our interfaces (setplane, > setcrtc, pageflip, or atomic). > > Are you using ioctls/libdrm directly or are you using igt_kms helpers? > IIRC, the IGT helpers will try to be extra helpful and automatically > size the plane to match the framebuffer (unless you override that > behavior), so that might be what's causing the confusion here. The problem is clear as day in drm_mode_page_flip_ioctl(): ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); if (ret) goto out; The fix should be easy; just extract the current src coordinates from the plane state and check those against the new fb size. And then hope that the plane state is really up to date. And I'm sure rotated cases will go boom in some other ways. Probably we should just switch over to using the full plane update for mmio flips to fix it. > > > Matt > > > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > > Excercise page flips with 90 degree rotation". May not be that great > > but shows the failure. > > > > I am not that hot on meddling with this code, nor do I feel > > competent to even try on my own at least. :/ Maybe just because the > > atomic and plane related rewrites have been going on for so long, > > and have multiple people involved, it all sounds pretty scary and > > fragile. > > > > But I think some sort of plan on how to fix this could be in order? > > > > Regards, > > > > Tvrtko > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-10-06 14:42 ` Ville Syrjälä @ 2015-10-06 15:16 ` Matt Roper 2015-10-06 16:28 ` Ville Syrjälä 0 siblings, 1 reply; 28+ messages in thread From: Matt Roper @ 2015-10-06 15:16 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: > On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: > > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > > > > > On 10/04/15 10:07, Sonika Jindal wrote: > > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > >--- > > > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > >index ceb2e61..f0bbc22 100644 > > > >--- a/drivers/gpu/drm/i915/intel_display.c > > > >+++ b/drivers/gpu/drm/i915/intel_display.c > > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > > > struct drm_rect *dest = &state->dst; > > > > struct drm_rect *src = &state->src; > > > > const struct drm_rect *clip = &state->clip; > > > >+ bool can_position = false; > > > > int ret; > > > > > > > > crtc = crtc ? crtc : plane->crtc; > > > > intel_crtc = to_intel_crtc(crtc); > > > > > > > >+ if (INTEL_INFO(dev)->gen >= 9) > > > >+ can_position = true; > > > >+ > > > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > > > src, dest, clip, > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > DRM_PLANE_HELPER_NO_SCALING, > > > >- false, true, &state->visible); > > > >+ can_position, true, > > > >+ &state->visible); > > > > if (ret) > > > > return ret; > > > > > > > > > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > > > ioctls to work with frame buffers which do not cover the plane, page > > > flips are not that lucky and fail roughly with: > > > > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > > > viewport 1920x1080+0+0. > > > > Maybe I'm misunderstanding your explanation, but a framebuffer is always > > required to fill/cover the plane scanning out of it. What this patch is > > supposed to be allowing is for the primary plane to not cover the entire > > CRTC (since that's something that only became possible for Intel > > hardware on the gen9+ platforms). I.e., the primary plane is now > > allowed to positioned and resized to cover a subset of the CRTC area, > > just like "sprite" planes have always been able to. > > > > If you've got a 1080x1080 framebuffer, then it's legal to have a > > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. > > However it is not legal to size the primary plane as 1920x1080 and use > > this same 1080x1080 framebuffer with any of our interfaces (setplane, > > setcrtc, pageflip, or atomic). > > > > Are you using ioctls/libdrm directly or are you using igt_kms helpers? > > IIRC, the IGT helpers will try to be extra helpful and automatically > > size the plane to match the framebuffer (unless you override that > > behavior), so that might be what's causing the confusion here. > > The problem is clear as day in drm_mode_page_flip_ioctl(): > ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); > if (ret) > goto out; > > The fix should be easy; just extract the current src coordinates from > the plane state and check those against the new fb size. And then hope > that the plane state is really up to date. Yep, that's the conclusion we came to once Tvrtko explained what he was seeing on IRC. I'm not sure whether non-atomic drivers have enough state setup by the default helpers to work properly. Worst case we'll just assume that a non-atomic driver won't support primary plane windowing (since none have in the past) and fall back to looking at the mode for legacy non-atomic drivers. > > And I'm sure rotated cases will go boom in some other ways. Probably > we should just switch over to using the full plane update for mmio > flips to fix it. Yeah; the core looks at a drm_plane->invert_dimensions field that's only set by omap. That should probably be updated to look at the state's rotation on atomic-capable drivers. Matt > > > > > > > Matt > > > > > > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > > > Excercise page flips with 90 degree rotation". May not be that great > > > but shows the failure. > > > > > > I am not that hot on meddling with this code, nor do I feel > > > competent to even try on my own at least. :/ Maybe just because the > > > atomic and plane related rewrites have been going on for so long, > > > and have multiple people involved, it all sounds pretty scary and > > > fragile. > > > > > > But I think some sort of plan on how to fix this could be in order? > > > > > > Regards, > > > > > > Tvrtko > > > > -- > > Matt Roper > > Graphics Software Engineer > > IoTG Platform Enabling & Development > > Intel Corporation > > (916) 356-2795 > > -- > Ville Syrjälä > Intel OTC -- 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] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-10-06 15:16 ` Matt Roper @ 2015-10-06 16:28 ` Ville Syrjälä 2015-10-07 14:19 ` Daniel Vetter 0 siblings, 1 reply; 28+ messages in thread From: Ville Syrjälä @ 2015-10-06 16:28 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote: > On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: > > On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: > > > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > > > > > > > On 10/04/15 10:07, Sonika Jindal wrote: > > > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > >--- > > > > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > >index ceb2e61..f0bbc22 100644 > > > > >--- a/drivers/gpu/drm/i915/intel_display.c > > > > >+++ b/drivers/gpu/drm/i915/intel_display.c > > > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > > > > struct drm_rect *dest = &state->dst; > > > > > struct drm_rect *src = &state->src; > > > > > const struct drm_rect *clip = &state->clip; > > > > >+ bool can_position = false; > > > > > int ret; > > > > > > > > > > crtc = crtc ? crtc : plane->crtc; > > > > > intel_crtc = to_intel_crtc(crtc); > > > > > > > > > >+ if (INTEL_INFO(dev)->gen >= 9) > > > > >+ can_position = true; > > > > >+ > > > > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > > > > src, dest, clip, > > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > >- false, true, &state->visible); > > > > >+ can_position, true, > > > > >+ &state->visible); > > > > > if (ret) > > > > > return ret; > > > > > > > > > > > > > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > > > > ioctls to work with frame buffers which do not cover the plane, page > > > > flips are not that lucky and fail roughly with: > > > > > > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > > > > viewport 1920x1080+0+0. > > > > > > Maybe I'm misunderstanding your explanation, but a framebuffer is always > > > required to fill/cover the plane scanning out of it. What this patch is > > > supposed to be allowing is for the primary plane to not cover the entire > > > CRTC (since that's something that only became possible for Intel > > > hardware on the gen9+ platforms). I.e., the primary plane is now > > > allowed to positioned and resized to cover a subset of the CRTC area, > > > just like "sprite" planes have always been able to. > > > > > > If you've got a 1080x1080 framebuffer, then it's legal to have a > > > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. > > > However it is not legal to size the primary plane as 1920x1080 and use > > > this same 1080x1080 framebuffer with any of our interfaces (setplane, > > > setcrtc, pageflip, or atomic). > > > > > > Are you using ioctls/libdrm directly or are you using igt_kms helpers? > > > IIRC, the IGT helpers will try to be extra helpful and automatically > > > size the plane to match the framebuffer (unless you override that > > > behavior), so that might be what's causing the confusion here. > > > > The problem is clear as day in drm_mode_page_flip_ioctl(): > > ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); > > if (ret) > > goto out; > > > > The fix should be easy; just extract the current src coordinates from > > the plane state and check those against the new fb size. And then hope > > that the plane state is really up to date. > > Yep, that's the conclusion we came to once Tvrtko explained what he was > seeing on IRC. I'm not sure whether non-atomic drivers have enough > state setup by the default helpers to work properly. Worst case we'll > just assume that a non-atomic driver won't support primary plane > windowing (since none have in the past) and fall back to looking at the > mode for legacy non-atomic drivers. > > > > > And I'm sure rotated cases will go boom in some other ways. Probably > > we should just switch over to using the full plane update for mmio > > flips to fix it. > > Yeah; the core looks at a drm_plane->invert_dimensions field that's only > set by omap. That should probably be updated to look at the state's > rotation on atomic-capable drivers. We can just look at the src coordinates. Those always match the fb orientation. > > > Matt > > > > > > > > > > > > Matt > > > > > > > > > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > > > > Excercise page flips with 90 degree rotation". May not be that great > > > > but shows the failure. > > > > > > > > I am not that hot on meddling with this code, nor do I feel > > > > competent to even try on my own at least. :/ Maybe just because the > > > > atomic and plane related rewrites have been going on for so long, > > > > and have multiple people involved, it all sounds pretty scary and > > > > fragile. > > > > > > > > But I think some sort of plan on how to fix this could be in order? > > > > > > > > Regards, > > > > > > > > Tvrtko > > > > > > -- > > > Matt Roper > > > Graphics Software Engineer > > > IoTG Platform Enabling & Development > > > Intel Corporation > > > (916) 356-2795 > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-10-06 16:28 ` Ville Syrjälä @ 2015-10-07 14:19 ` Daniel Vetter 2015-10-08 8:58 ` Tvrtko Ursulin 0 siblings, 1 reply; 28+ messages in thread From: Daniel Vetter @ 2015-10-07 14:19 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote: > On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote: > > On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: > > > On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: > > > > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > > > > > > > > > On 10/04/15 10:07, Sonika Jindal wrote: > > > > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > >--- > > > > > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > >index ceb2e61..f0bbc22 100644 > > > > > >--- a/drivers/gpu/drm/i915/intel_display.c > > > > > >+++ b/drivers/gpu/drm/i915/intel_display.c > > > > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > > > > > struct drm_rect *dest = &state->dst; > > > > > > struct drm_rect *src = &state->src; > > > > > > const struct drm_rect *clip = &state->clip; > > > > > >+ bool can_position = false; > > > > > > int ret; > > > > > > > > > > > > crtc = crtc ? crtc : plane->crtc; > > > > > > intel_crtc = to_intel_crtc(crtc); > > > > > > > > > > > >+ if (INTEL_INFO(dev)->gen >= 9) > > > > > >+ can_position = true; > > > > > >+ > > > > > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > > > > > src, dest, clip, > > > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > > >- false, true, &state->visible); > > > > > >+ can_position, true, > > > > > >+ &state->visible); > > > > > > if (ret) > > > > > > return ret; > > > > > > > > > > > > > > > > > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > > > > > ioctls to work with frame buffers which do not cover the plane, page > > > > > flips are not that lucky and fail roughly with: > > > > > > > > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > > > > > viewport 1920x1080+0+0. > > > > > > > > Maybe I'm misunderstanding your explanation, but a framebuffer is always > > > > required to fill/cover the plane scanning out of it. What this patch is > > > > supposed to be allowing is for the primary plane to not cover the entire > > > > CRTC (since that's something that only became possible for Intel > > > > hardware on the gen9+ platforms). I.e., the primary plane is now > > > > allowed to positioned and resized to cover a subset of the CRTC area, > > > > just like "sprite" planes have always been able to. > > > > > > > > If you've got a 1080x1080 framebuffer, then it's legal to have a > > > > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. > > > > However it is not legal to size the primary plane as 1920x1080 and use > > > > this same 1080x1080 framebuffer with any of our interfaces (setplane, > > > > setcrtc, pageflip, or atomic). > > > > > > > > Are you using ioctls/libdrm directly or are you using igt_kms helpers? > > > > IIRC, the IGT helpers will try to be extra helpful and automatically > > > > size the plane to match the framebuffer (unless you override that > > > > behavior), so that might be what's causing the confusion here. > > > > > > The problem is clear as day in drm_mode_page_flip_ioctl(): > > > ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); > > > if (ret) > > > goto out; > > > > > > The fix should be easy; just extract the current src coordinates from > > > the plane state and check those against the new fb size. And then hope > > > that the plane state is really up to date. > > > > Yep, that's the conclusion we came to once Tvrtko explained what he was > > seeing on IRC. I'm not sure whether non-atomic drivers have enough > > state setup by the default helpers to work properly. Worst case we'll > > just assume that a non-atomic driver won't support primary plane > > windowing (since none have in the past) and fall back to looking at the > > mode for legacy non-atomic drivers. > > > > > > > > And I'm sure rotated cases will go boom in some other ways. Probably > > > we should just switch over to using the full plane update for mmio > > > flips to fix it. > > > > Yeah; the core looks at a drm_plane->invert_dimensions field that's only > > set by omap. That should probably be updated to look at the state's > > rotation on atomic-capable drivers. > > We can just look at the src coordinates. Those always match the fb > orientation. Can we just not bother with legacy pageflips on rotated planes? setplane works and once you rotate it kinda gets nasty anyway. The problem I see is that with legacy pageflip we also need to hack up something that doesn't look at plane->state for legacy and all that for a grand total of about 2 drivers, both getting converted to atomic. -Daniel > > > > > > > Matt > > > > > > > > > > > > > > > > > Matt > > > > > > > > > > > > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > > > > > Excercise page flips with 90 degree rotation". May not be that great > > > > > but shows the failure. > > > > > > > > > > I am not that hot on meddling with this code, nor do I feel > > > > > competent to even try on my own at least. :/ Maybe just because the > > > > > atomic and plane related rewrites have been going on for so long, > > > > > and have multiple people involved, it all sounds pretty scary and > > > > > fragile. > > > > > > > > > > But I think some sort of plan on how to fix this could be in order? > > > > > > > > > > Regards, > > > > > > > > > > Tvrtko > > > > > > > > -- > > > > Matt Roper > > > > Graphics Software Engineer > > > > IoTG Platform Enabling & Development > > > > Intel Corporation > > > > (916) 356-2795 > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > > -- > > Matt Roper > > Graphics Software Engineer > > IoTG Platform Enabling & Development > > Intel Corporation > > (916) 356-2795 > > -- > Ville Syrjälä > Intel OTC -- Daniel Vetter Software Engineer, Intel Corporation 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] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-10-07 14:19 ` Daniel Vetter @ 2015-10-08 8:58 ` Tvrtko Ursulin 2015-10-16 12:23 ` Tvrtko Ursulin 0 siblings, 1 reply; 28+ messages in thread From: Tvrtko Ursulin @ 2015-10-08 8:58 UTC (permalink / raw) To: Daniel Vetter, Ville Syrjälä; +Cc: intel-gfx On 07/10/15 15:19, Daniel Vetter wrote: > On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote: >> On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote: >>> On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: >>>> On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: >>>>> On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: >>>>>> >>>>>> On 10/04/15 10:07, Sonika Jindal wrote: >>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >>>>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/intel_display.c | 7 ++++++- >>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>>>> index ceb2e61..f0bbc22 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, >>>>>>> struct drm_rect *dest = &state->dst; >>>>>>> struct drm_rect *src = &state->src; >>>>>>> const struct drm_rect *clip = &state->clip; >>>>>>> + bool can_position = false; >>>>>>> int ret; >>>>>>> >>>>>>> crtc = crtc ? crtc : plane->crtc; >>>>>>> intel_crtc = to_intel_crtc(crtc); >>>>>>> >>>>>>> + if (INTEL_INFO(dev)->gen >= 9) >>>>>>> + can_position = true; >>>>>>> + >>>>>>> ret = drm_plane_helper_check_update(plane, crtc, fb, >>>>>>> src, dest, clip, >>>>>>> DRM_PLANE_HELPER_NO_SCALING, >>>>>>> DRM_PLANE_HELPER_NO_SCALING, >>>>>>> - false, true, &state->visible); >>>>>>> + can_position, true, >>>>>>> + &state->visible); >>>>>>> if (ret) >>>>>>> return ret; >>>>>>> >>>>>>> >>>>>> >>>>>> I have discovered today that, while this allows SetCrtc and SetPlane >>>>>> ioctls to work with frame buffers which do not cover the plane, page >>>>>> flips are not that lucky and fail roughly with: >>>>>> >>>>>> [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC >>>>>> viewport 1920x1080+0+0. >>>>> >>>>> Maybe I'm misunderstanding your explanation, but a framebuffer is always >>>>> required to fill/cover the plane scanning out of it. What this patch is >>>>> supposed to be allowing is for the primary plane to not cover the entire >>>>> CRTC (since that's something that only became possible for Intel >>>>> hardware on the gen9+ platforms). I.e., the primary plane is now >>>>> allowed to positioned and resized to cover a subset of the CRTC area, >>>>> just like "sprite" planes have always been able to. >>>>> >>>>> If you've got a 1080x1080 framebuffer, then it's legal to have a >>>>> 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. >>>>> However it is not legal to size the primary plane as 1920x1080 and use >>>>> this same 1080x1080 framebuffer with any of our interfaces (setplane, >>>>> setcrtc, pageflip, or atomic). >>>>> >>>>> Are you using ioctls/libdrm directly or are you using igt_kms helpers? >>>>> IIRC, the IGT helpers will try to be extra helpful and automatically >>>>> size the plane to match the framebuffer (unless you override that >>>>> behavior), so that might be what's causing the confusion here. >>>> >>>> The problem is clear as day in drm_mode_page_flip_ioctl(): >>>> ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); >>>> if (ret) >>>> goto out; >>>> >>>> The fix should be easy; just extract the current src coordinates from >>>> the plane state and check those against the new fb size. And then hope >>>> that the plane state is really up to date. >>> >>> Yep, that's the conclusion we came to once Tvrtko explained what he was >>> seeing on IRC. I'm not sure whether non-atomic drivers have enough >>> state setup by the default helpers to work properly. Worst case we'll >>> just assume that a non-atomic driver won't support primary plane >>> windowing (since none have in the past) and fall back to looking at the >>> mode for legacy non-atomic drivers. >>> >>>> >>>> And I'm sure rotated cases will go boom in some other ways. Probably >>>> we should just switch over to using the full plane update for mmio >>>> flips to fix it. >>> >>> Yeah; the core looks at a drm_plane->invert_dimensions field that's only >>> set by omap. That should probably be updated to look at the state's >>> rotation on atomic-capable drivers. >> >> We can just look at the src coordinates. Those always match the fb >> orientation. > > Can we just not bother with legacy pageflips on rotated planes? setplane > works and once you rotate it kinda gets nasty anyway. I don't know - thought it is simple enough to make it work so why not? Just " [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip" I posted, plus Matt's "[PATCH] drm: Check fb against plane size rather than CRTC mode for pageflip" to allow smaller than mode planes. > The problem I see is that with legacy pageflip we also need to hack up > something that doesn't look at plane->state for legacy and all that for a > grand total of about 2 drivers, both getting converted to atomic. I'll leave the legacy/atomic/etc considerations to the experts. :) Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position 2015-10-08 8:58 ` Tvrtko Ursulin @ 2015-10-16 12:23 ` Tvrtko Ursulin 0 siblings, 0 replies; 28+ messages in thread From: Tvrtko Ursulin @ 2015-10-16 12:23 UTC (permalink / raw) To: Daniel Vetter, Ville Syrjälä; +Cc: intel-gfx On 08/10/15 09:58, Tvrtko Ursulin wrote: > On 07/10/15 15:19, Daniel Vetter wrote: >> On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote: >>> On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote: >>>> On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: >>>>> On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: >>>>>> On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: >>>>>>> >>>>>>> On 10/04/15 10:07, Sonika Jindal wrote: >>>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >>>>>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/intel_display.c | 7 ++++++- >>>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c >>>>>>>> b/drivers/gpu/drm/i915/intel_display.c >>>>>>>> index ceb2e61..f0bbc22 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>>>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct >>>>>>>> drm_plane *plane, >>>>>>>> struct drm_rect *dest = &state->dst; >>>>>>>> struct drm_rect *src = &state->src; >>>>>>>> const struct drm_rect *clip = &state->clip; >>>>>>>> + bool can_position = false; >>>>>>>> int ret; >>>>>>>> >>>>>>>> crtc = crtc ? crtc : plane->crtc; >>>>>>>> intel_crtc = to_intel_crtc(crtc); >>>>>>>> >>>>>>>> + if (INTEL_INFO(dev)->gen >= 9) >>>>>>>> + can_position = true; >>>>>>>> + >>>>>>>> ret = drm_plane_helper_check_update(plane, crtc, fb, >>>>>>>> src, dest, clip, >>>>>>>> DRM_PLANE_HELPER_NO_SCALING, >>>>>>>> DRM_PLANE_HELPER_NO_SCALING, >>>>>>>> - false, true, &state->visible); >>>>>>>> + can_position, true, >>>>>>>> + &state->visible); >>>>>>>> if (ret) >>>>>>>> return ret; >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> I have discovered today that, while this allows SetCrtc and SetPlane >>>>>>> ioctls to work with frame buffers which do not cover the plane, page >>>>>>> flips are not that lucky and fail roughly with: >>>>>>> >>>>>>> [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC >>>>>>> viewport 1920x1080+0+0. >>>>>> >>>>>> Maybe I'm misunderstanding your explanation, but a framebuffer is >>>>>> always >>>>>> required to fill/cover the plane scanning out of it. What this >>>>>> patch is >>>>>> supposed to be allowing is for the primary plane to not cover the >>>>>> entire >>>>>> CRTC (since that's something that only became possible for Intel >>>>>> hardware on the gen9+ platforms). I.e., the primary plane is now >>>>>> allowed to positioned and resized to cover a subset of the CRTC area, >>>>>> just like "sprite" planes have always been able to. >>>>>> >>>>>> If you've got a 1080x1080 framebuffer, then it's legal to have a >>>>>> 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. >>>>>> However it is not legal to size the primary plane as 1920x1080 and >>>>>> use >>>>>> this same 1080x1080 framebuffer with any of our interfaces (setplane, >>>>>> setcrtc, pageflip, or atomic). >>>>>> >>>>>> Are you using ioctls/libdrm directly or are you using igt_kms >>>>>> helpers? >>>>>> IIRC, the IGT helpers will try to be extra helpful and automatically >>>>>> size the plane to match the framebuffer (unless you override that >>>>>> behavior), so that might be what's causing the confusion here. >>>>> >>>>> The problem is clear as day in drm_mode_page_flip_ioctl(): >>>>> ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, >>>>> fb); >>>>> if (ret) >>>>> goto out; >>>>> >>>>> The fix should be easy; just extract the current src coordinates from >>>>> the plane state and check those against the new fb size. And then hope >>>>> that the plane state is really up to date. >>>> >>>> Yep, that's the conclusion we came to once Tvrtko explained what he was >>>> seeing on IRC. I'm not sure whether non-atomic drivers have enough >>>> state setup by the default helpers to work properly. Worst case we'll >>>> just assume that a non-atomic driver won't support primary plane >>>> windowing (since none have in the past) and fall back to looking at the >>>> mode for legacy non-atomic drivers. >>>> >>>>> >>>>> And I'm sure rotated cases will go boom in some other ways. Probably >>>>> we should just switch over to using the full plane update for mmio >>>>> flips to fix it. >>>> >>>> Yeah; the core looks at a drm_plane->invert_dimensions field that's >>>> only >>>> set by omap. That should probably be updated to look at the state's >>>> rotation on atomic-capable drivers. >>> >>> We can just look at the src coordinates. Those always match the fb >>> orientation. >> >> Can we just not bother with legacy pageflips on rotated planes? setplane >> works and once you rotate it kinda gets nasty anyway. > > I don't know - thought it is simple enough to make it work so why not? > Just " [PATCH] drm/i915: Consider plane rotation when calculating stride > in skl_do_mmio_flip" I posted, plus Matt's "[PATCH] drm: Check fb > against plane size rather than CRTC mode for pageflip" to allow smaller > than mode planes. > >> The problem I see is that with legacy pageflip we also need to hack up >> something that doesn't look at plane->state for legacy and all that for a >> grand total of about 2 drivers, both getting converted to atomic. > > I'll leave the legacy/atomic/etc considerations to the experts. :) Are we sure any efforts to support rotation in legacy page flips is not worth it? So far there were three patches for this: Plane programming fix (very simple) and an IGT test case (simple as well) from me, and a sub-crtc size plane support from Matt. It kind of remained hanging a bit so I think it would be good to make a definitive decision. Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2015-10-16 12:23 UTC | newest] Thread overview: 28+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-30 8:34 [PATCH 1/2] drm/i915/skl: Allow universal planes to position Sonika Jindal 2015-03-30 8:34 ` [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation Sonika Jindal 2015-03-30 12:15 ` shuang.he 2015-04-01 18:22 ` Matt Roper 2015-04-02 4:54 ` Jindal, Sonika 2015-04-02 8:55 ` Tvrtko Ursulin 2015-04-02 15:59 ` Matt Roper 2015-04-06 12:27 ` Jindal, Sonika 2015-04-07 8:13 ` Daniel Vetter 2015-04-07 8:22 ` Jindal, Sonika 2015-04-07 8:26 ` [PATCH] " Sonika Jindal 2015-04-07 10:59 ` shuang.he 2015-04-09 22:54 ` Matt Roper 2015-04-01 18:21 ` [PATCH 1/2] drm/i915/skl: Allow universal planes to position Matt Roper 2015-04-02 4:38 ` Jindal, Sonika 2015-04-02 15:48 ` Matt Roper 2015-04-06 5:20 ` Jindal, Sonika 2015-04-07 8:16 ` Daniel Vetter 2015-04-07 8:21 ` Jindal, Sonika -- strict thread matches above, loose matches on Subject: below -- 2015-04-10 9:07 Sonika Jindal 2015-10-06 13:32 ` Tvrtko Ursulin 2015-10-06 14:29 ` Matt Roper 2015-10-06 14:42 ` Ville Syrjälä 2015-10-06 15:16 ` Matt Roper 2015-10-06 16:28 ` Ville Syrjälä 2015-10-07 14:19 ` Daniel Vetter 2015-10-08 8:58 ` Tvrtko Ursulin 2015-10-16 12:23 ` Tvrtko Ursulin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox