Re: [PATCH 04/16] drm/fb-helper: No need to cache rotation and sw_rotations
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
> Signed-off-by: Noralf Trønnes
> ---
>
> 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
>
> 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 =
> _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, )) {
> + /* 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
[PATCH 04/16] drm/fb-helper: No need to cache rotation and sw_rotations
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
Signed-off-by: Noralf Trønnes
---
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.
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 =
_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, )) {
+ /* 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;
-
-
