[PATCH] drm/i915: rip out unnecessary calls to drm_mode_set_crtcinfo

[PATCH] drm/i915: rip out unnecessary calls to drm_mode_set_crtcinfo

[Daniel Vetter]

Our handling of the crtc timing computation has been nicely
cargo-culted with calls to drm_mode_set_crtcinfo sprinkled all over
the place. But with

commit f9bef081c3c3f77bec54454872e98d3ec635756f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Apr 15 19:53:19 2012 +0200

    drm/i915: don't clobber the special upscaling lvds timings

and

commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sat Jan 28 14:49:20 2012 +0100

    drm/i915: fixup interlaced vertical timings confusion, part 1

we now only set the crtc timing fields in the encoder->mode_fixup
(lvds only) and in crtc->mode_fixup (for everyone else). And since

commit 75c13993db592343bda1fd62f2555fea037d56bd
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sat Jan 28 23:48:46 2012 +0100

    drm/i915: fixup overlay checks for interlaced modes

the only places we actually need the crtc timings is in the mode_set
function.

So we can now safely rip out all the remaining calls to set_crtcinfo
left in the driver and clean up this confusion.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    1 -
 drivers/gpu/drm/i915/intel_overlay.c |    2 +-
 drivers/gpu/drm/i915/intel_sdvo.c    |    3 ---
 drivers/gpu/drm/i915/intel_tv.c      |    1 -
 4 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0eeaa83..f2d2a5a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5281,7 +5281,6 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
 	mode->vsync_end = ((vsync & 0xffff0000) >> 16) + 1;
 
 	drm_mode_set_name(mode);
-	drm_mode_set_crtcinfo(mode, 0);
 
 	return mode;
 }
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index dab185e..d05bfe7 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -263,7 +263,7 @@ i830_activate_pipe_a(struct drm_device *dev)
 	DRM_DEBUG_DRIVER("Enabling pipe A in order to enable overlay\n");
 
 	mode = drm_mode_duplicate(dev, &vesa_640x480);
-	drm_mode_set_crtcinfo(mode, 0);
+
 	if (!drm_crtc_helper_set_mode(&crtc->base, mode,
 				       crtc->base.x, crtc->base.y,
 				       crtc->base.fb))
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index c330efd..a3ccdcc 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1576,9 +1576,6 @@ end:
 			intel_sdvo->sdvo_lvds_fixed_mode =
 				drm_mode_duplicate(connector->dev, newmode);
 
-			drm_mode_set_crtcinfo(intel_sdvo->sdvo_lvds_fixed_mode,
-					      0);
-
 			intel_sdvo->is_lvds = true;
 			break;
 		}
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 2e626b8..3346612 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1249,7 +1249,6 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 	int type;
 
 	mode = reported_modes[0];
-	drm_mode_set_crtcinfo(&mode, 0);
 
 	if (force) {
 		struct intel_load_detect_pipe tmp;
-- 
1.7.7.6

Re: [PATCH] drm/i915: rip out unnecessary calls to drm_mode_set_crtcinfo

[Chris Wilson]

On Tue, 24 Apr 2012 15:41:37 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> the only places we actually need the crtc timings is in the mode_set
> function.
> 
> So we can now safely rip out all the remaining calls to set_crtcinfo
> left in the driver and clean up this confusion.

I have a little flicker of doubt because these tend to end up being
passed into the core drm as well as used during modeset. Even looking
through the instances of drm_mode_set_crtcinfo() in the core, I'm left
no the wiser as to which functions expect crtc_* to be filled in. As far
I can see the only place where set_crtcinfo() must be called is prior to
the final modeset, and so the crtc_* values are only ever used on the
adjusted_mode. Hence why the drm usage leaves me slightly puzzled.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: rip out unnecessary calls to drm_mode_set_crtcinfo

[Daniel Vetter]

On Tue, Apr 24, 2012 at 04:11:43PM +0100, Chris Wilson wrote:
> On Tue, 24 Apr 2012 15:41:37 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > the only places we actually need the crtc timings is in the mode_set
> > function.
> > 
> > So we can now safely rip out all the remaining calls to set_crtcinfo
> > left in the driver and clean up this confusion.
> 
> I have a little flicker of doubt because these tend to end up being
> passed into the core drm as well as used during modeset. Even looking
> through the instances of drm_mode_set_crtcinfo() in the core, I'm left
> no the wiser as to which functions expect crtc_* to be filled in. As far
> I can see the only place where set_crtcinfo() must be called is prior to
> the final modeset, and so the crtc_* values are only ever used on the
> adjusted_mode. Hence why the drm usage leaves me slightly puzzled.

I guess the idea of the drm core is that every time it creates a drm mode,
it also sets the timings. But afaics it never uses them, safe for the
precise vblank timestamp code (but that can only run on active modes, i.e.
after our mode_fixup functions have been called). The problem is that drm
core always sets CRTC_INTERLACE_HALVE_V, so the timings are pretty much
bogus for us anyway (at least with interlaced support).

So I guess it's the drivers job that every active modes needs to have crtc
timings that suits it, and with these patches we should have that. drm
core doesn't seem to care about modes that just get passed around.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/i915: rip out unnecessary calls to drm_mode_set_crtcinfo

[Daniel Vetter]

Our handling of the crtc timing computation has been nicely
cargo-culted with calls to drm_mode_set_crtcinfo sprinkled all over
the place. But with

commit f9bef081c3c3f77bec54454872e98d3ec635756f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Apr 15 19:53:19 2012 +0200

    drm/i915: don't clobber the special upscaling lvds timings

and

commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sat Jan 28 14:49:20 2012 +0100

    drm/i915: fixup interlaced vertical timings confusion, part 1

we now only set the crtc timing fields in the encoder->mode_fixup
(lvds only) and in crtc->mode_fixup (for everyone else). And since

commit 75c13993db592343bda1fd62f2555fea037d56bd
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sat Jan 28 23:48:46 2012 +0100

    drm/i915: fixup overlay checks for interlaced modes

the only places we actually need the crtc timings is in the mode_set
function.

I guess the idea of the drm core is that every time it creates a drm
mode, it also sets the timings. But afaics it never uses them, safe
for the precise vblank timestamp code (but that can only run on active
modes, i.e.  after our mode_fixup functions have been called). The
problem is that drm core always sets CRTC_INTERLACE_HALVE_V, so the
timings are pretty much bogus for us anyway (at least with interlaced
support).

So I guess it's the drivers job that every active modes needs to have
crtc timings that suits it, and with these patches we should have
that. drm core doesn't seem to care about modes that just get passed
around. Hence we can now safely rip out all the remaining calls to
set_crtcinfo left in the driver and clean up this confusion.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    1 -
 drivers/gpu/drm/i915/intel_overlay.c |    2 +-
 drivers/gpu/drm/i915/intel_sdvo.c    |    3 ---
 drivers/gpu/drm/i915/intel_tv.c      |    1 -
 4 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0eeaa83..f2d2a5a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5281,7 +5281,6 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
 	mode->vsync_end = ((vsync & 0xffff0000) >> 16) + 1;
 
 	drm_mode_set_name(mode);
-	drm_mode_set_crtcinfo(mode, 0);
 
 	return mode;
 }
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index dab185e..d05bfe7 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -263,7 +263,7 @@ i830_activate_pipe_a(struct drm_device *dev)
 	DRM_DEBUG_DRIVER("Enabling pipe A in order to enable overlay\n");
 
 	mode = drm_mode_duplicate(dev, &vesa_640x480);
-	drm_mode_set_crtcinfo(mode, 0);
+
 	if (!drm_crtc_helper_set_mode(&crtc->base, mode,
 				       crtc->base.x, crtc->base.y,
 				       crtc->base.fb))
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index c330efd..a3ccdcc 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1576,9 +1576,6 @@ end:
 			intel_sdvo->sdvo_lvds_fixed_mode =
 				drm_mode_duplicate(connector->dev, newmode);
 
-			drm_mode_set_crtcinfo(intel_sdvo->sdvo_lvds_fixed_mode,
-					      0);
-
 			intel_sdvo->is_lvds = true;
 			break;
 		}
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 2e626b8..3346612 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1249,7 +1249,6 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 	int type;
 
 	mode = reported_modes[0];
-	drm_mode_set_crtcinfo(&mode, 0);
 
 	if (force) {
 		struct intel_load_detect_pipe tmp;
-- 
1.7.7.6

Re: [PATCH] drm/i915: rip out unnecessary calls to drm_mode_set_crtcinfo

[Chris Wilson]

On Tue, 24 Apr 2012 17:37:56 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 24, 2012 at 04:11:43PM +0100, Chris Wilson wrote:
> > On Tue, 24 Apr 2012 15:41:37 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > the only places we actually need the crtc timings is in the mode_set
> > > function.
> > > 
> > > So we can now safely rip out all the remaining calls to set_crtcinfo
> > > left in the driver and clean up this confusion.
> > 
> > I have a little flicker of doubt because these tend to end up being
> > passed into the core drm as well as used during modeset. Even looking
> > through the instances of drm_mode_set_crtcinfo() in the core, I'm left
> > no the wiser as to which functions expect crtc_* to be filled in. As far
> > I can see the only place where set_crtcinfo() must be called is prior to
> > the final modeset, and so the crtc_* values are only ever used on the
> > adjusted_mode. Hence why the drm usage leaves me slightly puzzled.
> 
> I guess the idea of the drm core is that every time it creates a drm mode,
> it also sets the timings. But afaics it never uses them, safe for the
> precise vblank timestamp code (but that can only run on active modes, i.e.
> after our mode_fixup functions have been called). The problem is that drm
> core always sets CRTC_INTERLACE_HALVE_V, so the timings are pretty much
> bogus for us anyway (at least with interlaced support).
> 
> So I guess it's the drivers job that every active modes needs to have crtc
> timings that suits it, and with these patches we should have that. drm
> core doesn't seem to care about modes that just get passed around.

So just capture that explanation in the commit message and get QA's seal
of approval and job done.

The patch itself looks good and complete afaict, so r-b with the above.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: rip out unnecessary calls to drm_mode_set_crtcinfo

[Daniel Vetter]

On Thu, May 03, 2012 at 03:51:58PM +0200, Daniel Vetter wrote:
> Our handling of the crtc timing computation has been nicely
> cargo-culted with calls to drm_mode_set_crtcinfo sprinkled all over
> the place. But with
> 
> commit f9bef081c3c3f77bec54454872e98d3ec635756f
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sun Apr 15 19:53:19 2012 +0200
> 
>     drm/i915: don't clobber the special upscaling lvds timings
> 
> and
> 
> commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sat Jan 28 14:49:20 2012 +0100
> 
>     drm/i915: fixup interlaced vertical timings confusion, part 1
> 
> we now only set the crtc timing fields in the encoder->mode_fixup
> (lvds only) and in crtc->mode_fixup (for everyone else). And since
> 
> commit 75c13993db592343bda1fd62f2555fea037d56bd
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sat Jan 28 23:48:46 2012 +0100
> 
>     drm/i915: fixup overlay checks for interlaced modes
> 
> the only places we actually need the crtc timings is in the mode_set
> function.
> 
> I guess the idea of the drm core is that every time it creates a drm
> mode, it also sets the timings. But afaics it never uses them, safe
> for the precise vblank timestamp code (but that can only run on active
> modes, i.e.  after our mode_fixup functions have been called). The
> problem is that drm core always sets CRTC_INTERLACE_HALVE_V, so the
> timings are pretty much bogus for us anyway (at least with interlaced
> support).
> 
> So I guess it's the drivers job that every active modes needs to have
> crtc timings that suits it, and with these patches we should have
> that. drm core doesn't seem to care about modes that just get passed
> around. Hence we can now safely rip out all the remaining calls to
> set_crtcinfo left in the driver and clean up this confusion.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Queued for -next, thanks for the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48