[Intel-gfx] [PATCH 1/6] drm/i915: factor out compute_config from __intel_set_mode v3
This allows us to calculate the full pipe config before we do any mode
setting work.
v2:
- clarify comments about global vs. per-crtc mode set (Ander)
- clean up unnecessary pipe_config = NULL setting (Ander)
v3:
- fix pipe_config handling (alloc in compute_config, free in set_mode) (Jesse)
- fix arg order in set_mode (Jesse)
- fix failure path of set_config (Ander)
Signed-off-by: Jesse Barnes
---
drivers/gpu/drm/i915/intel_display.c | 105 ---
1 file changed, 74 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a9df85f..a3ebab8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10656,45 +10656,60 @@ static void update_scanline_offset(struct intel_crtc
*crtc)
crtc->scanline_offset = 1;
}
+static struct intel_crtc_config *
+intel_modeset_compute_config(struct drm_crtc *crtc,
+struct drm_display_mode *mode,
+struct drm_framebuffer *fb,
+unsigned *modeset_pipes,
+unsigned *prepare_pipes,
+unsigned *disable_pipes)
+{
+ struct intel_crtc_config *pipe_config = NULL;
+
+ intel_modeset_affected_pipes(crtc, modeset_pipes,
+prepare_pipes, disable_pipes);
+
+ if ((*modeset_pipes) == 0)
+ goto out;
+
+ /*
+* Note this needs changes when we start tracking multiple modes
+* and crtcs. At that point we'll need to compute the whole config
+* (i.e. one pipe_config for each crtc) rather than just the one
+* for this crtc.
+*/
+ pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
+ if (IS_ERR(pipe_config)) {
+ goto out;
+ }
+ intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
+ "[modeset]");
+ to_intel_crtc(crtc)->new_config = pipe_config;
+
+out:
+ return pipe_config;
+}
+
static int __intel_set_mode(struct drm_crtc *crtc,
struct drm_display_mode *mode,
- int x, int y, struct drm_framebuffer *fb)
+ int x, int y, struct drm_framebuffer *fb,
+ struct intel_crtc_config *pipe_config,
+ unsigned modeset_pipes,
+ unsigned prepare_pipes,
+ unsigned disable_pipes)
{
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_display_mode *saved_mode;
- struct intel_crtc_config *pipe_config = NULL;
struct intel_crtc *intel_crtc;
- unsigned disable_pipes, prepare_pipes, modeset_pipes;
int ret = 0;
saved_mode = kmalloc(sizeof(*saved_mode), GFP_KERNEL);
if (!saved_mode)
return -ENOMEM;
- intel_modeset_affected_pipes(crtc, &modeset_pipes,
-&prepare_pipes, &disable_pipes);
-
*saved_mode = crtc->mode;
- /* Hack: Because we don't (yet) support global modeset on multiple
-* crtcs, we don't keep track of the new mode for more than one crtc.
-* Hence simply check whether any bit is set in modeset_pipes in all the
-* pieces of code that are not yet converted to deal with mutliple crtcs
-* changing their mode at the same time. */
- if (modeset_pipes) {
- pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
- if (IS_ERR(pipe_config)) {
- ret = PTR_ERR(pipe_config);
- pipe_config = NULL;
-
- goto out;
- }
- intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
- "[modeset]");
- to_intel_crtc(crtc)->new_config = pipe_config;
- }
-
/*
* See if the config requires any additional preparation, e.g.
* to adjust global state with pipes off. We need to do this
@@ -10719,6 +10734,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
/* crtc->mode is already used by the ->mode_set callbacks, hence we need
* to set it here already despite that we pass it down the callchain.
+*
+* Note we'll need to fix this up when we start tracking multiple
+* pipes; here we assume a single modeset_pipe and only track the
+* single crtc and mode.
*/
if (modeset_pipes) {
crtc->mode = *mode;
@@ -10787,19 +10806,23 @@ done:
if (ret && crtc->enabled)
crtc->mode = *saved_mode;
-out:
kfree(pipe_config);
kfree(saved_mode);
return ret;
}
-static int intel_set_mode(struct drm_crtc *crtc,
-
Re: [Intel-gfx] [PATCH 1/6] drm/i915: factor out compute_config from __intel_set_mode v3
On Wed, Nov 05, 2014 at 02:26:06PM -0800, Jesse Barnes wrote:
> +static int intel_set_mode(struct drm_crtc *crtc,
> + struct drm_display_mode *mode,
> + int x, int y, struct drm_framebuffer *fb)
> +{
> + struct intel_crtc_config *pipe_config;
> + unsigned modeset_pipes, prepare_pipes, disable_pipes;
> +
> + pipe_config = intel_modeset_compute_config(crtc, mode, fb,
> +&modeset_pipes,
> +&prepare_pipes,
> +&disable_pipes);
> +
> + if (IS_ERR(pipe_config))
> + return PTR_ERR(pipe_config);
> +
> + return intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
> + modeset_pipes, prepare_pipes,
> + disable_pipes);
> +}
intel_set_mode() -> intel_set_mode_pipes() -> __intel_set_mode() wins
this morning's prize for causing confusion.
Does it make sense to wrap intel_crtc_config + pipes in a new struct to
avoid passing 4 new parameters down the chain? Or will that just be
extra churn to be rewritten by atomic?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: factor out compute_config from __intel_set_mode v3
On Thu, Nov 06, 2014 at 09:04:14AM +, Chris Wilson wrote:
> On Wed, Nov 05, 2014 at 02:26:06PM -0800, Jesse Barnes wrote:
> > +static int intel_set_mode(struct drm_crtc *crtc,
> > + struct drm_display_mode *mode,
> > + int x, int y, struct drm_framebuffer *fb)
> > +{
> > + struct intel_crtc_config *pipe_config;
> > + unsigned modeset_pipes, prepare_pipes, disable_pipes;
> > +
> > + pipe_config = intel_modeset_compute_config(crtc, mode, fb,
> > + &modeset_pipes,
> > + &prepare_pipes,
> > + &disable_pipes);
> > +
> > + if (IS_ERR(pipe_config))
> > + return PTR_ERR(pipe_config);
> > +
> > + return intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
> > + modeset_pipes, prepare_pipes,
> > + disable_pipes);
> > +}
>
> intel_set_mode() -> intel_set_mode_pipes() -> __intel_set_mode() wins
> this morning's prize for causing confusion.
>
> Does it make sense to wrap intel_crtc_config + pipes in a new struct to
> avoid passing 4 new parameters down the chain? Or will that just be
> extra churn to be rewritten by atomic?
Atomic has one big structure to track all the updated per-object states
(for crtcs, planes & connectors). Atm there's no provisions for
subclassing it, but would be trivial to add. Then we could throw all this
additional i915 state in there. I guess we might as well start with that.
One funny problem btw is all these ->new_ pointers we sprinkle all over
the place. Upstream atomic has completely free-standing new state objects,
with a bunch of helpers to fetch the new state for a given object from the
overall atomic update structure. So we have a bit of an impendance
mismatch. But I think we just need to handle that by setting (and
resetting when we don't actually commit the new state) these pointers when
entering the i915 atomic backend.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: factor out compute_config from __intel_set_mode v3
Reviewed-by: Ander Conselvan de Oliveira
On 11/06/2014 12:26 AM, Jesse Barnes wrote:
> This allows us to calculate the full pipe config before we do any mode
> setting work.
>
> v2:
>- clarify comments about global vs. per-crtc mode set (Ander)
>- clean up unnecessary pipe_config = NULL setting (Ander)
> v3:
>- fix pipe_config handling (alloc in compute_config, free in set_mode)
> (Jesse)
>- fix arg order in set_mode (Jesse)
>- fix failure path of set_config (Ander)
>
> Signed-off-by: Jesse Barnes
> ---
> drivers/gpu/drm/i915/intel_display.c | 105
> ---
> 1 file changed, 74 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a9df85f..a3ebab8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10656,45 +10656,60 @@ static void update_scanline_offset(struct
> intel_crtc *crtc)
> crtc->scanline_offset = 1;
> }
>
> +static struct intel_crtc_config *
> +intel_modeset_compute_config(struct drm_crtc *crtc,
> + struct drm_display_mode *mode,
> + struct drm_framebuffer *fb,
> + unsigned *modeset_pipes,
> + unsigned *prepare_pipes,
> + unsigned *disable_pipes)
> +{
> + struct intel_crtc_config *pipe_config = NULL;
> +
> + intel_modeset_affected_pipes(crtc, modeset_pipes,
> + prepare_pipes, disable_pipes);
> +
> + if ((*modeset_pipes) == 0)
> + goto out;
> +
> + /*
> + * Note this needs changes when we start tracking multiple modes
> + * and crtcs. At that point we'll need to compute the whole config
> + * (i.e. one pipe_config for each crtc) rather than just the one
> + * for this crtc.
> + */
> + pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
> + if (IS_ERR(pipe_config)) {
> + goto out;
> + }
> + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> +"[modeset]");
> + to_intel_crtc(crtc)->new_config = pipe_config;
> +
> +out:
> + return pipe_config;
> +}
> +
> static int __intel_set_mode(struct drm_crtc *crtc,
> struct drm_display_mode *mode,
> - int x, int y, struct drm_framebuffer *fb)
> + int x, int y, struct drm_framebuffer *fb,
> + struct intel_crtc_config *pipe_config,
> + unsigned modeset_pipes,
> + unsigned prepare_pipes,
> + unsigned disable_pipes)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_display_mode *saved_mode;
> - struct intel_crtc_config *pipe_config = NULL;
> struct intel_crtc *intel_crtc;
> - unsigned disable_pipes, prepare_pipes, modeset_pipes;
> int ret = 0;
>
> saved_mode = kmalloc(sizeof(*saved_mode), GFP_KERNEL);
> if (!saved_mode)
> return -ENOMEM;
>
> - intel_modeset_affected_pipes(crtc, &modeset_pipes,
> - &prepare_pipes, &disable_pipes);
> -
> *saved_mode = crtc->mode;
>
> - /* Hack: Because we don't (yet) support global modeset on multiple
> - * crtcs, we don't keep track of the new mode for more than one crtc.
> - * Hence simply check whether any bit is set in modeset_pipes in all the
> - * pieces of code that are not yet converted to deal with mutliple crtcs
> - * changing their mode at the same time. */
> - if (modeset_pipes) {
> - pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
> - if (IS_ERR(pipe_config)) {
> - ret = PTR_ERR(pipe_config);
> - pipe_config = NULL;
> -
> - goto out;
> - }
> - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> -"[modeset]");
> - to_intel_crtc(crtc)->new_config = pipe_config;
> - }
> -
> /*
>* See if the config requires any additional preparation, e.g.
>* to adjust global state with pipes off. We need to do this
> @@ -10719,6 +10734,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>
> /* crtc->mode is already used by the ->mode_set callbacks, hence we need
>* to set it here already despite that we pass it down the callchain.
> + *
> + * Note we'll need to fix this up when we start tracking multiple
> + * pipes; here we assume a single modeset_pipe and only track the
> + * single crtc and mode.
>*/
> if (modeset_pipes) {
> crtc->mode = *mode;
> @@ -10787,19 +10806,23 @@ done:
> if (ret && crtc->enabled)
