| Message ID | 1426240142-24538-5-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2015-03-19 at 00:12 +0000, Konduru, Chandra wrote: > > > -----Original Message----- > > From: Conselvan De Oliveira, Ander > > Sent: Friday, March 13, 2015 2:49 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Konduru, Chandra; Conselvan De Oliveira, Ander > > Subject: [PATCH 04/19] drm/i915: Allocate a crtc_state also when the crtc is > > being disabled > > > > For consistency, allocate a new crtc_state for a crtc that is being disabled. > > Previously only the enabled value of the current state would change. > > > > Signed-off-by: Ander Conselvan de Oliveira > > <ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++-------- > > --- > > 1 file changed, 25 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index b61e3f6..62b9021 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -11188,14 +11188,21 @@ intel_modeset_compute_config(struct drm_crtc > > *crtc, > > unsigned *prepare_pipes, > > unsigned *disable_pipes) > > { > > + struct drm_device *dev = crtc->dev; > > struct intel_crtc_state *pipe_config = NULL; > > + struct intel_crtc *intel_crtc; > > int ret = 0; > > > > intel_modeset_affected_pipes(crtc, modeset_pipes, > > prepare_pipes, disable_pipes); > > > > - if ((*modeset_pipes) == 0) > > - return NULL; > > + for_each_intel_crtc_masked(dev, *disable_pipes, intel_crtc) { > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > > + if (IS_ERR(pipe_config)) > > + return pipe_config; > > + > > + pipe_config->base.enable = false; > > + } > > > > /* > > * Note this needs changes when we start tracking multiple modes @@ - > > 11203,18 +11210,25 @@ intel_modeset_compute_config(struct drm_crtc *crtc, > > * (i.e. one pipe_config for each crtc) rather than just the one > > * for this crtc. > > */ > > - ret = intel_modeset_pipe_config(crtc, fb, mode, state); > > - if (ret) > > - return ERR_PTR(ret); > > + for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) { > > + /* FIXME: For now we still expect modeset_pipes has at most > > + * one bit set. */ > > + if (WARN_ON(&intel_crtc->base != crtc)) > > + continue; > > > > - pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); > > - if (IS_ERR(pipe_config)) > > - return pipe_config; > > + ret = intel_modeset_pipe_config(crtc, fb, mode, state); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > > + if (IS_ERR(pipe_config)) > > + return pipe_config; > > > > - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, > > - "[modeset]"); > > + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, > > + "[modeset]"); > > + } > > > > - return pipe_config; > > + return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); > > } > > Instead of calling 3 separate intel_atomic_get_crtc_state() > Can you have something like: > intel_modeset_compute_config() > { > pipe_config = intel_atomic_get_crtc_state(state, crtc); > > for each disabled pipe { > use pipe_config; > } > > for each mode_set pipe { > use pipe_config; > } > > return pipe_config; > } > > > Or the way currently done is to cover where disable_pipes != modeset_pipes? > By the way, when can it happen? Yep, disable_pipes can be different than modeset_pipes if we the mode set "steals" a connector. For instance, we could have pipe A driving HDMI-1 and then mode set to pipe B to drive HDMI-1. Pipe B will steal the encoder from pipe A, and cause it to be disable. In that case disable_pipes will have the bit for pipe A set, while modeset_pipes will have the bit for pipe B set. Ander > > > > > static int __intel_set_mode_setup_plls(struct drm_device *dev, > > -- > > 2.1.0 >
> -----Original Message----- > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] > Sent: Thursday, March 19, 2015 12:52 AM > To: Konduru, Chandra > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 04/19] drm/i915: Allocate a crtc_state also when the crtc is > being disabled > > On Thu, 2015-03-19 at 00:12 +0000, Konduru, Chandra wrote: > > > > > -----Original Message----- > > > From: Conselvan De Oliveira, Ander > > > Sent: Friday, March 13, 2015 2:49 AM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Konduru, Chandra; Conselvan De Oliveira, Ander > > > Subject: [PATCH 04/19] drm/i915: Allocate a crtc_state also when the > > > crtc is being disabled > > > > > > For consistency, allocate a new crtc_state for a crtc that is being disabled. > > > Previously only the enabled value of the current state would change. > > > > > > Signed-off-by: Ander Conselvan de Oliveira > > > <ander.conselvan.de.oliveira@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 36 > > > +++++++++++++++++++++++++-------- > > > --- > > > 1 file changed, 25 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index b61e3f6..62b9021 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -11188,14 +11188,21 @@ intel_modeset_compute_config(struct > > > drm_crtc *crtc, > > > unsigned *prepare_pipes, > > > unsigned *disable_pipes) > > > { > > > + struct drm_device *dev = crtc->dev; > > > struct intel_crtc_state *pipe_config = NULL; > > > + struct intel_crtc *intel_crtc; > > > int ret = 0; > > > > > > intel_modeset_affected_pipes(crtc, modeset_pipes, > > > prepare_pipes, disable_pipes); > > > > > > - if ((*modeset_pipes) == 0) > > > - return NULL; > > > + for_each_intel_crtc_masked(dev, *disable_pipes, intel_crtc) { > > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > > > + if (IS_ERR(pipe_config)) > > > + return pipe_config; > > > + > > > + pipe_config->base.enable = false; > > > + } > > > > > > /* > > > * Note this needs changes when we start tracking multiple modes > > > @@ - > > > 11203,18 +11210,25 @@ intel_modeset_compute_config(struct drm_crtc > *crtc, > > > * (i.e. one pipe_config for each crtc) rather than just the one > > > * for this crtc. > > > */ > > > - ret = intel_modeset_pipe_config(crtc, fb, mode, state); > > > - if (ret) > > > - return ERR_PTR(ret); > > > + for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) { > > > + /* FIXME: For now we still expect modeset_pipes has at most > > > + * one bit set. */ > > > + if (WARN_ON(&intel_crtc->base != crtc)) > > > + continue; > > > > > > - pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); > > > - if (IS_ERR(pipe_config)) > > > - return pipe_config; > > > + ret = intel_modeset_pipe_config(crtc, fb, mode, state); > > > + if (ret) > > > + return ERR_PTR(ret); > > > + > > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > > > + if (IS_ERR(pipe_config)) > > > + return pipe_config; > > > > > > - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, > > > - "[modeset]"); > > > + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, > > > + "[modeset]"); > > > + } > > > > > > - return pipe_config; > > > + return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); > > > } > > > > Instead of calling 3 separate intel_atomic_get_crtc_state() Can you > > have something like: > > intel_modeset_compute_config() > > { > > pipe_config = intel_atomic_get_crtc_state(state, crtc); > > > > for each disabled pipe { > > use pipe_config; > > } > > > > for each mode_set pipe { > > use pipe_config; > > } > > > > return pipe_config; > > } > > > > > > Or the way currently done is to cover where disable_pipes != modeset_pipes? > > By the way, when can it happen? > > Yep, disable_pipes can be different than modeset_pipes if we the mode set > "steals" a connector. For instance, we could have pipe A driving > HDMI-1 and then mode set to pipe B to drive HDMI-1. Pipe B will steal the > encoder from pipe A, and cause it to be disable. In that case disable_pipes will > have the bit for pipe A set, while modeset_pipes will have the bit for pipe B set. 1) Consider two simple scenarios: Case1: User code moving HDMI from A to B: drmModeSetCrtc(crtcA, HDMI); drmModeSetCrtc(crtcB, HDMI); /* moving HDMI to pipe B */ Case2: User code turning off HDMI: drmModeSetCrtc(crtcA, HDMI); drmModeSetCrtc(crtcA, disable); In both cases, driver will be disabling crtc for pipe A. In case 1, there is no associated crtc_state or compute & commit but directly triggering crtc_disable(crtcA). In case 2, there is associated crtc_state and associated compute and setmode calls crtc_disable(crtcA); Won't this cause trouble for low level functions (disable clocks, connectors, encoders, planes etc. etc...) acting on variables being computed and staged in their respective states? where case 1 calls with current crtc->config, and case 2 calls crt->config which is computed crtc_state 2) For example, to disable a plane differentiate between below two: plane being called to disable with fb is valid vs. plane being called to disable with fb is null. There is crtc->active somehow to take care this, but I think this should move to crtc_state. Same applies for any such other variables in crtc. And respective resource's functions should check its hosting crtc_state along with its own conditions to act on its resource. If not getting into this patch series, these changes should go into next series for achieving crtc atomicity. 3) Also low level enable/disable functions can start causing confusion if they aren't read/interpreted correctly. Either we should have resource_commit which further calls resource->enable() or resource->disable() depending on its own state and its hosting resource state; or have resource_commit calling resource->update() where it does either enable or disable based on state. We don't have above for crtc but should be done something like this if not in this patch but sometime after in order to achieve crtc atomicity. > > > Ander > > > > > > > > > static int __intel_set_mode_setup_plls(struct drm_device *dev, > > > -- > > > 2.1.0 > > >
On Thu, 2015-03-19 at 23:23 +0000, Konduru, Chandra wrote: > > > > -----Original Message----- > > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] > > Sent: Thursday, March 19, 2015 12:52 AM > > To: Konduru, Chandra > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH 04/19] drm/i915: Allocate a crtc_state also when the crtc is > > being disabled > > > > On Thu, 2015-03-19 at 00:12 +0000, Konduru, Chandra wrote: > > > > > > > -----Original Message----- > > > > From: Conselvan De Oliveira, Ander > > > > Sent: Friday, March 13, 2015 2:49 AM > > > > To: intel-gfx@lists.freedesktop.org > > > > Cc: Konduru, Chandra; Conselvan De Oliveira, Ander > > > > Subject: [PATCH 04/19] drm/i915: Allocate a crtc_state also when the > > > > crtc is being disabled > > > > > > > > For consistency, allocate a new crtc_state for a crtc that is being disabled. > > > > Previously only the enabled value of the current state would change. > > > > > > > > Signed-off-by: Ander Conselvan de Oliveira > > > > <ander.conselvan.de.oliveira@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 36 > > > > +++++++++++++++++++++++++-------- > > > > --- > > > > 1 file changed, 25 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index b61e3f6..62b9021 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -11188,14 +11188,21 @@ intel_modeset_compute_config(struct > > > > drm_crtc *crtc, > > > > unsigned *prepare_pipes, > > > > unsigned *disable_pipes) > > > > { > > > > + struct drm_device *dev = crtc->dev; > > > > struct intel_crtc_state *pipe_config = NULL; > > > > + struct intel_crtc *intel_crtc; > > > > int ret = 0; > > > > > > > > intel_modeset_affected_pipes(crtc, modeset_pipes, > > > > prepare_pipes, disable_pipes); > > > > > > > > - if ((*modeset_pipes) == 0) > > > > - return NULL; > > > > + for_each_intel_crtc_masked(dev, *disable_pipes, intel_crtc) { > > > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > > > > + if (IS_ERR(pipe_config)) > > > > + return pipe_config; > > > > + > > > > + pipe_config->base.enable = false; > > > > + } > > > > > > > > /* > > > > * Note this needs changes when we start tracking multiple modes > > > > @@ - > > > > 11203,18 +11210,25 @@ intel_modeset_compute_config(struct drm_crtc > > *crtc, > > > > * (i.e. one pipe_config for each crtc) rather than just the one > > > > * for this crtc. > > > > */ > > > > - ret = intel_modeset_pipe_config(crtc, fb, mode, state); > > > > - if (ret) > > > > - return ERR_PTR(ret); > > > > + for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) { > > > > + /* FIXME: For now we still expect modeset_pipes has at most > > > > + * one bit set. */ > > > > + if (WARN_ON(&intel_crtc->base != crtc)) > > > > + continue; > > > > > > > > - pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); > > > > - if (IS_ERR(pipe_config)) > > > > - return pipe_config; > > > > + ret = intel_modeset_pipe_config(crtc, fb, mode, state); > > > > + if (ret) > > > > + return ERR_PTR(ret); > > > > + > > > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > > > > + if (IS_ERR(pipe_config)) > > > > + return pipe_config; > > > > > > > > - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, > > > > - "[modeset]"); > > > > + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, > > > > + "[modeset]"); > > > > + } > > > > > > > > - return pipe_config; > > > > + return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); > > > > } > > > > > > Instead of calling 3 separate intel_atomic_get_crtc_state() Can you > > > have something like: > > > intel_modeset_compute_config() > > > { > > > pipe_config = intel_atomic_get_crtc_state(state, crtc); > > > > > > for each disabled pipe { > > > use pipe_config; > > > } > > > > > > for each mode_set pipe { > > > use pipe_config; > > > } > > > > > > return pipe_config; > > > } > > > > > > > > > Or the way currently done is to cover where disable_pipes != modeset_pipes? > > > By the way, when can it happen? > > > > Yep, disable_pipes can be different than modeset_pipes if we the mode set > > "steals" a connector. For instance, we could have pipe A driving > > HDMI-1 and then mode set to pipe B to drive HDMI-1. Pipe B will steal the > > encoder from pipe A, and cause it to be disable. In that case disable_pipes will > > have the bit for pipe A set, while modeset_pipes will have the bit for pipe B set. > > 1) > Consider two simple scenarios: > Case1: User code moving HDMI from A to B: > drmModeSetCrtc(crtcA, HDMI); > drmModeSetCrtc(crtcB, HDMI); /* moving HDMI to pipe B */ > > Case2: User code turning off HDMI: > drmModeSetCrtc(crtcA, HDMI); > drmModeSetCrtc(crtcA, disable); > > In both cases, driver will be disabling crtc for pipe A. > In case 1, there is no associated crtc_state or compute & commit but > directly triggering crtc_disable(crtcA). > In case 2, there is associated crtc_state and associated compute and setmode > calls crtc_disable(crtcA); > > Won't this cause trouble for low level functions (disable clocks, connectors, > encoders, planes etc. etc...) acting on variables being computed and staged > in their respective states? > where case 1 calls with current crtc->config, > and case 2 calls crt->config which is computed crtc_state It is inconsistent, yes. But at the moment, for the disable case, we just duplicate the crtc_state and set crtc_state->base.enable = false. As things stand at the moment, the net effect should be the same: we call the disable hook before changing the current state, and after we change the states, the only field that changed was crtc_state->base.enable. The only difference is what does intel_crtc->config points to. > 2) > For example, to disable a plane differentiate between below two: > plane being called to disable with fb is valid > vs. > plane being called to disable with fb is null. > > There is crtc->active somehow to take care this, but I think this should > move to crtc_state. Same applies for any such other variables in crtc. > And respective resource's functions should check its hosting crtc_state > along with its own conditions to act on its resource. > > If not getting into this patch series, these changes should go into next > series for achieving crtc atomicity. There's been discussion about crtc->active vs. crtc_state->base.active already. One problem is that the atomic semantics is different than the i915 one. We use crtc->active internally to mean the pipe is really active, so we only turn that on in the enable crtc hook and immediately disable it in the disable hook. We then use that value for sanity checking. The atomic active field may actually be true before we are finished committing, so it may be true while the crtc is still off. I think Matt had patches for this, but they were deferred until my patch series goes in. > 3) > Also low level enable/disable functions can start causing confusion > if they aren't read/interpreted correctly. Either we should have resource_commit > which further calls resource->enable() or resource->disable() depending > on its own state and its hosting resource state; or have resource_commit > calling resource->update() where it does either enable or disable based > on state. > > We don't have above for crtc but should be done something like this > if not in this patch but sometime after in order to achieve crtc atomicity. We will need something like that for the implementation of the atomic mode set, but I think we can treat that as an independent issue from this patch series. Ander > > > > > > > > Ander > > > > > > > > > > > > > static int __intel_set_mode_setup_plls(struct drm_device *dev, > > > > -- > > > > 2.1.0 > > > > > >
On Fri, Mar 20, 2015 at 10:40:37AM +0200, Ander Conselvan De Oliveira wrote: > On Thu, 2015-03-19 at 23:23 +0000, Konduru, Chandra wrote: > > 1) > > Consider two simple scenarios: > > Case1: User code moving HDMI from A to B: > > drmModeSetCrtc(crtcA, HDMI); > > drmModeSetCrtc(crtcB, HDMI); /* moving HDMI to pipe B */ > > > > Case2: User code turning off HDMI: > > drmModeSetCrtc(crtcA, HDMI); > > drmModeSetCrtc(crtcA, disable); > > > > In both cases, driver will be disabling crtc for pipe A. > > In case 1, there is no associated crtc_state or compute & commit but > > directly triggering crtc_disable(crtcA). > > In case 2, there is associated crtc_state and associated compute and setmode > > calls crtc_disable(crtcA); > > > > Won't this cause trouble for low level functions (disable clocks, connectors, > > encoders, planes etc. etc...) acting on variables being computed and staged > > in their respective states? > > where case 1 calls with current crtc->config, > > and case 2 calls crt->config which is computed crtc_state > > It is inconsistent, yes. But at the moment, for the disable case, we > just duplicate the crtc_state and set crtc_state->base.enable = false. > As things stand at the moment, the net effect should be the same: we > call the disable hook before changing the current state, and after we > change the states, the only field that changed was > crtc_state->base.enable. The only difference is what does > intel_crtc->config points to. I didn't digg into full details but don't we need to at least clear out shared resources like plls too? Or is that all guarded with checks for crtc_state->enable? Overall it sounds like clearing the crtc state properly when duplicating will be a lot of tricky fun. -Daniel
On Fri, 2015-03-20 at 10:51 +0100, Daniel Vetter wrote: > On Fri, Mar 20, 2015 at 10:40:37AM +0200, Ander Conselvan De Oliveira wrote: > > On Thu, 2015-03-19 at 23:23 +0000, Konduru, Chandra wrote: > > > 1) > > > Consider two simple scenarios: > > > Case1: User code moving HDMI from A to B: > > > drmModeSetCrtc(crtcA, HDMI); > > > drmModeSetCrtc(crtcB, HDMI); /* moving HDMI to pipe B */ > > > > > > Case2: User code turning off HDMI: > > > drmModeSetCrtc(crtcA, HDMI); > > > drmModeSetCrtc(crtcA, disable); > > > > > > In both cases, driver will be disabling crtc for pipe A. > > > In case 1, there is no associated crtc_state or compute & commit but > > > directly triggering crtc_disable(crtcA). > > > In case 2, there is associated crtc_state and associated compute and setmode > > > calls crtc_disable(crtcA); > > > > > > Won't this cause trouble for low level functions (disable clocks, connectors, > > > encoders, planes etc. etc...) acting on variables being computed and staged > > > in their respective states? > > > where case 1 calls with current crtc->config, > > > and case 2 calls crt->config which is computed crtc_state > > > > It is inconsistent, yes. But at the moment, for the disable case, we > > just duplicate the crtc_state and set crtc_state->base.enable = false. > > As things stand at the moment, the net effect should be the same: we > > call the disable hook before changing the current state, and after we > > change the states, the only field that changed was > > crtc_state->base.enable. The only difference is what does > > intel_crtc->config points to. > > I didn't digg into full details but don't we need to at least clear out > shared resources like plls too? Or is that all guarded with checks for > crtc_state->enable? We don't update crtc_state->shared_dpll during disable. But the crtc off function calls intel_put_shared_dpll() that removes the crtc from the shared dpll's crtc mask (in global state). When we enable we allocate a new state that has crtc_state->shared_dpll = DPLL_ID_PRIVATE. Bottom line is, we don't rely on the old state when enabling a crtc. > Overall it sounds like clearing the crtc state properly when duplicating > will be a lot of tricky fun. Yeah, sounds like there's still "fun" times ahead. Ander
On Fri, Mar 20, 2015 at 12:06:46PM +0200, Ander Conselvan De Oliveira wrote: > On Fri, 2015-03-20 at 10:51 +0100, Daniel Vetter wrote: > > On Fri, Mar 20, 2015 at 10:40:37AM +0200, Ander Conselvan De Oliveira wrote: > > > On Thu, 2015-03-19 at 23:23 +0000, Konduru, Chandra wrote: > > > > 1) > > > > Consider two simple scenarios: > > > > Case1: User code moving HDMI from A to B: > > > > drmModeSetCrtc(crtcA, HDMI); > > > > drmModeSetCrtc(crtcB, HDMI); /* moving HDMI to pipe B */ > > > > > > > > Case2: User code turning off HDMI: > > > > drmModeSetCrtc(crtcA, HDMI); > > > > drmModeSetCrtc(crtcA, disable); > > > > > > > > In both cases, driver will be disabling crtc for pipe A. > > > > In case 1, there is no associated crtc_state or compute & commit but > > > > directly triggering crtc_disable(crtcA). > > > > In case 2, there is associated crtc_state and associated compute and setmode > > > > calls crtc_disable(crtcA); > > > > > > > > Won't this cause trouble for low level functions (disable clocks, connectors, > > > > encoders, planes etc. etc...) acting on variables being computed and staged > > > > in their respective states? > > > > where case 1 calls with current crtc->config, > > > > and case 2 calls crt->config which is computed crtc_state > > > > > > It is inconsistent, yes. But at the moment, for the disable case, we > > > just duplicate the crtc_state and set crtc_state->base.enable = false. > > > As things stand at the moment, the net effect should be the same: we > > > call the disable hook before changing the current state, and after we > > > change the states, the only field that changed was > > > crtc_state->base.enable. The only difference is what does > > > intel_crtc->config points to. > > > > I didn't digg into full details but don't we need to at least clear out > > shared resources like plls too? Or is that all guarded with checks for > > crtc_state->enable? > > We don't update crtc_state->shared_dpll during disable. But the crtc off > function calls intel_put_shared_dpll() that removes the crtc from the > shared dpll's crtc mask (in global state). When we enable we allocate a > new state that has crtc_state->shared_dpll = DPLL_ID_PRIVATE. > > Bottom line is, we don't rely on the old state when enabling a crtc. Ah right I've missed that. Need to keep an eye out for this bit to make sure we keep it working. Also it's another case where we change the states still after the check phase, which is a big no-no in atomic land. > > Overall it sounds like clearing the crtc state properly when duplicating > > will be a lot of tricky fun. > > Yeah, sounds like there's still "fun" times ahead. Indeed. -Daniel
> -----Original Message----- > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] > Sent: Friday, March 20, 2015 1:41 AM > To: Konduru, Chandra > Cc: intel-gfx@lists.freedesktop.org; Roper, Matthew D > Subject: Re: [PATCH 04/19] drm/i915: Allocate a crtc_state also when the crtc is > being disabled > > On Thu, 2015-03-19 at 23:23 +0000, Konduru, Chandra wrote: > > > > > > > -----Original Message----- > > > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] > > > Sent: Thursday, March 19, 2015 12:52 AM > > > To: Konduru, Chandra > > > Cc: intel-gfx@lists.freedesktop.org > > > Subject: Re: [PATCH 04/19] drm/i915: Allocate a crtc_state also when > > > the crtc is being disabled > > > > > > On Thu, 2015-03-19 at 00:12 +0000, Konduru, Chandra wrote: > > > > > > > > > -----Original Message----- > > > > > From: Conselvan De Oliveira, Ander > > > > > Sent: Friday, March 13, 2015 2:49 AM > > > > > To: intel-gfx@lists.freedesktop.org > > > > > Cc: Konduru, Chandra; Conselvan De Oliveira, Ander > > > > > Subject: [PATCH 04/19] drm/i915: Allocate a crtc_state also when > > > > > the crtc is being disabled > > > > > > > > > > For consistency, allocate a new crtc_state for a crtc that is being > disabled. > > > > > Previously only the enabled value of the current state would change. > > > > > > > > > > Signed-off-by: Ander Conselvan de Oliveira > > > > > <ander.conselvan.de.oliveira@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 36 > > > > > +++++++++++++++++++++++++-------- > > > > > --- > > > > > 1 file changed, 25 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > > index b61e3f6..62b9021 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -11188,14 +11188,21 @@ intel_modeset_compute_config(struct > > > > > drm_crtc *crtc, > > > > > unsigned *prepare_pipes, > > > > > unsigned *disable_pipes) { > > > > > + struct drm_device *dev = crtc->dev; > > > > > struct intel_crtc_state *pipe_config = NULL; > > > > > + struct intel_crtc *intel_crtc; > > > > > int ret = 0; > > > > > > > > > > intel_modeset_affected_pipes(crtc, modeset_pipes, > > > > > prepare_pipes, disable_pipes); > > > > > > > > > > - if ((*modeset_pipes) == 0) > > > > > - return NULL; > > > > > + for_each_intel_crtc_masked(dev, *disable_pipes, intel_crtc) { > > > > > + pipe_config = intel_atomic_get_crtc_state(state, > intel_crtc); > > > > > + if (IS_ERR(pipe_config)) > > > > > + return pipe_config; > > > > > + > > > > > + pipe_config->base.enable = false; > > > > > + } > > > > > > > > > > /* > > > > > * Note this needs changes when we start tracking multiple > > > > > modes @@ - > > > > > 11203,18 +11210,25 @@ intel_modeset_compute_config(struct > > > > > drm_crtc > > > *crtc, > > > > > * (i.e. one pipe_config for each crtc) rather than just the one > > > > > * for this crtc. > > > > > */ > > > > > - ret = intel_modeset_pipe_config(crtc, fb, mode, state); > > > > > - if (ret) > > > > > - return ERR_PTR(ret); > > > > > + for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) { > > > > > + /* FIXME: For now we still expect modeset_pipes has at > most > > > > > + * one bit set. */ > > > > > + if (WARN_ON(&intel_crtc->base != crtc)) > > > > > + continue; > > > > > > > > > > - pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); > > > > > - if (IS_ERR(pipe_config)) > > > > > - return pipe_config; > > > > > + ret = intel_modeset_pipe_config(crtc, fb, mode, state); > > > > > + if (ret) > > > > > + return ERR_PTR(ret); > > > > > + > > > > > + pipe_config = intel_atomic_get_crtc_state(state, > intel_crtc); > > > > > + if (IS_ERR(pipe_config)) > > > > > + return pipe_config; > > > > > > > > > > - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, > > > > > - "[modeset]"); > > > > > + intel_dump_pipe_config(to_intel_crtc(crtc), > pipe_config, > > > > > + "[modeset]"); > > > > > + } > > > > > > > > > > - return pipe_config; > > > > > + return intel_atomic_get_crtc_state(state, > > > > > +to_intel_crtc(crtc)); > > > > > } > > > > > > > > Instead of calling 3 separate intel_atomic_get_crtc_state() Can > > > > you have something like: > > > > intel_modeset_compute_config() > > > > { > > > > pipe_config = intel_atomic_get_crtc_state(state, crtc); > > > > > > > > for each disabled pipe { > > > > use pipe_config; > > > > } > > > > > > > > for each mode_set pipe { > > > > use pipe_config; > > > > } > > > > > > > > return pipe_config; > > > > } > > > > > > > > > > > > Or the way currently done is to cover where disable_pipes != > modeset_pipes? > > > > By the way, when can it happen? > > > > > > Yep, disable_pipes can be different than modeset_pipes if we the > > > mode set "steals" a connector. For instance, we could have pipe A > > > driving > > > HDMI-1 and then mode set to pipe B to drive HDMI-1. Pipe B will > > > steal the encoder from pipe A, and cause it to be disable. In that > > > case disable_pipes will have the bit for pipe A set, while modeset_pipes will > have the bit for pipe B set. > > > > 1) > > Consider two simple scenarios: > > Case1: User code moving HDMI from A to B: > > drmModeSetCrtc(crtcA, HDMI); > > drmModeSetCrtc(crtcB, HDMI); /* moving HDMI to pipe B */ > > > > Case2: User code turning off HDMI: > > drmModeSetCrtc(crtcA, HDMI); > > drmModeSetCrtc(crtcA, disable); > > > > In both cases, driver will be disabling crtc for pipe A. > > In case 1, there is no associated crtc_state or compute & commit but > > directly triggering crtc_disable(crtcA). > > In case 2, there is associated crtc_state and associated compute and > > setmode calls crtc_disable(crtcA); > > > > Won't this cause trouble for low level functions (disable clocks, > > connectors, encoders, planes etc. etc...) acting on variables being > > computed and staged in their respective states? > > where case 1 calls with current crtc->config, > > and case 2 calls crt->config which is computed crtc_state > > It is inconsistent, yes. But at the moment, for the disable case, we just duplicate > the crtc_state and set crtc_state->base.enable = false. > As things stand at the moment, the net effect should be the same: we call the > disable hook before changing the current state, and after we change the states, > the only field that changed was crtc_state->base.enable. The only difference is > what does intel_crtc->config points to. As things stand atm, this may be ok. But as soon as low level functions truly depend on state and its hosting crtc state to act on, this becomes an issue if crtc->config pointing to new state vs. current state. I saw this as an issue while implementing scalers, but managed to make it work. I think this is one another next steps as part of full atomic crtc. > > > 2) > > For example, to disable a plane differentiate between below two: > > plane being called to disable with fb is valid > > vs. > > plane being called to disable with fb is null. > > > > There is crtc->active somehow to take care this, but I think this > > should move to crtc_state. Same applies for any such other variables in crtc. > > And respective resource's functions should check its hosting > > crtc_state along with its own conditions to act on its resource. > > > > If not getting into this patch series, these changes should go into > > next series for achieving crtc atomicity. > > There's been discussion about crtc->active vs. crtc_state->base.active already. > One problem is that the atomic semantics is different than the > i915 one. Yeah, this is something we need to settle on. > We use crtc->active internally to mean the pipe is really active, so we > only turn that on in the enable crtc hook and immediately disable it in the disable > hook. We then use that value for sanity checking. I think it seems opposite to the atomic semantics. But will we be using crtc_state->base.active in atomic semantics way? May be need to settle on in next steps after this series if not now. > > The atomic active field may actually be true before we are finished committing, > so it may be true while the crtc is still off. > > I think Matt had patches for this, but they were deferred until my patch series > goes in. > > > 3) > > Also low level enable/disable functions can start causing confusion if > > they aren't read/interpreted correctly. Either we should have > > resource_commit which further calls resource->enable() or > > resource->disable() depending on its own state and its hosting > > resource state; or have resource_commit calling resource->update() > > where it does either enable or disable based on state. > > > > We don't have above for crtc but should be done something like this if > > not in this patch but sometime after in order to achieve crtc atomicity. > > We will need something like that for the implementation of the atomic mode > set, but I think we can treat that as an independent issue from this patch series. That is fine as long as it is binned for next time. > > > Ander > > > > > > > > > > > > > > Ander > > > > > > > > > > > > > > > > > static int __intel_set_mode_setup_plls(struct drm_device *dev, > > > > > -- > > > > > 2.1.0 > > > > > > > > > >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b61e3f6..62b9021 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11188,14 +11188,21 @@ intel_modeset_compute_config(struct drm_crtc *crtc, unsigned *prepare_pipes, unsigned *disable_pipes) { + struct drm_device *dev = crtc->dev; struct intel_crtc_state *pipe_config = NULL; + struct intel_crtc *intel_crtc; int ret = 0; intel_modeset_affected_pipes(crtc, modeset_pipes, prepare_pipes, disable_pipes); - if ((*modeset_pipes) == 0) - return NULL; + for_each_intel_crtc_masked(dev, *disable_pipes, intel_crtc) { + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); + if (IS_ERR(pipe_config)) + return pipe_config; + + pipe_config->base.enable = false; + } /* * Note this needs changes when we start tracking multiple modes @@ -11203,18 +11210,25 @@ intel_modeset_compute_config(struct drm_crtc *crtc, * (i.e. one pipe_config for each crtc) rather than just the one * for this crtc. */ - ret = intel_modeset_pipe_config(crtc, fb, mode, state); - if (ret) - return ERR_PTR(ret); + for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) { + /* FIXME: For now we still expect modeset_pipes has at most + * one bit set. */ + if (WARN_ON(&intel_crtc->base != crtc)) + continue; - pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); - if (IS_ERR(pipe_config)) - return pipe_config; + ret = intel_modeset_pipe_config(crtc, fb, mode, state); + if (ret) + return ERR_PTR(ret); + + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); + if (IS_ERR(pipe_config)) + return pipe_config; - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, - "[modeset]"); + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, + "[modeset]"); + } - return pipe_config; + return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); } static int __intel_set_mode_setup_plls(struct drm_device *dev,
For consistency, allocate a new crtc_state for a crtc that is being disabled. Previously only the enabled value of the current state would change. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)