| Message ID | 1426398946-5900-9-git-send-email-chandra.konduru@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Mar 15, 2015 at 6:55 AM, Chandra Konduru <chandra.konduru@intel.com> wrote: > + /* > + * check for rect size: > + * min sizes in case of scaling involved > + * max sizes in all cases > + */ > + if ((need_scaling && > + (src_w < scaler->min_src_w || src_h < scaler->min_src_h || > + dst_w < scaler->min_dst_w || dst_h < scaler->min_dst_h)) || > + > + src_w > scaler->max_src_w || src_h > scaler->max_src_h || > + dst_w > scaler->max_dst_w || dst_h > scaler->max_dst_h) { Ok let's hope I've traced all the min/max stuff in your patches correctly. It looks like we only ever initialized them to fixed values, never changed them and only use them here. Spreading out the values like that without having a real need for this flexibility makes review really hard imo. What about instead adding a skl_check_scale_limits functions which does all these checks here and uses hardcoded values? That way you could move the commits about the various values (e.g. only 34% scaling and the other easier-to-understand limits) right next to the code that checks these limits? There's also some confusion with the overly generic (imo) old sprite code and its scaling limit checks. Imo we can look at that later on. -Daniel
> -----Original Message----- > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of > Daniel Vetter > Sent: Wednesday, March 18, 2015 1:16 AM > To: Konduru, Chandra > Cc: intel-gfx; Conselvan De Oliveira, Ander; Vetter, Daniel > Subject: Re: [Intel-gfx] [PATCH 08/21] drm/i915: Add helper function to update > scaler_users in crtc_state > > On Sun, Mar 15, 2015 at 6:55 AM, Chandra Konduru > <chandra.konduru@intel.com> wrote: > > + /* > > + * check for rect size: > > + * min sizes in case of scaling involved > > + * max sizes in all cases > > + */ > > + if ((need_scaling && > > + (src_w < scaler->min_src_w || src_h < scaler->min_src_h || > > + dst_w < scaler->min_dst_w || dst_h < > > + scaler->min_dst_h)) || > > + > > + src_w > scaler->max_src_w || src_h > scaler->max_src_h || > > + dst_w > scaler->max_dst_w || dst_h > > > + scaler->max_dst_h) { > > Ok let's hope I've traced all the min/max stuff in your patches correctly. It looks > like we only ever initialized them to fixed values, never changed them and only > use them here. Spreading out the values like that without having a real need for > this flexibility makes review really hard imo. > > What about instead adding a skl_check_scale_limits functions which does all > these checks here and uses hardcoded values? That way you could move the > commits about the various values (e.g. only 34% scaling and the other easier-to- > understand limits) right next to the code that checks these limits? I agree that some of limits are fixed, ratios aren't fixed. So kept them in state and updated during modeset. There isn't much complexity with current approach. > > There's also some confusion with the overly generic (imo) old sprite code and its > scaling limit checks. Imo we can look at that later on. Agree. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Mar 19, 2015 at 05:43:24PM +0000, Konduru, Chandra wrote: > > > > -----Original Message----- > > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of > > Daniel Vetter > > Sent: Wednesday, March 18, 2015 1:16 AM > > To: Konduru, Chandra > > Cc: intel-gfx; Conselvan De Oliveira, Ander; Vetter, Daniel > > Subject: Re: [Intel-gfx] [PATCH 08/21] drm/i915: Add helper function to update > > scaler_users in crtc_state > > > > On Sun, Mar 15, 2015 at 6:55 AM, Chandra Konduru > > <chandra.konduru@intel.com> wrote: > > > + /* > > > + * check for rect size: > > > + * min sizes in case of scaling involved > > > + * max sizes in all cases > > > + */ > > > + if ((need_scaling && > > > + (src_w < scaler->min_src_w || src_h < scaler->min_src_h || > > > + dst_w < scaler->min_dst_w || dst_h < > > > + scaler->min_dst_h)) || > > > + > > > + src_w > scaler->max_src_w || src_h > scaler->max_src_h || > > > + dst_w > scaler->max_dst_w || dst_h > > > > + scaler->max_dst_h) { > > > > Ok let's hope I've traced all the min/max stuff in your patches correctly. It looks > > like we only ever initialized them to fixed values, never changed them and only > > use them here. Spreading out the values like that without having a real need for > > this flexibility makes review really hard imo. > > > > What about instead adding a skl_check_scale_limits functions which does all > > these checks here and uses hardcoded values? That way you could move the > > commits about the various values (e.g. only 34% scaling and the other easier-to- > > understand limits) right next to the code that checks these limits? > > I agree that some of limits are fixed, ratios aren't fixed. So kept them > in state and updated during modeset. There isn't much complexity with current > approach. Hm, where are the ratios? I haven't found that part in your series ... -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5591282..0eb120d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12356,6 +12356,148 @@ intel_cleanup_plane_fb(struct drm_plane *plane, } } +/* + * Update crtc_state->scaler_users for requested plane or crtc based on state. + * + * plane (in) + * NULL - for crtc + * not NULL - for plane + * Return + * 0 - scaler_usage updated successfully + * error - requested scaling cannot be supported or other error condition + */ +int +skl_update_scaler_users( + struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state, + struct intel_plane *intel_plane, struct intel_plane_state *plane_state) +{ + struct drm_device *dev = intel_crtc->base.dev; + int need_scaling; + int idx; + int src_w, src_h, dst_w, dst_h; + int scaler_id; + struct drm_framebuffer *fb; + struct intel_crtc_scaler_state *scaler_state; + struct intel_scaler *scaler; + + if (!intel_crtc || !crtc_state || INTEL_INFO(dev)->gen < 9 || + (intel_plane && intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)) + return 0; + + scaler_state = &crtc_state->scaler_state; + scaler = &scaler_state->scalers[0]; + + if (!scaler_state->num_scalers) { + DRM_DEBUG_KMS("crtc_state = %p, num_scalers = %d\n", crtc_state, + scaler_state->num_scalers); + return 0; + } + + idx = intel_plane ? drm_plane_index(&intel_plane->base) : SKL_CRTC_INDEX; + fb = intel_plane ? plane_state->base.fb : NULL; + + if (intel_plane) { + src_w = drm_rect_width(&plane_state->src) >> 16; + src_h = drm_rect_height(&plane_state->src) >> 16; + dst_w = drm_rect_width(&plane_state->dst); + dst_h = drm_rect_height(&plane_state->dst); + scaler_id = plane_state->scaler_id; + } else { + struct drm_display_mode *adjusted_mode = + &crtc_state->base.adjusted_mode; + src_w = crtc_state->pipe_src_w; + src_h = crtc_state->pipe_src_h; + dst_w = adjusted_mode->hdisplay; + dst_h = adjusted_mode->vdisplay; + scaler_id = scaler_state->scaler_id; + } + need_scaling = (src_w != dst_w || src_h != dst_h); + + /* + * if plane is being disabled or scaler is no more required + * - free scaler binded to this plane/crtc + * - in order to do this, update crtc->scaler_usage + * + * Here scaler state in crtc_state is set free so that + * scaler can be assigned to other user. Actual register + * update to free the scaler is done in plane/panel-fit programming. + * For this purpose crtc/plane_state->scaler_id isn't reset here. + */ + if (!need_scaling || (intel_plane && (!fb || !plane_state->visible))) { + if (scaler_id >= 0) { + scaler_state->scaler_users &= ~(1 << idx); + scaler_state->scalers[scaler_id].in_use = 0; + + DRM_DEBUG_KMS("Staged freeing scaler id %u.%u from %s:%d " + "crtc_state = %p scaler_users = 0x%x\n", + intel_crtc->pipe, scaler_id, intel_plane ? "PLANE" : "CRTC", + intel_plane ? intel_plane->base.base.id : + intel_crtc->base.base.id, crtc_state, + scaler_state->scaler_users); + } + return 0; + } + + /* + * check for rect size: + * min sizes in case of scaling involved + * max sizes in all cases + */ + if ((need_scaling && + (src_w < scaler->min_src_w || src_h < scaler->min_src_h || + dst_w < scaler->min_dst_w || dst_h < scaler->min_dst_h)) || + + src_w > scaler->max_src_w || src_h > scaler->max_src_h || + dst_w > scaler->max_dst_w || dst_h > scaler->max_dst_h) { + DRM_DEBUG_KMS("%s:%d scaler_user index %u.%u: src %ux%u dst %ux%u " + "size is out of scaler range\n", + intel_plane ? "PLANE" : "CRTC", + intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id, + intel_crtc->pipe, idx, src_w, src_h, dst_w, dst_h); + return -EINVAL; + } + + /* check colorkey */ + if (intel_plane && need_scaling && intel_colorkey_enabled(intel_plane)) { + DRM_DEBUG_KMS("PLANE:%d scaling with color key not allowed", + intel_plane->base.base.id); + return -EINVAL; + } + + /* Check src format */ + if (intel_plane && need_scaling) { + switch (fb->pixel_format) { + case DRM_FORMAT_RGB565: + case DRM_FORMAT_XBGR8888: + case DRM_FORMAT_XRGB8888: + case DRM_FORMAT_ABGR8888: + case DRM_FORMAT_ARGB8888: + 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: + break; + default: + DRM_DEBUG_KMS("PLANE:%d FB:%d format 0x%x not supported\n", + intel_plane->base.base.id, fb->base.id, fb->pixel_format); + return -EINVAL; + } + } + + /* mark this plane as a scaler user in crtc_state */ + scaler_state->scaler_users |= (1 << idx); + DRM_DEBUG_KMS("%s:%d staged scaling request for %ux%u->%ux%u " + "crtc_state = %p scaler_users = 0x%x\n", + intel_plane ? "PLANE" : "CRTC", + intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id, + src_w, src_h, dst_w, dst_h, crtc_state, scaler_state->scaler_users); + return 0; +} + static int intel_check_primary_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 d9a3b64..b56baba 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1138,6 +1138,9 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_state *pipe_config); void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc); void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file); +int skl_update_scaler_users(struct intel_crtc *intel_crtc, + struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane, + struct intel_plane_state *plane_state); /* intel_dp.c */ void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
This helper function stages a scaler request for a plane/crtc into crtc_state->scaler_users (which is a bit field). It also performs required checks before staging any change into scaler_state. Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 142 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 3 + 2 files changed, 145 insertions(+)