* [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state [not found] <20190502194956.218441-1-sean@poorly.run> @ 2019-05-02 19:49 ` Sean Paul 2019-05-03 8:19 ` Daniel Vetter 2019-05-11 19:12 ` Laurent Pinchart 0 siblings, 2 replies; 10+ messages in thread From: Sean Paul @ 2019-05-02 19:49 UTC (permalink / raw) To: dri-devel Cc: Maxime Ripard, nouveau, David Airlie, linux-kernel, linux-renesas-soc, Kieran Bingham, Sean Paul, Ben Skeggs, Rodrigo Vivi, Sean Paul, intel-gfx, Laurent Pinchart From: Sean Paul <seanpaul@chromium.org> Everyone who implements connector_helper_funcs->atomic_check reaches into the connector state to get the atomic state. Instead of continuing this pattern, change the callback signature to just give atomic state and let the driver determine what it does and does not need from it. Eventually all atomic functions should do this, but that's just too much busy work for me. Changes in v3: - Added to the set Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Ben Skeggs <bskeggs@redhat.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Cc: Eric Anholt <eric@anholt.net> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- drivers/gpu/drm/i915/intel_atomic.c | 8 +++++--- drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++--- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- drivers/gpu/drm/i915/intel_tv.c | 8 +++++--- drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++-- drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++----- drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++--- include/drm/drm_modeset_helper_vtables.h | 2 +- 10 files changed, 37 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 9d9e47276839..fa5a367507c1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } if (funcs->atomic_check) - ret = funcs->atomic_check(connector, new_connector_state); + ret = funcs->atomic_check(connector, state); if (ret) return ret; @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, continue; if (funcs->atomic_check) - ret = funcs->atomic_check(connector, new_connector_state); + ret = funcs->atomic_check(connector, state); if (ret) return ret; } diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index b844e8840c6f..e8a5b82e9242 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, } int intel_digital_connector_atomic_check(struct drm_connector *conn, - struct drm_connector_state *new_state) + struct drm_atomic_state *state) { + struct drm_connector_state *new_state = + drm_atomic_get_new_connector_state(state, conn); struct intel_digital_connector_state *new_conn_state = to_intel_digital_connector_state(new_state); struct drm_connector_state *old_state = - drm_atomic_get_old_connector_state(new_state->state, conn); + drm_atomic_get_old_connector_state(state, conn); struct intel_digital_connector_state *old_conn_state = to_intel_digital_connector_state(old_state); struct drm_crtc_state *crtc_state; @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, if (!new_state->crtc) return 0; - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); /* * These properties are handled by fastset, and might not end diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 19d81cef2ab6..89cfec128ba0 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, static int intel_dp_mst_atomic_check(struct drm_connector *connector, - struct drm_connector_state *new_conn_state) + struct drm_atomic_state *state) { - struct drm_atomic_state *state = new_conn_state->state; + struct drm_connector_state *new_conn_state = + drm_atomic_get_new_connector_state(state, connector); struct drm_connector_state *old_conn_state = drm_atomic_get_old_connector_state(state, connector); struct intel_connector *intel_connector = @@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr; int ret; - ret = intel_digital_connector_atomic_check(connector, new_conn_state); + ret = intel_digital_connector_atomic_check(connector, state); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f8c7b291fdc3..88571b8e8d62 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2481,7 +2481,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, struct drm_property *property, u64 val); int intel_digital_connector_atomic_check(struct drm_connector *conn, - struct drm_connector_state *new_state); + struct drm_atomic_state *state); struct drm_connector_state * intel_digital_connector_duplicate_state(struct drm_connector *connector); diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 68f497493d43..72ea164b971c 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -2342,9 +2342,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = { }; static int intel_sdvo_atomic_check(struct drm_connector *conn, - struct drm_connector_state *new_conn_state) + struct drm_atomic_state *state) { - struct drm_atomic_state *state = new_conn_state->state; + struct drm_connector_state *new_conn_state = + drm_atomic_get_new_connector_state(state, conn); struct drm_connector_state *old_conn_state = drm_atomic_get_old_connector_state(state, conn); struct intel_sdvo_connector_state *old_state = @@ -2356,13 +2357,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn, (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) || memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) { struct drm_crtc_state *crtc_state = - drm_atomic_get_new_crtc_state(new_conn_state->state, + drm_atomic_get_new_crtc_state(state, new_conn_state->crtc); crtc_state->connectors_changed = true; } - return intel_digital_connector_atomic_check(conn, new_conn_state); + return intel_digital_connector_atomic_check(conn, state); } static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = { diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 3924c4944e1f..a41c5b467c14 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1817,16 +1817,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { }; static int intel_tv_atomic_check(struct drm_connector *connector, - struct drm_connector_state *new_state) + struct drm_atomic_state *state) { + struct drm_connector_state *new_state; struct drm_crtc_state *new_crtc_state; struct drm_connector_state *old_state; + new_state = drm_atomic_get_new_connector_state(state, connector); if (!new_state->crtc) return 0; - old_state = drm_atomic_get_old_connector_state(new_state->state, connector); - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); + old_state = drm_atomic_get_old_connector_state(state, connector); + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); if (old_state->tv.mode != new_state->tv.mode || old_state->tv.margins.left != new_state->tv.margins.left || diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 4b1650f51955..7ba373f493b2 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector) static int nv50_mstc_atomic_check(struct drm_connector *connector, - struct drm_connector_state *new_conn_state) + struct drm_atomic_state *state) { - struct drm_atomic_state *state = new_conn_state->state; struct nv50_mstc *mstc = nv50_mstc(connector); struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; + struct drm_connector_state *new_conn_state = + drm_atomic_get_new_connector_state(state, connector); struct drm_connector_state *old_conn_state = drm_atomic_get_old_connector_state(state, connector); struct drm_crtc_state *crtc_state; diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 620b51aab291..5b81ba2a7f27 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector) } static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, - struct drm_connector_state *state) + struct drm_atomic_state *state) { struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); const struct drm_display_mode *panel_mode; + struct drm_connector_state *conn_state; struct drm_crtc_state *crtc_state; - if (!state->crtc) + conn_state = drm_atomic_get_new_connector_state(state, connector); + if (!conn_state->crtc) return 0; if (list_empty(&connector->modes)) { @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, struct drm_display_mode, head); /* We're not allowed to modify the resolution. */ - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); + if (!crtc_state) + return -EINVAL; if (crtc_state->mode.hdisplay != panel_mode->hdisplay || crtc_state->mode.vdisplay != panel_mode->vdisplay) diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index c8b89a78f9f4..96f91c1b4b6e 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = { }; static int vc4_txp_connector_atomic_check(struct drm_connector *conn, - struct drm_connector_state *conn_state) + struct drm_atomic_state *state) { + struct drm_connector_state *conn_state; struct drm_crtc_state *crtc_state; struct drm_framebuffer *fb; int i; + conn_state = drm_atomic_get_new_connector_state(state, conn); if (!conn_state->writeback_job || !conn_state->writeback_job->fb) return 0; - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, - conn_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); fb = conn_state->writeback_job->fb; if (fb->width != crtc_state->mode.hdisplay || diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index de57fb40cb6e..adc8b7cf64b5 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -1020,7 +1020,7 @@ struct drm_connector_helper_funcs { * deadlock. */ int (*atomic_check)(struct drm_connector *connector, - struct drm_connector_state *state); + struct drm_atomic_state *state); /** * @atomic_commit: -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state 2019-05-02 19:49 ` [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state Sean Paul @ 2019-05-03 8:19 ` Daniel Vetter 2019-05-11 19:12 ` Laurent Pinchart 1 sibling, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2019-05-03 8:19 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, Sean Paul, Daniel Vetter, Ville Syrjälä, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Ben Skeggs, Laurent Pinchart, Kieran Bingham, Eric Anholt, Maarten Lankhorst, Maxime Ripard, David Airlie, linux-kernel, intel-gfx, nouveau, linux-renesas-soc On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > Everyone who implements connector_helper_funcs->atomic_check reaches > into the connector state to get the atomic state. Instead of continuing > this pattern, change the callback signature to just give atomic state > and let the driver determine what it does and does not need from it. > > Eventually all atomic functions should do this, but that's just too much > busy work for me. > > Changes in v3: > - Added to the set > > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Cc: Eric Anholt <eric@anholt.net> > Signed-off-by: Sean Paul <seanpaul@chromium.org> Assuming it compiles everywhere and intel-gfx-ci approves too Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > drivers/gpu/drm/i915/intel_atomic.c | 8 +++++--- > drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++--- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- > drivers/gpu/drm/i915/intel_tv.c | 8 +++++--- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++-- > drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++----- > drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++--- > include/drm/drm_modeset_helper_vtables.h | 2 +- > 10 files changed, 37 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 9d9e47276839..fa5a367507c1 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > } > > if (funcs->atomic_check) > - ret = funcs->atomic_check(connector, new_connector_state); > + ret = funcs->atomic_check(connector, state); > if (ret) > return ret; > > @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > continue; > > if (funcs->atomic_check) > - ret = funcs->atomic_check(connector, new_connector_state); > + ret = funcs->atomic_check(connector, state); > if (ret) > return ret; > } > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index b844e8840c6f..e8a5b82e9242 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > } > > int intel_digital_connector_atomic_check(struct drm_connector *conn, > - struct drm_connector_state *new_state) > + struct drm_atomic_state *state) > { > + struct drm_connector_state *new_state = > + drm_atomic_get_new_connector_state(state, conn); > struct intel_digital_connector_state *new_conn_state = > to_intel_digital_connector_state(new_state); > struct drm_connector_state *old_state = > - drm_atomic_get_old_connector_state(new_state->state, conn); > + drm_atomic_get_old_connector_state(state, conn); > struct intel_digital_connector_state *old_conn_state = > to_intel_digital_connector_state(old_state); > struct drm_crtc_state *crtc_state; > @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > if (!new_state->crtc) > return 0; > > - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > /* > * These properties are handled by fastset, and might not end > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index 19d81cef2ab6..89cfec128ba0 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > > static int > intel_dp_mst_atomic_check(struct drm_connector *connector, > - struct drm_connector_state *new_conn_state) > + struct drm_atomic_state *state) > { > - struct drm_atomic_state *state = new_conn_state->state; > + struct drm_connector_state *new_conn_state = > + drm_atomic_get_new_connector_state(state, connector); > struct drm_connector_state *old_conn_state = > drm_atomic_get_old_connector_state(state, connector); > struct intel_connector *intel_connector = > @@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, > struct drm_dp_mst_topology_mgr *mgr; > int ret; > > - ret = intel_digital_connector_atomic_check(connector, new_conn_state); > + ret = intel_digital_connector_atomic_check(connector, state); > if (ret) > return ret; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index f8c7b291fdc3..88571b8e8d62 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -2481,7 +2481,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > struct drm_property *property, > u64 val); > int intel_digital_connector_atomic_check(struct drm_connector *conn, > - struct drm_connector_state *new_state); > + struct drm_atomic_state *state); > struct drm_connector_state * > intel_digital_connector_duplicate_state(struct drm_connector *connector); > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index 68f497493d43..72ea164b971c 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -2342,9 +2342,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = { > }; > > static int intel_sdvo_atomic_check(struct drm_connector *conn, > - struct drm_connector_state *new_conn_state) > + struct drm_atomic_state *state) > { > - struct drm_atomic_state *state = new_conn_state->state; > + struct drm_connector_state *new_conn_state = > + drm_atomic_get_new_connector_state(state, conn); > struct drm_connector_state *old_conn_state = > drm_atomic_get_old_connector_state(state, conn); > struct intel_sdvo_connector_state *old_state = > @@ -2356,13 +2357,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn, > (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) || > memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) { > struct drm_crtc_state *crtc_state = > - drm_atomic_get_new_crtc_state(new_conn_state->state, > + drm_atomic_get_new_crtc_state(state, > new_conn_state->crtc); > > crtc_state->connectors_changed = true; > } > > - return intel_digital_connector_atomic_check(conn, new_conn_state); > + return intel_digital_connector_atomic_check(conn, state); > } > > static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = { > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > index 3924c4944e1f..a41c5b467c14 100644 > --- a/drivers/gpu/drm/i915/intel_tv.c > +++ b/drivers/gpu/drm/i915/intel_tv.c > @@ -1817,16 +1817,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > }; > > static int intel_tv_atomic_check(struct drm_connector *connector, > - struct drm_connector_state *new_state) > + struct drm_atomic_state *state) > { > + struct drm_connector_state *new_state; > struct drm_crtc_state *new_crtc_state; > struct drm_connector_state *old_state; > > + new_state = drm_atomic_get_new_connector_state(state, connector); > if (!new_state->crtc) > return 0; > > - old_state = drm_atomic_get_old_connector_state(new_state->state, connector); > - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > + old_state = drm_atomic_get_old_connector_state(state, connector); > + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > if (old_state->tv.mode != new_state->tv.mode || > old_state->tv.margins.left != new_state->tv.margins.left || > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 4b1650f51955..7ba373f493b2 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector) > > static int > nv50_mstc_atomic_check(struct drm_connector *connector, > - struct drm_connector_state *new_conn_state) > + struct drm_atomic_state *state) > { > - struct drm_atomic_state *state = new_conn_state->state; > struct nv50_mstc *mstc = nv50_mstc(connector); > struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; > + struct drm_connector_state *new_conn_state = > + drm_atomic_get_new_connector_state(state, connector); > struct drm_connector_state *old_conn_state = > drm_atomic_get_old_connector_state(state, connector); > struct drm_crtc_state *crtc_state; > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > index 620b51aab291..5b81ba2a7f27 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector) > } > > static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > - struct drm_connector_state *state) > + struct drm_atomic_state *state) > { > struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); > const struct drm_display_mode *panel_mode; > + struct drm_connector_state *conn_state; > struct drm_crtc_state *crtc_state; > > - if (!state->crtc) > + conn_state = drm_atomic_get_new_connector_state(state, connector); > + if (!conn_state->crtc) > return 0; > > if (list_empty(&connector->modes)) { > @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > struct drm_display_mode, head); > > /* We're not allowed to modify the resolution. */ > - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); > - if (IS_ERR(crtc_state)) > - return PTR_ERR(crtc_state); > + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); > + if (!crtc_state) > + return -EINVAL; > > if (crtc_state->mode.hdisplay != panel_mode->hdisplay || > crtc_state->mode.vdisplay != panel_mode->vdisplay) > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > index c8b89a78f9f4..96f91c1b4b6e 100644 > --- a/drivers/gpu/drm/vc4/vc4_txp.c > +++ b/drivers/gpu/drm/vc4/vc4_txp.c > @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = { > }; > > static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > - struct drm_connector_state *conn_state) > + struct drm_atomic_state *state) > { > + struct drm_connector_state *conn_state; > struct drm_crtc_state *crtc_state; > struct drm_framebuffer *fb; > int i; > > + conn_state = drm_atomic_get_new_connector_state(state, conn); > if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > return 0; > > - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, > - conn_state->crtc); > + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > > fb = conn_state->writeback_job->fb; > if (fb->width != crtc_state->mode.hdisplay || > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index de57fb40cb6e..adc8b7cf64b5 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -1020,7 +1020,7 @@ struct drm_connector_helper_funcs { > * deadlock. > */ > int (*atomic_check)(struct drm_connector *connector, > - struct drm_connector_state *state); > + struct drm_atomic_state *state); > > /** > * @atomic_commit: > -- > Sean Paul, Software Engineer, Google / Chromium OS > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state 2019-05-02 19:49 ` [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state Sean Paul 2019-05-03 8:19 ` Daniel Vetter @ 2019-05-11 19:12 ` Laurent Pinchart [not found] ` <20190511191202.GL13043-N3hz7ZxfLydczECFQUw77jytWr6r+dGw0E9HWUfgJXw@public.gmane.org> 2019-05-13 14:47 ` Daniel Vetter 1 sibling, 2 replies; 10+ messages in thread From: Laurent Pinchart @ 2019-05-11 19:12 UTC (permalink / raw) To: Sean Paul Cc: Maxime Ripard, nouveau, David Airlie, linux-kernel, linux-renesas-soc, Kieran Bingham, Sean Paul, dri-devel, Rodrigo Vivi, intel-gfx, Ben Skeggs Hi Sean, Thank you for the patch. On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > Everyone who implements connector_helper_funcs->atomic_check reaches > into the connector state to get the atomic state. Instead of continuing > this pattern, change the callback signature to just give atomic state > and let the driver determine what it does and does not need from it. > > Eventually all atomic functions should do this, but that's just too much > busy work for me. Given that drivers also access the connector state, isn't this slightly more inefficient ? > Changes in v3: > - Added to the set > > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Cc: Eric Anholt <eric@anholt.net> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > drivers/gpu/drm/i915/intel_atomic.c | 8 +++++--- > drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++--- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- > drivers/gpu/drm/i915/intel_tv.c | 8 +++++--- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++-- > drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++----- > drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++--- > include/drm/drm_modeset_helper_vtables.h | 2 +- > 10 files changed, 37 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 9d9e47276839..fa5a367507c1 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > } > > if (funcs->atomic_check) > - ret = funcs->atomic_check(connector, new_connector_state); > + ret = funcs->atomic_check(connector, state); > if (ret) > return ret; > > @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > continue; > > if (funcs->atomic_check) > - ret = funcs->atomic_check(connector, new_connector_state); > + ret = funcs->atomic_check(connector, state); > if (ret) > return ret; > } > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index b844e8840c6f..e8a5b82e9242 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > } > > int intel_digital_connector_atomic_check(struct drm_connector *conn, > - struct drm_connector_state *new_state) > + struct drm_atomic_state *state) > { > + struct drm_connector_state *new_state = > + drm_atomic_get_new_connector_state(state, conn); > struct intel_digital_connector_state *new_conn_state = > to_intel_digital_connector_state(new_state); > struct drm_connector_state *old_state = > - drm_atomic_get_old_connector_state(new_state->state, conn); > + drm_atomic_get_old_connector_state(state, conn); > struct intel_digital_connector_state *old_conn_state = > to_intel_digital_connector_state(old_state); > struct drm_crtc_state *crtc_state; > @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > if (!new_state->crtc) > return 0; > > - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > /* > * These properties are handled by fastset, and might not end > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index 19d81cef2ab6..89cfec128ba0 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > > static int > intel_dp_mst_atomic_check(struct drm_connector *connector, > - struct drm_connector_state *new_conn_state) > + struct drm_atomic_state *state) > { > - struct drm_atomic_state *state = new_conn_state->state; > + struct drm_connector_state *new_conn_state = > + drm_atomic_get_new_connector_state(state, connector); > struct drm_connector_state *old_conn_state = > drm_atomic_get_old_connector_state(state, connector); > struct intel_connector *intel_connector = > @@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, > struct drm_dp_mst_topology_mgr *mgr; > int ret; > > - ret = intel_digital_connector_atomic_check(connector, new_conn_state); > + ret = intel_digital_connector_atomic_check(connector, state); > if (ret) > return ret; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index f8c7b291fdc3..88571b8e8d62 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -2481,7 +2481,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > struct drm_property *property, > u64 val); > int intel_digital_connector_atomic_check(struct drm_connector *conn, > - struct drm_connector_state *new_state); > + struct drm_atomic_state *state); > struct drm_connector_state * > intel_digital_connector_duplicate_state(struct drm_connector *connector); > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index 68f497493d43..72ea164b971c 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -2342,9 +2342,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = { > }; > > static int intel_sdvo_atomic_check(struct drm_connector *conn, > - struct drm_connector_state *new_conn_state) > + struct drm_atomic_state *state) > { > - struct drm_atomic_state *state = new_conn_state->state; > + struct drm_connector_state *new_conn_state = > + drm_atomic_get_new_connector_state(state, conn); > struct drm_connector_state *old_conn_state = > drm_atomic_get_old_connector_state(state, conn); > struct intel_sdvo_connector_state *old_state = > @@ -2356,13 +2357,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn, > (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) || > memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) { > struct drm_crtc_state *crtc_state = > - drm_atomic_get_new_crtc_state(new_conn_state->state, > + drm_atomic_get_new_crtc_state(state, > new_conn_state->crtc); > > crtc_state->connectors_changed = true; > } > > - return intel_digital_connector_atomic_check(conn, new_conn_state); > + return intel_digital_connector_atomic_check(conn, state); > } > > static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = { > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > index 3924c4944e1f..a41c5b467c14 100644 > --- a/drivers/gpu/drm/i915/intel_tv.c > +++ b/drivers/gpu/drm/i915/intel_tv.c > @@ -1817,16 +1817,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > }; > > static int intel_tv_atomic_check(struct drm_connector *connector, > - struct drm_connector_state *new_state) > + struct drm_atomic_state *state) > { > + struct drm_connector_state *new_state; > struct drm_crtc_state *new_crtc_state; > struct drm_connector_state *old_state; > > + new_state = drm_atomic_get_new_connector_state(state, connector); > if (!new_state->crtc) > return 0; > > - old_state = drm_atomic_get_old_connector_state(new_state->state, connector); > - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > + old_state = drm_atomic_get_old_connector_state(state, connector); > + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > if (old_state->tv.mode != new_state->tv.mode || > old_state->tv.margins.left != new_state->tv.margins.left || > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 4b1650f51955..7ba373f493b2 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector) > > static int > nv50_mstc_atomic_check(struct drm_connector *connector, > - struct drm_connector_state *new_conn_state) > + struct drm_atomic_state *state) > { > - struct drm_atomic_state *state = new_conn_state->state; > struct nv50_mstc *mstc = nv50_mstc(connector); > struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; > + struct drm_connector_state *new_conn_state = > + drm_atomic_get_new_connector_state(state, connector); > struct drm_connector_state *old_conn_state = > drm_atomic_get_old_connector_state(state, connector); > struct drm_crtc_state *crtc_state; > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > index 620b51aab291..5b81ba2a7f27 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector) > } > > static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > - struct drm_connector_state *state) > + struct drm_atomic_state *state) > { > struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); > const struct drm_display_mode *panel_mode; > + struct drm_connector_state *conn_state; > struct drm_crtc_state *crtc_state; > > - if (!state->crtc) > + conn_state = drm_atomic_get_new_connector_state(state, connector); > + if (!conn_state->crtc) > return 0; > > if (list_empty(&connector->modes)) { > @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > struct drm_display_mode, head); > > /* We're not allowed to modify the resolution. */ > - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); > - if (IS_ERR(crtc_state)) > - return PTR_ERR(crtc_state); > + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); > + if (!crtc_state) > + return -EINVAL; > > if (crtc_state->mode.hdisplay != panel_mode->hdisplay || > crtc_state->mode.vdisplay != panel_mode->vdisplay) > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > index c8b89a78f9f4..96f91c1b4b6e 100644 > --- a/drivers/gpu/drm/vc4/vc4_txp.c > +++ b/drivers/gpu/drm/vc4/vc4_txp.c > @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = { > }; > > static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > - struct drm_connector_state *conn_state) > + struct drm_atomic_state *state) > { > + struct drm_connector_state *conn_state; > struct drm_crtc_state *crtc_state; > struct drm_framebuffer *fb; > int i; > > + conn_state = drm_atomic_get_new_connector_state(state, conn); > if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > return 0; > > - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, > - conn_state->crtc); > + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > > fb = conn_state->writeback_job->fb; > if (fb->width != crtc_state->mode.hdisplay || > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index de57fb40cb6e..adc8b7cf64b5 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -1020,7 +1020,7 @@ struct drm_connector_helper_funcs { > * deadlock. > */ > int (*atomic_check)(struct drm_connector *connector, > - struct drm_connector_state *state); > + struct drm_atomic_state *state); > > /** > * @atomic_commit: -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20190511191202.GL13043-N3hz7ZxfLydczECFQUw77jytWr6r+dGw0E9HWUfgJXw@public.gmane.org>]
* Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state [not found] ` <20190511191202.GL13043-N3hz7ZxfLydczECFQUw77jytWr6r+dGw0E9HWUfgJXw@public.gmane.org> @ 2019-05-13 14:38 ` Sean Paul 2019-05-16 12:00 ` Laurent Pinchart 0 siblings, 1 reply; 10+ messages in thread From: Sean Paul @ 2019-05-13 14:38 UTC (permalink / raw) To: Laurent Pinchart Cc: Maxime Ripard, nouveau, David Airlie, Joonas Lahtinen, Maarten Lankhorst, LKML, Jani Nikula, linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, Eric Anholt, Kieran Bingham, Sean Paul, dri-devel, Daniel Vetter, Rodrigo Vivi, Intel Graphics Development, Ville Syrjälä, Ben Skeggs On Sat, May 11, 2019 at 3:12 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Sean, > > Thank you for the patch. > Hey Laurent, Thanks for looking! > On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > Everyone who implements connector_helper_funcs->atomic_check reaches > > into the connector state to get the atomic state. Instead of continuing > > this pattern, change the callback signature to just give atomic state > > and let the driver determine what it does and does not need from it. > > > > Eventually all atomic functions should do this, but that's just too much > > busy work for me. > > Given that drivers also access the connector state, isn't this slightly > more inefficient ? > Inefficient in terms of what? Agree that in isolation this patch might seem unnecessary, but it ties in with the encoder and bridge CLs which accept drm_atomic_state in their hooks. In general the idea is to convert all atomic functions to take overall atomic state instead of just their object state. Reality has proven to be more complicated and we need more access than what the current implementation provides. Sean > > Changes in v3: > > - Added to the set > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Ben Skeggs <bskeggs@redhat.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Cc: Eric Anholt <eric@anholt.net> > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > > drivers/gpu/drm/i915/intel_atomic.c | 8 +++++--- > > drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++--- > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- > > drivers/gpu/drm/i915/intel_tv.c | 8 +++++--- > > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++-- > > drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++----- > > drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++--- > > include/drm/drm_modeset_helper_vtables.h | 2 +- > > 10 files changed, 37 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 9d9e47276839..fa5a367507c1 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > } > > > > if (funcs->atomic_check) > > - ret = funcs->atomic_check(connector, new_connector_state); > > + ret = funcs->atomic_check(connector, state); > > if (ret) > > return ret; > > > > @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > continue; > > > > if (funcs->atomic_check) > > - ret = funcs->atomic_check(connector, new_connector_state); > > + ret = funcs->atomic_check(connector, state); > > if (ret) > > return ret; > > } > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > > index b844e8840c6f..e8a5b82e9242 100644 > > --- a/drivers/gpu/drm/i915/intel_atomic.c > > +++ b/drivers/gpu/drm/i915/intel_atomic.c > > @@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > > } > > > > int intel_digital_connector_atomic_check(struct drm_connector *conn, > > - struct drm_connector_state *new_state) > > + struct drm_atomic_state *state) > > { > > + struct drm_connector_state *new_state = > > + drm_atomic_get_new_connector_state(state, conn); > > struct intel_digital_connector_state *new_conn_state = > > to_intel_digital_connector_state(new_state); > > struct drm_connector_state *old_state = > > - drm_atomic_get_old_connector_state(new_state->state, conn); > > + drm_atomic_get_old_connector_state(state, conn); > > struct intel_digital_connector_state *old_conn_state = > > to_intel_digital_connector_state(old_state); > > struct drm_crtc_state *crtc_state; > > @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > > if (!new_state->crtc) > > return 0; > > > > - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > > + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > > > /* > > * These properties are handled by fastset, and might not end > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > index 19d81cef2ab6..89cfec128ba0 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > > > > static int > > intel_dp_mst_atomic_check(struct drm_connector *connector, > > - struct drm_connector_state *new_conn_state) > > + struct drm_atomic_state *state) > > { > > - struct drm_atomic_state *state = new_conn_state->state; > > + struct drm_connector_state *new_conn_state = > > + drm_atomic_get_new_connector_state(state, connector); > > struct drm_connector_state *old_conn_state = > > drm_atomic_get_old_connector_state(state, connector); > > struct intel_connector *intel_connector = > > @@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, > > struct drm_dp_mst_topology_mgr *mgr; > > int ret; > > > > - ret = intel_digital_connector_atomic_check(connector, new_conn_state); > > + ret = intel_digital_connector_atomic_check(connector, state); > > if (ret) > > return ret; > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index f8c7b291fdc3..88571b8e8d62 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -2481,7 +2481,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > > struct drm_property *property, > > u64 val); > > int intel_digital_connector_atomic_check(struct drm_connector *conn, > > - struct drm_connector_state *new_state); > > + struct drm_atomic_state *state); > > struct drm_connector_state * > > intel_digital_connector_duplicate_state(struct drm_connector *connector); > > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > index 68f497493d43..72ea164b971c 100644 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > @@ -2342,9 +2342,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = { > > }; > > > > static int intel_sdvo_atomic_check(struct drm_connector *conn, > > - struct drm_connector_state *new_conn_state) > > + struct drm_atomic_state *state) > > { > > - struct drm_atomic_state *state = new_conn_state->state; > > + struct drm_connector_state *new_conn_state = > > + drm_atomic_get_new_connector_state(state, conn); > > struct drm_connector_state *old_conn_state = > > drm_atomic_get_old_connector_state(state, conn); > > struct intel_sdvo_connector_state *old_state = > > @@ -2356,13 +2357,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn, > > (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) || > > memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) { > > struct drm_crtc_state *crtc_state = > > - drm_atomic_get_new_crtc_state(new_conn_state->state, > > + drm_atomic_get_new_crtc_state(state, > > new_conn_state->crtc); > > > > crtc_state->connectors_changed = true; > > } > > > > - return intel_digital_connector_atomic_check(conn, new_conn_state); > > + return intel_digital_connector_atomic_check(conn, state); > > } > > > > static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = { > > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > > index 3924c4944e1f..a41c5b467c14 100644 > > --- a/drivers/gpu/drm/i915/intel_tv.c > > +++ b/drivers/gpu/drm/i915/intel_tv.c > > @@ -1817,16 +1817,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > > }; > > > > static int intel_tv_atomic_check(struct drm_connector *connector, > > - struct drm_connector_state *new_state) > > + struct drm_atomic_state *state) > > { > > + struct drm_connector_state *new_state; > > struct drm_crtc_state *new_crtc_state; > > struct drm_connector_state *old_state; > > > > + new_state = drm_atomic_get_new_connector_state(state, connector); > > if (!new_state->crtc) > > return 0; > > > > - old_state = drm_atomic_get_old_connector_state(new_state->state, connector); > > - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > > + old_state = drm_atomic_get_old_connector_state(state, connector); > > + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > > > if (old_state->tv.mode != new_state->tv.mode || > > old_state->tv.margins.left != new_state->tv.margins.left || > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > > index 4b1650f51955..7ba373f493b2 100644 > > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > > @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector) > > > > static int > > nv50_mstc_atomic_check(struct drm_connector *connector, > > - struct drm_connector_state *new_conn_state) > > + struct drm_atomic_state *state) > > { > > - struct drm_atomic_state *state = new_conn_state->state; > > struct nv50_mstc *mstc = nv50_mstc(connector); > > struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; > > + struct drm_connector_state *new_conn_state = > > + drm_atomic_get_new_connector_state(state, connector); > > struct drm_connector_state *old_conn_state = > > drm_atomic_get_old_connector_state(state, connector); > > struct drm_crtc_state *crtc_state; > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > index 620b51aab291..5b81ba2a7f27 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector) > > } > > > > static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > > - struct drm_connector_state *state) > > + struct drm_atomic_state *state) > > { > > struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); > > const struct drm_display_mode *panel_mode; > > + struct drm_connector_state *conn_state; > > struct drm_crtc_state *crtc_state; > > > > - if (!state->crtc) > > + conn_state = drm_atomic_get_new_connector_state(state, connector); > > + if (!conn_state->crtc) > > return 0; > > > > if (list_empty(&connector->modes)) { > > @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > > struct drm_display_mode, head); > > > > /* We're not allowed to modify the resolution. */ > > - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); > > - if (IS_ERR(crtc_state)) > > - return PTR_ERR(crtc_state); > > + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); > > + if (!crtc_state) > > + return -EINVAL; > > > > if (crtc_state->mode.hdisplay != panel_mode->hdisplay || > > crtc_state->mode.vdisplay != panel_mode->vdisplay) > > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > > index c8b89a78f9f4..96f91c1b4b6e 100644 > > --- a/drivers/gpu/drm/vc4/vc4_txp.c > > +++ b/drivers/gpu/drm/vc4/vc4_txp.c > > @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = { > > }; > > > > static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > > - struct drm_connector_state *conn_state) > > + struct drm_atomic_state *state) > > { > > + struct drm_connector_state *conn_state; > > struct drm_crtc_state *crtc_state; > > struct drm_framebuffer *fb; > > int i; > > > > + conn_state = drm_atomic_get_new_connector_state(state, conn); > > if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > > return 0; > > > > - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, > > - conn_state->crtc); > > + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > > > > fb = conn_state->writeback_job->fb; > > if (fb->width != crtc_state->mode.hdisplay || > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > > index de57fb40cb6e..adc8b7cf64b5 100644 > > --- a/include/drm/drm_modeset_helper_vtables.h > > +++ b/include/drm/drm_modeset_helper_vtables.h > > @@ -1020,7 +1020,7 @@ struct drm_connector_helper_funcs { > > * deadlock. > > */ > > int (*atomic_check)(struct drm_connector *connector, > > - struct drm_connector_state *state); > > + struct drm_atomic_state *state); > > > > /** > > * @atomic_commit: > > -- > Regards, > > Laurent Pinchart _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state 2019-05-13 14:38 ` Sean Paul @ 2019-05-16 12:00 ` Laurent Pinchart 2019-05-16 14:21 ` Sean Paul 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2019-05-16 12:00 UTC (permalink / raw) To: Sean Paul Cc: Maxime Ripard, nouveau, David Airlie, LKML, linux-renesas-soc, Kieran Bingham, Sean Paul, dri-devel, Intel Graphics Development, Ben Skeggs Hi Sean, On Mon, May 13, 2019 at 10:38:58AM -0400, Sean Paul wrote: > On Sat, May 11, 2019 at 3:12 PM Laurent Pinchart wrote: > > On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > >> From: Sean Paul <seanpaul@chromium.org> > >> > >> Everyone who implements connector_helper_funcs->atomic_check reaches > >> into the connector state to get the atomic state. Instead of continuing > >> this pattern, change the callback signature to just give atomic state > >> and let the driver determine what it does and does not need from it. > >> > >> Eventually all atomic functions should do this, but that's just too much > >> busy work for me. > > > > Given that drivers also access the connector state, isn't this slightly > > more inefficient ? > > Inefficient in terms of what? In terms of the operation having to lookup the connector state, when the caller has it and can easily pass it. As Daniel commented, this may be the price to pay for a cleaner API, but I wonder how much overhead all the state tracking is costing. > Agree that in isolation this patch might seem unnecessary, but it ties > in with the encoder and bridge CLs which accept drm_atomic_state in CLs ? > their hooks. In general the idea is to convert all atomic functions to > take overall atomic state instead of just their object state. Reality > has proven to be more complicated and we need more access than what > the current implementation provides. > > Sean > > >> Changes in v3: > >> - Added to the set > >> > >> Cc: Daniel Vetter <daniel@ffwll.ch> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Jani Nikula <jani.nikula@linux.intel.com> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> Cc: Ben Skeggs <bskeggs@redhat.com> > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> Cc: Eric Anholt <eric@anholt.net> > >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > >> --- > >> drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > >> drivers/gpu/drm/i915/intel_atomic.c | 8 +++++--- > >> drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++--- > >> drivers/gpu/drm/i915/intel_drv.h | 2 +- > >> drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- > >> drivers/gpu/drm/i915/intel_tv.c | 8 +++++--- > >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++-- > >> drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++----- > >> drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++--- > >> include/drm/drm_modeset_helper_vtables.h | 2 +- > >> 10 files changed, 37 insertions(+), 27 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >> index 9d9e47276839..fa5a367507c1 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > >> } > >> > >> if (funcs->atomic_check) > >> - ret = funcs->atomic_check(connector, new_connector_state); > >> + ret = funcs->atomic_check(connector, state); > >> if (ret) > >> return ret; > >> > >> @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > >> continue; > >> > >> if (funcs->atomic_check) > >> - ret = funcs->atomic_check(connector, new_connector_state); > >> + ret = funcs->atomic_check(connector, state); > >> if (ret) > >> return ret; > >> } > >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > >> index b844e8840c6f..e8a5b82e9242 100644 > >> --- a/drivers/gpu/drm/i915/intel_atomic.c > >> +++ b/drivers/gpu/drm/i915/intel_atomic.c > >> @@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > >> } > >> > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > >> - struct drm_connector_state *new_state) > >> + struct drm_atomic_state *state) > >> { > >> + struct drm_connector_state *new_state = > >> + drm_atomic_get_new_connector_state(state, conn); > >> struct intel_digital_connector_state *new_conn_state = > >> to_intel_digital_connector_state(new_state); > >> struct drm_connector_state *old_state = > >> - drm_atomic_get_old_connector_state(new_state->state, conn); > >> + drm_atomic_get_old_connector_state(state, conn); > >> struct intel_digital_connector_state *old_conn_state = > >> to_intel_digital_connector_state(old_state); > >> struct drm_crtc_state *crtc_state; > >> @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > >> if (!new_state->crtc) > >> return 0; > >> > >> - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > >> + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > >> > >> /* > >> * These properties are handled by fastset, and might not end > >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > >> index 19d81cef2ab6..89cfec128ba0 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c > >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > >> @@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > >> > >> static int > >> intel_dp_mst_atomic_check(struct drm_connector *connector, > >> - struct drm_connector_state *new_conn_state) > >> + struct drm_atomic_state *state) > >> { > >> - struct drm_atomic_state *state = new_conn_state->state; > >> + struct drm_connector_state *new_conn_state = > >> + drm_atomic_get_new_connector_state(state, connector); > >> struct drm_connector_state *old_conn_state = > >> drm_atomic_get_old_connector_state(state, connector); > >> struct intel_connector *intel_connector = > >> @@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, > >> struct drm_dp_mst_topology_mgr *mgr; > >> int ret; > >> > >> - ret = intel_digital_connector_atomic_check(connector, new_conn_state); > >> + ret = intel_digital_connector_atomic_check(connector, state); > >> if (ret) > >> return ret; > >> > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index f8c7b291fdc3..88571b8e8d62 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -2481,7 +2481,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > >> struct drm_property *property, > >> u64 val); > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > >> - struct drm_connector_state *new_state); > >> + struct drm_atomic_state *state); > >> struct drm_connector_state * > >> intel_digital_connector_duplicate_state(struct drm_connector *connector); > >> > >> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > >> index 68f497493d43..72ea164b971c 100644 > >> --- a/drivers/gpu/drm/i915/intel_sdvo.c > >> +++ b/drivers/gpu/drm/i915/intel_sdvo.c > >> @@ -2342,9 +2342,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = { > >> }; > >> > >> static int intel_sdvo_atomic_check(struct drm_connector *conn, > >> - struct drm_connector_state *new_conn_state) > >> + struct drm_atomic_state *state) > >> { > >> - struct drm_atomic_state *state = new_conn_state->state; > >> + struct drm_connector_state *new_conn_state = > >> + drm_atomic_get_new_connector_state(state, conn); > >> struct drm_connector_state *old_conn_state = > >> drm_atomic_get_old_connector_state(state, conn); > >> struct intel_sdvo_connector_state *old_state = > >> @@ -2356,13 +2357,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn, > >> (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) || > >> memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) { > >> struct drm_crtc_state *crtc_state = > >> - drm_atomic_get_new_crtc_state(new_conn_state->state, > >> + drm_atomic_get_new_crtc_state(state, > >> new_conn_state->crtc); > >> > >> crtc_state->connectors_changed = true; > >> } > >> > >> - return intel_digital_connector_atomic_check(conn, new_conn_state); > >> + return intel_digital_connector_atomic_check(conn, state); > >> } > >> > >> static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = { > >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > >> index 3924c4944e1f..a41c5b467c14 100644 > >> --- a/drivers/gpu/drm/i915/intel_tv.c > >> +++ b/drivers/gpu/drm/i915/intel_tv.c > >> @@ -1817,16 +1817,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > >> }; > >> > >> static int intel_tv_atomic_check(struct drm_connector *connector, > >> - struct drm_connector_state *new_state) > >> + struct drm_atomic_state *state) > >> { > >> + struct drm_connector_state *new_state; > >> struct drm_crtc_state *new_crtc_state; > >> struct drm_connector_state *old_state; > >> > >> + new_state = drm_atomic_get_new_connector_state(state, connector); > >> if (!new_state->crtc) > >> return 0; > >> > >> - old_state = drm_atomic_get_old_connector_state(new_state->state, connector); > >> - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > >> + old_state = drm_atomic_get_old_connector_state(state, connector); > >> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > >> > >> if (old_state->tv.mode != new_state->tv.mode || > >> old_state->tv.margins.left != new_state->tv.margins.left || > >> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > >> index 4b1650f51955..7ba373f493b2 100644 > >> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > >> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > >> @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector) > >> > >> static int > >> nv50_mstc_atomic_check(struct drm_connector *connector, > >> - struct drm_connector_state *new_conn_state) > >> + struct drm_atomic_state *state) > >> { > >> - struct drm_atomic_state *state = new_conn_state->state; > >> struct nv50_mstc *mstc = nv50_mstc(connector); > >> struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; > >> + struct drm_connector_state *new_conn_state = > >> + drm_atomic_get_new_connector_state(state, connector); > >> struct drm_connector_state *old_conn_state = > >> drm_atomic_get_old_connector_state(state, connector); > >> struct drm_crtc_state *crtc_state; > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > >> index 620b51aab291..5b81ba2a7f27 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > >> @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector) > >> } > >> > >> static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > >> - struct drm_connector_state *state) > >> + struct drm_atomic_state *state) > >> { > >> struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); > >> const struct drm_display_mode *panel_mode; > >> + struct drm_connector_state *conn_state; > >> struct drm_crtc_state *crtc_state; > >> > >> - if (!state->crtc) > >> + conn_state = drm_atomic_get_new_connector_state(state, connector); > >> + if (!conn_state->crtc) > >> return 0; > >> > >> if (list_empty(&connector->modes)) { > >> @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > >> struct drm_display_mode, head); > >> > >> /* We're not allowed to modify the resolution. */ > >> - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); > >> - if (IS_ERR(crtc_state)) > >> - return PTR_ERR(crtc_state); > >> + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); > >> + if (!crtc_state) > >> + return -EINVAL; > >> > >> if (crtc_state->mode.hdisplay != panel_mode->hdisplay || > >> crtc_state->mode.vdisplay != panel_mode->vdisplay) > >> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > >> index c8b89a78f9f4..96f91c1b4b6e 100644 > >> --- a/drivers/gpu/drm/vc4/vc4_txp.c > >> +++ b/drivers/gpu/drm/vc4/vc4_txp.c > >> @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = { > >> }; > >> > >> static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > >> - struct drm_connector_state *conn_state) > >> + struct drm_atomic_state *state) > >> { > >> + struct drm_connector_state *conn_state; > >> struct drm_crtc_state *crtc_state; > >> struct drm_framebuffer *fb; > >> int i; > >> > >> + conn_state = drm_atomic_get_new_connector_state(state, conn); > >> if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > >> return 0; > >> > >> - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, > >> - conn_state->crtc); > >> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > >> > >> fb = conn_state->writeback_job->fb; > >> if (fb->width != crtc_state->mode.hdisplay || > >> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > >> index de57fb40cb6e..adc8b7cf64b5 100644 > >> --- a/include/drm/drm_modeset_helper_vtables.h > >> +++ b/include/drm/drm_modeset_helper_vtables.h > >> @@ -1020,7 +1020,7 @@ struct drm_connector_helper_funcs { > >> * deadlock. > >> */ > >> int (*atomic_check)(struct drm_connector *connector, > >> - struct drm_connector_state *state); > >> + struct drm_atomic_state *state); > >> > >> /** > >> * @atomic_commit: -- Regards, Laurent Pinchart _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state 2019-05-16 12:00 ` Laurent Pinchart @ 2019-05-16 14:21 ` Sean Paul 0 siblings, 0 replies; 10+ messages in thread From: Sean Paul @ 2019-05-16 14:21 UTC (permalink / raw) To: Laurent Pinchart Cc: Sean Paul, dri-devel, Sean Paul, Daniel Vetter, Ville Syrjälä, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Ben Skeggs, Kieran Bingham, Eric Anholt, Maarten Lankhorst, Maxime Ripard, David Airlie, LKML, Intel Graphics Development, nouveau On Thu, May 16, 2019 at 03:00:01PM +0300, Laurent Pinchart wrote: > Hi Sean, > > On Mon, May 13, 2019 at 10:38:58AM -0400, Sean Paul wrote: > > On Sat, May 11, 2019 at 3:12 PM Laurent Pinchart wrote: > > > On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > > >> From: Sean Paul <seanpaul@chromium.org> > > >> > > >> Everyone who implements connector_helper_funcs->atomic_check reaches > > >> into the connector state to get the atomic state. Instead of continuing > > >> this pattern, change the callback signature to just give atomic state > > >> and let the driver determine what it does and does not need from it. > > >> > > >> Eventually all atomic functions should do this, but that's just too much > > >> busy work for me. > > > > > > Given that drivers also access the connector state, isn't this slightly > > > more inefficient ? > > > > Inefficient in terms of what? > > In terms of the operation having to lookup the connector state, when the > caller has it and can easily pass it. As Daniel commented, this may be > the price to pay for a cleaner API, but I wonder how much overhead all > the state tracking is costing. > It'd be interesting to quantify, but iirc the last time we benchmarked atomic check commits they were virtually free (~thousands/s). > > Agree that in isolation this patch might seem unnecessary, but it ties > > in with the encoder and bridge CLs which accept drm_atomic_state in > > CLs ? Googleism for patches, I usually catch these before sending... guess I missed one. Sean > > > their hooks. In general the idea is to convert all atomic functions to > > take overall atomic state instead of just their object state. Reality > > has proven to be more complicated and we need more access than what > > the current implementation provides. > > > > Sean > > > > >> Changes in v3: > > >> - Added to the set > > >> > > >> Cc: Daniel Vetter <daniel@ffwll.ch> > > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> Cc: Jani Nikula <jani.nikula@linux.intel.com> > > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > >> Cc: Ben Skeggs <bskeggs@redhat.com> > > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > >> Cc: Eric Anholt <eric@anholt.net> > > >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > > >> --- > > >> drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > > >> drivers/gpu/drm/i915/intel_atomic.c | 8 +++++--- > > >> drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++--- > > >> drivers/gpu/drm/i915/intel_drv.h | 2 +- > > >> drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- > > >> drivers/gpu/drm/i915/intel_tv.c | 8 +++++--- > > >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++-- > > >> drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++----- > > >> drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++--- > > >> include/drm/drm_modeset_helper_vtables.h | 2 +- > > >> 10 files changed, 37 insertions(+), 27 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > >> index 9d9e47276839..fa5a367507c1 100644 > > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > > >> @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > >> } > > >> > > >> if (funcs->atomic_check) > > >> - ret = funcs->atomic_check(connector, new_connector_state); > > >> + ret = funcs->atomic_check(connector, state); > > >> if (ret) > > >> return ret; > > >> > > >> @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > >> continue; > > >> > > >> if (funcs->atomic_check) > > >> - ret = funcs->atomic_check(connector, new_connector_state); > > >> + ret = funcs->atomic_check(connector, state); > > >> if (ret) > > >> return ret; > > >> } > > >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > > >> index b844e8840c6f..e8a5b82e9242 100644 > > >> --- a/drivers/gpu/drm/i915/intel_atomic.c > > >> +++ b/drivers/gpu/drm/i915/intel_atomic.c > > >> @@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > > >> } > > >> > > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > > >> - struct drm_connector_state *new_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> + struct drm_connector_state *new_state = > > >> + drm_atomic_get_new_connector_state(state, conn); > > >> struct intel_digital_connector_state *new_conn_state = > > >> to_intel_digital_connector_state(new_state); > > >> struct drm_connector_state *old_state = > > >> - drm_atomic_get_old_connector_state(new_state->state, conn); > > >> + drm_atomic_get_old_connector_state(state, conn); > > >> struct intel_digital_connector_state *old_conn_state = > > >> to_intel_digital_connector_state(old_state); > > >> struct drm_crtc_state *crtc_state; > > >> @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > > >> if (!new_state->crtc) > > >> return 0; > > >> > > >> - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > > >> + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > >> > > >> /* > > >> * These properties are handled by fastset, and might not end > > >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > >> index 19d81cef2ab6..89cfec128ba0 100644 > > >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > >> @@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > > >> > > >> static int > > >> intel_dp_mst_atomic_check(struct drm_connector *connector, > > >> - struct drm_connector_state *new_conn_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> - struct drm_atomic_state *state = new_conn_state->state; > > >> + struct drm_connector_state *new_conn_state = > > >> + drm_atomic_get_new_connector_state(state, connector); > > >> struct drm_connector_state *old_conn_state = > > >> drm_atomic_get_old_connector_state(state, connector); > > >> struct intel_connector *intel_connector = > > >> @@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, > > >> struct drm_dp_mst_topology_mgr *mgr; > > >> int ret; > > >> > > >> - ret = intel_digital_connector_atomic_check(connector, new_conn_state); > > >> + ret = intel_digital_connector_atomic_check(connector, state); > > >> if (ret) > > >> return ret; > > >> > > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > >> index f8c7b291fdc3..88571b8e8d62 100644 > > >> --- a/drivers/gpu/drm/i915/intel_drv.h > > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > > >> @@ -2481,7 +2481,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > > >> struct drm_property *property, > > >> u64 val); > > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > > >> - struct drm_connector_state *new_state); > > >> + struct drm_atomic_state *state); > > >> struct drm_connector_state * > > >> intel_digital_connector_duplicate_state(struct drm_connector *connector); > > >> > > >> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > >> index 68f497493d43..72ea164b971c 100644 > > >> --- a/drivers/gpu/drm/i915/intel_sdvo.c > > >> +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > >> @@ -2342,9 +2342,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = { > > >> }; > > >> > > >> static int intel_sdvo_atomic_check(struct drm_connector *conn, > > >> - struct drm_connector_state *new_conn_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> - struct drm_atomic_state *state = new_conn_state->state; > > >> + struct drm_connector_state *new_conn_state = > > >> + drm_atomic_get_new_connector_state(state, conn); > > >> struct drm_connector_state *old_conn_state = > > >> drm_atomic_get_old_connector_state(state, conn); > > >> struct intel_sdvo_connector_state *old_state = > > >> @@ -2356,13 +2357,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn, > > >> (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) || > > >> memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) { > > >> struct drm_crtc_state *crtc_state = > > >> - drm_atomic_get_new_crtc_state(new_conn_state->state, > > >> + drm_atomic_get_new_crtc_state(state, > > >> new_conn_state->crtc); > > >> > > >> crtc_state->connectors_changed = true; > > >> } > > >> > > >> - return intel_digital_connector_atomic_check(conn, new_conn_state); > > >> + return intel_digital_connector_atomic_check(conn, state); > > >> } > > >> > > >> static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = { > > >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > > >> index 3924c4944e1f..a41c5b467c14 100644 > > >> --- a/drivers/gpu/drm/i915/intel_tv.c > > >> +++ b/drivers/gpu/drm/i915/intel_tv.c > > >> @@ -1817,16 +1817,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > > >> }; > > >> > > >> static int intel_tv_atomic_check(struct drm_connector *connector, > > >> - struct drm_connector_state *new_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> + struct drm_connector_state *new_state; > > >> struct drm_crtc_state *new_crtc_state; > > >> struct drm_connector_state *old_state; > > >> > > >> + new_state = drm_atomic_get_new_connector_state(state, connector); > > >> if (!new_state->crtc) > > >> return 0; > > >> > > >> - old_state = drm_atomic_get_old_connector_state(new_state->state, connector); > > >> - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > > >> + old_state = drm_atomic_get_old_connector_state(state, connector); > > >> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > >> > > >> if (old_state->tv.mode != new_state->tv.mode || > > >> old_state->tv.margins.left != new_state->tv.margins.left || > > >> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > > >> index 4b1650f51955..7ba373f493b2 100644 > > >> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > > >> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > > >> @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector) > > >> > > >> static int > > >> nv50_mstc_atomic_check(struct drm_connector *connector, > > >> - struct drm_connector_state *new_conn_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> - struct drm_atomic_state *state = new_conn_state->state; > > >> struct nv50_mstc *mstc = nv50_mstc(connector); > > >> struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; > > >> + struct drm_connector_state *new_conn_state = > > >> + drm_atomic_get_new_connector_state(state, connector); > > >> struct drm_connector_state *old_conn_state = > > >> drm_atomic_get_old_connector_state(state, connector); > > >> struct drm_crtc_state *crtc_state; > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > >> index 620b51aab291..5b81ba2a7f27 100644 > > >> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > >> @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector) > > >> } > > >> > > >> static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > > >> - struct drm_connector_state *state) > > >> + struct drm_atomic_state *state) > > >> { > > >> struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); > > >> const struct drm_display_mode *panel_mode; > > >> + struct drm_connector_state *conn_state; > > >> struct drm_crtc_state *crtc_state; > > >> > > >> - if (!state->crtc) > > >> + conn_state = drm_atomic_get_new_connector_state(state, connector); > > >> + if (!conn_state->crtc) > > >> return 0; > > >> > > >> if (list_empty(&connector->modes)) { > > >> @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > > >> struct drm_display_mode, head); > > >> > > >> /* We're not allowed to modify the resolution. */ > > >> - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); > > >> - if (IS_ERR(crtc_state)) > > >> - return PTR_ERR(crtc_state); > > >> + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); > > >> + if (!crtc_state) > > >> + return -EINVAL; > > >> > > >> if (crtc_state->mode.hdisplay != panel_mode->hdisplay || > > >> crtc_state->mode.vdisplay != panel_mode->vdisplay) > > >> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > > >> index c8b89a78f9f4..96f91c1b4b6e 100644 > > >> --- a/drivers/gpu/drm/vc4/vc4_txp.c > > >> +++ b/drivers/gpu/drm/vc4/vc4_txp.c > > >> @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = { > > >> }; > > >> > > >> static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > > >> - struct drm_connector_state *conn_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> + struct drm_connector_state *conn_state; > > >> struct drm_crtc_state *crtc_state; > > >> struct drm_framebuffer *fb; > > >> int i; > > >> > > >> + conn_state = drm_atomic_get_new_connector_state(state, conn); > > >> if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > > >> return 0; > > >> > > >> - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, > > >> - conn_state->crtc); > > >> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > > >> > > >> fb = conn_state->writeback_job->fb; > > >> if (fb->width != crtc_state->mode.hdisplay || > > >> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > > >> index de57fb40cb6e..adc8b7cf64b5 100644 > > >> --- a/include/drm/drm_modeset_helper_vtables.h > > >> +++ b/include/drm/drm_modeset_helper_vtables.h > > >> @@ -1020,7 +1020,7 @@ struct drm_connector_helper_funcs { > > >> * deadlock. > > >> */ > > >> int (*atomic_check)(struct drm_connector *connector, > > >> - struct drm_connector_state *state); > > >> + struct drm_atomic_state *state); > > >> > > >> /** > > >> * @atomic_commit: > > -- > Regards, > > Laurent Pinchart -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state 2019-05-11 19:12 ` Laurent Pinchart [not found] ` <20190511191202.GL13043-N3hz7ZxfLydczECFQUw77jytWr6r+dGw0E9HWUfgJXw@public.gmane.org> @ 2019-05-13 14:47 ` Daniel Vetter 2019-05-16 12:02 ` Laurent Pinchart 1 sibling, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2019-05-13 14:47 UTC (permalink / raw) To: Laurent Pinchart Cc: Maxime Ripard, nouveau, David Airlie, linux-kernel, linux-renesas-soc, Kieran Bingham, Sean Paul, dri-devel, intel-gfx, Ben Skeggs On Sat, May 11, 2019 at 10:12:02PM +0300, Laurent Pinchart wrote: > Hi Sean, > > Thank you for the patch. > > On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > Everyone who implements connector_helper_funcs->atomic_check reaches > > into the connector state to get the atomic state. Instead of continuing > > this pattern, change the callback signature to just give atomic state > > and let the driver determine what it does and does not need from it. > > > > Eventually all atomic functions should do this, but that's just too much > > busy work for me. > > Given that drivers also access the connector state, isn't this slightly > more inefficient ? It's atomic code, we're trying to optimize for clean code at the expense of a bit of runtime overhead due to more pointer chasing. And I agree with the general push, the pile of old/new_state pointers of various objects we're passing around is confusing. Passing the overall drm_atomic_state seems much more reasonable, and with that we can get everything else. Plus it's much more obvious whether you have the old/new state (since that's explicit when you look it up from the drm_atomic_state). If we ever see this show up in profile, and it starts mattering, first thing we need is a hashtable I think (atm it's list walking, which is just terrible). But thus far no one cares. -Daniel > > > Changes in v3: > > - Added to the set > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Ben Skeggs <bskeggs@redhat.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Cc: Eric Anholt <eric@anholt.net> > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > > drivers/gpu/drm/i915/intel_atomic.c | 8 +++++--- > > drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++--- > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- > > drivers/gpu/drm/i915/intel_tv.c | 8 +++++--- > > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++-- > > drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++----- > > drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++--- > > include/drm/drm_modeset_helper_vtables.h | 2 +- > > 10 files changed, 37 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 9d9e47276839..fa5a367507c1 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > } > > > > if (funcs->atomic_check) > > - ret = funcs->atomic_check(connector, new_connector_state); > > + ret = funcs->atomic_check(connector, state); > > if (ret) > > return ret; > > > > @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > continue; > > > > if (funcs->atomic_check) > > - ret = funcs->atomic_check(connector, new_connector_state); > > + ret = funcs->atomic_check(connector, state); > > if (ret) > > return ret; > > } > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > > index b844e8840c6f..e8a5b82e9242 100644 > > --- a/drivers/gpu/drm/i915/intel_atomic.c > > +++ b/drivers/gpu/drm/i915/intel_atomic.c > > @@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > > } > > > > int intel_digital_connector_atomic_check(struct drm_connector *conn, > > - struct drm_connector_state *new_state) > > + struct drm_atomic_state *state) > > { > > + struct drm_connector_state *new_state = > > + drm_atomic_get_new_connector_state(state, conn); > > struct intel_digital_connector_state *new_conn_state = > > to_intel_digital_connector_state(new_state); > > struct drm_connector_state *old_state = > > - drm_atomic_get_old_connector_state(new_state->state, conn); > > + drm_atomic_get_old_connector_state(state, conn); > > struct intel_digital_connector_state *old_conn_state = > > to_intel_digital_connector_state(old_state); > > struct drm_crtc_state *crtc_state; > > @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > > if (!new_state->crtc) > > return 0; > > > > - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > > + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > > > /* > > * These properties are handled by fastset, and might not end > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > index 19d81cef2ab6..89cfec128ba0 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > > > > static int > > intel_dp_mst_atomic_check(struct drm_connector *connector, > > - struct drm_connector_state *new_conn_state) > > + struct drm_atomic_state *state) > > { > > - struct drm_atomic_state *state = new_conn_state->state; > > + struct drm_connector_state *new_conn_state = > > + drm_atomic_get_new_connector_state(state, connector); > > struct drm_connector_state *old_conn_state = > > drm_atomic_get_old_connector_state(state, connector); > > struct intel_connector *intel_connector = > > @@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, > > struct drm_dp_mst_topology_mgr *mgr; > > int ret; > > > > - ret = intel_digital_connector_atomic_check(connector, new_conn_state); > > + ret = intel_digital_connector_atomic_check(connector, state); > > if (ret) > > return ret; > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index f8c7b291fdc3..88571b8e8d62 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -2481,7 +2481,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > > struct drm_property *property, > > u64 val); > > int intel_digital_connector_atomic_check(struct drm_connector *conn, > > - struct drm_connector_state *new_state); > > + struct drm_atomic_state *state); > > struct drm_connector_state * > > intel_digital_connector_duplicate_state(struct drm_connector *connector); > > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > index 68f497493d43..72ea164b971c 100644 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > @@ -2342,9 +2342,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = { > > }; > > > > static int intel_sdvo_atomic_check(struct drm_connector *conn, > > - struct drm_connector_state *new_conn_state) > > + struct drm_atomic_state *state) > > { > > - struct drm_atomic_state *state = new_conn_state->state; > > + struct drm_connector_state *new_conn_state = > > + drm_atomic_get_new_connector_state(state, conn); > > struct drm_connector_state *old_conn_state = > > drm_atomic_get_old_connector_state(state, conn); > > struct intel_sdvo_connector_state *old_state = > > @@ -2356,13 +2357,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn, > > (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) || > > memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) { > > struct drm_crtc_state *crtc_state = > > - drm_atomic_get_new_crtc_state(new_conn_state->state, > > + drm_atomic_get_new_crtc_state(state, > > new_conn_state->crtc); > > > > crtc_state->connectors_changed = true; > > } > > > > - return intel_digital_connector_atomic_check(conn, new_conn_state); > > + return intel_digital_connector_atomic_check(conn, state); > > } > > > > static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = { > > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > > index 3924c4944e1f..a41c5b467c14 100644 > > --- a/drivers/gpu/drm/i915/intel_tv.c > > +++ b/drivers/gpu/drm/i915/intel_tv.c > > @@ -1817,16 +1817,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > > }; > > > > static int intel_tv_atomic_check(struct drm_connector *connector, > > - struct drm_connector_state *new_state) > > + struct drm_atomic_state *state) > > { > > + struct drm_connector_state *new_state; > > struct drm_crtc_state *new_crtc_state; > > struct drm_connector_state *old_state; > > > > + new_state = drm_atomic_get_new_connector_state(state, connector); > > if (!new_state->crtc) > > return 0; > > > > - old_state = drm_atomic_get_old_connector_state(new_state->state, connector); > > - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > > + old_state = drm_atomic_get_old_connector_state(state, connector); > > + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > > > if (old_state->tv.mode != new_state->tv.mode || > > old_state->tv.margins.left != new_state->tv.margins.left || > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > > index 4b1650f51955..7ba373f493b2 100644 > > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > > @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector) > > > > static int > > nv50_mstc_atomic_check(struct drm_connector *connector, > > - struct drm_connector_state *new_conn_state) > > + struct drm_atomic_state *state) > > { > > - struct drm_atomic_state *state = new_conn_state->state; > > struct nv50_mstc *mstc = nv50_mstc(connector); > > struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; > > + struct drm_connector_state *new_conn_state = > > + drm_atomic_get_new_connector_state(state, connector); > > struct drm_connector_state *old_conn_state = > > drm_atomic_get_old_connector_state(state, connector); > > struct drm_crtc_state *crtc_state; > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > index 620b51aab291..5b81ba2a7f27 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector) > > } > > > > static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > > - struct drm_connector_state *state) > > + struct drm_atomic_state *state) > > { > > struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); > > const struct drm_display_mode *panel_mode; > > + struct drm_connector_state *conn_state; > > struct drm_crtc_state *crtc_state; > > > > - if (!state->crtc) > > + conn_state = drm_atomic_get_new_connector_state(state, connector); > > + if (!conn_state->crtc) > > return 0; > > > > if (list_empty(&connector->modes)) { > > @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > > struct drm_display_mode, head); > > > > /* We're not allowed to modify the resolution. */ > > - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); > > - if (IS_ERR(crtc_state)) > > - return PTR_ERR(crtc_state); > > + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); > > + if (!crtc_state) > > + return -EINVAL; > > > > if (crtc_state->mode.hdisplay != panel_mode->hdisplay || > > crtc_state->mode.vdisplay != panel_mode->vdisplay) > > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > > index c8b89a78f9f4..96f91c1b4b6e 100644 > > --- a/drivers/gpu/drm/vc4/vc4_txp.c > > +++ b/drivers/gpu/drm/vc4/vc4_txp.c > > @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = { > > }; > > > > static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > > - struct drm_connector_state *conn_state) > > + struct drm_atomic_state *state) > > { > > + struct drm_connector_state *conn_state; > > struct drm_crtc_state *crtc_state; > > struct drm_framebuffer *fb; > > int i; > > > > + conn_state = drm_atomic_get_new_connector_state(state, conn); > > if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > > return 0; > > > > - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, > > - conn_state->crtc); > > + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > > > > fb = conn_state->writeback_job->fb; > > if (fb->width != crtc_state->mode.hdisplay || > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > > index de57fb40cb6e..adc8b7cf64b5 100644 > > --- a/include/drm/drm_modeset_helper_vtables.h > > +++ b/include/drm/drm_modeset_helper_vtables.h > > @@ -1020,7 +1020,7 @@ struct drm_connector_helper_funcs { > > * deadlock. > > */ > > int (*atomic_check)(struct drm_connector *connector, > > - struct drm_connector_state *state); > > + struct drm_atomic_state *state); > > > > /** > > * @atomic_commit: > > -- > Regards, > > Laurent Pinchart -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state 2019-05-13 14:47 ` Daniel Vetter @ 2019-05-16 12:02 ` Laurent Pinchart 2019-05-16 12:07 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2019-05-16 12:02 UTC (permalink / raw) To: Sean Paul, dri-devel, Sean Paul, Ville Syrjälä, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Ben Skeggs, Kieran Bingham, Eric Anholt, Maarten Lankhorst, Maxime Ripard, David Airlie, linux-kernel, intel-gfx, nouveau, linux-renesas-soc Hi Daniel, On Mon, May 13, 2019 at 04:47:47PM +0200, Daniel Vetter wrote: > On Sat, May 11, 2019 at 10:12:02PM +0300, Laurent Pinchart wrote: > > On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > >> From: Sean Paul <seanpaul@chromium.org> > >> > >> Everyone who implements connector_helper_funcs->atomic_check reaches > >> into the connector state to get the atomic state. Instead of continuing > >> this pattern, change the callback signature to just give atomic state > >> and let the driver determine what it does and does not need from it. > >> > >> Eventually all atomic functions should do this, but that's just too much > >> busy work for me. > > > > Given that drivers also access the connector state, isn't this slightly > > more inefficient ? > > It's atomic code, we're trying to optimize for clean code at the expense > of a bit of runtime overhead due to more pointer chasing. And I agree with > the general push, the pile of old/new_state pointers of various objects > we're passing around is confusing. Passing the overall drm_atomic_state > seems much more reasonable, and with that we can get everything else. Plus > it's much more obvious whether you have the old/new state (since that's > explicit when you look it up from the drm_atomic_state). Yes, I agree it's cleaner. I just hope the atomic state tracking cost can be kept under control :-) By the way, this is likely not going to happen as it would be way too intrusive, but it would be nice to rename drm_atomic_state to drm_atomic_transaction (or something similar). It doesn't model a state, but a change between an old state to a new state. This confused me in the past, and I'm sure it can still be confusing to newcomers. > If we ever see this show up in profile, and it starts mattering, first > thing we need is a hashtable I think (atm it's list walking, which is just > terrible). But thus far no one cares. > > >> Changes in v3: > >> - Added to the set > >> > >> Cc: Daniel Vetter <daniel@ffwll.ch> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Jani Nikula <jani.nikula@linux.intel.com> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> Cc: Ben Skeggs <bskeggs@redhat.com> > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> Cc: Eric Anholt <eric@anholt.net> > >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > >> --- > >> drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > >> drivers/gpu/drm/i915/intel_atomic.c | 8 +++++--- > >> drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++--- > >> drivers/gpu/drm/i915/intel_drv.h | 2 +- > >> drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- > >> drivers/gpu/drm/i915/intel_tv.c | 8 +++++--- > >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++-- > >> drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++----- > >> drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++--- > >> include/drm/drm_modeset_helper_vtables.h | 2 +- > >> 10 files changed, 37 insertions(+), 27 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >> index 9d9e47276839..fa5a367507c1 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > >> } > >> > >> if (funcs->atomic_check) > >> - ret = funcs->atomic_check(connector, new_connector_state); > >> + ret = funcs->atomic_check(connector, state); > >> if (ret) > >> return ret; > >> > >> @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > >> continue; > >> > >> if (funcs->atomic_check) > >> - ret = funcs->atomic_check(connector, new_connector_state); > >> + ret = funcs->atomic_check(connector, state); > >> if (ret) > >> return ret; > >> } > >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > >> index b844e8840c6f..e8a5b82e9242 100644 > >> --- a/drivers/gpu/drm/i915/intel_atomic.c > >> +++ b/drivers/gpu/drm/i915/intel_atomic.c > >> @@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > >> } > >> > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > >> - struct drm_connector_state *new_state) > >> + struct drm_atomic_state *state) > >> { > >> + struct drm_connector_state *new_state = > >> + drm_atomic_get_new_connector_state(state, conn); > >> struct intel_digital_connector_state *new_conn_state = > >> to_intel_digital_connector_state(new_state); > >> struct drm_connector_state *old_state = > >> - drm_atomic_get_old_connector_state(new_state->state, conn); > >> + drm_atomic_get_old_connector_state(state, conn); > >> struct intel_digital_connector_state *old_conn_state = > >> to_intel_digital_connector_state(old_state); > >> struct drm_crtc_state *crtc_state; > >> @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > >> if (!new_state->crtc) > >> return 0; > >> > >> - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > >> + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > >> > >> /* > >> * These properties are handled by fastset, and might not end > >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > >> index 19d81cef2ab6..89cfec128ba0 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c > >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > >> @@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > >> > >> static int > >> intel_dp_mst_atomic_check(struct drm_connector *connector, > >> - struct drm_connector_state *new_conn_state) > >> + struct drm_atomic_state *state) > >> { > >> - struct drm_atomic_state *state = new_conn_state->state; > >> + struct drm_connector_state *new_conn_state = > >> + drm_atomic_get_new_connector_state(state, connector); > >> struct drm_connector_state *old_conn_state = > >> drm_atomic_get_old_connector_state(state, connector); > >> struct intel_connector *intel_connector = > >> @@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, > >> struct drm_dp_mst_topology_mgr *mgr; > >> int ret; > >> > >> - ret = intel_digital_connector_atomic_check(connector, new_conn_state); > >> + ret = intel_digital_connector_atomic_check(connector, state); > >> if (ret) > >> return ret; > >> > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index f8c7b291fdc3..88571b8e8d62 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -2481,7 +2481,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > >> struct drm_property *property, > >> u64 val); > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > >> - struct drm_connector_state *new_state); > >> + struct drm_atomic_state *state); > >> struct drm_connector_state * > >> intel_digital_connector_duplicate_state(struct drm_connector *connector); > >> > >> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > >> index 68f497493d43..72ea164b971c 100644 > >> --- a/drivers/gpu/drm/i915/intel_sdvo.c > >> +++ b/drivers/gpu/drm/i915/intel_sdvo.c > >> @@ -2342,9 +2342,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = { > >> }; > >> > >> static int intel_sdvo_atomic_check(struct drm_connector *conn, > >> - struct drm_connector_state *new_conn_state) > >> + struct drm_atomic_state *state) > >> { > >> - struct drm_atomic_state *state = new_conn_state->state; > >> + struct drm_connector_state *new_conn_state = > >> + drm_atomic_get_new_connector_state(state, conn); > >> struct drm_connector_state *old_conn_state = > >> drm_atomic_get_old_connector_state(state, conn); > >> struct intel_sdvo_connector_state *old_state = > >> @@ -2356,13 +2357,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn, > >> (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) || > >> memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) { > >> struct drm_crtc_state *crtc_state = > >> - drm_atomic_get_new_crtc_state(new_conn_state->state, > >> + drm_atomic_get_new_crtc_state(state, > >> new_conn_state->crtc); > >> > >> crtc_state->connectors_changed = true; > >> } > >> > >> - return intel_digital_connector_atomic_check(conn, new_conn_state); > >> + return intel_digital_connector_atomic_check(conn, state); > >> } > >> > >> static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = { > >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > >> index 3924c4944e1f..a41c5b467c14 100644 > >> --- a/drivers/gpu/drm/i915/intel_tv.c > >> +++ b/drivers/gpu/drm/i915/intel_tv.c > >> @@ -1817,16 +1817,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > >> }; > >> > >> static int intel_tv_atomic_check(struct drm_connector *connector, > >> - struct drm_connector_state *new_state) > >> + struct drm_atomic_state *state) > >> { > >> + struct drm_connector_state *new_state; > >> struct drm_crtc_state *new_crtc_state; > >> struct drm_connector_state *old_state; > >> > >> + new_state = drm_atomic_get_new_connector_state(state, connector); > >> if (!new_state->crtc) > >> return 0; > >> > >> - old_state = drm_atomic_get_old_connector_state(new_state->state, connector); > >> - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > >> + old_state = drm_atomic_get_old_connector_state(state, connector); > >> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > >> > >> if (old_state->tv.mode != new_state->tv.mode || > >> old_state->tv.margins.left != new_state->tv.margins.left || > >> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > >> index 4b1650f51955..7ba373f493b2 100644 > >> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > >> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > >> @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector) > >> > >> static int > >> nv50_mstc_atomic_check(struct drm_connector *connector, > >> - struct drm_connector_state *new_conn_state) > >> + struct drm_atomic_state *state) > >> { > >> - struct drm_atomic_state *state = new_conn_state->state; > >> struct nv50_mstc *mstc = nv50_mstc(connector); > >> struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; > >> + struct drm_connector_state *new_conn_state = > >> + drm_atomic_get_new_connector_state(state, connector); > >> struct drm_connector_state *old_conn_state = > >> drm_atomic_get_old_connector_state(state, connector); > >> struct drm_crtc_state *crtc_state; > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > >> index 620b51aab291..5b81ba2a7f27 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > >> @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector) > >> } > >> > >> static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > >> - struct drm_connector_state *state) > >> + struct drm_atomic_state *state) > >> { > >> struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); > >> const struct drm_display_mode *panel_mode; > >> + struct drm_connector_state *conn_state; > >> struct drm_crtc_state *crtc_state; > >> > >> - if (!state->crtc) > >> + conn_state = drm_atomic_get_new_connector_state(state, connector); > >> + if (!conn_state->crtc) > >> return 0; > >> > >> if (list_empty(&connector->modes)) { > >> @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > >> struct drm_display_mode, head); > >> > >> /* We're not allowed to modify the resolution. */ > >> - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); > >> - if (IS_ERR(crtc_state)) > >> - return PTR_ERR(crtc_state); > >> + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); > >> + if (!crtc_state) > >> + return -EINVAL; > >> > >> if (crtc_state->mode.hdisplay != panel_mode->hdisplay || > >> crtc_state->mode.vdisplay != panel_mode->vdisplay) > >> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > >> index c8b89a78f9f4..96f91c1b4b6e 100644 > >> --- a/drivers/gpu/drm/vc4/vc4_txp.c > >> +++ b/drivers/gpu/drm/vc4/vc4_txp.c > >> @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = { > >> }; > >> > >> static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > >> - struct drm_connector_state *conn_state) > >> + struct drm_atomic_state *state) > >> { > >> + struct drm_connector_state *conn_state; > >> struct drm_crtc_state *crtc_state; > >> struct drm_framebuffer *fb; > >> int i; > >> > >> + conn_state = drm_atomic_get_new_connector_state(state, conn); > >> if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > >> return 0; > >> > >> - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, > >> - conn_state->crtc); > >> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > >> > >> fb = conn_state->writeback_job->fb; > >> if (fb->width != crtc_state->mode.hdisplay || > >> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > >> index de57fb40cb6e..adc8b7cf64b5 100644 > >> --- a/include/drm/drm_modeset_helper_vtables.h > >> +++ b/include/drm/drm_modeset_helper_vtables.h > >> @@ -1020,7 +1020,7 @@ struct drm_connector_helper_funcs { > >> * deadlock. > >> */ > >> int (*atomic_check)(struct drm_connector *connector, > >> - struct drm_connector_state *state); > >> + struct drm_atomic_state *state); > >> > >> /** > >> * @atomic_commit: -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state 2019-05-16 12:02 ` Laurent Pinchart @ 2019-05-16 12:07 ` Daniel Vetter 2019-05-16 13:28 ` Ville Syrjälä 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2019-05-16 12:07 UTC (permalink / raw) To: Laurent Pinchart Cc: Sean Paul, dri-devel, Sean Paul, Ville Syrjälä, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Ben Skeggs, Kieran Bingham, Eric Anholt, Maarten Lankhorst, Maxime Ripard, David Airlie, Linux Kernel Mailing List, intel-gfx, Nouveau Dev On Thu, May 16, 2019 at 2:02 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > On Mon, May 13, 2019 at 04:47:47PM +0200, Daniel Vetter wrote: > > On Sat, May 11, 2019 at 10:12:02PM +0300, Laurent Pinchart wrote: > > > On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > > >> From: Sean Paul <seanpaul@chromium.org> > > >> > > >> Everyone who implements connector_helper_funcs->atomic_check reaches > > >> into the connector state to get the atomic state. Instead of continuing > > >> this pattern, change the callback signature to just give atomic state > > >> and let the driver determine what it does and does not need from it. > > >> > > >> Eventually all atomic functions should do this, but that's just too much > > >> busy work for me. > > > > > > Given that drivers also access the connector state, isn't this slightly > > > more inefficient ? > > > > It's atomic code, we're trying to optimize for clean code at the expense > > of a bit of runtime overhead due to more pointer chasing. And I agree with > > the general push, the pile of old/new_state pointers of various objects > > we're passing around is confusing. Passing the overall drm_atomic_state > > seems much more reasonable, and with that we can get everything else. Plus > > it's much more obvious whether you have the old/new state (since that's > > explicit when you look it up from the drm_atomic_state). > > Yes, I agree it's cleaner. I just hope the atomic state tracking cost > can be kept under control :-) > > By the way, this is likely not going to happen as it would be way too > intrusive, but it would be nice to rename drm_atomic_state to > drm_atomic_transaction (or something similar). It doesn't model a state, > but a change between an old state to a new state. This confused me in > the past, and I'm sure it can still be confusing to newcomers. Why are you the first to suggest this, this is awesome! drm_atomic_state is indeed not a state, but a transaction representing how we go from the old to the new state. I think this is awesome enough that we should actually try to make it happen. Tree-wide cocci + bribing Dave on St. Patrick's day with lots of beer might be enough :-) -Daniel > > If we ever see this show up in profile, and it starts mattering, first > > thing we need is a hashtable I think (atm it's list walking, which is just > > terrible). But thus far no one cares. > > > > >> Changes in v3: > > >> - Added to the set > > >> > > >> Cc: Daniel Vetter <daniel@ffwll.ch> > > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> Cc: Jani Nikula <jani.nikula@linux.intel.com> > > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > >> Cc: Ben Skeggs <bskeggs@redhat.com> > > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > >> Cc: Eric Anholt <eric@anholt.net> > > >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > > >> --- > > >> drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > > >> drivers/gpu/drm/i915/intel_atomic.c | 8 +++++--- > > >> drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++--- > > >> drivers/gpu/drm/i915/intel_drv.h | 2 +- > > >> drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- > > >> drivers/gpu/drm/i915/intel_tv.c | 8 +++++--- > > >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++-- > > >> drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++----- > > >> drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++--- > > >> include/drm/drm_modeset_helper_vtables.h | 2 +- > > >> 10 files changed, 37 insertions(+), 27 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > >> index 9d9e47276839..fa5a367507c1 100644 > > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > > >> @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > >> } > > >> > > >> if (funcs->atomic_check) > > >> - ret = funcs->atomic_check(connector, new_connector_state); > > >> + ret = funcs->atomic_check(connector, state); > > >> if (ret) > > >> return ret; > > >> > > >> @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > >> continue; > > >> > > >> if (funcs->atomic_check) > > >> - ret = funcs->atomic_check(connector, new_connector_state); > > >> + ret = funcs->atomic_check(connector, state); > > >> if (ret) > > >> return ret; > > >> } > > >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > > >> index b844e8840c6f..e8a5b82e9242 100644 > > >> --- a/drivers/gpu/drm/i915/intel_atomic.c > > >> +++ b/drivers/gpu/drm/i915/intel_atomic.c > > >> @@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > > >> } > > >> > > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > > >> - struct drm_connector_state *new_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> + struct drm_connector_state *new_state = > > >> + drm_atomic_get_new_connector_state(state, conn); > > >> struct intel_digital_connector_state *new_conn_state = > > >> to_intel_digital_connector_state(new_state); > > >> struct drm_connector_state *old_state = > > >> - drm_atomic_get_old_connector_state(new_state->state, conn); > > >> + drm_atomic_get_old_connector_state(state, conn); > > >> struct intel_digital_connector_state *old_conn_state = > > >> to_intel_digital_connector_state(old_state); > > >> struct drm_crtc_state *crtc_state; > > >> @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > > >> if (!new_state->crtc) > > >> return 0; > > >> > > >> - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > > >> + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > >> > > >> /* > > >> * These properties are handled by fastset, and might not end > > >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > >> index 19d81cef2ab6..89cfec128ba0 100644 > > >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > >> @@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > > >> > > >> static int > > >> intel_dp_mst_atomic_check(struct drm_connector *connector, > > >> - struct drm_connector_state *new_conn_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> - struct drm_atomic_state *state = new_conn_state->state; > > >> + struct drm_connector_state *new_conn_state = > > >> + drm_atomic_get_new_connector_state(state, connector); > > >> struct drm_connector_state *old_conn_state = > > >> drm_atomic_get_old_connector_state(state, connector); > > >> struct intel_connector *intel_connector = > > >> @@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, > > >> struct drm_dp_mst_topology_mgr *mgr; > > >> int ret; > > >> > > >> - ret = intel_digital_connector_atomic_check(connector, new_conn_state); > > >> + ret = intel_digital_connector_atomic_check(connector, state); > > >> if (ret) > > >> return ret; > > >> > > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > >> index f8c7b291fdc3..88571b8e8d62 100644 > > >> --- a/drivers/gpu/drm/i915/intel_drv.h > > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > > >> @@ -2481,7 +2481,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > > >> struct drm_property *property, > > >> u64 val); > > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > > >> - struct drm_connector_state *new_state); > > >> + struct drm_atomic_state *state); > > >> struct drm_connector_state * > > >> intel_digital_connector_duplicate_state(struct drm_connector *connector); > > >> > > >> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > >> index 68f497493d43..72ea164b971c 100644 > > >> --- a/drivers/gpu/drm/i915/intel_sdvo.c > > >> +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > >> @@ -2342,9 +2342,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = { > > >> }; > > >> > > >> static int intel_sdvo_atomic_check(struct drm_connector *conn, > > >> - struct drm_connector_state *new_conn_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> - struct drm_atomic_state *state = new_conn_state->state; > > >> + struct drm_connector_state *new_conn_state = > > >> + drm_atomic_get_new_connector_state(state, conn); > > >> struct drm_connector_state *old_conn_state = > > >> drm_atomic_get_old_connector_state(state, conn); > > >> struct intel_sdvo_connector_state *old_state = > > >> @@ -2356,13 +2357,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn, > > >> (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) || > > >> memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) { > > >> struct drm_crtc_state *crtc_state = > > >> - drm_atomic_get_new_crtc_state(new_conn_state->state, > > >> + drm_atomic_get_new_crtc_state(state, > > >> new_conn_state->crtc); > > >> > > >> crtc_state->connectors_changed = true; > > >> } > > >> > > >> - return intel_digital_connector_atomic_check(conn, new_conn_state); > > >> + return intel_digital_connector_atomic_check(conn, state); > > >> } > > >> > > >> static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = { > > >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > > >> index 3924c4944e1f..a41c5b467c14 100644 > > >> --- a/drivers/gpu/drm/i915/intel_tv.c > > >> +++ b/drivers/gpu/drm/i915/intel_tv.c > > >> @@ -1817,16 +1817,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > > >> }; > > >> > > >> static int intel_tv_atomic_check(struct drm_connector *connector, > > >> - struct drm_connector_state *new_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> + struct drm_connector_state *new_state; > > >> struct drm_crtc_state *new_crtc_state; > > >> struct drm_connector_state *old_state; > > >> > > >> + new_state = drm_atomic_get_new_connector_state(state, connector); > > >> if (!new_state->crtc) > > >> return 0; > > >> > > >> - old_state = drm_atomic_get_old_connector_state(new_state->state, connector); > > >> - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); > > >> + old_state = drm_atomic_get_old_connector_state(state, connector); > > >> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc); > > >> > > >> if (old_state->tv.mode != new_state->tv.mode || > > >> old_state->tv.margins.left != new_state->tv.margins.left || > > >> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > > >> index 4b1650f51955..7ba373f493b2 100644 > > >> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > > >> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > > >> @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector) > > >> > > >> static int > > >> nv50_mstc_atomic_check(struct drm_connector *connector, > > >> - struct drm_connector_state *new_conn_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> - struct drm_atomic_state *state = new_conn_state->state; > > >> struct nv50_mstc *mstc = nv50_mstc(connector); > > >> struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; > > >> + struct drm_connector_state *new_conn_state = > > >> + drm_atomic_get_new_connector_state(state, connector); > > >> struct drm_connector_state *old_conn_state = > > >> drm_atomic_get_old_connector_state(state, connector); > > >> struct drm_crtc_state *crtc_state; > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > >> index 620b51aab291..5b81ba2a7f27 100644 > > >> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > >> @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector) > > >> } > > >> > > >> static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > > >> - struct drm_connector_state *state) > > >> + struct drm_atomic_state *state) > > >> { > > >> struct rcar_lvds *lvds = connector_to_rcar_lvds(connector); > > >> const struct drm_display_mode *panel_mode; > > >> + struct drm_connector_state *conn_state; > > >> struct drm_crtc_state *crtc_state; > > >> > > >> - if (!state->crtc) > > >> + conn_state = drm_atomic_get_new_connector_state(state, connector); > > >> + if (!conn_state->crtc) > > >> return 0; > > >> > > >> if (list_empty(&connector->modes)) { > > >> @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector, > > >> struct drm_display_mode, head); > > >> > > >> /* We're not allowed to modify the resolution. */ > > >> - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); > > >> - if (IS_ERR(crtc_state)) > > >> - return PTR_ERR(crtc_state); > > >> + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); > > >> + if (!crtc_state) > > >> + return -EINVAL; > > >> > > >> if (crtc_state->mode.hdisplay != panel_mode->hdisplay || > > >> crtc_state->mode.vdisplay != panel_mode->vdisplay) > > >> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > > >> index c8b89a78f9f4..96f91c1b4b6e 100644 > > >> --- a/drivers/gpu/drm/vc4/vc4_txp.c > > >> +++ b/drivers/gpu/drm/vc4/vc4_txp.c > > >> @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = { > > >> }; > > >> > > >> static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > > >> - struct drm_connector_state *conn_state) > > >> + struct drm_atomic_state *state) > > >> { > > >> + struct drm_connector_state *conn_state; > > >> struct drm_crtc_state *crtc_state; > > >> struct drm_framebuffer *fb; > > >> int i; > > >> > > >> + conn_state = drm_atomic_get_new_connector_state(state, conn); > > >> if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > > >> return 0; > > >> > > >> - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, > > >> - conn_state->crtc); > > >> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > > >> > > >> fb = conn_state->writeback_job->fb; > > >> if (fb->width != crtc_state->mode.hdisplay || > > >> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > > >> index de57fb40cb6e..adc8b7cf64b5 100644 > > >> --- a/include/drm/drm_modeset_helper_vtables.h > > >> +++ b/include/drm/drm_modeset_helper_vtables.h > > >> @@ -1020,7 +1020,7 @@ struct drm_connector_helper_funcs { > > >> * deadlock. > > >> */ > > >> int (*atomic_check)(struct drm_connector *connector, > > >> - struct drm_connector_state *state); > > >> + struct drm_atomic_state *state); > > >> > > >> /** > > >> * @atomic_commit: > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state 2019-05-16 12:07 ` Daniel Vetter @ 2019-05-16 13:28 ` Ville Syrjälä 0 siblings, 0 replies; 10+ messages in thread From: Ville Syrjälä @ 2019-05-16 13:28 UTC (permalink / raw) To: Daniel Vetter Cc: David Airlie, Maxime Ripard, Nouveau Dev, dri-devel, Linux Kernel Mailing List, open list:DRM DRIVERS FOR RENESAS, Kieran Bingham, Sean Paul, Laurent Pinchart, Rodrigo Vivi, Sean Paul, intel-gfx, Ben Skeggs On Thu, May 16, 2019 at 02:07:34PM +0200, Daniel Vetter wrote: > On Thu, May 16, 2019 at 2:02 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Daniel, > > > > On Mon, May 13, 2019 at 04:47:47PM +0200, Daniel Vetter wrote: > > > On Sat, May 11, 2019 at 10:12:02PM +0300, Laurent Pinchart wrote: > > > > On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > > > >> From: Sean Paul <seanpaul@chromium.org> > > > >> > > > >> Everyone who implements connector_helper_funcs->atomic_check reaches > > > >> into the connector state to get the atomic state. Instead of continuing > > > >> this pattern, change the callback signature to just give atomic state > > > >> and let the driver determine what it does and does not need from it. > > > >> > > > >> Eventually all atomic functions should do this, but that's just too much > > > >> busy work for me. > > > > > > > > Given that drivers also access the connector state, isn't this slightly > > > > more inefficient ? > > > > > > It's atomic code, we're trying to optimize for clean code at the expense > > > of a bit of runtime overhead due to more pointer chasing. And I agree with > > > the general push, the pile of old/new_state pointers of various objects > > > we're passing around is confusing. Passing the overall drm_atomic_state > > > seems much more reasonable, and with that we can get everything else. Plus > > > it's much more obvious whether you have the old/new state (since that's > > > explicit when you look it up from the drm_atomic_state). > > > > Yes, I agree it's cleaner. I just hope the atomic state tracking cost > > can be kept under control :-) > > > > By the way, this is likely not going to happen as it would be way too > > intrusive, but it would be nice to rename drm_atomic_state to > > drm_atomic_transaction (or something similar). It doesn't model a state, > > but a change between an old state to a new state. This confused me in > > the past, and I'm sure it can still be confusing to newcomers. > > Why are you the first to suggest this, this is awesome! Can't quite tell if that's irony or not. Anyways, this has been suggested before but no volunteers stepped forward. > drm_atomic_state is indeed not a state, but a transaction representing > how we go from the old to the new state. On a semi-related topic, I've occasionally pondered about moving mode_changed & co. from the obj states to the top level state/transaction (maybe stored as a bitmask). But that would definitely not be a trivial sed job. -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-05-16 14:21 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20190502194956.218441-1-sean@poorly.run>
2019-05-02 19:49 ` [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state Sean Paul
2019-05-03 8:19 ` Daniel Vetter
2019-05-11 19:12 ` Laurent Pinchart
[not found] ` <20190511191202.GL13043-N3hz7ZxfLydczECFQUw77jytWr6r+dGw0E9HWUfgJXw@public.gmane.org>
2019-05-13 14:38 ` Sean Paul
2019-05-16 12:00 ` Laurent Pinchart
2019-05-16 14:21 ` Sean Paul
2019-05-13 14:47 ` Daniel Vetter
2019-05-16 12:02 ` Laurent Pinchart
2019-05-16 12:07 ` Daniel Vetter
2019-05-16 13:28 ` Ville Syrjälä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox