[Intel-gfx] Request for feedback : [RFC] Added a new 'window size' property for connector
Goel, Akash
akash.goel at intel.com
Fri Mar 7 13:45:26 CET 2014
Thanks for your feedback.
> No, fb_id is passed through setcrtc (or soon through setplane, I hope).
Actually mentioned 'fb_id' also, because if PIPESRC programming is also being updated (as Panel fitter input), then the 'fb' already attached to 'crtc' may not be aligned with it. So there could be a momentary corruption caused when internal modeset is done by Driver (to change the Panel fitter setting), which will last till the next page flip (or setplane).
> No, we need both the border and the PIPESRC information. hdisplay/vdisplays isn't enough since the user facing mode structure doesn't have vblank_start/end fields. Hence we can't specify borders.
> If we don't want to extend the display mode, we should add border properties of some sort. I know some drivers have added something like that to the connectors, but again since the mode is applied to the crtc, ideally these properties
> should also be on the crtc.
Sorry but it's not quite clear. This is what is my understanding so far, please kindly correct wherever wrong.
1. We need border info for the output of Panel fitter, which will determine the display window. Using the border info we will manipulate the hblank/vblank start/end fields.
2. Currently the values of 'hdisplay/vdisplay' fields sent in 'drm_mode_modeinfo' structure gets programmed in PIPESRC. So if we are adding border info to the same structure, then we can use the 'hdisplay/vdisplay' fields only for
PIPESRC. But if we are defining a new property, to convey the border info, then we need the PIPESRC information also along with it.
Please confirm that should we add a new a 'blob' type property for 'crtc' ?
> Checks like that need to happen at the modeset compute stage, so we can optimize away all the case. We shouldn't sprinkle special cases here and there.
Sorry but actually I meant to have this check in modeset compute function only.
Best regards
Akash
-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
Sent: Friday, March 07, 2014 2:58 PM
To: Goel, Akash
Cc: Chris Wilson; intel-gfx at lists.freedesktop.org; G, Pallavi; Purushothaman, Vijay A
Subject: Re: Request for feedback : [RFC] Added a new 'window size' property for connector
On Fri, Mar 07, 2014 at 09:08:40AM +0000, Goel, Akash wrote:
> Hi Ville,
>
> Please review this patch. This is just a provisional patch.
> Here we have tried to extend the 'scaling mode' property by adding a new 'manual' mode and User will have to specify the display window (Panel fitter's output) through another property. But this may not be enough.
The panel fitter source size property should be on the crtc, not the connector.
>
> To support the arbitrary use of Panel fitter, as per your & Chris's suggestions, we can use the following 2 approaches.
>
> 1. Introduce a new property (of blob type) through which a structure would be passed, which will have the Panel fitter input (PIPESRC) & output info. Using this Driver can then do a modeset internally.
> 'fb_id' info will also be required to passed here, conforming to the PIPESRC.
No, fb_id is passed through setcrtc (or soon through setplane, I hope).
> Or
> 2. Add the border info to the display mode structure which will control the Panel fitter output. The panel fitter input (i.e. PIPESRC) will be provided through the regular 'hdisplay', 'vdisplay' fields.
No, we need both the border and the PIPESRC information.
hdisplay/vdisplays isn't enough since the user facing mode structure doesn't have vblank_start/end fields. Hence we can't specify borders.
If we don't want to extend the display mode, we should add border properties of some sort. I know some drivers have added something like that to the connectors, but again since the mode is applied to the crtc, ideally these properties should also be on the crtc.
> This way the PIPESRC programing & Panel fitter mode update could be done in a single modeset call.
This is a separate issue. We shouldn't add kludges for it since it will get solved properly by the atomic modeset ioctl.
>
> In both the cases, we can have an additional check that if only the Panel fitter input (fb size) is getting changed & Panel fitter reprogramming is not required, then a full modeset can be avoided except the PIPESRC programming.
> This could allow to dynamically flip the frame buffers of different resolution without a modeset.
Checks like that need to happen at the modeset compute stage, so we can optimize away all the case. We shouldn't sprinkle special cases here and there.
>
> Best regards
> Akash
>
> -----Original Message-----
> From: Goel, Akash
> Sent: Tuesday, March 04, 2014 3:42 PM
> To: intel-gfx at lists.freedesktop.org
> Cc: Goel, Akash; G, Pallavi
> Subject: [RFC] drm/i915 : Added a new 'window size' property for
> connector
>
> From: Akash Goel <akash.goel at intel.com>
>
> Added a new 'window size' property for connectors.
> This can be used to control the output window size of Panel fitter.
> Also addd a new mode 'Manual' to existing 'scaling mode'
> property.
>
> Signed-off-by: G Pallavi <pallavi.g at intel.com>
> Signed-off-by: Akash Goel <akash.goel at intel.com>
> ---
> drivers/gpu/drm/drm_crtc.c | 1 +
> drivers/gpu/drm/i915/intel_dp.c | 58 ++++++++++++++++++---
> drivers/gpu/drm/i915/intel_drv.h | 8 +++
> drivers/gpu/drm/i915/intel_panel.c | 104 +++++++++++++++++++++++++++++++++++++
> include/drm/drm_crtc.h | 3 ++
> include/uapi/drm/drm_mode.h | 1 +
> 6 files changed, 167 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 35ea15d..1d237b5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -123,6 +123,7 @@ static const struct drm_prop_enum_list drm_scaling_mode_enum_list[] =
> { DRM_MODE_SCALE_FULLSCREEN, "Full" },
> { DRM_MODE_SCALE_CENTER, "Center" },
> { DRM_MODE_SCALE_ASPECT, "Full aspect" },
> + { DRM_MODE_SCALE_MANUAL, "Manual" },
> };
>
> /*
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c index c512d78..c697b7a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -886,12 +886,22 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> adjusted_mode);
> - if (!HAS_PCH_SPLIT(dev))
> - intel_gmch_panel_fitting(intel_crtc, pipe_config,
> - intel_connector->panel.fitting_mode);
> - else
> - intel_pch_panel_fitting(intel_crtc, pipe_config,
> - intel_connector->panel.fitting_mode);
> + if (!HAS_PCH_SPLIT(dev)) {
> + if (intel_connector->panel.fitting_mode == DRM_MODE_SCALE_MANUAL)
> + intel_gmch_manual_panel_fitting(intel_crtc, pipe_config,
> + intel_connector->panel.display_window_size);
> + else
> + intel_gmch_panel_fitting(intel_crtc, pipe_config,
> + intel_connector->panel.fitting_mode);
> + }
> + else {
> + if (intel_connector->panel.fitting_mode == DRM_MODE_SCALE_MANUAL)
> + intel_pch_manual_panel_fitting(intel_crtc, pipe_config,
> + intel_connector->panel.display_window_size);
> + else
> + intel_pch_panel_fitting(intel_crtc, pipe_config,
> + intel_connector->panel.fitting_mode);
> + }
> }
>
> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) @@ -3376,14 +3386,36 @@ intel_dp_set_property(struct drm_connector *connector,
> }
>
> if (intel_connector->panel.fitting_mode == val) {
> - /* the eDP scaling property is not changed */
> - return 0;
> + /* Check if display window size has changed */
> + if ((val == DRM_MODE_SCALE_MANUAL) &&
> + intel_connector->panel.display_window_size_changed)
> + intel_connector->panel.display_window_size_changed = 0;
> + else {
> + /* the eDP scaling property is not changed */
> + return 0;
> + }
> }
> intel_connector->panel.fitting_mode = val;
>
> + if (val != DRM_MODE_SCALE_MANUAL)
> + intel_connector->panel.display_window_size = 0;
> +
> goto done;
> }
>
> + if (is_edp(intel_dp) &&
> + property == connector->dev->mode_config.window_size_property) {
> + if (intel_connector->panel.display_window_size == val) {
> + /* the eDP panel window size property is not changed */
> + intel_connector->panel.display_window_size_changed = 0;
> + return 0;
> + }
> + intel_connector->panel.display_window_size = val;
> + intel_connector->panel.display_window_size_changed = 1;
> +
> + return 0;
> + }
> +
> return -EINVAL;
>
> done:
> @@ -3517,6 +3549,16 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> connector->dev->mode_config.scaling_mode_property,
> DRM_MODE_SCALE_ASPECT);
> intel_connector->panel.fitting_mode = DRM_MODE_SCALE_ASPECT;
> +
> + connector->dev->mode_config.window_size_property =
> + drm_property_create_range(connector->dev, 0,
> + "window size", 0, 0xFFFFFFFF);
> + drm_object_attach_property(
> + &connector->base,
> + connector->dev->mode_config.window_size_property,
> + 0);
> + intel_connector->panel.display_window_size = 0;
> + intel_connector->panel.display_window_size_changed = 0;
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a4ffc02..5247b6b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -157,6 +157,8 @@ struct intel_panel {
> struct drm_display_mode *fixed_mode;
> struct drm_display_mode *downclock_mode;
> int fitting_mode;
> + u32 display_window_size;
> + bool display_window_size_changed;
>
> /* backlight */
> struct {
> @@ -841,9 +843,15 @@ void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, void intel_pch_panel_fitting(struct intel_crtc *crtc,
> struct intel_crtc_config *pipe_config,
> int fitting_mode);
> +void intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> + struct intel_crtc_config *pipe_config,
> + u32 display_window_size);
> void intel_gmch_panel_fitting(struct intel_crtc *crtc,
> struct intel_crtc_config *pipe_config,
> int fitting_mode);
> +void intel_gmch_manual_panel_fitting(struct intel_crtc *crtc,
> + struct intel_crtc_config *pipe_config,
> + u32 display_window_size);
> void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
> u32 max);
> int intel_panel_setup_backlight(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c
> b/drivers/gpu/drm/i915/intel_panel.c
> index cb05840..2965975 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -114,6 +114,48 @@ done:
> pipe_config->pch_pfit.enabled = pipe_config->pch_pfit.size != 0; }
>
> +void
> +intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> + struct intel_crtc_config *pipe_config,
> + u32 display_window_size)
> +{
> + struct drm_display_mode *adjusted_mode;
> + int x, y;
> + u32 tot_width, tot_height;
> + u32 dst_width, dst_height;
> +
> + adjusted_mode = &pipe_config->adjusted_mode;
> +
> + tot_width = adjusted_mode->crtc_hdisplay;
> + tot_height = adjusted_mode->crtc_vdisplay;
> +
> + dst_width = ((display_window_size >> 16) & 0xffff);
> + dst_height = (display_window_size & 0xffff);
> +
> + x = y = 0;
> +
> + if (tot_width < dst_width) {
> + DRM_ERROR("width is too big\n");
> + return;
> + } else if (dst_width & 1) {
> + DRM_ERROR("width must be even\n");
> + return;
> + } else if (tot_height < dst_height) {
> + DRM_ERROR("height is too big\n");
> + return;
> + } else if (dst_height & 1) {
> + DRM_ERROR("height must be even\n");
> + return;
> + }
> +
> + x = (adjusted_mode->hdisplay - dst_width + 1)/2;
> + y = (adjusted_mode->vdisplay - dst_height + 1)/2;
> +
> + pipe_config->pch_pfit.pos = (x << 16) | y;
> + pipe_config->pch_pfit.size = (dst_width << 16) | dst_height;
> + pipe_config->pch_pfit.enabled = pipe_config->pch_pfit.size != 0; }
> +
> static void
> centre_horizontally(struct drm_display_mode *mode,
> int width)
> @@ -323,6 +365,68 @@ out:
> pipe_config->gmch_pfit.lvds_border_bits = border; }
>
> +void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> + struct intel_crtc_config *pipe_config,
> + u32 display_window_size)
> +{
> + u32 pfit_control = 0, border = 0;
> + u32 pf_horizontal_ratio, pf_vertical_ratio;
> + struct drm_display_mode *adjusted_mode;
> + u32 tot_width, tot_height;
> + u32 src_width, src_height; /* pipesrc.x, pipesrc.y */
> + u32 dst_width, dst_height;
> +
> + adjusted_mode = &pipe_config->adjusted_mode;
> +
> + src_width = pipe_config->pipe_src_w;
> + src_height = pipe_config->pipe_src_h;
> +
> + tot_width = adjusted_mode->crtc_hdisplay;
> + tot_height = adjusted_mode->crtc_vdisplay;
> +
> + dst_width = ((display_window_size >> 16) & 0xffff);
> + dst_height = (display_window_size & 0xffff);
> +
> + pf_horizontal_ratio = panel_fitter_scaling(src_width, dst_width);
> + pf_vertical_ratio = panel_fitter_scaling(src_height, dst_height);
> +
> +/* Max Downscale ratio of 1.125, expressed in 1.12 fixed point format
> +*/ #define MAX_DOWNSCALE_RATIO (0x9 << 9)
> +
> + if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
> + DRM_ERROR("width is too small\n");
> + return;
> + } else if (tot_width < dst_width) {
> + DRM_ERROR("width is too big\n");
> + return;
> + } else if (dst_width & 1) {
> + DRM_ERROR("width must be even\n");
> + return;
> + } else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
> + DRM_ERROR("height is too small\n");
> + return;
> + } else if (tot_height < dst_height) {
> + DRM_ERROR("height is too big\n");
> + return;
> + } else if (dst_height & 1) {
> + DRM_ERROR("height must be even\n");
> + return;
> + }
> +
> + centre_horizontally(adjusted_mode, dst_width);
> + centre_vertically(adjusted_mode, dst_height);
> + border = LVDS_BORDER_ENABLE;
> +
> + /* PFIT_SCALING_PROGRAMMED is de-featured on BYT */
> + pfit_control |= PFIT_ENABLE | PFIT_SCALING_AUTO;
> + pfit_control |= ((intel_crtc->pipe << PFIT_PIPE_SHIFT) |
> +PFIT_FILTER_FUZZY);
> +
> + pipe_config->gmch_pfit.control = pfit_control;
> + pipe_config->gmch_pfit.lvds_border_bits = border;
> +
> + return;
> +}
> +
> static u32 intel_panel_compute_brightness(struct intel_connector *connector,
> u32 val)
> {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
> f764654..625d7fd 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -902,6 +902,9 @@ struct drm_mode_config {
> struct drm_property *scaling_mode_property;
> struct drm_property *dirty_info_property;
>
> + /* Panel fitter output property */
> + struct drm_property *window_size_property;
> +
> /* dumb ioctl parameters */
> uint32_t preferred_depth, prefer_shadow;
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index f104c26..a187dac 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -87,6 +87,7 @@
> #define DRM_MODE_SCALE_FULLSCREEN 1 /* Full screen, ignore aspect */
> #define DRM_MODE_SCALE_CENTER 2 /* Centered, no scaling */
> #define DRM_MODE_SCALE_ASPECT 3 /* Full screen, preserve aspect */
> +#define DRM_MODE_SCALE_MANUAL 4 /* User can control display window size */
>
> /* Dithering mode options */
> #define DRM_MODE_DITHERING_OFF 0
> --
> 1.8.5.2
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list