| Message ID | 1418301491-23020-6-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 11, 2014 at 02:38:08PM +0200, Ander Conselvan de Oliveira wrote: > So that we can get rid of the new_config pointer later. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index da5af23..a9f3034 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4897,27 +4897,39 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > } > > /* compute the max pixel clock for new configuration */ > -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv, > + struct intel_crtc *mode_set_crtc, I'm not sure if you need to pass this. drm_crtc_state has a backpointer to the corresponding drm_crtc now, so if we ensure that's set by our driver when creating a new pipe_config, we could probably get away with just passing the state here. > + struct intel_crtc_state *crtc_new_state) > { > struct drm_device *dev = dev_priv->dev; > struct intel_crtc *intel_crtc; > int max_pixclk = 0; > + int pixclk; > > for_each_intel_crtc(dev, intel_crtc) { > - if (intel_crtc->new_enabled) > - max_pixclk = max(max_pixclk, > - intel_crtc->new_config->base.adjusted_mode.crtc_clock); > + if (!intel_crtc->new_enabled) > + continue; Not really related to this patch specifically, but I'm a little fuzzy on the distinction between drm_crtc->enabled and drm_crtc_state->enable (and then by extension intel_crtc->new_enabled). It's not immediately clear to me why we can't just have a single 'enabled' in the base state structure and then when we have a new config that we're passing around, the enabled flag in that structure would serve the purpose that intel_crtc->new_enabled does for i915 today. I'm probably misunderstanding something. Matt > + > + if (intel_crtc == mode_set_crtc) > + pixclk = crtc_new_state->base.adjusted_mode.crtc_clock; > + else > + pixclk = intel_crtc->config->base.adjusted_mode.crtc_clock; > + > + max_pixclk = max(max_pixclk, pixclk); > } > > return max_pixclk; > } > > static void valleyview_modeset_global_pipes(struct drm_device *dev, > - unsigned *prepare_pipes) > + unsigned *prepare_pipes, > + struct intel_crtc *mode_set_crtc, > + struct intel_crtc_state *crtc_new_state) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc; > - int max_pixclk = intel_mode_max_pixclk(dev_priv); > + int max_pixclk = > + intel_mode_max_pixclk(dev_priv, mode_set_crtc, crtc_new_state); > > if (valleyview_calc_cdclk(dev_priv, max_pixclk) == > dev_priv->vlv_cdclk_freq) > @@ -4932,7 +4944,7 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev, > static void valleyview_modeset_global_resources(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - int max_pixclk = intel_mode_max_pixclk(dev_priv); > + int max_pixclk = intel_mode_max_pixclk(dev_priv, NULL, NULL); > int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk); > > if (req_cdclk != dev_priv->vlv_cdclk_freq) { > @@ -10956,7 +10968,9 @@ static int __intel_set_mode(struct drm_crtc *crtc, > * adjusted_mode bits in the crtc directly. > */ > if (IS_VALLEYVIEW(dev)) { > - valleyview_modeset_global_pipes(dev, &prepare_pipes); > + valleyview_modeset_global_pipes(dev, &prepare_pipes, > + to_intel_crtc(crtc), > + pipe_config); > > /* may have added more to prepare_pipes than we should */ > prepare_pipes &= ~disable_pipes; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 11, 2014 at 02:38:08PM +0200, Ander Conselvan de Oliveira wrote: > So that we can get rid of the new_config pointer later. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index da5af23..a9f3034 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4897,27 +4897,39 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > } > > /* compute the max pixel clock for new configuration */ > -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv, > + struct intel_crtc *mode_set_crtc, > + struct intel_crtc_state *crtc_new_state) > { > struct drm_device *dev = dev_priv->dev; > struct intel_crtc *intel_crtc; > int max_pixclk = 0; > + int pixclk; > > for_each_intel_crtc(dev, intel_crtc) { > - if (intel_crtc->new_enabled) > - max_pixclk = max(max_pixclk, > - intel_crtc->new_config->base.adjusted_mode.crtc_clock); > + if (!intel_crtc->new_enabled) > + continue; > + > + if (intel_crtc == mode_set_crtc) > + pixclk = crtc_new_state->base.adjusted_mode.crtc_clock; > + else > + pixclk = intel_crtc->config->base.adjusted_mode.crtc_clock; > + > + max_pixclk = max(max_pixclk, pixclk); I think we're now going backwards. We want atomic modesets, so we need to be able to compute this stuff when modesetting multiple pipes. That was precisely why I added new_config. > } > > return max_pixclk; > } > > static void valleyview_modeset_global_pipes(struct drm_device *dev, > - unsigned *prepare_pipes) > + unsigned *prepare_pipes, > + struct intel_crtc *mode_set_crtc, > + struct intel_crtc_state *crtc_new_state) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc; > - int max_pixclk = intel_mode_max_pixclk(dev_priv); > + int max_pixclk = > + intel_mode_max_pixclk(dev_priv, mode_set_crtc, crtc_new_state); > > if (valleyview_calc_cdclk(dev_priv, max_pixclk) == > dev_priv->vlv_cdclk_freq) > @@ -4932,7 +4944,7 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev, > static void valleyview_modeset_global_resources(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - int max_pixclk = intel_mode_max_pixclk(dev_priv); > + int max_pixclk = intel_mode_max_pixclk(dev_priv, NULL, NULL); > int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk); > > if (req_cdclk != dev_priv->vlv_cdclk_freq) { > @@ -10956,7 +10968,9 @@ static int __intel_set_mode(struct drm_crtc *crtc, > * adjusted_mode bits in the crtc directly. > */ > if (IS_VALLEYVIEW(dev)) { > - valleyview_modeset_global_pipes(dev, &prepare_pipes); > + valleyview_modeset_global_pipes(dev, &prepare_pipes, > + to_intel_crtc(crtc), > + pipe_config); > > /* may have added more to prepare_pipes than we should */ > prepare_pipes &= ~disable_pipes; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Dec 15, 2014 at 11:17:45AM -0800, Matt Roper wrote: > On Thu, Dec 11, 2014 at 02:38:08PM +0200, Ander Conselvan de Oliveira wrote: > > So that we can get rid of the new_config pointer later. > > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++-------- > > 1 file changed, 22 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index da5af23..a9f3034 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4897,27 +4897,39 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > > } > > > > /* compute the max pixel clock for new configuration */ > > -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) > > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv, > > + struct intel_crtc *mode_set_crtc, > > I'm not sure if you need to pass this. drm_crtc_state has a backpointer > to the corresponding drm_crtc now, so if we ensure that's set by our > driver when creating a new pipe_config, we could probably get away with > just passing the state here. > > > + struct intel_crtc_state *crtc_new_state) > > { > > struct drm_device *dev = dev_priv->dev; > > struct intel_crtc *intel_crtc; > > int max_pixclk = 0; > > + int pixclk; > > > > for_each_intel_crtc(dev, intel_crtc) { > > - if (intel_crtc->new_enabled) > > - max_pixclk = max(max_pixclk, > > - intel_crtc->new_config->base.adjusted_mode.crtc_clock); > > + if (!intel_crtc->new_enabled) > > + continue; > > Not really related to this patch specifically, but I'm a little fuzzy on > the distinction between drm_crtc->enabled and drm_crtc_state->enable > (and then by extension intel_crtc->new_enabled). It's not immediately > clear to me why we can't just have a single 'enabled' in the base state > structure and then when we have a new config that we're passing around, > the enabled flag in that structure would serve the purpose that > intel_crtc->new_enabled does for i915 today. I'm probably > misunderstanding something. Yeah that sounds like where we want to go. No more random bits of state sprinkled all over the place, and instead have it all neatly collected in the state struct. > > > Matt > > > + > > + if (intel_crtc == mode_set_crtc) > > + pixclk = crtc_new_state->base.adjusted_mode.crtc_clock; > > + else > > + pixclk = intel_crtc->config->base.adjusted_mode.crtc_clock; > > + > > + max_pixclk = max(max_pixclk, pixclk); > > } > > > > return max_pixclk; > > } > > > > static void valleyview_modeset_global_pipes(struct drm_device *dev, > > - unsigned *prepare_pipes) > > + unsigned *prepare_pipes, > > + struct intel_crtc *mode_set_crtc, > > + struct intel_crtc_state *crtc_new_state) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct intel_crtc *intel_crtc; > > - int max_pixclk = intel_mode_max_pixclk(dev_priv); > > + int max_pixclk = > > + intel_mode_max_pixclk(dev_priv, mode_set_crtc, crtc_new_state); > > > > if (valleyview_calc_cdclk(dev_priv, max_pixclk) == > > dev_priv->vlv_cdclk_freq) > > @@ -4932,7 +4944,7 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev, > > static void valleyview_modeset_global_resources(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > - int max_pixclk = intel_mode_max_pixclk(dev_priv); > > + int max_pixclk = intel_mode_max_pixclk(dev_priv, NULL, NULL); > > int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk); > > > > if (req_cdclk != dev_priv->vlv_cdclk_freq) { > > @@ -10956,7 +10968,9 @@ static int __intel_set_mode(struct drm_crtc *crtc, > > * adjusted_mode bits in the crtc directly. > > */ > > if (IS_VALLEYVIEW(dev)) { > > - valleyview_modeset_global_pipes(dev, &prepare_pipes); > > + valleyview_modeset_global_pipes(dev, &prepare_pipes, > > + to_intel_crtc(crtc), > > + pipe_config); > > > > /* may have added more to prepare_pipes than we should */ > > prepare_pipes &= ~disable_pipes; > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Dec 15, 2014 at 09:30:44PM +0200, Ville Syrjälä wrote: > On Mon, Dec 15, 2014 at 11:17:45AM -0800, Matt Roper wrote: > > Not really related to this patch specifically, but I'm a little fuzzy on > > the distinction between drm_crtc->enabled and drm_crtc_state->enable > > (and then by extension intel_crtc->new_enabled). It's not immediately > > clear to me why we can't just have a single 'enabled' in the base state > > structure and then when we have a new config that we're passing around, > > the enabled flag in that structure would serve the purpose that > > intel_crtc->new_enabled does for i915 today. I'm probably > > misunderstanding something. > > Yeah that sounds like where we want to go. No more random bits of > state sprinkled all over the place, and instead have it all neatly > collected in the state struct. Yeah, that's the plane. I don't think we can get rid of ->new_config and ->config though since conceptually for async updates we need to commit the sw side synchronously (so that the next atomic update is relative to the currently scheduled state update). But internally in the driver backend we need to still have the staging and only update once all the things from the old state have been shut down. But everything else should just go in there. The other problem is other drivers not yet converted to atomic. Until we have those we can't get rid of duplicated state variables in core drm structs. But as soon as i915 is fully converted we can forget about them, since then helpers will update those for us. Or at least should - we might need to move§ some code from the atomic helpers used only by drivers with crtc helper hooks to make that work correctly and put it into core code. -Daniel
On Mon, Dec 15, 2014 at 09:24:23PM +0200, Ville Syrjälä wrote: > On Thu, Dec 11, 2014 at 02:38:08PM +0200, Ander Conselvan de Oliveira wrote: > > So that we can get rid of the new_config pointer later. > > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++-------- > > 1 file changed, 22 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index da5af23..a9f3034 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4897,27 +4897,39 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > > } > > > > /* compute the max pixel clock for new configuration */ > > -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) > > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv, > > + struct intel_crtc *mode_set_crtc, > > + struct intel_crtc_state *crtc_new_state) > > { > > struct drm_device *dev = dev_priv->dev; > > struct intel_crtc *intel_crtc; > > int max_pixclk = 0; > > + int pixclk; > > > > for_each_intel_crtc(dev, intel_crtc) { > > - if (intel_crtc->new_enabled) > > - max_pixclk = max(max_pixclk, > > - intel_crtc->new_config->base.adjusted_mode.crtc_clock); > > + if (!intel_crtc->new_enabled) > > + continue; > > + > > + if (intel_crtc == mode_set_crtc) > > + pixclk = crtc_new_state->base.adjusted_mode.crtc_clock; > > + else > > + pixclk = intel_crtc->config->base.adjusted_mode.crtc_clock; > > + > > + max_pixclk = max(max_pixclk, pixclk); > > I think we're now going backwards. We want atomic modesets, so we > need to be able to compute this stuff when modesetting multiple pipes. > That was precisely why I added new_config. We've already started going backwards in this area with the latest round of fastboot patches from Jesse. I've discussed this a bit with him and the plan is that we'll do it that way and Ander gets shafted^W signed up to sort out the mess. But yeah if there's a shortcut to avoid some of that detour it would be good. -Daniel
On 16-12-2014 11:34, Daniel Vetter wrote: > On Mon, Dec 15, 2014 at 09:24:23PM +0200, Ville Syrjälä wrote: >> On Thu, Dec 11, 2014 at 02:38:08PM +0200, Ander Conselvan de Oliveira wrote: >>> So that we can get rid of the new_config pointer later. >>> >>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++-------- >>> 1 file changed, 22 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index da5af23..a9f3034 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -4897,27 +4897,39 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, >>> } >>> >>> /* compute the max pixel clock for new configuration */ >>> -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) >>> +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv, >>> + struct intel_crtc *mode_set_crtc, >>> + struct intel_crtc_state *crtc_new_state) >>> { >>> struct drm_device *dev = dev_priv->dev; >>> struct intel_crtc *intel_crtc; >>> int max_pixclk = 0; >>> + int pixclk; >>> >>> for_each_intel_crtc(dev, intel_crtc) { >>> - if (intel_crtc->new_enabled) >>> - max_pixclk = max(max_pixclk, >>> - intel_crtc->new_config->base.adjusted_mode.crtc_clock); >>> + if (!intel_crtc->new_enabled) >>> + continue; >>> + >>> + if (intel_crtc == mode_set_crtc) >>> + pixclk = crtc_new_state->base.adjusted_mode.crtc_clock; >>> + else >>> + pixclk = intel_crtc->config->base.adjusted_mode.crtc_clock; >>> + >>> + max_pixclk = max(max_pixclk, pixclk); >> >> I think we're now going backwards. We want atomic modesets, so we >> need to be able to compute this stuff when modesetting multiple pipes. >> That was precisely why I added new_config. > > We've already started going backwards in this area with the latest round > of fastboot patches from Jesse. I've discussed this a bit with him and the > plan is that we'll do it that way and Ander gets shafted^W signed up to > sort out the mess. > > But yeah if there's a shortcut to avoid some of that detour it would be > good. I should improve the commit message. I agree this patch is a step backwards, but the idea is to move stuff out of the way so we can move forwards again. The ultimate goal is to pass a drm_atomic_state struct, and derive the pipe_config from that. Ander
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index da5af23..a9f3034 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4897,27 +4897,39 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, } /* compute the max pixel clock for new configuration */ -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv, + struct intel_crtc *mode_set_crtc, + struct intel_crtc_state *crtc_new_state) { struct drm_device *dev = dev_priv->dev; struct intel_crtc *intel_crtc; int max_pixclk = 0; + int pixclk; for_each_intel_crtc(dev, intel_crtc) { - if (intel_crtc->new_enabled) - max_pixclk = max(max_pixclk, - intel_crtc->new_config->base.adjusted_mode.crtc_clock); + if (!intel_crtc->new_enabled) + continue; + + if (intel_crtc == mode_set_crtc) + pixclk = crtc_new_state->base.adjusted_mode.crtc_clock; + else + pixclk = intel_crtc->config->base.adjusted_mode.crtc_clock; + + max_pixclk = max(max_pixclk, pixclk); } return max_pixclk; } static void valleyview_modeset_global_pipes(struct drm_device *dev, - unsigned *prepare_pipes) + unsigned *prepare_pipes, + struct intel_crtc *mode_set_crtc, + struct intel_crtc_state *crtc_new_state) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc; - int max_pixclk = intel_mode_max_pixclk(dev_priv); + int max_pixclk = + intel_mode_max_pixclk(dev_priv, mode_set_crtc, crtc_new_state); if (valleyview_calc_cdclk(dev_priv, max_pixclk) == dev_priv->vlv_cdclk_freq) @@ -4932,7 +4944,7 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev, static void valleyview_modeset_global_resources(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - int max_pixclk = intel_mode_max_pixclk(dev_priv); + int max_pixclk = intel_mode_max_pixclk(dev_priv, NULL, NULL); int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk); if (req_cdclk != dev_priv->vlv_cdclk_freq) { @@ -10956,7 +10968,9 @@ static int __intel_set_mode(struct drm_crtc *crtc, * adjusted_mode bits in the crtc directly. */ if (IS_VALLEYVIEW(dev)) { - valleyview_modeset_global_pipes(dev, &prepare_pipes); + valleyview_modeset_global_pipes(dev, &prepare_pipes, + to_intel_crtc(crtc), + pipe_config); /* may have added more to prepare_pipes than we should */ prepare_pipes &= ~disable_pipes;
So that we can get rid of the new_config pointer later. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)