| Message ID | 1427704497-28344-2-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 '*'
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
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 >
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
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 > >
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 >>> >
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
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 >
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);
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(-)