| Message ID | 1426240142-24538-17-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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 16/19] drm/i915: Check lane sharing between pipes B & C using > atomic state > > Makes that code atomic ready. > > Signed-off-by: Ander Conselvan de Oliveira > <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 49 ++++++++++++++++++++++++++++++- > ----- > 1 file changed, 42 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index e720a48..8c97186 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5537,13 +5537,20 @@ bool intel_connector_get_hw_state(struct > intel_connector *connector) > return encoder->get_hw_state(encoder, &pipe); } > > -static int pipe_required_fdi_lanes(struct drm_device *dev, enum pipe pipe) > +static int pipe_required_fdi_lanes(struct drm_atomic_state *state, > + enum pipe pipe) > { > struct intel_crtc *crtc = > - to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe)); > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, pipe)); > + struct intel_crtc_state *crtc_state; > + > + crtc_state = intel_atomic_get_crtc_state(state, crtc); > + if (WARN_ON(IS_ERR(crtc_state))) { > + /* Cause modeset to fail due to excess lanes. */ > + return 5; > + } > > - if (crtc->base.state->enable && > - crtc->config->has_pch_encoder) > + if (crtc_state->base.enable && crtc_state->has_pch_encoder) > return crtc->config->fdi_lanes; > > return 0; > @@ -5552,6 +5559,8 @@ static int pipe_required_fdi_lanes(struct drm_device > *dev, enum pipe pipe) static bool ironlake_check_fdi_lanes(struct drm_device > *dev, enum pipe pipe, > struct intel_crtc_state *pipe_config) { > + struct drm_atomic_state *state = pipe_config->base.state; > + > DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n", > pipe_name(pipe), pipe_config->fdi_lanes); > if (pipe_config->fdi_lanes > 4) { > @@ -5579,7 +5588,7 @@ static bool ironlake_check_fdi_lanes(struct > drm_device *dev, enum pipe pipe, > return true; > case PIPE_B: > if (pipe_config->fdi_lanes > 2 && > - pipe_required_fdi_lanes(dev, PIPE_C) > 0) { > + pipe_required_fdi_lanes(state, PIPE_C) > 0) { > DRM_DEBUG_KMS("invalid shared fdi lane config on > pipe %c: %i lanes\n", > pipe_name(pipe), pipe_config->fdi_lanes); > return false; > @@ -5591,7 +5600,7 @@ static bool ironlake_check_fdi_lanes(struct > drm_device *dev, enum pipe pipe, > pipe_name(pipe), pipe_config->fdi_lanes); > return false; > } > - if (pipe_required_fdi_lanes(dev, PIPE_B) > 2) { > + if (pipe_required_fdi_lanes(state, PIPE_B) > 2) { > DRM_DEBUG_KMS("fdi link B uses too many lanes to > enable link C\n"); > return false; > } > @@ -5601,15 +5610,41 @@ static bool ironlake_check_fdi_lanes(struct > drm_device *dev, enum pipe pipe, > } > } > > +static int add_pipe_b_c_to_state(struct drm_atomic_state *state) { > + struct intel_crtc *pipe_B = > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_B)); > + struct intel_crtc *pipe_C = > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_C)); > + struct intel_crtc_state *crtc_state; > + > + crtc_state = intel_atomic_get_crtc_state(state, pipe_B); > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); > + > + crtc_state = intel_atomic_get_crtc_state(state, pipe_C); > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); > + > + return 0; > +} > + > #define RETRY 1 > static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, > struct intel_crtc_state *pipe_config) { > struct drm_device *dev = intel_crtc->base.dev; > struct drm_display_mode *adjusted_mode = &pipe_config- > >base.adjusted_mode; > - int lane, link_bw, fdi_dotclock; > + int lane, link_bw, fdi_dotclock, ret; > bool setup_ok, needs_recompute = false; > > + if (IS_IVYBRIDGE(dev) && > + (intel_crtc->pipe == PIPE_B || intel_crtc->pipe == PIPE_C)) { > + ret = add_pipe_b_c_to_state(pipe_config->base.state); In this scenario, crtc_states are created for both pipe B & C as an operation on one can affect the other. I may be missing something here, but where is the other crtc_state being used: compute/check flow and/or commit flow? > + if (ret < 0) > + return ret; > + } > + > retry: > /* FDI is a binary signal running at ~2.7GHz, encoding > * each output octet as 10 bits. The actual frequency > -- > 2.1.0
On Thu, 2015-03-19 at 20:58 +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 16/19] drm/i915: Check lane sharing between pipes B & C using > > atomic state > > > > Makes that code atomic ready. > > > > Signed-off-by: Ander Conselvan de Oliveira > > <ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 49 ++++++++++++++++++++++++++++++- > > ----- > > 1 file changed, 42 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index e720a48..8c97186 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5537,13 +5537,20 @@ bool intel_connector_get_hw_state(struct > > intel_connector *connector) > > return encoder->get_hw_state(encoder, &pipe); } > > > > -static int pipe_required_fdi_lanes(struct drm_device *dev, enum pipe pipe) > > +static int pipe_required_fdi_lanes(struct drm_atomic_state *state, > > + enum pipe pipe) > > { > > struct intel_crtc *crtc = > > - to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe)); > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, pipe)); > > + struct intel_crtc_state *crtc_state; > > + > > + crtc_state = intel_atomic_get_crtc_state(state, crtc); > > + if (WARN_ON(IS_ERR(crtc_state))) { > > + /* Cause modeset to fail due to excess lanes. */ > > + return 5; > > + } > > > > - if (crtc->base.state->enable && > > - crtc->config->has_pch_encoder) > > + if (crtc_state->base.enable && crtc_state->has_pch_encoder) > > return crtc->config->fdi_lanes; > > > > return 0; > > @@ -5552,6 +5559,8 @@ static int pipe_required_fdi_lanes(struct drm_device > > *dev, enum pipe pipe) static bool ironlake_check_fdi_lanes(struct drm_device > > *dev, enum pipe pipe, > > struct intel_crtc_state *pipe_config) { > > + struct drm_atomic_state *state = pipe_config->base.state; > > + > > DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n", > > pipe_name(pipe), pipe_config->fdi_lanes); > > if (pipe_config->fdi_lanes > 4) { > > @@ -5579,7 +5588,7 @@ static bool ironlake_check_fdi_lanes(struct > > drm_device *dev, enum pipe pipe, > > return true; > > case PIPE_B: > > if (pipe_config->fdi_lanes > 2 && > > - pipe_required_fdi_lanes(dev, PIPE_C) > 0) { > > + pipe_required_fdi_lanes(state, PIPE_C) > 0) { > > DRM_DEBUG_KMS("invalid shared fdi lane config on > > pipe %c: %i lanes\n", > > pipe_name(pipe), pipe_config->fdi_lanes); > > return false; > > @@ -5591,7 +5600,7 @@ static bool ironlake_check_fdi_lanes(struct > > drm_device *dev, enum pipe pipe, > > pipe_name(pipe), pipe_config->fdi_lanes); > > return false; > > } > > - if (pipe_required_fdi_lanes(dev, PIPE_B) > 2) { > > + if (pipe_required_fdi_lanes(state, PIPE_B) > 2) { > > DRM_DEBUG_KMS("fdi link B uses too many lanes to > > enable link C\n"); > > return false; > > } > > @@ -5601,15 +5610,41 @@ static bool ironlake_check_fdi_lanes(struct > > drm_device *dev, enum pipe pipe, > > } > > } > > > > +static int add_pipe_b_c_to_state(struct drm_atomic_state *state) { > > + struct intel_crtc *pipe_B = > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_B)); > > + struct intel_crtc *pipe_C = > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_C)); > > + struct intel_crtc_state *crtc_state; > > + > > + crtc_state = intel_atomic_get_crtc_state(state, pipe_B); > > + if (IS_ERR(crtc_state)) > > + return PTR_ERR(crtc_state); > > + > > + crtc_state = intel_atomic_get_crtc_state(state, pipe_C); > > + if (IS_ERR(crtc_state)) > > + return PTR_ERR(crtc_state); > > + > > + return 0; > > +} > > + > > #define RETRY 1 > > static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, > > struct intel_crtc_state *pipe_config) { > > struct drm_device *dev = intel_crtc->base.dev; > > struct drm_display_mode *adjusted_mode = &pipe_config- > > >base.adjusted_mode; > > - int lane, link_bw, fdi_dotclock; > > + int lane, link_bw, fdi_dotclock, ret; > > bool setup_ok, needs_recompute = false; > > > > + if (IS_IVYBRIDGE(dev) && > > + (intel_crtc->pipe == PIPE_B || intel_crtc->pipe == PIPE_C)) { > > + ret = add_pipe_b_c_to_state(pipe_config->base.state); > > In this scenario, crtc_states are created for both pipe B & C as an operation > on one can affect the other. I may be missing something here, > but where is the other crtc_state being used: compute/check flow and/or > commit flow? The function pipe_required_fdi_lanes() above is changed in this patch to use the crtc_state in the drm atomic state to determined FDI lane availability. At this point we don't yet allow changes to multiple pipes, so the mode set is rejected if the pipe that is not being mode set uses too many lanes. Ander > > > + if (ret < 0) > > + return ret; > > + } > > + > > retry: > > /* FDI is a binary signal running at ~2.7GHz, encoding > > * each output octet as 10 bits. The actual frequency > > -- > > 2.1.0 > --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
> -----Original Message----- > From: Conselvan De Oliveira, Ander > Sent: Thursday, March 19, 2015 11:46 PM > To: Konduru, Chandra > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 16/19] drm/i915: Check lane sharing between pipes B & C > using atomic state > > On Thu, 2015-03-19 at 20:58 +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 16/19] drm/i915: Check lane sharing between pipes B > > > & C using atomic state > > > > > > Makes that code atomic ready. > > > > > > Signed-off-by: Ander Conselvan de Oliveira > > > <ander.conselvan.de.oliveira@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 49 > > > ++++++++++++++++++++++++++++++- > > > ----- > > > 1 file changed, 42 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index e720a48..8c97186 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -5537,13 +5537,20 @@ bool intel_connector_get_hw_state(struct > > > intel_connector *connector) > > > return encoder->get_hw_state(encoder, &pipe); } > > > > > > -static int pipe_required_fdi_lanes(struct drm_device *dev, enum > > > pipe pipe) > > > +static int pipe_required_fdi_lanes(struct drm_atomic_state *state, > > > + enum pipe pipe) > > > { > > > struct intel_crtc *crtc = > > > - to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe)); > > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, pipe)); > > > + struct intel_crtc_state *crtc_state; > > > + > > > + crtc_state = intel_atomic_get_crtc_state(state, crtc); > > > + if (WARN_ON(IS_ERR(crtc_state))) { > > > + /* Cause modeset to fail due to excess lanes. */ > > > + return 5; > > > + } > > > > > > - if (crtc->base.state->enable && > > > - crtc->config->has_pch_encoder) > > > + if (crtc_state->base.enable && crtc_state->has_pch_encoder) > > > return crtc->config->fdi_lanes; > > > > > > return 0; > > > @@ -5552,6 +5559,8 @@ static int pipe_required_fdi_lanes(struct > > > drm_device *dev, enum pipe pipe) static bool > > > ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, > > > struct intel_crtc_state *pipe_config) { > > > + struct drm_atomic_state *state = pipe_config->base.state; > > > + > > > DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n", > > > pipe_name(pipe), pipe_config->fdi_lanes); > > > if (pipe_config->fdi_lanes > 4) { > > > @@ -5579,7 +5588,7 @@ static bool ironlake_check_fdi_lanes(struct > > > drm_device *dev, enum pipe pipe, > > > return true; > > > case PIPE_B: > > > if (pipe_config->fdi_lanes > 2 && > > > - pipe_required_fdi_lanes(dev, PIPE_C) > 0) { > > > + pipe_required_fdi_lanes(state, PIPE_C) > 0) { > > > DRM_DEBUG_KMS("invalid shared fdi lane config on > pipe %c: %i > > > lanes\n", > > > pipe_name(pipe), pipe_config->fdi_lanes); > > > return false; > > > @@ -5591,7 +5600,7 @@ static bool ironlake_check_fdi_lanes(struct > > > drm_device *dev, enum pipe pipe, > > > pipe_name(pipe), pipe_config->fdi_lanes); > > > return false; > > > } > > > - if (pipe_required_fdi_lanes(dev, PIPE_B) > 2) { > > > + if (pipe_required_fdi_lanes(state, PIPE_B) > 2) { > > > DRM_DEBUG_KMS("fdi link B uses too many lanes to > enable link > > > C\n"); > > > return false; > > > } > > > @@ -5601,15 +5610,41 @@ static bool ironlake_check_fdi_lanes(struct > > > drm_device *dev, enum pipe pipe, > > > } > > > } > > > > > > +static int add_pipe_b_c_to_state(struct drm_atomic_state *state) { > > > + struct intel_crtc *pipe_B = > > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_B)); > > > + struct intel_crtc *pipe_C = > > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_C)); > > > + struct intel_crtc_state *crtc_state; > > > + > > > + crtc_state = intel_atomic_get_crtc_state(state, pipe_B); > > > + if (IS_ERR(crtc_state)) > > > + return PTR_ERR(crtc_state); > > > + > > > + crtc_state = intel_atomic_get_crtc_state(state, pipe_C); > > > + if (IS_ERR(crtc_state)) > > > + return PTR_ERR(crtc_state); > > > + > > > + return 0; > > > +} > > > + > > > #define RETRY 1 > > > static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, > > > struct intel_crtc_state *pipe_config) { > > > struct drm_device *dev = intel_crtc->base.dev; > > > struct drm_display_mode *adjusted_mode = &pipe_config- > > > >base.adjusted_mode; > > > - int lane, link_bw, fdi_dotclock; > > > + int lane, link_bw, fdi_dotclock, ret; > > > bool setup_ok, needs_recompute = false; > > > > > > + if (IS_IVYBRIDGE(dev) && > > > + (intel_crtc->pipe == PIPE_B || intel_crtc->pipe == PIPE_C)) { > > > + ret = add_pipe_b_c_to_state(pipe_config->base.state); > > > > In this scenario, crtc_states are created for both pipe B & C as an > > operation on one can affect the other. I may be missing something > > here, but where is the other crtc_state being used: compute/check flow > > and/or commit flow? > > The function pipe_required_fdi_lanes() above is changed in this patch to use the > crtc_state in the drm atomic state to determined FDI lane availability. At this > point we don't yet allow changes to multiple pipes, so the mode set is rejected if > the pipe that is not being mode set uses too many lanes. If request requires too many lanes then mode set is rejected, that is fine. But my query is when the request requires lanes that can be supported. In that case where the other crtc_state is being used? > > Ander > > > > > > + if (ret < 0) > > > + return ret; > > > + } > > > + > > > retry: > > > /* FDI is a binary signal running at ~2.7GHz, encoding > > > * each output octet as 10 bits. The actual frequency > > > -- > > > 2.1.0 > >
On Sun, 2015-03-22 at 16:20 +0000, Konduru, Chandra wrote: > > > > -----Original Message----- > > From: Conselvan De Oliveira, Ander > > Sent: Thursday, March 19, 2015 11:46 PM > > To: Konduru, Chandra > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH 16/19] drm/i915: Check lane sharing between pipes B & C > > using atomic state > > > > On Thu, 2015-03-19 at 20:58 +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 16/19] drm/i915: Check lane sharing between pipes B > > > > & C using atomic state > > > > > > > > Makes that code atomic ready. > > > > > > > > Signed-off-by: Ander Conselvan de Oliveira > > > > <ander.conselvan.de.oliveira@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 49 > > > > ++++++++++++++++++++++++++++++- > > > > ----- > > > > 1 file changed, 42 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index e720a48..8c97186 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -5537,13 +5537,20 @@ bool intel_connector_get_hw_state(struct > > > > intel_connector *connector) > > > > return encoder->get_hw_state(encoder, &pipe); } > > > > > > > > -static int pipe_required_fdi_lanes(struct drm_device *dev, enum > > > > pipe pipe) > > > > +static int pipe_required_fdi_lanes(struct drm_atomic_state *state, > > > > + enum pipe pipe) > > > > { > > > > struct intel_crtc *crtc = > > > > - to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe)); > > > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, pipe)); > > > > + struct intel_crtc_state *crtc_state; > > > > + > > > > + crtc_state = intel_atomic_get_crtc_state(state, crtc); > > > > + if (WARN_ON(IS_ERR(crtc_state))) { > > > > + /* Cause modeset to fail due to excess lanes. */ > > > > + return 5; > > > > + } > > > > > > > > - if (crtc->base.state->enable && > > > > - crtc->config->has_pch_encoder) > > > > + if (crtc_state->base.enable && crtc_state->has_pch_encoder) > > > > return crtc->config->fdi_lanes; > > > > > > > > return 0; > > > > @@ -5552,6 +5559,8 @@ static int pipe_required_fdi_lanes(struct > > > > drm_device *dev, enum pipe pipe) static bool > > > > ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, > > > > struct intel_crtc_state *pipe_config) { > > > > + struct drm_atomic_state *state = pipe_config->base.state; > > > > + > > > > DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n", > > > > pipe_name(pipe), pipe_config->fdi_lanes); > > > > if (pipe_config->fdi_lanes > 4) { > > > > @@ -5579,7 +5588,7 @@ static bool ironlake_check_fdi_lanes(struct > > > > drm_device *dev, enum pipe pipe, > > > > return true; > > > > case PIPE_B: > > > > if (pipe_config->fdi_lanes > 2 && > > > > - pipe_required_fdi_lanes(dev, PIPE_C) > 0) { > > > > + pipe_required_fdi_lanes(state, PIPE_C) > 0) { > > > > DRM_DEBUG_KMS("invalid shared fdi lane config on > > pipe %c: %i > > > > lanes\n", > > > > pipe_name(pipe), pipe_config->fdi_lanes); > > > > return false; > > > > @@ -5591,7 +5600,7 @@ static bool ironlake_check_fdi_lanes(struct > > > > drm_device *dev, enum pipe pipe, > > > > pipe_name(pipe), pipe_config->fdi_lanes); > > > > return false; > > > > } > > > > - if (pipe_required_fdi_lanes(dev, PIPE_B) > 2) { > > > > + if (pipe_required_fdi_lanes(state, PIPE_B) > 2) { > > > > DRM_DEBUG_KMS("fdi link B uses too many lanes to > > enable link > > > > C\n"); > > > > return false; > > > > } > > > > @@ -5601,15 +5610,41 @@ static bool ironlake_check_fdi_lanes(struct > > > > drm_device *dev, enum pipe pipe, > > > > } > > > > } > > > > > > > > +static int add_pipe_b_c_to_state(struct drm_atomic_state *state) { > > > > + struct intel_crtc *pipe_B = > > > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_B)); > > > > + struct intel_crtc *pipe_C = > > > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_C)); > > > > + struct intel_crtc_state *crtc_state; > > > > + > > > > + crtc_state = intel_atomic_get_crtc_state(state, pipe_B); > > > > + if (IS_ERR(crtc_state)) > > > > + return PTR_ERR(crtc_state); > > > > + > > > > + crtc_state = intel_atomic_get_crtc_state(state, pipe_C); > > > > + if (IS_ERR(crtc_state)) > > > > + return PTR_ERR(crtc_state); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > #define RETRY 1 > > > > static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, > > > > struct intel_crtc_state *pipe_config) { > > > > struct drm_device *dev = intel_crtc->base.dev; > > > > struct drm_display_mode *adjusted_mode = &pipe_config- > > > > >base.adjusted_mode; > > > > - int lane, link_bw, fdi_dotclock; > > > > + int lane, link_bw, fdi_dotclock, ret; > > > > bool setup_ok, needs_recompute = false; > > > > > > > > + if (IS_IVYBRIDGE(dev) && > > > > + (intel_crtc->pipe == PIPE_B || intel_crtc->pipe == PIPE_C)) { > > > > + ret = add_pipe_b_c_to_state(pipe_config->base.state); > > > > > > In this scenario, crtc_states are created for both pipe B & C as an > > > operation on one can affect the other. I may be missing something > > > here, but where is the other crtc_state being used: compute/check flow > > > and/or commit flow? > > > > The function pipe_required_fdi_lanes() above is changed in this patch to use the > > crtc_state in the drm atomic state to determined FDI lane availability. At this > > point we don't yet allow changes to multiple pipes, so the mode set is rejected if > > the pipe that is not being mode set uses too many lanes. > > If request requires too many lanes then mode set is rejected, that is fine. > But my query is when the request requires lanes that can be supported. > In that case where the other crtc_state is being used? Same place, pipe_required_fdi_lanes() called from ironlake_check_fdi_lanes(). Ander
> -----Original Message----- > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] > Sent: Monday, March 23, 2015 12:34 AM > To: Konduru, Chandra > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 16/19] drm/i915: Check lane sharing between pipes B & C > using atomic state > > On Sun, 2015-03-22 at 16:20 +0000, Konduru, Chandra wrote: > > > > > > > -----Original Message----- > > > From: Conselvan De Oliveira, Ander > > > Sent: Thursday, March 19, 2015 11:46 PM > > > To: Konduru, Chandra > > > Cc: intel-gfx@lists.freedesktop.org > > > Subject: Re: [PATCH 16/19] drm/i915: Check lane sharing between > > > pipes B & C using atomic state > > > > > > On Thu, 2015-03-19 at 20:58 +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 16/19] drm/i915: Check lane sharing between > > > > > pipes B & C using atomic state > > > > > > > > > > Makes that code atomic ready. > > > > > > > > > > Signed-off-by: Ander Conselvan de Oliveira > > > > > <ander.conselvan.de.oliveira@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 49 > > > > > ++++++++++++++++++++++++++++++- > > > > > ----- > > > > > 1 file changed, 42 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > > index e720a48..8c97186 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -5537,13 +5537,20 @@ bool intel_connector_get_hw_state(struct > > > > > intel_connector *connector) > > > > > return encoder->get_hw_state(encoder, &pipe); } > > > > > > > > > > -static int pipe_required_fdi_lanes(struct drm_device *dev, enum > > > > > pipe pipe) > > > > > +static int pipe_required_fdi_lanes(struct drm_atomic_state *state, > > > > > + enum pipe pipe) > > > > > { > > > > > struct intel_crtc *crtc = > > > > > - to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe)); > > > > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, > pipe)); > > > > > + struct intel_crtc_state *crtc_state; > > > > > + > > > > > + crtc_state = intel_atomic_get_crtc_state(state, crtc); > > > > > + if (WARN_ON(IS_ERR(crtc_state))) { > > > > > + /* Cause modeset to fail due to excess lanes. */ > > > > > + return 5; > > > > > + } > > > > > > > > > > - if (crtc->base.state->enable && > > > > > - crtc->config->has_pch_encoder) > > > > > + if (crtc_state->base.enable && crtc_state->has_pch_encoder) > > > > > return crtc->config->fdi_lanes; > > > > > > > > > > return 0; > > > > > @@ -5552,6 +5559,8 @@ static int pipe_required_fdi_lanes(struct > > > > > drm_device *dev, enum pipe pipe) static bool > > > > > ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, > > > > > struct intel_crtc_state *pipe_config) { > > > > > + struct drm_atomic_state *state = pipe_config->base.state; > > > > > + > > > > > DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n", > > > > > pipe_name(pipe), pipe_config->fdi_lanes); > > > > > if (pipe_config->fdi_lanes > 4) { @@ -5579,7 +5588,7 @@ static > > > > > bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe > > > > > pipe, > > > > > return true; > > > > > case PIPE_B: > > > > > if (pipe_config->fdi_lanes > 2 && > > > > > - pipe_required_fdi_lanes(dev, PIPE_C) > 0) { > > > > > + pipe_required_fdi_lanes(state, PIPE_C) > 0) { > > > > > DRM_DEBUG_KMS("invalid shared fdi lane config on > > > pipe %c: %i > > > > > lanes\n", > > > > > pipe_name(pipe), pipe_config->fdi_lanes); > > > > > return false; > > > > > @@ -5591,7 +5600,7 @@ static bool > > > > > ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, > > > > > pipe_name(pipe), pipe_config->fdi_lanes); > > > > > return false; > > > > > } > > > > > - if (pipe_required_fdi_lanes(dev, PIPE_B) > 2) { > > > > > + if (pipe_required_fdi_lanes(state, PIPE_B) > 2) { > > > > > DRM_DEBUG_KMS("fdi link B uses too many lanes to > > > enable link > > > > > C\n"); > > > > > return false; > > > > > } > > > > > @@ -5601,15 +5610,41 @@ static bool > > > > > ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, > > > > > } > > > > > } > > > > > > > > > > +static int add_pipe_b_c_to_state(struct drm_atomic_state *state) { > > > > > + struct intel_crtc *pipe_B = > > > > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, > PIPE_B)); > > > > > + struct intel_crtc *pipe_C = > > > > > + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, > PIPE_C)); > > > > > + struct intel_crtc_state *crtc_state; > > > > > + > > > > > + crtc_state = intel_atomic_get_crtc_state(state, pipe_B); > > > > > + if (IS_ERR(crtc_state)) > > > > > + return PTR_ERR(crtc_state); > > > > > + > > > > > + crtc_state = intel_atomic_get_crtc_state(state, pipe_C); > > > > > + if (IS_ERR(crtc_state)) > > > > > + return PTR_ERR(crtc_state); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > #define RETRY 1 > > > > > static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, > > > > > struct intel_crtc_state *pipe_config) { > > > > > struct drm_device *dev = intel_crtc->base.dev; > > > > > struct drm_display_mode *adjusted_mode = &pipe_config- > > > > > >base.adjusted_mode; > > > > > - int lane, link_bw, fdi_dotclock; > > > > > + int lane, link_bw, fdi_dotclock, ret; > > > > > bool setup_ok, needs_recompute = false; > > > > > > > > > > + if (IS_IVYBRIDGE(dev) && > > > > > + (intel_crtc->pipe == PIPE_B || intel_crtc->pipe == PIPE_C)) { > > > > > + ret = add_pipe_b_c_to_state(pipe_config->base.state); > > > > > > > > In this scenario, crtc_states are created for both pipe B & C as > > > > an operation on one can affect the other. I may be missing > > > > something here, but where is the other crtc_state being used: > > > > compute/check flow and/or commit flow? > > > > > > The function pipe_required_fdi_lanes() above is changed in this > > > patch to use the crtc_state in the drm atomic state to determined > > > FDI lane availability. At this point we don't yet allow changes to > > > multiple pipes, so the mode set is rejected if the pipe that is not being mode > set uses too many lanes. > > > > If request requires too many lanes then mode set is rejected, that is fine. > > But my query is when the request requires lanes that can be supported. > > In that case where the other crtc_state is being used? > > Same place, pipe_required_fdi_lanes() called from ironlake_check_fdi_lanes(). Ok, got it. > > Ander >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e720a48..8c97186 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5537,13 +5537,20 @@ bool intel_connector_get_hw_state(struct intel_connector *connector) return encoder->get_hw_state(encoder, &pipe); } -static int pipe_required_fdi_lanes(struct drm_device *dev, enum pipe pipe) +static int pipe_required_fdi_lanes(struct drm_atomic_state *state, + enum pipe pipe) { struct intel_crtc *crtc = - to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe)); + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, pipe)); + struct intel_crtc_state *crtc_state; + + crtc_state = intel_atomic_get_crtc_state(state, crtc); + if (WARN_ON(IS_ERR(crtc_state))) { + /* Cause modeset to fail due to excess lanes. */ + return 5; + } - if (crtc->base.state->enable && - crtc->config->has_pch_encoder) + if (crtc_state->base.enable && crtc_state->has_pch_encoder) return crtc->config->fdi_lanes; return 0; @@ -5552,6 +5559,8 @@ static int pipe_required_fdi_lanes(struct drm_device *dev, enum pipe pipe) static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, struct intel_crtc_state *pipe_config) { + struct drm_atomic_state *state = pipe_config->base.state; + DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n", pipe_name(pipe), pipe_config->fdi_lanes); if (pipe_config->fdi_lanes > 4) { @@ -5579,7 +5588,7 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, return true; case PIPE_B: if (pipe_config->fdi_lanes > 2 && - pipe_required_fdi_lanes(dev, PIPE_C) > 0) { + pipe_required_fdi_lanes(state, PIPE_C) > 0) { DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %c: %i lanes\n", pipe_name(pipe), pipe_config->fdi_lanes); return false; @@ -5591,7 +5600,7 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, pipe_name(pipe), pipe_config->fdi_lanes); return false; } - if (pipe_required_fdi_lanes(dev, PIPE_B) > 2) { + if (pipe_required_fdi_lanes(state, PIPE_B) > 2) { DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n"); return false; } @@ -5601,15 +5610,41 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, } } +static int add_pipe_b_c_to_state(struct drm_atomic_state *state) +{ + struct intel_crtc *pipe_B = + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_B)); + struct intel_crtc *pipe_C = + to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_C)); + struct intel_crtc_state *crtc_state; + + crtc_state = intel_atomic_get_crtc_state(state, pipe_B); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + + crtc_state = intel_atomic_get_crtc_state(state, pipe_C); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + + return 0; +} + #define RETRY 1 static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, struct intel_crtc_state *pipe_config) { struct drm_device *dev = intel_crtc->base.dev; struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; - int lane, link_bw, fdi_dotclock; + int lane, link_bw, fdi_dotclock, ret; bool setup_ok, needs_recompute = false; + if (IS_IVYBRIDGE(dev) && + (intel_crtc->pipe == PIPE_B || intel_crtc->pipe == PIPE_C)) { + ret = add_pipe_b_c_to_state(pipe_config->base.state); + if (ret < 0) + return ret; + } + retry: /* FDI is a binary signal running at ~2.7GHz, encoding * each output octet as 10 bits. The actual frequency
Makes that code atomic ready. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 49 ++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-)