[PATCH v2] drm: Pass the full state to connectors atomic functions
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Nov 19 15:21:48 UTC 2020
On Wed, Nov 18, 2020 at 10:47:58AM +0100, Maxime Ripard wrote:
> The current atomic helpers have either their object state being passed as
> an argument or the full atomic state.
>
> The former is the pattern that was done at first, before switching to the
> latter for new hooks or when it was needed.
>
> Now that the CRTCs have been converted, let's move forward with the
> connectors to provide a consistent interface.
>
> The conversion was done using the coccinelle script below, and built tested
> on all the drivers.
>
> @@
> identifier connector, connector_state;
> @@
>
> struct drm_connector_helper_funcs {
> ...
> struct drm_encoder* (*atomic_best_encoder)(struct drm_connector *connector,
> - struct drm_connector_state *connector_state);
> + struct drm_atomic_state *state);
> ...
> }
>
> @@
> identifier connector, connector_state;
> @@
>
> struct drm_connector_helper_funcs {
> ...
> void (*atomic_commit)(struct drm_connector *connector,
> - struct drm_connector_state *connector_state);
> + struct drm_atomic_state *state);
> ...
> }
>
> @@
> struct drm_connector_helper_funcs *FUNCS;
> identifier state;
> identifier connector, connector_state;
> identifier f;
> @@
>
> f(..., struct drm_atomic_state *state, ...)
> {
> <+...
> - FUNCS->atomic_commit(connector, connector_state);
> + FUNCS->atomic_commit(connector, state);
> ...+>
> }
>
> @@
> struct drm_connector_helper_funcs *FUNCS;
> identifier state;
> identifier connector, connector_state;
> identifier var, f;
> @@
>
> f(struct drm_atomic_state *state, ...)
I think there was some way to deal with these variants using a single
rule, but maybe that required the use of the parameter list stuff
which IIRC was a bit ugly. Probably not worth the hassle here.
> {
> <+...
> - var = FUNCS->atomic_best_encoder(connector, connector_state);
> + var = FUNCS->atomic_best_encoder(connector, state);
> ...+>
> }
>
> @ connector_atomic_func @
> identifier helpers;
> identifier func;
> @@
>
> (
> static struct drm_connector_helper_funcs helpers = {
> ...,
> .atomic_best_encoder = func,
> ...,
> };
> |
> static struct drm_connector_helper_funcs helpers = {
> ...,
> .atomic_commit = func,
> ...,
> };
> )
>
> @@
> identifier connector_atomic_func.func;
> identifier connector;
> symbol state;
> @@
>
> func(struct drm_connector *connector,
> - struct drm_connector_state *state
> + struct drm_connector_state *connector_state
> )
> {
> ...
> - state
> + connector_state
> ...
> }
>
> @ ignores_state @
> identifier connector_atomic_func.func;
> identifier connector, connector_state;
> @@
>
> func(struct drm_connector *connector,
> struct drm_connector_state *connector_state)
> {
> ... when != connector_state
> }
>
> @ adds_state depends on connector_atomic_func && !ignores_state @
> identifier connector_atomic_func.func;
> identifier connector, connector_state;
> @@
>
> func(struct drm_connector *connector, struct drm_connector_state *connector_state)
> {
> + struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state, connector);
> ...
> }
>
> @ depends on connector_atomic_func @
> identifier connector_atomic_func.func;
> identifier connector_state;
> identifier connector;
> @@
>
> func(struct drm_connector *connector,
> - struct drm_connector_state *connector_state
> + struct drm_atomic_state *state
> )
> { ... }
>
> @ include depends on adds_state @
> @@
>
> #include <drm/drm_atomic.h>
>
> @ no_include depends on !include && adds_state @
> @@
>
> + #include <drm/drm_atomic.h>
> #include <drm/...>
Nicely done with the depends. Also never used that stuff myself
so thanks for the tutorial :)
Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Cc: Leo Li <sunpeng.li at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: "Christian König" <christian.koenig at amd.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
> Cc: Melissa Wen <melissa.srw at gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa at gmail.com>
> Acked-by: Thomas Zimmermann <tzimmermann at suse.de>
> Acked-by: Harry Wentland <harry.wentland at amd.com>
> Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
>
> ---
>
> Changes from v1:
> - Added missing coccinelle script
> ---
> .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 ++++-
> drivers/gpu/drm/drm_atomic_helper.c | 8 ++++----
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 +++++--
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 ++++-
> drivers/gpu/drm/vc4/vc4_txp.c | 4 +++-
> drivers/gpu/drm/vkms/vkms_writeback.c | 7 +++++--
> include/drm/drm_modeset_helper_vtables.h | 13 ++++++-------
> 7 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 6f975c16779d..8ab0b9060d2b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -24,6 +24,7 @@
> */
>
> #include <linux/version.h>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_dp_mst_helper.h>
> #include <drm/drm_dp_helper.h>
> @@ -252,8 +253,10 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>
> static struct drm_encoder *
> dm_mst_atomic_best_encoder(struct drm_connector *connector,
> - struct drm_connector_state *connector_state)
> + struct drm_atomic_state *state)
> {
> + struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> + connector);
> struct drm_device *dev = connector->dev;
> struct amdgpu_device *adev = drm_to_adev(dev);
> struct amdgpu_crtc *acrtc = to_amdgpu_crtc(connector_state->crtc);
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ddd0e3239150..ba1507036f26 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -122,7 +122,8 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> continue;
>
> if (funcs->atomic_best_encoder)
> - new_encoder = funcs->atomic_best_encoder(connector, new_conn_state);
> + new_encoder = funcs->atomic_best_encoder(connector,
> + state);
> else if (funcs->best_encoder)
> new_encoder = funcs->best_encoder(connector);
> else
> @@ -345,8 +346,7 @@ update_connector_routing(struct drm_atomic_state *state,
> funcs = connector->helper_private;
>
> if (funcs->atomic_best_encoder)
> - new_encoder = funcs->atomic_best_encoder(connector,
> - new_connector_state);
> + new_encoder = funcs->atomic_best_encoder(connector, state);
> else if (funcs->best_encoder)
> new_encoder = funcs->best_encoder(connector);
> else
> @@ -1313,7 +1313,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>
> if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
> WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> - funcs->atomic_commit(connector, new_conn_state);
> + funcs->atomic_commit(connector, old_state);
> }
> }
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index c8fcec4d0788..4f05ffa3e761 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -23,6 +23,7 @@
> *
> */
>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_edid.h>
> #include <drm/drm_probe_helper.h>
> @@ -719,11 +720,13 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
> }
>
> static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *connector,
> - struct drm_connector_state *state)
> + struct drm_atomic_state *state)
> {
> + struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> + connector);
> struct intel_connector *intel_connector = to_intel_connector(connector);
> struct intel_dp *intel_dp = intel_connector->mst_port;
> - struct intel_crtc *crtc = to_intel_crtc(state->crtc);
> + struct intel_crtc *crtc = to_intel_crtc(connector_state->crtc);
>
> return &intel_dp->mst_encoders[crtc->pipe]->base.base;
> }
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index b111fe24a06b..911c2cbe6aa3 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -32,6 +32,7 @@
> #include <linux/hdmi.h>
> #include <linux/component.h>
>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_dp_helper.h>
> #include <drm/drm_edid.h>
> @@ -1161,8 +1162,10 @@ nv50_msto_new(struct drm_device *dev, struct nv50_head *head, int id)
>
> static struct drm_encoder *
> nv50_mstc_atomic_best_encoder(struct drm_connector *connector,
> - struct drm_connector_state *connector_state)
> + struct drm_atomic_state *state)
> {
> + struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> + connector);
> struct nv50_mstc *mstc = nv50_mstc(connector);
> struct drm_crtc *crtc = connector_state->crtc;
>
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 34612edcabbd..8aa5220885f4 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -273,8 +273,10 @@ static int vc4_txp_connector_atomic_check(struct drm_connector *conn,
> }
>
> static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
> - struct drm_connector_state *conn_state)
> + struct drm_atomic_state *state)
> {
> + struct drm_connector_state *conn_state = drm_atomic_get_new_connector_state(state,
> + conn);
> struct vc4_txp *txp = connector_to_vc4_txp(conn);
> struct drm_gem_cma_object *gem;
> struct drm_display_mode *mode;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 67f80ab1e85f..78fdc1d59186 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -2,6 +2,7 @@
>
> #include <linux/dma-buf-map.h>
>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_fourcc.h>
> #include <drm/drm_writeback.h>
> #include <drm/drm_probe_helper.h>
> @@ -105,8 +106,10 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> }
>
> static void vkms_wb_atomic_commit(struct drm_connector *conn,
> - struct drm_connector_state *state)
> + struct drm_atomic_state *state)
> {
> + struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> + conn);
> struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> struct vkms_output *output = &vkmsdev->output;
> struct drm_writeback_connector *wb_conn = &output->wb_connector;
> @@ -122,7 +125,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> crtc_state->active_writeback = conn_state->writeback_job->priv;
> crtc_state->wb_pending = true;
> spin_unlock_irq(&output->composer_lock);
> - drm_writeback_queue_job(wb_conn, state);
> + drm_writeback_queue_job(wb_conn, connector_state);
> }
>
> static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index f2de050085be..16ff3fa148f5 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1044,9 +1044,8 @@ struct drm_connector_helper_funcs {
> * NOTE:
> *
> * This function is called in the check phase of an atomic update. The
> - * driver is not allowed to change anything outside of the free-standing
> - * state objects passed-in or assembled in the overall &drm_atomic_state
> - * update tracking structure.
> + * driver is not allowed to change anything outside of the
> + * &drm_atomic_state update tracking structure passed in.
> *
> * RETURNS:
> *
> @@ -1056,7 +1055,7 @@ struct drm_connector_helper_funcs {
> * for this.
> */
> struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
> - struct drm_connector_state *connector_state);
> + struct drm_atomic_state *state);
>
> /**
> * @atomic_check:
> @@ -1097,15 +1096,15 @@ struct drm_connector_helper_funcs {
> *
> * This hook is to be used by drivers implementing writeback connectors
> * that need a point when to commit the writeback job to the hardware.
> - * The writeback_job to commit is available in
> - * &drm_connector_state.writeback_job.
> + * The writeback_job to commit is available in the new connector state,
> + * in &drm_connector_state.writeback_job.
> *
> * This hook is optional.
> *
> * This callback is used by the atomic modeset helpers.
> */
> void (*atomic_commit)(struct drm_connector *connector,
> - struct drm_connector_state *state);
> + struct drm_atomic_state *state);
>
> /**
> * @prepare_writeback_job:
> --
> 2.28.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
More information about the dri-devel
mailing list