Re: [PATCH 04/16] drm/fb-helper: No need to cache rotation and sw_rotations

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Mar 26, 2019 at 06:55:34PM +0100, Noralf Trønnes wrote:
> Getting rotation info is cheap so we can do it on demand.
> 
> This is done in preparation for the removal of struct drm_fb_helper_crtc.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> 
> Hans, 
> 
> You had this comment inline in restore_fbdev_mode_atomic() the last time
> I sent this out:
> 
>   We want plane_state->rotation to be set to DRM_MODE_ROTATE_0 in the else
>   case, AFAIK new_plane_state starts with the current state and rotation
>   may have a different value there.
>   
>   Otherwise this looks good to me.
> 
> Rotation is reset for each plane in the code section above the one I'm
> changing.

I think best to let Hans review/test this in detail. lgtm at a glance.
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Noralf.
> 
>  drivers/gpu/drm/drm_fb_helper.c | 131 ++++++++++++++++----------------
>  include/drm/drm_fb_helper.h     |   8 --
>  2 files changed, 65 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index b91df658db59..e1b147fdd3f9 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -387,6 +387,49 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>  
> +/* Check if the plane can hw rotate to match panel orientation */
> +static bool drm_fb_helper_panel_rotation(struct drm_mode_set *modeset,
> +					 unsigned int *rotation)
> +{
> +	struct drm_connector *connector = modeset->connectors[0];
> +	struct drm_plane *plane = modeset->crtc->primary;
> +	u64 valid_mask = 0;
> +	unsigned int i;
> +
> +	if (!modeset->num_connectors)
> +		return false;
> +
> +	switch (connector->display_info.panel_orientation) {
> +	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
> +		*rotation = DRM_MODE_ROTATE_180;
> +		break;
> +	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
> +		*rotation = DRM_MODE_ROTATE_90;
> +		break;
> +	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
> +		*rotation = DRM_MODE_ROTATE_270;
> +		break;
> +	default:
> +		*rotation = DRM_MODE_ROTATE_0;
> +	}
> +
> +	/*
> +	 * TODO: support 90 / 270 degree hardware rotation,
> +	 * depending on the hardware this may require the framebuffer
> +	 * to be in a specific tiling format.
> +	 */
> +	if (*rotation != DRM_MODE_ROTATE_180 || !plane->rotation_property)
> +		return false;
> +
> +	for (i = 0; i < plane->rotation_property->num_values; i++)
> +		valid_mask |= (1ULL << plane->rotation_property->values[i]);
> +
> +	if (!(*rotation & valid_mask))
> +		return false;
> +
> +	return true;
> +}
> +
>  static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
>  {
>  	struct drm_device *dev = fb_helper->dev;
> @@ -427,10 +470,13 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  	for (i = 0; i < fb_helper->crtc_count; i++) {
>  		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
>  		struct drm_plane *primary = mode_set->crtc->primary;
> +		unsigned int rotation;
>  
> -		/* Cannot fail as we've already gotten the plane state above */
> -		plane_state = drm_atomic_get_new_plane_state(state, primary);
> -		plane_state->rotation = fb_helper->crtc_info[i].rotation;
> +		if (drm_fb_helper_panel_rotation(mode_set, &rotation)) {
> +			/* Cannot fail as we've already gotten the plane state above */
> +			plane_state = drm_atomic_get_new_plane_state(state, primary);
> +			plane_state->rotation = rotation;
> +		}
>  
>  		ret = __drm_atomic_helper_set_config(mode_set, state);
>  		if (ret != 0)
> @@ -881,7 +927,6 @@ int drm_fb_helper_init(struct drm_device *dev,
>  		if (!fb_helper->crtc_info[i].mode_set.connectors)
>  			goto out_free;
>  		fb_helper->crtc_info[i].mode_set.num_connectors = 0;
> -		fb_helper->crtc_info[i].rotation = DRM_MODE_ROTATE_0;
>  	}
>  
>  	i = 0;
> @@ -2500,62 +2545,6 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  	return best_score;
>  }
>  
> -/*
> - * This function checks if rotation is necessary because of panel orientation
> - * and if it is, if it is supported.
> - * If rotation is necessary and supported, it gets set in fb_crtc.rotation.
> - * If rotation is necessary but not supported, a DRM_MODE_ROTATE_* flag gets
> - * or-ed into fb_helper->sw_rotations. In drm_setup_crtcs_fb() we check if only
> - * one bit is set and then we set fb_info.fbcon_rotate_hint to make fbcon do
> - * the unsupported rotation.
> - */
> -static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
> -				    struct drm_fb_helper_crtc *fb_crtc,
> -				    struct drm_connector *connector)
> -{
> -	struct drm_plane *plane = fb_crtc->mode_set.crtc->primary;
> -	uint64_t valid_mask = 0;
> -	int i, rotation;
> -
> -	fb_crtc->rotation = DRM_MODE_ROTATE_0;
> -
> -	switch (connector->display_info.panel_orientation) {
> -	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
> -		rotation = DRM_MODE_ROTATE_180;
> -		break;
> -	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
> -		rotation = DRM_MODE_ROTATE_90;
> -		break;
> -	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
> -		rotation = DRM_MODE_ROTATE_270;
> -		break;
> -	default:
> -		rotation = DRM_MODE_ROTATE_0;
> -	}
> -
> -	/*
> -	 * TODO: support 90 / 270 degree hardware rotation,
> -	 * depending on the hardware this may require the framebuffer
> -	 * to be in a specific tiling format.
> -	 */
> -	if (rotation != DRM_MODE_ROTATE_180 || !plane->rotation_property) {
> -		fb_helper->sw_rotations |= rotation;
> -		return;
> -	}
> -
> -	for (i = 0; i < plane->rotation_property->num_values; i++)
> -		valid_mask |= (1ULL << plane->rotation_property->values[i]);
> -
> -	if (!(rotation & valid_mask)) {
> -		fb_helper->sw_rotations |= rotation;
> -		return;
> -	}
> -
> -	fb_crtc->rotation = rotation;
> -	/* Rotating in hardware, fbcon should not rotate */
> -	fb_helper->sw_rotations |= DRM_MODE_ROTATE_0;
> -}
> -
>  static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  			    u32 width, u32 height)
>  {
> @@ -2615,7 +2604,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  		drm_fb_helper_modeset_release(fb_helper,
>  					      &fb_helper->crtc_info[i].mode_set);
>  
> -	fb_helper->sw_rotations = 0;
>  	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		struct drm_display_mode *mode = modes[i];
>  		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
> @@ -2635,7 +2623,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  			modeset->mode = drm_mode_duplicate(dev,
>  							   fb_crtc->desired_mode);
>  			drm_connector_get(connector);
> -			drm_setup_crtc_rotation(fb_helper, fb_crtc, connector);
>  			modeset->connectors[modeset->num_connectors++] = connector;
>  			modeset->x = offset->x;
>  			modeset->y = offset->y;
> @@ -2658,11 +2645,23 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>  {
>  	struct fb_info *info = fb_helper->fbdev;
> +	unsigned int rotation, sw_rotations = 0;
>  	int i;
>  
> -	for (i = 0; i < fb_helper->crtc_count; i++)
> -		if (fb_helper->crtc_info[i].mode_set.num_connectors)
> -			fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
> +	for (i = 0; i < fb_helper->crtc_count; i++) {
> +		struct drm_mode_set *modeset = &fb_helper->crtc_info[i].mode_set;
> +
> +		if (!modeset->num_connectors)
> +			continue;
> +
> +		modeset->fb = fb_helper->fb;
> +
> +		if (drm_fb_helper_panel_rotation(modeset, &rotation))
> +			/* Rotating in hardware, fbcon should not rotate */
> +			sw_rotations |= DRM_MODE_ROTATE_0;
> +		else
> +			sw_rotations |= rotation;
> +	}
>  
>  	mutex_lock(&fb_helper->dev->mode_config.mutex);
>  	drm_fb_helper_for_each_connector(fb_helper, i) {
> @@ -2678,7 +2677,7 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>  	}
>  	mutex_unlock(&fb_helper->dev->mode_config.mutex);
>  
> -	switch (fb_helper->sw_rotations) {
> +	switch (sw_rotations) {
>  	case DRM_MODE_ROTATE_0:
>  		info->fbcon_rotate_hint = FB_ROTATE_UR;
>  		break;
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index bb9acea61369..cff1aa222886 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -50,7 +50,6 @@ struct drm_fb_helper_crtc {
>  	struct drm_mode_set mode_set;
>  	struct drm_display_mode *desired_mode;
>  	int x, y;
> -	int rotation;
>  };
>  
>  /**
> @@ -175,13 +174,6 @@ struct drm_fb_helper {
>  	struct drm_fb_helper_crtc *crtc_info;
>  	int connector_count;
>  	int connector_info_alloc_count;
> -	/**
> -	 * @sw_rotations:
> -	 * Bitmask of all rotations requested for panel-orientation which
> -	 * could not be handled in hardware. If only one bit is set
> -	 * fbdev->fbcon_rotate_hint gets set to the requested rotation.
> -	 */
> -	int sw_rotations;
>  	/**
>  	 * @connector_info:
>  	 *
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel