On Sun, Apr 07, 2019 at 06:52:39PM +0200, Noralf Trønnes wrote:
> Move the modeset commit code to drm_client_modeset.
> No changes except exporting API.
>
> v2: Move to drm_client_modeset.c instead of drm_client.c
>
> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_client_modeset.c | 287 +++++++++++++++++++++++++++
Please add a new chapter in Documentation/gpu to pull in all the nice new
kerneldoc you've typed.
> drivers/gpu/drm/drm_fb_helper.c | 282 --------------------------
> include/drm/drm_client.h | 4 +
> 3 files changed, 291 insertions(+), 282 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index bb34222c9db8..94b52f97c06b 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -11,9 +11,14 @@
> #include <linux/mutex.h>
> #include <linux/slab.h>
>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_client.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_device.h>
> +#include <drm/drm_drv.h>
> +
> +#include "drm_crtc_internal.h"
> +#include "drm_internal.h"
>
> /* Used by drm_client and drm_fb_helper */
> int drm_client_modeset_create(struct drm_client_dev *client)
> @@ -100,3 +105,285 @@ struct drm_mode_set *drm_client_find_modeset(struct drm_client_dev *client, stru
> }
> /* TODO: Remove export when modeset code has been moved over */
> EXPORT_SYMBOL(drm_client_find_modeset);
> +
> +/**
> + * drm_client_panel_rotation() - Check panel orientation
> + * @modeset: DRM modeset
> + * @rotation: Returned rotation value
> + *
> + * This function checks if the primary plane in @modeset can hw rotate to match
> + * the panel orientation on its connector.
> + *
> + * Note: Currently only 0 and 180 degrees are supported.
> + *
> + * Return:
> + * True if the plane can do the rotation, false otherwise.
> + */
> +bool drm_client_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 drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool active)
> +{
> + struct drm_device *dev = client->dev;
> + struct drm_plane_state *plane_state;
> + struct drm_plane *plane;
> + struct drm_atomic_state *state;
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_mode_set *mode_set;
> + int ret;
> +
> + drm_modeset_acquire_init(&ctx, 0);
> +
> + state = drm_atomic_state_alloc(dev);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out_ctx;
> + }
> +
> + state->acquire_ctx = &ctx;
> +retry:
> + drm_for_each_plane(plane, dev) {
> + plane_state = drm_atomic_get_plane_state(state, plane);
> + if (IS_ERR(plane_state)) {
> + ret = PTR_ERR(plane_state);
> + goto out_state;
> + }
> +
> + plane_state->rotation = DRM_MODE_ROTATE_0;
> +
> + /* disable non-primary: */
> + if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> + continue;
> +
> + ret = __drm_atomic_helper_disable_plane(plane, plane_state);
> + if (ret != 0)
> + goto out_state;
> + }
> +
> + drm_client_for_each_modeset(mode_set, client) {
> + struct drm_plane *primary = mode_set->crtc->primary;
> + unsigned int rotation;
> +
> + if (drm_client_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)
> + goto out_state;
> +
> + /*
> + * __drm_atomic_helper_set_config() sets active when a
> + * mode is set, unconditionally clear it if we force DPMS off
> + */
> + if (!active) {
> + struct drm_crtc *crtc = mode_set->crtc;
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> + crtc_state->active = false;
> + }
> + }
> +
> + ret = drm_atomic_commit(state);
> +
> +out_state:
> + if (ret == -EDEADLK)
> + goto backoff;
> +
> + drm_atomic_state_put(state);
> +out_ctx:
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> +
> + return ret;
> +
> +backoff:
> + drm_atomic_state_clear(state);
> + drm_modeset_backoff(&ctx);
> +
> + goto retry;
> +}
> +
> +static int drm_client_modeset_commit_legacy(struct drm_client_dev *client)
> +{
> + struct drm_device *dev = client->dev;
> + struct drm_mode_set *mode_set;
> + struct drm_plane *plane;
> + int ret = 0;
> +
> + drm_modeset_lock_all(dev);
> + drm_for_each_plane(plane, dev) {
> + if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> + drm_plane_force_disable(plane);
> +
> + if (plane->rotation_property)
> + drm_mode_plane_set_obj_prop(plane,
> + plane->rotation_property,
> + DRM_MODE_ROTATE_0);
> + }
> +
> + drm_client_for_each_modeset(mode_set, client) {
> + struct drm_crtc *crtc = mode_set->crtc;
> +
> + if (crtc->funcs->cursor_set2) {
> + ret = crtc->funcs->cursor_set2(crtc, NULL, 0, 0, 0, 0, 0);
> + if (ret)
> + goto out;
> + } else if (crtc->funcs->cursor_set) {
> + ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
> + if (ret)
> + goto out;
> + }
> +
> + ret = drm_mode_set_config_internal(mode_set);
> + if (ret)
> + goto out;
> + }
> +out:
> + drm_modeset_unlock_all(dev);
> +
> + return ret;
> +}
> +
> +/**
> + * drm_client_modeset_commit_force() - Force commit CRTC configuration
> + * @client: DRM client
> + *
> + * Commit modeset configuration to crtcs without checking if there is a DRM master.
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_client_modeset_commit_force(struct drm_client_dev *client)
> +{
> + struct drm_device *dev = client->dev;
> + int ret;
> +
> + mutex_lock(&client->modeset_mutex);
> + if (drm_drv_uses_atomic_modeset(dev))
> + ret = drm_client_modeset_commit_atomic(client, true);
> + else
> + ret = drm_client_modeset_commit_legacy(client);
> + mutex_unlock(&client->modeset_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_client_modeset_commit_force);
> +
> +/**
> + * drm_client_modeset_commit() - Commit CRTC configuration
> + * @client: DRM client
> + *
> + * Commit modeset configuration to crtcs.
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_client_modeset_commit(struct drm_client_dev *client)
> +{
> + struct drm_device *dev = client->dev;
> + int ret;
> +
> + if (!drm_master_internal_acquire(dev))
> + return -EBUSY;
> +
> + ret = drm_client_modeset_commit_force(client);
> +
> + drm_master_internal_release(dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_client_modeset_commit);
> +
> +static void drm_client_modeset_dpms_legacy(struct drm_client_dev *client, int dpms_mode)
> +{
> + struct drm_device *dev = client->dev;
> + struct drm_connector *connector;
> + struct drm_mode_set *modeset;
> + int j;
> +
> + drm_modeset_lock_all(dev);
> + drm_client_for_each_modeset(modeset, client) {
> + if (!modeset->crtc->enabled)
> + continue;
> +
> + for (j = 0; j < modeset->num_connectors; j++) {
> + connector = modeset->connectors[j];
> + connector->funcs->dpms(connector, dpms_mode);
> + drm_object_property_set_value(&connector->base,
> + dev->mode_config.dpms_property, dpms_mode);
> + }
> + }
> + drm_modeset_unlock_all(dev);
> +}
> +
> +/**
> + * drm_client_modeset_dpms() - Set DPMS mode
> + * @client: DRM client
> + * @mode: DPMS mode
> + *
> + * Note: For atomic drivers @mode is reduced to on/off.
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
> +{
> + struct drm_device *dev = client->dev;
> + int ret = 0;
> +
> + if (!drm_master_internal_acquire(dev))
> + return -EBUSY;
> +
> + mutex_lock(&client->modeset_mutex);
> + if (drm_drv_uses_atomic_modeset(dev))
> + ret = drm_client_modeset_commit_atomic(client, mode == DRM_MODE_DPMS_ON);
> + else
> + drm_client_modeset_dpms_legacy(client, mode);
> + mutex_unlock(&client->modeset_mutex);
> +
> + drm_master_internal_release(dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_client_modeset_dpms);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0229e187e6de..73fbcd748ddd 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -40,10 +40,7 @@
> #include <drm/drm_fb_helper.h>
> #include <drm/drm_crtc_helper.h>
> #include <drm/drm_atomic.h>
> -#include <drm/drm_atomic_helper.h>
>
> -#include "drm_crtc_internal.h"
> -#include "drm_crtc_helper_internal.h"
> #include "drm_internal.h"
>
> static bool drm_fbdev_emulation = true;
> @@ -388,233 +385,6 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
> }
> EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>
> -/**
> - * drm_client_panel_rotation() - Check panel orientation
> - * @modeset: DRM modeset
> - * @rotation: Returned rotation value
> - *
> - * This function checks if the primary plane in @modeset can hw rotate to match
> - * the panel orientation on its connector.
> - *
> - * Note: Currently only 0 and 180 degrees are supported.
> - *
> - * Return:
> - * True if the plane can do the rotation, false otherwise.
> - */
> -bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
Why not static? Doesn't seem to be used by anything outside of
drm_client_modeset.c.
> -{
> - 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 drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool active)
> -{
> - struct drm_device *dev = client->dev;
> - struct drm_plane_state *plane_state;
> - struct drm_plane *plane;
> - struct drm_atomic_state *state;
> - struct drm_modeset_acquire_ctx ctx;
> - struct drm_mode_set *mode_set;
> - int ret;
> -
> - drm_modeset_acquire_init(&ctx, 0);
> -
> - state = drm_atomic_state_alloc(dev);
> - if (!state) {
> - ret = -ENOMEM;
> - goto out_ctx;
> - }
> -
> - state->acquire_ctx = &ctx;
> -retry:
> - drm_for_each_plane(plane, dev) {
> - plane_state = drm_atomic_get_plane_state(state, plane);
> - if (IS_ERR(plane_state)) {
> - ret = PTR_ERR(plane_state);
> - goto out_state;
> - }
> -
> - plane_state->rotation = DRM_MODE_ROTATE_0;
> -
> - /* disable non-primary: */
> - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> - continue;
> -
> - ret = __drm_atomic_helper_disable_plane(plane, plane_state);
> - if (ret != 0)
> - goto out_state;
> - }
> -
> - drm_client_for_each_modeset(mode_set, client) {
> - struct drm_plane *primary = mode_set->crtc->primary;
> - unsigned int rotation;
> -
> - if (drm_client_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)
> - goto out_state;
> -
> - /*
> - * __drm_atomic_helper_set_config() sets active when a
> - * mode is set, unconditionally clear it if we force DPMS off
> - */
> - if (!active) {
> - struct drm_crtc *crtc = mode_set->crtc;
> - struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> -
> - crtc_state->active = false;
> - }
> - }
> -
> - ret = drm_atomic_commit(state);
> -
> -out_state:
> - if (ret == -EDEADLK)
> - goto backoff;
> -
> - drm_atomic_state_put(state);
> -out_ctx:
> - drm_modeset_drop_locks(&ctx);
> - drm_modeset_acquire_fini(&ctx);
> -
> - return ret;
> -
> -backoff:
> - drm_atomic_state_clear(state);
> - drm_modeset_backoff(&ctx);
> -
> - goto retry;
> -}
> -
> -static int drm_client_modeset_commit_legacy(struct drm_client_dev *client)
> -{
> - struct drm_device *dev = client->dev;
> - struct drm_mode_set *mode_set;
> - struct drm_plane *plane;
> - int ret = 0;
> -
> - drm_modeset_lock_all(dev);
> - drm_for_each_plane(plane, dev) {
> - if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> - drm_plane_force_disable(plane);
> -
> - if (plane->rotation_property)
> - drm_mode_plane_set_obj_prop(plane,
> - plane->rotation_property,
> - DRM_MODE_ROTATE_0);
> - }
> -
> - drm_client_for_each_modeset(mode_set, client) {
> - struct drm_crtc *crtc = mode_set->crtc;
> -
> - if (crtc->funcs->cursor_set2) {
> - ret = crtc->funcs->cursor_set2(crtc, NULL, 0, 0, 0, 0, 0);
> - if (ret)
> - goto out;
> - } else if (crtc->funcs->cursor_set) {
> - ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
> - if (ret)
> - goto out;
> - }
> -
> - ret = drm_mode_set_config_internal(mode_set);
> - if (ret)
> - goto out;
> - }
> -out:
> - drm_modeset_unlock_all(dev);
> -
> - return ret;
> -}
> -
> -/**
> - * drm_client_modeset_commit_force() - Force commit CRTC configuration
> - * @client: DRM client
> - *
> - * Commit modeset configuration to crtcs without checking if there is a DRM master.
> - *
> - * Returns:
> - * Zero on success or negative error code on failure.
> - */
> -int drm_client_modeset_commit_force(struct drm_client_dev *client)
> -{
> - struct drm_device *dev = client->dev;
> - int ret;
> -
> - mutex_lock(&client->modeset_mutex);
> - if (drm_drv_uses_atomic_modeset(dev))
> - ret = drm_client_modeset_commit_atomic(client, true);
> - else
> - ret = drm_client_modeset_commit_legacy(client);
> - mutex_unlock(&client->modeset_mutex);
> -
> - return ret;
> -}
> -
> -/**
> - * drm_client_modeset_commit() - Commit CRTC configuration
> - * @client: DRM client
> - *
> - * Commit modeset configuration to crtcs.
> - *
> - * Returns:
> - * Zero on success or negative error code on failure.
> - */
> -int drm_client_modeset_commit(struct drm_client_dev *client)
> -{
> - struct drm_device *dev = client->dev;
> - int ret;
> -
> - if (!drm_master_internal_acquire(dev))
> - return -EBUSY;
> -
> - ret = drm_client_modeset_commit_force(client);
> -
> - drm_master_internal_release(dev);
> -
> - return ret;
> -}
> -
> /**
> * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> * @fb_helper: driver-allocated fbdev helper, can be NULL
> @@ -704,58 +474,6 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = {
> static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { };
> #endif
>
> -static void drm_client_modeset_dpms_legacy(struct drm_client_dev *client, int dpms_mode)
> -{
> - struct drm_device *dev = client->dev;
> - struct drm_connector *connector;
> - struct drm_mode_set *modeset;
> - int j;
> -
> - drm_modeset_lock_all(dev);
> - drm_client_for_each_modeset(modeset, client) {
> - if (!modeset->crtc->enabled)
> - continue;
> -
> - for (j = 0; j < modeset->num_connectors; j++) {
> - connector = modeset->connectors[j];
> - connector->funcs->dpms(connector, dpms_mode);
> - drm_object_property_set_value(&connector->base,
> - dev->mode_config.dpms_property, dpms_mode);
> - }
> - }
> - drm_modeset_unlock_all(dev);
> -}
> -
> -/**
> - * drm_client_modeset_dpms() - Set DPMS mode
> - * @client: DRM client
> - * @mode: DPMS mode
> - *
> - * Note: For atomic drivers @mode is reduced to on/off.
> - *
> - * Returns:
> - * Zero on success or negative error code on failure.
> - */
> -int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
> -{
> - struct drm_device *dev = client->dev;
> - int ret = 0;
> -
> - if (!drm_master_internal_acquire(dev))
> - return -EBUSY;
> -
> - mutex_lock(&client->modeset_mutex);
> - if (drm_drv_uses_atomic_modeset(dev))
> - ret = drm_client_modeset_commit_atomic(client, mode == DRM_MODE_DPMS_ON);
> - else
> - drm_client_modeset_dpms_legacy(client, mode);
> - mutex_unlock(&client->modeset_mutex);
> -
> - drm_master_internal_release(dev);
> -
> - return ret;
> -}
> -
> static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> {
> struct drm_fb_helper *fb_helper = info->par;
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 858c8be70870..64b725b318f2 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -154,6 +154,10 @@ int drm_client_modeset_create(struct drm_client_dev *client);
> void drm_client_modeset_free(struct drm_client_dev *client);
> void drm_client_modeset_release(struct drm_client_dev *client);
> struct drm_mode_set *drm_client_find_modeset(struct drm_client_dev *client, struct drm_crtc *crtc);
> +bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation);
> +int drm_client_modeset_commit_force(struct drm_client_dev *client);
I think latest here the _force postfix stopped making sense. It's just a
commit. Also I'm wondering whether we shouldn't pull the
master_acquire_internal into these helpers here, there's not really a
use-case I can think of where we should not check for other masters.
Only two kernel modeset requests want to ignore master status:
- debug enter/leave, which is utterly broken by design (and outright
disable for any atomic driver)
- panic handling, for which we now have a really nice plan, plus first
sketches of an implementation.
Cheers, Daniel
> +int drm_client_modeset_commit(struct drm_client_dev *client);
> +int drm_client_modeset_dpms(struct drm_client_dev *client, int mode);
>
> /**
> * drm_client_for_each_modeset() - Iterate over client modesets
> --
> 2.20.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
