[PATCH 0/8] lvds cleanup

[PATCH 0/8] lvds cleanup

[Daniel Vetter]

Hi all,

This is the first cleanup from my next stab at reworking the modeset code, with
the ultimate goal that we can compute the entire configuration (fdi config, pll
config, sharing of global resources) up-front, before touching the hw at all.
Together with some neat hw state readout this should make fastboot much more
solid, and obviously it's a requirement to properly implement the check mode of
atomic modeset.

Here I move some of the lvds stuff out of line, simple to better see through the
jungle. The newly-added pre_pll_enable callback might be unnecessary in the end,
since I think we should also move the pll enabling into the crtc_enable callback
and out of ->mode_set. Also, we need some notion of exclusive pch_pll (which the
lvds port needs to obey the modeset sequence) and stop disabling pch plls
unconditionally, since they might be in use by another active pipe. But that is
all stuff on top, once the entire clock handling rework settles.

For context, my current wip (iow: where I am stuck atm ...):

http://cgit.freedesktop.org/~danvet/drm/log/?h=modeset-rework

Comments, flames and test reports highly welcome.

Cheers, Daniel

Daniel Vetter (8):
  drm/i915: add encoder->pre_pll_enable callback
  drm/i915: replace ad-hoc dual-link lvds checks
  drm/i915: move is_dual_link_lvds to intel_lvds.c
  drm/i915: track is_dual_link in intel_lvds
  drm/i915: add intel_lvds->reg
  drm/i915: move intel_update_lvds to intel_lvds->pre_pll_enable
  drm/i915: enable intel_lvds->pre_pll_enable for ilk+, too
  drm/i915: rip out pre-DDI stuff from haswell_crtc_mode_set

 drivers/gpu/drm/i915/intel_display.c | 287 +++--------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_lvds.c    | 161 +++++++++++++++++---
 3 files changed, 164 insertions(+), 286 deletions(-)

-- 
1.7.11.7

[PATCH 1/8] drm/i915: add encoder->pre_pll_enable callback

[Daniel Vetter]

Currently we have two encoder specific bits in the common mode_set
functions:
- lvds pin pair enabling
- dp m/n setting and computation

Both need to happen before we enable the pll. Since that is done in
the crtc_mode_set functions, we need to add a new callback to be able
to move them to the encoder code (where they belong).

I think that we can move the pll enabling down quite a bit, which
might allow us to eventually merge encoder->pre_enable with this new
pre_pll_enable callbakc. But for now this will allow us to clean
things up a bit.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2ecc7f8..1ad6d34 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4465,6 +4465,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 	u32 dpll;
 	bool is_sdvo;
@@ -4533,6 +4534,10 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
 	POSTING_READ(DPLL(pipe));
 	udelay(150);
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->pre_pll_enable)
+			encoder->pre_pll_enable(encoder);
+
 	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
 	 * This is an exception to the general rule that mode_set doesn't turn
 	 * things on.
@@ -4577,6 +4582,7 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 	u32 dpll;
 
@@ -4610,6 +4616,10 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
 	POSTING_READ(DPLL(pipe));
 	udelay(150);
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->pre_pll_enable)
+			encoder->pre_pll_enable(encoder);
+
 	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
 	 * This is an exception to the general rule that mode_set doesn't turn
 	 * things on.
@@ -5537,6 +5547,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		I915_WRITE(TRANSDPLINK_N1(pipe), 0);
 	}
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->pre_pll_enable)
+			encoder->pre_pll_enable(encoder);
+
 	if (intel_crtc->pch_pll) {
 		I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bcc5241..42a40a1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -153,6 +153,7 @@ struct intel_encoder {
 	bool cloneable;
 	bool connectors_active;
 	void (*hot_plug)(struct intel_encoder *);
+	void (*pre_pll_enable)(struct intel_encoder *);
 	void (*pre_enable)(struct intel_encoder *);
 	void (*enable)(struct intel_encoder *);
 	void (*disable)(struct intel_encoder *);
-- 
1.7.11.7

Re: [PATCH 1/8] drm/i915: add encoder->pre_pll_enable callback

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Currently we have two encoder specific bits in the common mode_set
> functions:
> - lvds pin pair enabling
> - dp m/n setting and computation
>
> Both need to happen before we enable the pll.

Not true, at least for the docs I checked (gen6+), setting/computing
the m/n registers can be done anytime before enabling the CPU pipe.
Please change the commit message :)

> Since that is done in
> the crtc_mode_set functions, we need to add a new callback to be able
> to move them to the encoder code (where they belong).
>
> I think that we can move the pll enabling down quite a bit, which
> might allow us to eventually merge encoder->pre_enable with this new
> pre_pll_enable callbakc. But for now this will allow us to clean
> things up a bit.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ecc7f8..1ad6d34 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4465,6 +4465,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc,

Don't we also need to patch vlv_update_pll?

>         struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +       struct intel_encoder *encoder;

We kinda have a "naming standard" where variables of type "struct
intel_xxx" are called "intel_xxx" and variables of type "struct
drm_xxx" are called "xxx". I I'd vote to call this intel_encoder.

>         int pipe = intel_crtc->pipe;
>         u32 dpll;
>         bool is_sdvo;
> @@ -4533,6 +4534,10 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
>         POSTING_READ(DPLL(pipe));
>         udelay(150);
>
> +       for_each_encoder_on_crtc(dev, crtc, encoder)
> +               if (encoder->pre_pll_enable)
> +                       encoder->pre_pll_enable(encoder);
> +
>         /* The LVDS pin pair needs to be on before the DPLLs are enabled.
>          * This is an exception to the general rule that mode_set doesn't turn
>          * things on.
> @@ -4577,6 +4582,7 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
>         struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +       struct intel_encoder *encoder;

Same here.

>         int pipe = intel_crtc->pipe;
>         u32 dpll;
>
> @@ -4610,6 +4616,10 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
>         POSTING_READ(DPLL(pipe));
>         udelay(150);
>
> +       for_each_encoder_on_crtc(dev, crtc, encoder)
> +               if (encoder->pre_pll_enable)
> +                       encoder->pre_pll_enable(encoder);
> +
>         /* The LVDS pin pair needs to be on before the DPLLs are enabled.
>          * This is an exception to the general rule that mode_set doesn't turn
>          * things on.
> @@ -5537,6 +5547,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                 I915_WRITE(TRANSDPLINK_N1(pipe), 0);
>         }
>
> +       for_each_encoder_on_crtc(dev, crtc, encoder)
> +               if (encoder->pre_pll_enable)
> +                       encoder->pre_pll_enable(encoder);
> +
>         if (intel_crtc->pch_pll) {
>                 I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bcc5241..42a40a1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -153,6 +153,7 @@ struct intel_encoder {
>         bool cloneable;
>         bool connectors_active;
>         void (*hot_plug)(struct intel_encoder *);
> +       void (*pre_pll_enable)(struct intel_encoder *);
>         void (*pre_enable)(struct intel_encoder *);
>         void (*enable)(struct intel_encoder *);
>         void (*disable)(struct intel_encoder *);
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/8] drm/i915: add encoder->pre_pll_enable callback

[Daniel Vetter]

On Fri, Nov 16, 2012 at 5:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Hi
>
> 2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> Currently we have two encoder specific bits in the common mode_set
>> functions:
>> - lvds pin pair enabling
>> - dp m/n setting and computation
>>
>> Both need to happen before we enable the pll.
>
> Not true, at least for the docs I checked (gen6+), setting/computing
> the m/n registers can be done anytime before enabling the CPU pipe.
> Please change the commit message :)

Yeah, I've written this commit message before I've cleared up my
confusion around this code. Now I think that even pre_pll_enable isn't
strictly required, but we need it because we currently enable the pll
in ->mode_set already. Which is bogus. I'll update the commit message.

>> Since that is done in
>> the crtc_mode_set functions, we need to add a new callback to be able
>> to move them to the encoder code (where they belong).
>>
>> I think that we can move the pll enabling down quite a bit, which
>> might allow us to eventually merge encoder->pre_enable with this new
>> pre_pll_enable callbakc. But for now this will allow us to clean
>> things up a bit.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 2ecc7f8..1ad6d34 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4465,6 +4465,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
>
> Don't we also need to patch vlv_update_pll?

Luckily vlv doesn't support lvds. I can add that to the commit message, too.

>>         struct drm_device *dev = crtc->dev;
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +       struct intel_encoder *encoder;
>
> We kinda have a "naming standard" where variables of type "struct
> intel_xxx" are called "intel_xxx" and variables of type "struct
> drm_xxx" are called "xxx". I I'd vote to call this intel_encoder.

I kinda want to move to the intel_xxx variant being the main one, now
that we rely much less on the common helper stuff. Safes tons of
needless downcasting, but will result in a bit of intermediate
inconsistency.

Generally I think we should cut down on our usage of prefixes a bit,
after reading too many patches from Ben I have to admit that it's
easier on the eyes ;-) So I'd prefer if we leave things as-is. And in
any case, if a function is too big or has too many local variables
that you typecheck a local variable quickly, it's too big. I know, we
suck on that metric ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/8] drm/i915: add encoder->pre_pll_enable callback

[Paulo Zanoni]

Hi

2012/11/16 Daniel Vetter <daniel.vetter@ffwll.ch>:
> On Fri, Nov 16, 2012 at 5:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> Hi
>>
>> 2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
>>> Currently we have two encoder specific bits in the common mode_set
>>> functions:
>>> - lvds pin pair enabling
>>> - dp m/n setting and computation
>>>
>>> Both need to happen before we enable the pll.
>>
>> Not true, at least for the docs I checked (gen6+), setting/computing
>> the m/n registers can be done anytime before enabling the CPU pipe.
>> Please change the commit message :)
>
> Yeah, I've written this commit message before I've cleared up my
> confusion around this code. Now I think that even pre_pll_enable isn't
> strictly required, but we need it because we currently enable the pll
> in ->mode_set already. Which is bogus. I'll update the commit message.
>
>>> Since that is done in
>>> the crtc_mode_set functions, we need to add a new callback to be able
>>> to move them to the encoder code (where they belong).
>>>
>>> I think that we can move the pll enabling down quite a bit, which
>>> might allow us to eventually merge encoder->pre_enable with this new
>>> pre_pll_enable callbakc. But for now this will allow us to clean
>>> things up a bit.
>>>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 2ecc7f8..1ad6d34 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -4465,6 +4465,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
>>
>> Don't we also need to patch vlv_update_pll?
>
> Luckily vlv doesn't support lvds. I can add that to the commit message, too.
>
>>>         struct drm_device *dev = crtc->dev;
>>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>> +       struct intel_encoder *encoder;
>>
>> We kinda have a "naming standard" where variables of type "struct
>> intel_xxx" are called "intel_xxx" and variables of type "struct
>> drm_xxx" are called "xxx". I I'd vote to call this intel_encoder.
>
> I kinda want to move to the intel_xxx variant being the main one, now
> that we rely much less on the common helper stuff. Safes tons of
> needless downcasting, but will result in a bit of intermediate
> inconsistency.
>
> Generally I think we should cut down on our usage of prefixes a bit,
> after reading too many patches from Ben I have to admit that it's
> easier on the eyes ;-) So I'd prefer if we leave things as-is. And in
> any case, if a function is too big or has too many local variables
> that you typecheck a local variable quickly, it's too big. I know, we
> suck on that metric ...

I noticed you don't always use intel_xxx, and I actually thought it
was mostly due to distraction/not-caring instead of actually trying to
change the style. In some of my patches I even added intel_ prefixes
to a lot of variables that were missing. If we're going to change the
standard, it's ok, as long as we know we're changing the standard :)

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

[PATCH 2/8] drm/i915: replace ad-hoc dual-link lvds checks

[Daniel Vetter]

... with is_dual_link_lvds introduced in

commit b03543857fd75876b96e10d4320b775e95041bb7
Author: Takashi Iwai <tiwai@suse.de>
Date:   Tue Mar 20 13:07:05 2012 +0100

    drm/i915: Check VBIOS value for determining LVDS dual channel mode, too

All these checks predate this commit and have simply been overlooked.
Since we don't support switching between single-link and dual-link
modes anyway, this different checks could at best only get in the way
of refactorings, and in the worst case cause inconsistencies.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1ad6d34..0973391 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -690,8 +690,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 	intel_clock_t clock;
 	int err = target;
 
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
-	    (I915_READ(LVDS)) != 0) {
+	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
 		/*
 		 * For LVDS, if the panel is on, just rely on its current
 		 * settings for dual-channel.  We haven't figured out how to
@@ -766,8 +765,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 			lvds_reg = PCH_LVDS;
 		else
 			lvds_reg = LVDS;
-		if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) ==
-		    LVDS_CLKB_POWER_UP)
+		if (is_dual_link_lvds(dev_priv, lvds_reg))
 			clock.p2 = limit->p2.p2_fast;
 		else
 			clock.p2 = limit->p2.p2_slow;
@@ -5360,7 +5358,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	if (is_lvds) {
 		if ((intel_panel_use_ssc(dev_priv) &&
 		     dev_priv->lvds_ssc_freq == 100) ||
-		    (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
+		    is_dual_link_lvds(dev_priv, PCH_LVDS))
 			factor = 25;
 	} else if (is_sdvo && is_tv)
 		factor = 20;
-- 
1.7.11.7

Re: [PATCH 2/8] drm/i915: replace ad-hoc dual-link lvds checks

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> ... with is_dual_link_lvds introduced in
>
> commit b03543857fd75876b96e10d4320b775e95041bb7
> Author: Takashi Iwai <tiwai@suse.de>
> Date:   Tue Mar 20 13:07:05 2012 +0100
>
>     drm/i915: Check VBIOS value for determining LVDS dual channel mode, too
>
> All these checks predate this commit and have simply been overlooked.
> Since we don't support switching between single-link and dual-link
> modes anyway, this different checks could at best only get in the way
> of refactorings, and in the worst case cause inconsistencies.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1ad6d34..0973391 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -690,8 +690,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>         intel_clock_t clock;
>         int err = target;
>
> -       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
> -           (I915_READ(LVDS)) != 0) {
> +       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {

This chunk doesn't seem to do exactly what the commit message says. I
tried to git-blame the lines and got a little confused. Maybe this
chunk deserves its own commit with an explanation. Maybe what you
really want to do is to revert commit
832cc28d5bc676331e6376d940ae45d5937aa688 instead of removing the line?
If you really want to remove the line, you may also update the comment
immediately below?

The chunks below look correct.

>                 /*
>                  * For LVDS, if the panel is on, just rely on its current
>                  * settings for dual-channel.  We haven't figured out how to
> @@ -766,8 +765,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>                         lvds_reg = PCH_LVDS;
>                 else
>                         lvds_reg = LVDS;
> -               if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) ==
> -                   LVDS_CLKB_POWER_UP)
> +               if (is_dual_link_lvds(dev_priv, lvds_reg))
>                         clock.p2 = limit->p2.p2_fast;
>                 else
>                         clock.p2 = limit->p2.p2_slow;
> @@ -5360,7 +5358,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>         if (is_lvds) {
>                 if ((intel_panel_use_ssc(dev_priv) &&
>                      dev_priv->lvds_ssc_freq == 100) ||
> -                   (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
> +                   is_dual_link_lvds(dev_priv, PCH_LVDS))
>                         factor = 25;
>         } else if (is_sdvo && is_tv)
>                 factor = 20;
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/8] drm/i915: replace ad-hoc dual-link lvds checks

[Daniel Vetter]

On Fri, Nov 16, 2012 at 5:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1ad6d34..0973391 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -690,8 +690,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>>         intel_clock_t clock;
>>         int err = target;
>>
>> -       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
>> -           (I915_READ(LVDS)) != 0) {
>> +       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
>
> This chunk doesn't seem to do exactly what the commit message says. I
> tried to git-blame the lines and got a little confused. Maybe this
> chunk deserves its own commit with an explanation. Maybe what you
> really want to do is to revert commit
> 832cc28d5bc676331e6376d940ae45d5937aa688 instead of removing the line?
> If you really want to remove the line, you may also update the comment
> immediately below?

Afaict the LVDS register check is only to make sure that we don't read
garbage. Iow the code doesn't handle the more robust dual-link mode
checking introduced in b03543857fd75876b96e10d4320b77

If we switch over to that method to check for dual-link, we also don't
need to do the (rather bogus imo) register check and hence can just
drop it.

I've dug around in the commit history, and the last patch to touch
this code predates b03543857fd75876b, hence why I think we should just
drop the register check and use the new is-dual-link function,
assuming that this has simple been overlooked in the conversion.

Does that make sense?
-Daniel


>
> The chunks below look correct.
>
>>                 /*
>>                  * For LVDS, if the panel is on, just rely on its current
>>                  * settings for dual-channel.  We haven't figured out how to
>> @@ -766,8 +765,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>>                         lvds_reg = PCH_LVDS;
>>                 else
>>                         lvds_reg = LVDS;
>> -               if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) ==
>> -                   LVDS_CLKB_POWER_UP)
>> +               if (is_dual_link_lvds(dev_priv, lvds_reg))
>>                         clock.p2 = limit->p2.p2_fast;



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/8] drm/i915: replace ad-hoc dual-link lvds checks

[Paulo Zanoni]

Hi

2012/11/16 Daniel Vetter <daniel.vetter@ffwll.ch>:
> On Fri, Nov 16, 2012 at 5:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 1ad6d34..0973391 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -690,8 +690,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>>>         intel_clock_t clock;
>>>         int err = target;
>>>
>>> -       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
>>> -           (I915_READ(LVDS)) != 0) {
>>> +       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
>>
>> This chunk doesn't seem to do exactly what the commit message says. I
>> tried to git-blame the lines and got a little confused. Maybe this
>> chunk deserves its own commit with an explanation. Maybe what you
>> really want to do is to revert commit
>> 832cc28d5bc676331e6376d940ae45d5937aa688 instead of removing the line?
>> If you really want to remove the line, you may also update the comment
>> immediately below?
>
> Afaict the LVDS register check is only to make sure that we don't read
> garbage. Iow the code doesn't handle the more robust dual-link mode
> checking introduced in b03543857fd75876b96e10d4320b77
>
> If we switch over to that method to check for dual-link, we also don't
> need to do the (rather bogus imo) register check and hence can just
> drop it.
>
> I've dug around in the commit history, and the last patch to touch
> this code predates b03543857fd75876b, hence why I think we should just
> drop the register check and use the new is-dual-link function,
> assuming that this has simple been overlooked in the conversion.

Well, take a look at patch 832cc28d5bc676331e6376d940ae45d5937aa688
and then read the 4-line comment that's inside the "if" statement. If
we're going to completely remove the I915_READ line, shouldn't we also
update the comment ("if the panel is on") ?

>
> Does that make sense?
> -Daniel
>
>
>>
>> The chunks below look correct.
>>
>>>                 /*
>>>                  * For LVDS, if the panel is on, just rely on its current
>>>                  * settings for dual-channel.  We haven't figured out how to
>>> @@ -766,8 +765,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>>>                         lvds_reg = PCH_LVDS;
>>>                 else
>>>                         lvds_reg = LVDS;
>>> -               if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) ==
>>> -                   LVDS_CLKB_POWER_UP)
>>> +               if (is_dual_link_lvds(dev_priv, lvds_reg))
>>>                         clock.p2 = limit->p2.p2_fast;
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 2/8] drm/i915: replace ad-hoc dual-link lvds checks

[Daniel Vetter]

On Fri, Nov 16, 2012 at 6:07 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Well, take a look at patch 832cc28d5bc676331e6376d940ae45d5937aa688
> and then read the 4-line comment that's inside the "if" statement. If
> we're going to completely remove the I915_READ line, shouldn't we also
> update the comment ("if the panel is on") ?

Yeah, that comment needs to be updated. And that commit message is
wrong, since there are clearly bios versions out there which do not
unconditionally set the dual-link bits in the lvds reg.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/8] drm/i915: move is_dual_link_lvds to intel_lvds.c

[Daniel Vetter]

Just a prep patch to make this a property of intel_lvds. Makes more
sense, removes clutter from intel_display.c and eventually I want to
move all the encoder special cases wrt clock handling to encoders
anyway.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 60 +++---------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_lvds.c    | 53 +++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0973391..7309790 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -472,61 +472,14 @@ static void vlv_init_dpio(struct drm_device *dev)
 	POSTING_READ(DPIO_CTL);
 }
 
-static int intel_dual_link_lvds_callback(const struct dmi_system_id *id)
-{
-	DRM_INFO("Forcing lvds to dual link mode on %s\n", id->ident);
-	return 1;
-}
-
-static const struct dmi_system_id intel_dual_link_lvds[] = {
-	{
-		.callback = intel_dual_link_lvds_callback,
-		.ident = "Apple MacBook Pro (Core i5/i7 Series)",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro8,2"),
-		},
-	},
-	{ }	/* terminating entry */
-};
-
-static bool is_dual_link_lvds(struct drm_i915_private *dev_priv,
-			      unsigned int reg)
-{
-	unsigned int val;
-
-	/* use the module option value if specified */
-	if (i915_lvds_channel_mode > 0)
-		return i915_lvds_channel_mode == 2;
-
-	if (dmi_check_system(intel_dual_link_lvds))
-		return true;
-
-	if (dev_priv->lvds_val)
-		val = dev_priv->lvds_val;
-	else {
-		/* BIOS should set the proper LVDS register value at boot, but
-		 * in reality, it doesn't set the value when the lid is closed;
-		 * we need to check "the value to be set" in VBT when LVDS
-		 * register is uninitialized.
-		 */
-		val = I915_READ(reg);
-		if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
-			val = dev_priv->bios_lvds_val;
-		dev_priv->lvds_val = val;
-	}
-	return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
-}
-
 static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
 						int refclk)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	const intel_limit_t *limit;
 
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
-		if (is_dual_link_lvds(dev_priv, PCH_LVDS)) {
+		if (is_dual_link_lvds(dev)) {
 			/* LVDS dual channel */
 			if (refclk == 100000)
 				limit = &intel_limits_ironlake_dual_lvds_100m;
@@ -550,11 +503,10 @@ static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
 static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	const intel_limit_t *limit;
 
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
-		if (is_dual_link_lvds(dev_priv, LVDS))
+		if (is_dual_link_lvds(dev))
 			/* LVDS with dual channel */
 			limit = &intel_limits_g4x_dual_channel_lvds;
 		else
@@ -686,7 +638,6 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	intel_clock_t clock;
 	int err = target;
 
@@ -697,7 +648,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 		 * reliably set up different single/dual channel state, if we
 		 * even can.
 		 */
-		if (is_dual_link_lvds(dev_priv, LVDS))
+		if (is_dual_link_lvds(dev))
 			clock.p2 = limit->p2.p2_fast;
 		else
 			clock.p2 = limit->p2.p2_slow;
@@ -750,7 +701,6 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 			intel_clock_t *best_clock)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	intel_clock_t clock;
 	int max_n;
 	bool found;
@@ -765,7 +715,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 			lvds_reg = PCH_LVDS;
 		else
 			lvds_reg = LVDS;
-		if (is_dual_link_lvds(dev_priv, lvds_reg))
+		if (is_dual_link_lvds(dev))
 			clock.p2 = limit->p2.p2_fast;
 		else
 			clock.p2 = limit->p2.p2_slow;
@@ -5358,7 +5308,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	if (is_lvds) {
 		if ((intel_panel_use_ssc(dev_priv) &&
 		     dev_priv->lvds_ssc_freq == 100) ||
-		    is_dual_link_lvds(dev_priv, PCH_LVDS))
+		    is_dual_link_lvds(dev))
 			factor = 25;
 	} else if (is_sdvo && is_tv)
 		factor = 20;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 42a40a1..ee4a4ba 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -439,6 +439,7 @@ extern void intel_mark_idle(struct drm_device *dev);
 extern void intel_mark_fb_busy(struct drm_i915_gem_object *obj);
 extern void intel_mark_fb_idle(struct drm_i915_gem_object *obj);
 extern bool intel_lvds_init(struct drm_device *dev);
+extern bool is_dual_link_lvds(struct drm_device *dev);
 extern void intel_dp_init(struct drm_device *dev, int output_reg,
 			  enum port port);
 extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index ffa0051..2303984 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -900,6 +900,59 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev,
 	return false;
 }
 
+static int intel_dual_link_lvds_callback(const struct dmi_system_id *id)
+{
+	DRM_INFO("Forcing lvds to dual link mode on %s\n", id->ident);
+	return 1;
+}
+
+static const struct dmi_system_id intel_dual_link_lvds[] = {
+	{
+		.callback = intel_dual_link_lvds_callback,
+		.ident = "Apple MacBook Pro (Core i5/i7 Series)",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro8,2"),
+		},
+	},
+	{ }	/* terminating entry */
+};
+
+bool is_dual_link_lvds(struct drm_device *dev)
+{
+	unsigned int val;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 lvds_reg;
+
+	if (HAS_PCH_SPLIT(dev)) {
+		lvds_reg = PCH_LVDS;
+	} else {
+		lvds_reg = LVDS;
+	}
+
+	/* use the module option value if specified */
+	if (i915_lvds_channel_mode > 0)
+		return i915_lvds_channel_mode == 2;
+
+	if (dmi_check_system(intel_dual_link_lvds))
+		return true;
+
+	if (dev_priv->lvds_val)
+		val = dev_priv->lvds_val;
+	else {
+		/* BIOS should set the proper LVDS register value at boot, but
+		 * in reality, it doesn't set the value when the lid is closed;
+		 * we need to check "the value to be set" in VBT when LVDS
+		 * register is uninitialized.
+		 */
+		val = I915_READ(lvds_reg);
+		if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
+			val = dev_priv->bios_lvds_val;
+		dev_priv->lvds_val = val;
+	}
+	return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
+}
+
 static bool intel_lvds_supported(struct drm_device *dev)
 {
 	/* With the introduction of the PCH we gained a dedicated
-- 
1.7.11.7

Re: [PATCH 3/8] drm/i915: move is_dual_link_lvds to intel_lvds.c

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Just a prep patch to make this a property of intel_lvds. Makes more
> sense, removes clutter from intel_display.c and eventually I want to
> move all the encoder special cases wrt clock handling to encoders
> anyway.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 60 +++---------------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_lvds.c    | 53 +++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0973391..7309790 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -472,61 +472,14 @@ static void vlv_init_dpio(struct drm_device *dev)
>         POSTING_READ(DPIO_CTL);
>  }
>
> -static int intel_dual_link_lvds_callback(const struct dmi_system_id *id)
> -{
> -       DRM_INFO("Forcing lvds to dual link mode on %s\n", id->ident);
> -       return 1;
> -}
> -
> -static const struct dmi_system_id intel_dual_link_lvds[] = {
> -       {
> -               .callback = intel_dual_link_lvds_callback,
> -               .ident = "Apple MacBook Pro (Core i5/i7 Series)",
> -               .matches = {
> -                       DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> -                       DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro8,2"),
> -               },
> -       },
> -       { }     /* terminating entry */
> -};
> -
> -static bool is_dual_link_lvds(struct drm_i915_private *dev_priv,
> -                             unsigned int reg)
> -{
> -       unsigned int val;
> -
> -       /* use the module option value if specified */
> -       if (i915_lvds_channel_mode > 0)
> -               return i915_lvds_channel_mode == 2;
> -
> -       if (dmi_check_system(intel_dual_link_lvds))
> -               return true;
> -
> -       if (dev_priv->lvds_val)
> -               val = dev_priv->lvds_val;
> -       else {
> -               /* BIOS should set the proper LVDS register value at boot, but
> -                * in reality, it doesn't set the value when the lid is closed;
> -                * we need to check "the value to be set" in VBT when LVDS
> -                * register is uninitialized.
> -                */
> -               val = I915_READ(reg);
> -               if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
> -                       val = dev_priv->bios_lvds_val;
> -               dev_priv->lvds_val = val;
> -       }
> -       return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
> -}
> -
>  static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
>                                                 int refclk)
>  {
>         struct drm_device *dev = crtc->dev;
> -       struct drm_i915_private *dev_priv = dev->dev_private;
>         const intel_limit_t *limit;
>
>         if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> -               if (is_dual_link_lvds(dev_priv, PCH_LVDS)) {
> +               if (is_dual_link_lvds(dev)) {
>                         /* LVDS dual channel */
>                         if (refclk == 100000)
>                                 limit = &intel_limits_ironlake_dual_lvds_100m;
> @@ -550,11 +503,10 @@ static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
>  static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
>  {
>         struct drm_device *dev = crtc->dev;
> -       struct drm_i915_private *dev_priv = dev->dev_private;
>         const intel_limit_t *limit;
>
>         if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> -               if (is_dual_link_lvds(dev_priv, LVDS))
> +               if (is_dual_link_lvds(dev))
>                         /* LVDS with dual channel */
>                         limit = &intel_limits_g4x_dual_channel_lvds;
>                 else
> @@ -686,7 +638,6 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>
>  {
>         struct drm_device *dev = crtc->dev;
> -       struct drm_i915_private *dev_priv = dev->dev_private;
>         intel_clock_t clock;
>         int err = target;
>
> @@ -697,7 +648,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>                  * reliably set up different single/dual channel state, if we
>                  * even can.
>                  */
> -               if (is_dual_link_lvds(dev_priv, LVDS))
> +               if (is_dual_link_lvds(dev))
>                         clock.p2 = limit->p2.p2_fast;
>                 else
>                         clock.p2 = limit->p2.p2_slow;
> @@ -750,7 +701,6 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>                         intel_clock_t *best_clock)
>  {
>         struct drm_device *dev = crtc->dev;
> -       struct drm_i915_private *dev_priv = dev->dev_private;
>         intel_clock_t clock;
>         int max_n;
>         bool found;
> @@ -765,7 +715,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>                         lvds_reg = PCH_LVDS;
>                 else
>                         lvds_reg = LVDS;
> -               if (is_dual_link_lvds(dev_priv, lvds_reg))
> +               if (is_dual_link_lvds(dev))
>                         clock.p2 = limit->p2.p2_fast;
>                 else
>                         clock.p2 = limit->p2.p2_slow;
> @@ -5358,7 +5308,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>         if (is_lvds) {
>                 if ((intel_panel_use_ssc(dev_priv) &&
>                      dev_priv->lvds_ssc_freq == 100) ||
> -                   is_dual_link_lvds(dev_priv, PCH_LVDS))
> +                   is_dual_link_lvds(dev))
>                         factor = 25;
>         } else if (is_sdvo && is_tv)
>                 factor = 20;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 42a40a1..ee4a4ba 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -439,6 +439,7 @@ extern void intel_mark_idle(struct drm_device *dev);
>  extern void intel_mark_fb_busy(struct drm_i915_gem_object *obj);
>  extern void intel_mark_fb_idle(struct drm_i915_gem_object *obj);
>  extern bool intel_lvds_init(struct drm_device *dev);
> +extern bool is_dual_link_lvds(struct drm_device *dev);
>  extern void intel_dp_init(struct drm_device *dev, int output_reg,
>                           enum port port);
>  extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index ffa0051..2303984 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -900,6 +900,59 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev,
>         return false;
>  }
>
> +static int intel_dual_link_lvds_callback(const struct dmi_system_id *id)
> +{
> +       DRM_INFO("Forcing lvds to dual link mode on %s\n", id->ident);
> +       return 1;
> +}
> +
> +static const struct dmi_system_id intel_dual_link_lvds[] = {
> +       {
> +               .callback = intel_dual_link_lvds_callback,
> +               .ident = "Apple MacBook Pro (Core i5/i7 Series)",
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro8,2"),
> +               },
> +       },
> +       { }     /* terminating entry */
> +};
> +
> +bool is_dual_link_lvds(struct drm_device *dev)

The last time I did something similar someone asked me to add "intel_"
prefix to the function name.

With or without the intel_ prefix:

Yay! Encoder-specific code moving to encoder-specific file!

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +{
> +       unsigned int val;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       u32 lvds_reg;
> +
> +       if (HAS_PCH_SPLIT(dev)) {
> +               lvds_reg = PCH_LVDS;
> +       } else {
> +               lvds_reg = LVDS;
> +       }
> +
> +       /* use the module option value if specified */
> +       if (i915_lvds_channel_mode > 0)
> +               return i915_lvds_channel_mode == 2;
> +
> +       if (dmi_check_system(intel_dual_link_lvds))
> +               return true;
> +
> +       if (dev_priv->lvds_val)
> +               val = dev_priv->lvds_val;
> +       else {
> +               /* BIOS should set the proper LVDS register value at boot, but
> +                * in reality, it doesn't set the value when the lid is closed;
> +                * we need to check "the value to be set" in VBT when LVDS
> +                * register is uninitialized.
> +                */
> +               val = I915_READ(lvds_reg);
> +               if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
> +                       val = dev_priv->bios_lvds_val;
> +               dev_priv->lvds_val = val;
> +       }
> +       return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
> +}
> +
>  static bool intel_lvds_supported(struct drm_device *dev)
>  {
>         /* With the introduction of the PCH we gained a dedicated
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 4/8] drm/i915: track is_dual_link in intel_lvds

[Daniel Vetter]

Yeah, all users (both the clock selection special cases and the lvds
pin pair stuff) are still in common code, but this will change.

v2: Rebase on top of Jani Nikula's panel rework.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_lvds.c | 43 +++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 2303984..e4ae3a6 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -52,6 +52,7 @@ struct intel_lvds_encoder {
 	u32 pfit_control;
 	u32 pfit_pgm_ratios;
 	bool pfit_dirty;
+	bool is_dual_link;
 
 	struct intel_lvds_connector *attached_connector;
 };
@@ -920,6 +921,23 @@ static const struct dmi_system_id intel_dual_link_lvds[] = {
 
 bool is_dual_link_lvds(struct drm_device *dev)
 {
+	struct intel_encoder *encoder;
+	struct intel_lvds_encoder *lvds_encoder;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
+			    base.head) {
+		if (encoder->type == INTEL_OUTPUT_LVDS) {
+			lvds_encoder = to_lvds_encoder(&encoder->base);
+
+			return lvds_encoder->is_dual_link;
+		}
+	}
+
+	return false;
+}
+
+static bool __is_dual_link_lvds(struct drm_device *dev)
+{
 	unsigned int val;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 lvds_reg;
@@ -937,19 +955,15 @@ bool is_dual_link_lvds(struct drm_device *dev)
 	if (dmi_check_system(intel_dual_link_lvds))
 		return true;
 
-	if (dev_priv->lvds_val)
-		val = dev_priv->lvds_val;
-	else {
-		/* BIOS should set the proper LVDS register value at boot, but
-		 * in reality, it doesn't set the value when the lid is closed;
-		 * we need to check "the value to be set" in VBT when LVDS
-		 * register is uninitialized.
-		 */
-		val = I915_READ(lvds_reg);
-		if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
-			val = dev_priv->bios_lvds_val;
-		dev_priv->lvds_val = val;
-	}
+	/* BIOS should set the proper LVDS register value at boot, but
+	 * in reality, it doesn't set the value when the lid is closed;
+	 * we need to check "the value to be set" in VBT when LVDS
+	 * register is uninitialized.
+	 */
+	val = I915_READ(lvds_reg);
+	if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
+		val = dev_priv->bios_lvds_val;
+
 	return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
 }
 
@@ -1148,6 +1162,8 @@ bool intel_lvds_init(struct drm_device *dev)
 		goto failed;
 
 out:
+	lvds_encoder->is_dual_link = __is_dual_link_lvds(dev);
+
 	/*
 	 * Unlock registers and just
 	 * leave them unlocked
@@ -1164,6 +1180,7 @@ out:
 		DRM_DEBUG_KMS("lid notifier registration failed\n");
 		lvds_connector->lid_notifier.notifier_call = NULL;
 	}
+
 	drm_sysfs_connector_add(connector);
 
 	intel_panel_init(&intel_connector->panel, fixed_mode);
-- 
1.7.11.7

Re: [PATCH 4/8] drm/i915: track is_dual_link in intel_lvds

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Yeah, all users (both the clock selection special cases and the lvds
> pin pair stuff) are still in common code, but this will change.
>
> v2: Rebase on top of Jani Nikula's panel rework.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 43 +++++++++++++++++++++++++++------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 2303984..e4ae3a6 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -52,6 +52,7 @@ struct intel_lvds_encoder {
>         u32 pfit_control;
>         u32 pfit_pgm_ratios;
>         bool pfit_dirty;
> +       bool is_dual_link;
>
>         struct intel_lvds_connector *attached_connector;
>  };
> @@ -920,6 +921,23 @@ static const struct dmi_system_id intel_dual_link_lvds[] = {
>
>  bool is_dual_link_lvds(struct drm_device *dev)
>  {
> +       struct intel_encoder *encoder;
> +       struct intel_lvds_encoder *lvds_encoder;
> +
> +       list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> +                           base.head) {
> +               if (encoder->type == INTEL_OUTPUT_LVDS) {
> +                       lvds_encoder = to_lvds_encoder(&encoder->base);
> +
> +                       return lvds_encoder->is_dual_link;
> +               }
> +       }
> +
> +       return false;
> +}
> +
> +static bool __is_dual_link_lvds(struct drm_device *dev)

How about "compute_is_dual_link_lvds" or "check_dual_link_lvds" ?

> +{
>         unsigned int val;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 lvds_reg;
> @@ -937,19 +955,15 @@ bool is_dual_link_lvds(struct drm_device *dev)
>         if (dmi_check_system(intel_dual_link_lvds))
>                 return true;
>
> -       if (dev_priv->lvds_val)
> -               val = dev_priv->lvds_val;
> -       else {
> -               /* BIOS should set the proper LVDS register value at boot, but
> -                * in reality, it doesn't set the value when the lid is closed;
> -                * we need to check "the value to be set" in VBT when LVDS
> -                * register is uninitialized.
> -                */
> -               val = I915_READ(lvds_reg);
> -               if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
> -                       val = dev_priv->bios_lvds_val;
> -               dev_priv->lvds_val = val;
> -       }
> +       /* BIOS should set the proper LVDS register value at boot, but
> +        * in reality, it doesn't set the value when the lid is closed;
> +        * we need to check "the value to be set" in VBT when LVDS
> +        * register is uninitialized.
> +        */
> +       val = I915_READ(lvds_reg);
> +       if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
> +               val = dev_priv->bios_lvds_val;
> +
>         return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
>  }
>
> @@ -1148,6 +1162,8 @@ bool intel_lvds_init(struct drm_device *dev)
>                 goto failed;
>
>  out:
> +       lvds_encoder->is_dual_link = __is_dual_link_lvds(dev);
> +
>         /*
>          * Unlock registers and just
>          * leave them unlocked
> @@ -1164,6 +1180,7 @@ out:
>                 DRM_DEBUG_KMS("lid notifier registration failed\n");
>                 lvds_connector->lid_notifier.notifier_call = NULL;
>         }
> +

Was this chunk on purpose?

>         drm_sysfs_connector_add(connector);
>
>         intel_panel_init(&intel_connector->panel, fixed_mode);
> --

Please also add this chunk to your patch:

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ab3c6a6..d39dfa9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -707,7 +707,6 @@ typedef struct drm_i915_private {
        unsigned int display_clock_mode:1;
        int lvds_ssc_freq;
        unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
-       unsigned int lvds_val; /* used for checking LVDS channel mode */
        struct {
                int rate;
                int lanes;



> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 5/8] drm/i915: add intel_lvds->reg

[Daniel Vetter]

To ditch at least some of the PCH_SPLIT ? PCH_LVDS : LVDS code ...

v2: Rebase on top of Jani Nikula's panel rework.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_lvds.c | 48 ++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index e4ae3a6..972ef12 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -53,6 +53,7 @@ struct intel_lvds_encoder {
 	u32 pfit_pgm_ratios;
 	bool pfit_dirty;
 	bool is_dual_link;
+	u32 reg;
 
 	struct intel_lvds_connector *attached_connector;
 };
@@ -72,15 +73,10 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
 {
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 lvds_reg, tmp;
-
-	if (HAS_PCH_SPLIT(dev)) {
-		lvds_reg = PCH_LVDS;
-	} else {
-		lvds_reg = LVDS;
-	}
+	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
+	u32 tmp;
 
-	tmp = I915_READ(lvds_reg);
+	tmp = I915_READ(lvds_encoder->reg);
 
 	if (!(tmp & LVDS_PORT_EN))
 		return false;
@@ -102,19 +98,17 @@ static void intel_enable_lvds(struct intel_encoder *encoder)
 	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 ctl_reg, lvds_reg, stat_reg;
+	u32 ctl_reg, stat_reg;
 
 	if (HAS_PCH_SPLIT(dev)) {
 		ctl_reg = PCH_PP_CONTROL;
-		lvds_reg = PCH_LVDS;
 		stat_reg = PCH_PP_STATUS;
 	} else {
 		ctl_reg = PP_CONTROL;
-		lvds_reg = LVDS;
 		stat_reg = PP_STATUS;
 	}
 
-	I915_WRITE(lvds_reg, I915_READ(lvds_reg) | LVDS_PORT_EN);
+	I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) | LVDS_PORT_EN);
 
 	if (lvds_encoder->pfit_dirty) {
 		/*
@@ -133,7 +127,7 @@ static void intel_enable_lvds(struct intel_encoder *encoder)
 	}
 
 	I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
-	POSTING_READ(lvds_reg);
+	POSTING_READ(lvds_encoder->reg);
 	if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000))
 		DRM_ERROR("timed out waiting for panel to power on\n");
 
@@ -145,15 +139,13 @@ static void intel_disable_lvds(struct intel_encoder *encoder)
 	struct drm_device *dev = encoder->base.dev;
 	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 ctl_reg, lvds_reg, stat_reg;
+	u32 ctl_reg, stat_reg;
 
 	if (HAS_PCH_SPLIT(dev)) {
 		ctl_reg = PCH_PP_CONTROL;
-		lvds_reg = PCH_LVDS;
 		stat_reg = PCH_PP_STATUS;
 	} else {
 		ctl_reg = PP_CONTROL;
-		lvds_reg = LVDS;
 		stat_reg = PP_STATUS;
 	}
 
@@ -168,8 +160,8 @@ static void intel_disable_lvds(struct intel_encoder *encoder)
 		lvds_encoder->pfit_dirty = true;
 	}
 
-	I915_WRITE(lvds_reg, I915_READ(lvds_reg) & ~LVDS_PORT_EN);
-	POSTING_READ(lvds_reg);
+	I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) & ~LVDS_PORT_EN);
+	POSTING_READ(lvds_encoder->reg);
 }
 
 static int intel_lvds_mode_valid(struct drm_connector *connector,
@@ -936,17 +928,11 @@ bool is_dual_link_lvds(struct drm_device *dev)
 	return false;
 }
 
-static bool __is_dual_link_lvds(struct drm_device *dev)
+static bool __is_dual_link_lvds(struct intel_lvds_encoder *lvds_encoder)
 {
+	struct drm_device *dev = lvds_encoder->base.base.dev;
 	unsigned int val;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 lvds_reg;
-
-	if (HAS_PCH_SPLIT(dev)) {
-		lvds_reg = PCH_LVDS;
-	} else {
-		lvds_reg = LVDS;
-	}
 
 	/* use the module option value if specified */
 	if (i915_lvds_channel_mode > 0)
@@ -960,7 +946,7 @@ static bool __is_dual_link_lvds(struct drm_device *dev)
 	 * we need to check "the value to be set" in VBT when LVDS
 	 * register is uninitialized.
 	 */
-	val = I915_READ(lvds_reg);
+	val = I915_READ(lvds_encoder->reg);
 	if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
 		val = dev_priv->bios_lvds_val;
 
@@ -1073,6 +1059,12 @@ bool intel_lvds_init(struct drm_device *dev)
 	connector->interlace_allowed = false;
 	connector->doublescan_allowed = false;
 
+	if (HAS_PCH_SPLIT(dev)) {
+		lvds_encoder->reg = PCH_LVDS;
+	} else {
+		lvds_encoder->reg = LVDS;
+	}
+
 	/* create the scaling mode property */
 	drm_mode_create_scaling_mode_property(dev);
 	drm_connector_attach_property(&intel_connector->base,
@@ -1162,7 +1154,7 @@ bool intel_lvds_init(struct drm_device *dev)
 		goto failed;
 
 out:
-	lvds_encoder->is_dual_link = __is_dual_link_lvds(dev);
+	lvds_encoder->is_dual_link = __is_dual_link_lvds(lvds_encoder);
 
 	/*
 	 * Unlock registers and just
-- 
1.7.11.7

Re: [PATCH 5/8] drm/i915: add intel_lvds->reg

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> To ditch at least some of the PCH_SPLIT ? PCH_LVDS : LVDS code ...
>
> v2: Rebase on top of Jani Nikula's panel rework.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 48 ++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index e4ae3a6..972ef12 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -53,6 +53,7 @@ struct intel_lvds_encoder {
>         u32 pfit_pgm_ratios;
>         bool pfit_dirty;
>         bool is_dual_link;
> +       u32 reg;
>
>         struct intel_lvds_connector *attached_connector;
>  };
> @@ -72,15 +73,10 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
>  {
>         struct drm_device *dev = encoder->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 lvds_reg, tmp;
> -
> -       if (HAS_PCH_SPLIT(dev)) {
> -               lvds_reg = PCH_LVDS;
> -       } else {
> -               lvds_reg = LVDS;
> -       }
> +       struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
> +       u32 tmp;
>
> -       tmp = I915_READ(lvds_reg);
> +       tmp = I915_READ(lvds_encoder->reg);
>
>         if (!(tmp & LVDS_PORT_EN))
>                 return false;
> @@ -102,19 +98,17 @@ static void intel_enable_lvds(struct intel_encoder *encoder)
>         struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
>         struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 ctl_reg, lvds_reg, stat_reg;
> +       u32 ctl_reg, stat_reg;
>
>         if (HAS_PCH_SPLIT(dev)) {
>                 ctl_reg = PCH_PP_CONTROL;
> -               lvds_reg = PCH_LVDS;
>                 stat_reg = PCH_PP_STATUS;
>         } else {
>                 ctl_reg = PP_CONTROL;
> -               lvds_reg = LVDS;
>                 stat_reg = PP_STATUS;
>         }
>
> -       I915_WRITE(lvds_reg, I915_READ(lvds_reg) | LVDS_PORT_EN);
> +       I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) | LVDS_PORT_EN);
>
>         if (lvds_encoder->pfit_dirty) {
>                 /*
> @@ -133,7 +127,7 @@ static void intel_enable_lvds(struct intel_encoder *encoder)
>         }
>
>         I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
> -       POSTING_READ(lvds_reg);
> +       POSTING_READ(lvds_encoder->reg);
>         if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000))
>                 DRM_ERROR("timed out waiting for panel to power on\n");
>
> @@ -145,15 +139,13 @@ static void intel_disable_lvds(struct intel_encoder *encoder)
>         struct drm_device *dev = encoder->base.dev;
>         struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 ctl_reg, lvds_reg, stat_reg;
> +       u32 ctl_reg, stat_reg;
>
>         if (HAS_PCH_SPLIT(dev)) {
>                 ctl_reg = PCH_PP_CONTROL;
> -               lvds_reg = PCH_LVDS;
>                 stat_reg = PCH_PP_STATUS;
>         } else {
>                 ctl_reg = PP_CONTROL;
> -               lvds_reg = LVDS;
>                 stat_reg = PP_STATUS;
>         }
>
> @@ -168,8 +160,8 @@ static void intel_disable_lvds(struct intel_encoder *encoder)
>                 lvds_encoder->pfit_dirty = true;
>         }
>
> -       I915_WRITE(lvds_reg, I915_READ(lvds_reg) & ~LVDS_PORT_EN);
> -       POSTING_READ(lvds_reg);
> +       I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) & ~LVDS_PORT_EN);
> +       POSTING_READ(lvds_encoder->reg);
>  }
>
>  static int intel_lvds_mode_valid(struct drm_connector *connector,
> @@ -936,17 +928,11 @@ bool is_dual_link_lvds(struct drm_device *dev)
>         return false;
>  }
>
> -static bool __is_dual_link_lvds(struct drm_device *dev)
> +static bool __is_dual_link_lvds(struct intel_lvds_encoder *lvds_encoder)
>  {
> +       struct drm_device *dev = lvds_encoder->base.base.dev;
>         unsigned int val;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 lvds_reg;
> -
> -       if (HAS_PCH_SPLIT(dev)) {
> -               lvds_reg = PCH_LVDS;
> -       } else {
> -               lvds_reg = LVDS;
> -       }
>
>         /* use the module option value if specified */
>         if (i915_lvds_channel_mode > 0)
> @@ -960,7 +946,7 @@ static bool __is_dual_link_lvds(struct drm_device *dev)
>          * we need to check "the value to be set" in VBT when LVDS
>          * register is uninitialized.
>          */
> -       val = I915_READ(lvds_reg);
> +       val = I915_READ(lvds_encoder->reg);
>         if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
>                 val = dev_priv->bios_lvds_val;
>
> @@ -1073,6 +1059,12 @@ bool intel_lvds_init(struct drm_device *dev)
>         connector->interlace_allowed = false;
>         connector->doublescan_allowed = false;
>
> +       if (HAS_PCH_SPLIT(dev)) {
> +               lvds_encoder->reg = PCH_LVDS;
> +       } else {
> +               lvds_encoder->reg = LVDS;
> +       }
> +
>         /* create the scaling mode property */
>         drm_mode_create_scaling_mode_property(dev);
>         drm_connector_attach_property(&intel_connector->base,
> @@ -1162,7 +1154,7 @@ bool intel_lvds_init(struct drm_device *dev)
>                 goto failed;
>
>  out:
> -       lvds_encoder->is_dual_link = __is_dual_link_lvds(dev);
> +       lvds_encoder->is_dual_link = __is_dual_link_lvds(lvds_encoder);
>
>         /*
>          * Unlock registers and just
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 6/8] drm/i915: move intel_update_lvds to intel_lvds->pre_pll_enable

[Daniel Vetter]

A few things needed to change:
- HAS_PCH_SPLIT since ilk+ is not yet converted to this.
- s/LVDS/intel_lvds->reg/ to prep for ilk conversion
- replace the clock.p2 == 7 check with a is_dual_link check
- s/adjusted_mode/intel_lvds->fixed_mode

v2: Rebase on top of Jani Nikula's panel rework. I'm wondering whether
we shouldn't add an attached_panel pointer to intel_encoder, to
replace the encoder private ->attached_connector pointers, since
that's essentially what we need.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 59 ------------------------------------
 drivers/gpu/drm/i915/intel_lvds.c    | 57 ++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7309790..bd3fa2b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4259,51 +4259,6 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
 	}
 }
 
-static void intel_update_lvds(struct drm_crtc *crtc, intel_clock_t *clock,
-			      struct drm_display_mode *adjusted_mode)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;
-	u32 temp;
-
-	temp = I915_READ(LVDS);
-	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
-	if (pipe == 1) {
-		temp |= LVDS_PIPEB_SELECT;
-	} else {
-		temp &= ~LVDS_PIPEB_SELECT;
-	}
-	/* set the corresponsding LVDS_BORDER bit */
-	temp |= dev_priv->lvds_border_bits;
-	/* Set the B0-B3 data pairs corresponding to whether we're going to
-	 * set the DPLLs for dual-channel mode or not.
-	 */
-	if (clock->p2 == 7)
-		temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
-	else
-		temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
-
-	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
-	 * appropriately here, but we need to look more thoroughly into how
-	 * panels behave in the two modes.
-	 */
-	/* set the dithering flag on LVDS as needed */
-	if (INTEL_INFO(dev)->gen >= 4) {
-		if (dev_priv->lvds_dither)
-			temp |= LVDS_ENABLE_DITHER;
-		else
-			temp &= ~LVDS_ENABLE_DITHER;
-	}
-	temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
-	if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
-		temp |= LVDS_HSYNC_POLARITY;
-	if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
-		temp |= LVDS_VSYNC_POLARITY;
-	I915_WRITE(LVDS, temp);
-}
-
 static void vlv_update_pll(struct drm_crtc *crtc,
 			   struct drm_display_mode *mode,
 			   struct drm_display_mode *adjusted_mode,
@@ -4486,13 +4441,6 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
 		if (encoder->pre_pll_enable)
 			encoder->pre_pll_enable(encoder);
 
-	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
-	 * This is an exception to the general rule that mode_set doesn't turn
-	 * things on.
-	 */
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
-		intel_update_lvds(crtc, clock, adjusted_mode);
-
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 
@@ -4568,13 +4516,6 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
 		if (encoder->pre_pll_enable)
 			encoder->pre_pll_enable(encoder);
 
-	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
-	 * This is an exception to the general rule that mode_set doesn't turn
-	 * things on.
-	 */
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
-		intel_update_lvds(crtc, clock, adjusted_mode);
-
 	I915_WRITE(DPLL(pipe), dpll);
 
 	/* Wait for the clocks to stabilize. */
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 972ef12..057e29a 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -89,6 +89,62 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
 	return true;
 }
 
+/* The LVDS pin pair needs to be on before the DPLLs are enabled.
+ * This is an exception to the general rule that mode_set doesn't turn
+ * things on.
+ */
+static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
+{
+	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct drm_display_mode *fixed_mode =
+		lvds_encoder->attached_connector->base.panel.fixed_mode;
+	int pipe = intel_crtc->pipe;
+	u32 temp;
+
+	/* pch split platforms are not yet converted. */
+	if (HAS_PCH_SPLIT(dev))
+		return;
+
+	temp = I915_READ(lvds_encoder->reg);
+	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
+	if (pipe == 1) {
+		temp |= LVDS_PIPEB_SELECT;
+	} else {
+		temp &= ~LVDS_PIPEB_SELECT;
+	}
+	/* set the corresponsding LVDS_BORDER bit */
+	temp |= dev_priv->lvds_border_bits;
+	/* Set the B0-B3 data pairs corresponding to whether we're going to
+	 * set the DPLLs for dual-channel mode or not.
+	 */
+	if (lvds_encoder->is_dual_link)
+		temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
+	else
+		temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
+
+	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
+	 * appropriately here, but we need to look more thoroughly into how
+	 * panels behave in the two modes.
+	 */
+	/* set the dithering flag on LVDS as needed */
+	if (INTEL_INFO(dev)->gen >= 4) {
+		if (dev_priv->lvds_dither)
+			temp |= LVDS_ENABLE_DITHER;
+		else
+			temp &= ~LVDS_ENABLE_DITHER;
+	}
+	temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
+	if (fixed_mode->flags & DRM_MODE_FLAG_NHSYNC)
+		temp |= LVDS_HSYNC_POLARITY;
+	if (fixed_mode->flags & DRM_MODE_FLAG_NVSYNC)
+		temp |= LVDS_VSYNC_POLARITY;
+
+	I915_WRITE(lvds_encoder->reg, temp);
+}
+
 /**
  * Sets the power state for the panel.
  */
@@ -1038,6 +1094,7 @@ bool intel_lvds_init(struct drm_device *dev)
 			 DRM_MODE_ENCODER_LVDS);
 
 	intel_encoder->enable = intel_enable_lvds;
+	intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds;
 	intel_encoder->disable = intel_disable_lvds;
 	intel_encoder->get_hw_state = intel_lvds_get_hw_state;
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
-- 
1.7.11.7

Re: [PATCH 6/8] drm/i915: move intel_update_lvds to intel_lvds->pre_pll_enable

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> A few things needed to change:
> - HAS_PCH_SPLIT since ilk+ is not yet converted to this.
> - s/LVDS/intel_lvds->reg/ to prep for ilk conversion
> - replace the clock.p2 == 7 check with a is_dual_link check
> - s/adjusted_mode/intel_lvds->fixed_mode
>
> v2: Rebase on top of Jani Nikula's panel rework. I'm wondering whether
> we shouldn't add an attached_panel pointer to intel_encoder, to
> replace the encoder private ->attached_connector pointers, since
> that's essentially what we need.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 59 ------------------------------------
>  drivers/gpu/drm/i915/intel_lvds.c    | 57 ++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7309790..bd3fa2b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4259,51 +4259,6 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
>         }
>  }
>
> -static void intel_update_lvds(struct drm_crtc *crtc, intel_clock_t *clock,
> -                             struct drm_display_mode *adjusted_mode)
> -{
> -       struct drm_device *dev = crtc->dev;
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -       int pipe = intel_crtc->pipe;
> -       u32 temp;
> -
> -       temp = I915_READ(LVDS);
> -       temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> -       if (pipe == 1) {
> -               temp |= LVDS_PIPEB_SELECT;
> -       } else {
> -               temp &= ~LVDS_PIPEB_SELECT;
> -       }
> -       /* set the corresponsding LVDS_BORDER bit */
> -       temp |= dev_priv->lvds_border_bits;
> -       /* Set the B0-B3 data pairs corresponding to whether we're going to
> -        * set the DPLLs for dual-channel mode or not.
> -        */
> -       if (clock->p2 == 7)
> -               temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
> -       else
> -               temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
> -
> -       /* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
> -        * appropriately here, but we need to look more thoroughly into how
> -        * panels behave in the two modes.
> -        */
> -       /* set the dithering flag on LVDS as needed */
> -       if (INTEL_INFO(dev)->gen >= 4) {
> -               if (dev_priv->lvds_dither)
> -                       temp |= LVDS_ENABLE_DITHER;
> -               else
> -                       temp &= ~LVDS_ENABLE_DITHER;
> -       }
> -       temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
> -       if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
> -               temp |= LVDS_HSYNC_POLARITY;
> -       if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
> -               temp |= LVDS_VSYNC_POLARITY;
> -       I915_WRITE(LVDS, temp);
> -}
> -
>  static void vlv_update_pll(struct drm_crtc *crtc,
>                            struct drm_display_mode *mode,
>                            struct drm_display_mode *adjusted_mode,
> @@ -4486,13 +4441,6 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
>                 if (encoder->pre_pll_enable)
>                         encoder->pre_pll_enable(encoder);
>
> -       /* The LVDS pin pair needs to be on before the DPLLs are enabled.
> -        * This is an exception to the general rule that mode_set doesn't turn
> -        * things on.
> -        */
> -       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
> -               intel_update_lvds(crtc, clock, adjusted_mode);
> -
>         if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
>                 intel_dp_set_m_n(crtc, mode, adjusted_mode);
>
> @@ -4568,13 +4516,6 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
>                 if (encoder->pre_pll_enable)
>                         encoder->pre_pll_enable(encoder);
>
> -       /* The LVDS pin pair needs to be on before the DPLLs are enabled.
> -        * This is an exception to the general rule that mode_set doesn't turn
> -        * things on.
> -        */
> -       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
> -               intel_update_lvds(crtc, clock, adjusted_mode);
> -
>         I915_WRITE(DPLL(pipe), dpll);
>
>         /* Wait for the clocks to stabilize. */
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 972ef12..057e29a 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -89,6 +89,62 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
>         return true;
>  }
>
> +/* The LVDS pin pair needs to be on before the DPLLs are enabled.
> + * This is an exception to the general rule that mode_set doesn't turn
> + * things on.
> + */
> +static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
> +{
> +       struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
> +       struct drm_device *dev = encoder->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +       struct drm_display_mode *fixed_mode =
> +               lvds_encoder->attached_connector->base.panel.fixed_mode;
> +       int pipe = intel_crtc->pipe;
> +       u32 temp;
> +
> +       /* pch split platforms are not yet converted. */
> +       if (HAS_PCH_SPLIT(dev))
> +               return;
> +
> +       temp = I915_READ(lvds_encoder->reg);
> +       temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> +       if (pipe == 1) {
> +               temp |= LVDS_PIPEB_SELECT;
> +       } else {
> +               temp &= ~LVDS_PIPEB_SELECT;
> +       }
> +       /* set the corresponsding LVDS_BORDER bit */
> +       temp |= dev_priv->lvds_border_bits;
> +       /* Set the B0-B3 data pairs corresponding to whether we're going to
> +        * set the DPLLs for dual-channel mode or not.
> +        */
> +       if (lvds_encoder->is_dual_link)
> +               temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
> +       else
> +               temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
> +
> +       /* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
> +        * appropriately here, but we need to look more thoroughly into how
> +        * panels behave in the two modes.
> +        */
> +       /* set the dithering flag on LVDS as needed */
> +       if (INTEL_INFO(dev)->gen >= 4) {
> +               if (dev_priv->lvds_dither)
> +                       temp |= LVDS_ENABLE_DITHER;
> +               else
> +                       temp &= ~LVDS_ENABLE_DITHER;
> +       }
> +       temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
> +       if (fixed_mode->flags & DRM_MODE_FLAG_NHSYNC)
> +               temp |= LVDS_HSYNC_POLARITY;
> +       if (fixed_mode->flags & DRM_MODE_FLAG_NVSYNC)
> +               temp |= LVDS_VSYNC_POLARITY;
> +
> +       I915_WRITE(lvds_encoder->reg, temp);
> +}
> +
>  /**
>   * Sets the power state for the panel.
>   */
> @@ -1038,6 +1094,7 @@ bool intel_lvds_init(struct drm_device *dev)
>                          DRM_MODE_ENCODER_LVDS);
>
>         intel_encoder->enable = intel_enable_lvds;
> +       intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds;
>         intel_encoder->disable = intel_disable_lvds;
>         intel_encoder->get_hw_state = intel_lvds_get_hw_state;
>         intel_connector->get_hw_state = intel_connector_get_hw_state;
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 7/8] drm/i915: enable intel_lvds->pre_pll_enable for ilk+, too

[Daniel Vetter]

Only two things needed adjustment:
- pipe select for PCH_CPT
- There's no dithering bit on ilk+ in the lvds ctl reg

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 40 ------------------------------------
 drivers/gpu/drm/i915/intel_lvds.c    | 24 ++++++++++++++--------
 2 files changed, 15 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bd3fa2b..68c0524 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5324,7 +5324,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	bool ok, has_reduced_clock = false;
 	bool is_lvds = false, is_dp = false, is_cpu_edp = false;
 	struct intel_encoder *encoder;
-	u32 temp;
 	int ret;
 	bool dither, fdi_config_ok;
 
@@ -5387,45 +5386,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	} else
 		intel_put_pch_pll(intel_crtc);
 
-	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
-	 * This is an exception to the general rule that mode_set doesn't turn
-	 * things on.
-	 */
-	if (is_lvds) {
-		temp = I915_READ(PCH_LVDS);
-		temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
-		if (HAS_PCH_CPT(dev)) {
-			temp &= ~PORT_TRANS_SEL_MASK;
-			temp |= PORT_TRANS_SEL_CPT(pipe);
-		} else {
-			if (pipe == 1)
-				temp |= LVDS_PIPEB_SELECT;
-			else
-				temp &= ~LVDS_PIPEB_SELECT;
-		}
-
-		/* set the corresponsding LVDS_BORDER bit */
-		temp |= dev_priv->lvds_border_bits;
-		/* Set the B0-B3 data pairs corresponding to whether we're going to
-		 * set the DPLLs for dual-channel mode or not.
-		 */
-		if (clock.p2 == 7)
-			temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
-		else
-			temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
-
-		/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
-		 * appropriately here, but we need to look more thoroughly into how
-		 * panels behave in the two modes.
-		 */
-		temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
-		if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
-			temp |= LVDS_HSYNC_POLARITY;
-		if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
-			temp |= LVDS_VSYNC_POLARITY;
-		I915_WRITE(PCH_LVDS, temp);
-	}
-
 	if (is_dp && !is_cpu_edp) {
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 	} else {
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 057e29a..b4025c1 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -104,17 +104,20 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
 	int pipe = intel_crtc->pipe;
 	u32 temp;
 
-	/* pch split platforms are not yet converted. */
-	if (HAS_PCH_SPLIT(dev))
-		return;
-
 	temp = I915_READ(lvds_encoder->reg);
 	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
-	if (pipe == 1) {
-		temp |= LVDS_PIPEB_SELECT;
+
+	if (HAS_PCH_CPT(dev)) {
+		temp &= ~PORT_TRANS_SEL_MASK;
+		temp |= PORT_TRANS_SEL_CPT(pipe);
 	} else {
-		temp &= ~LVDS_PIPEB_SELECT;
+		if (pipe == 1) {
+			temp |= LVDS_PIPEB_SELECT;
+		} else {
+			temp &= ~LVDS_PIPEB_SELECT;
+		}
 	}
+
 	/* set the corresponsding LVDS_BORDER bit */
 	temp |= dev_priv->lvds_border_bits;
 	/* Set the B0-B3 data pairs corresponding to whether we're going to
@@ -129,8 +132,11 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
 	 * appropriately here, but we need to look more thoroughly into how
 	 * panels behave in the two modes.
 	 */
-	/* set the dithering flag on LVDS as needed */
-	if (INTEL_INFO(dev)->gen >= 4) {
+
+	/* Set the dithering flag on LVDS as needed, note that there is no
+	 * special lvds dither control bit on pch-split platforms, dithering is
+	 * only controlled through the PIPECONF reg. */
+	if (INTEL_INFO(dev)->gen == 4) {
 		if (dev_priv->lvds_dither)
 			temp |= LVDS_ENABLE_DITHER;
 		else
-- 
1.7.11.7

Re: [PATCH 7/8] drm/i915: enable intel_lvds->pre_pll_enable for ilk+, too

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Only two things needed adjustment:
> - pipe select for PCH_CPT
> - There's no dithering bit on ilk+ in the lvds ctl reg
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 40 ------------------------------------
>  drivers/gpu/drm/i915/intel_lvds.c    | 24 ++++++++++++++--------
>  2 files changed, 15 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bd3fa2b..68c0524 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5324,7 +5324,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         bool ok, has_reduced_clock = false;
>         bool is_lvds = false, is_dp = false, is_cpu_edp = false;
>         struct intel_encoder *encoder;
> -       u32 temp;
>         int ret;
>         bool dither, fdi_config_ok;
>
> @@ -5387,45 +5386,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         } else
>                 intel_put_pch_pll(intel_crtc);
>
> -       /* The LVDS pin pair needs to be on before the DPLLs are enabled.
> -        * This is an exception to the general rule that mode_set doesn't turn
> -        * things on.
> -        */
> -       if (is_lvds) {
> -               temp = I915_READ(PCH_LVDS);
> -               temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> -               if (HAS_PCH_CPT(dev)) {
> -                       temp &= ~PORT_TRANS_SEL_MASK;
> -                       temp |= PORT_TRANS_SEL_CPT(pipe);
> -               } else {
> -                       if (pipe == 1)
> -                               temp |= LVDS_PIPEB_SELECT;
> -                       else
> -                               temp &= ~LVDS_PIPEB_SELECT;
> -               }
> -
> -               /* set the corresponsding LVDS_BORDER bit */
> -               temp |= dev_priv->lvds_border_bits;
> -               /* Set the B0-B3 data pairs corresponding to whether we're going to
> -                * set the DPLLs for dual-channel mode or not.
> -                */
> -               if (clock.p2 == 7)
> -                       temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
> -               else
> -                       temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
> -
> -               /* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
> -                * appropriately here, but we need to look more thoroughly into how
> -                * panels behave in the two modes.
> -                */
> -               temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
> -               if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
> -                       temp |= LVDS_HSYNC_POLARITY;
> -               if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
> -                       temp |= LVDS_VSYNC_POLARITY;
> -               I915_WRITE(PCH_LVDS, temp);
> -       }
> -
>         if (is_dp && !is_cpu_edp) {
>                 intel_dp_set_m_n(crtc, mode, adjusted_mode);
>         } else {
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 057e29a..b4025c1 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -104,17 +104,20 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>         int pipe = intel_crtc->pipe;
>         u32 temp;
>
> -       /* pch split platforms are not yet converted. */
> -       if (HAS_PCH_SPLIT(dev))
> -               return;
> -
>         temp = I915_READ(lvds_encoder->reg);
>         temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> -       if (pipe == 1) {
> -               temp |= LVDS_PIPEB_SELECT;
> +
> +       if (HAS_PCH_CPT(dev)) {
> +               temp &= ~PORT_TRANS_SEL_MASK;
> +               temp |= PORT_TRANS_SEL_CPT(pipe);
>         } else {
> -               temp &= ~LVDS_PIPEB_SELECT;
> +               if (pipe == 1) {
> +                       temp |= LVDS_PIPEB_SELECT;
> +               } else {
> +                       temp &= ~LVDS_PIPEB_SELECT;
> +               }
>         }
> +
>         /* set the corresponsding LVDS_BORDER bit */
>         temp |= dev_priv->lvds_border_bits;
>         /* Set the B0-B3 data pairs corresponding to whether we're going to
> @@ -129,8 +132,11 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>          * appropriately here, but we need to look more thoroughly into how
>          * panels behave in the two modes.
>          */
> -       /* set the dithering flag on LVDS as needed */
> -       if (INTEL_INFO(dev)->gen >= 4) {
> +
> +       /* Set the dithering flag on LVDS as needed, note that there is no
> +        * special lvds dither control bit on pch-split platforms, dithering is
> +        * only controlled through the PIPECONF reg. */
> +       if (INTEL_INFO(dev)->gen == 4) {
>                 if (dev_priv->lvds_dither)
>                         temp |= LVDS_ENABLE_DITHER;
>                 else
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 8/8] drm/i915: rip out pre-DDI stuff from haswell_crtc_mode_set

[Daniel Vetter]

Especially getting rid of all things lvds is ... great!

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 114 +----------------------------------
 1 file changed, 1 insertion(+), 113 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 68c0524..27fc014 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5465,23 +5465,14 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 	int num_connectors = 0;
-	intel_clock_t clock, reduced_clock;
-	u32 dpll = 0, fp = 0, fp2 = 0;
-	bool ok, has_reduced_clock = false;
-	bool is_lvds = false, is_dp = false, is_cpu_edp = false;
+	bool is_dp = false, is_cpu_edp = false;
 	struct intel_encoder *encoder;
-	u32 temp;
 	int ret;
 	bool dither;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
-		case INTEL_OUTPUT_LVDS:
-			is_lvds = true;
-			break;
 		case INTEL_OUTPUT_DISPLAYPORT:
-			is_dp = true;
-			break;
 		case INTEL_OUTPUT_EDP:
 			is_dp = true;
 			if (!intel_encoder_is_pch_edp(&encoder->base))
@@ -5512,93 +5503,15 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	if (!intel_ddi_pll_mode_set(crtc, adjusted_mode->clock))
 		return -EINVAL;
 
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-		ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
-					     &has_reduced_clock,
-					     &reduced_clock);
-		if (!ok) {
-			DRM_ERROR("Couldn't find PLL settings for mode!\n");
-			return -EINVAL;
-		}
-	}
-
 	/* Ensure that the cursor is valid for the new mode before changing... */
 	intel_crtc_update_cursor(crtc, true);
 
 	/* determine panel color depth */
 	dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp, mode);
-	if (is_lvds && dev_priv->lvds_dither)
-		dither = true;
 
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
 
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-		fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
-		if (has_reduced_clock)
-			fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
-			      reduced_clock.m2;
-
-		dpll = ironlake_compute_dpll(intel_crtc, adjusted_mode, &clock,
-					     fp);
-
-		/* CPU eDP is the only output that doesn't need a PCH PLL of its
-		 * own on pre-Haswell/LPT generation */
-		if (!is_cpu_edp) {
-			struct intel_pch_pll *pll;
-
-			pll = intel_get_pch_pll(intel_crtc, dpll, fp);
-			if (pll == NULL) {
-				DRM_DEBUG_DRIVER("failed to find PLL for pipe %d\n",
-						 pipe);
-				return -EINVAL;
-			}
-		} else
-			intel_put_pch_pll(intel_crtc);
-
-		/* The LVDS pin pair needs to be on before the DPLLs are
-		 * enabled.  This is an exception to the general rule that
-		 * mode_set doesn't turn things on.
-		 */
-		if (is_lvds) {
-			temp = I915_READ(PCH_LVDS);
-			temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
-			if (HAS_PCH_CPT(dev)) {
-				temp &= ~PORT_TRANS_SEL_MASK;
-				temp |= PORT_TRANS_SEL_CPT(pipe);
-			} else {
-				if (pipe == 1)
-					temp |= LVDS_PIPEB_SELECT;
-				else
-					temp &= ~LVDS_PIPEB_SELECT;
-			}
-
-			/* set the corresponsding LVDS_BORDER bit */
-			temp |= dev_priv->lvds_border_bits;
-			/* Set the B0-B3 data pairs corresponding to whether
-			 * we're going to set the DPLLs for dual-channel mode or
-			 * not.
-			 */
-			if (clock.p2 == 7)
-				temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
-			else
-				temp &= ~(LVDS_B0B3_POWER_UP |
-					  LVDS_CLKB_POWER_UP);
-
-			/* It would be nice to set 24 vs 18-bit mode
-			 * (LVDS_A3_POWER_UP) appropriately here, but we need to
-			 * look more thoroughly into how panels behave in the
-			 * two modes.
-			 */
-			temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
-			if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
-				temp |= LVDS_HSYNC_POLARITY;
-			if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
-				temp |= LVDS_VSYNC_POLARITY;
-			I915_WRITE(PCH_LVDS, temp);
-		}
-	}
-
 	if (is_dp && !is_cpu_edp) {
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 	} else {
@@ -5613,31 +5526,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	}
 
 	intel_crtc->lowfreq_avail = false;
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-		if (intel_crtc->pch_pll) {
-			I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
-
-			/* Wait for the clocks to stabilize. */
-			POSTING_READ(intel_crtc->pch_pll->pll_reg);
-			udelay(150);
-
-			/* The pixel multiplier can only be updated once the
-			 * DPLL is enabled and the clocks are stable.
-			 *
-			 * So write it again.
-			 */
-			I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
-		}
-
-		if (intel_crtc->pch_pll) {
-			if (is_lvds && has_reduced_clock && i915_powersave) {
-				I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
-				intel_crtc->lowfreq_avail = true;
-			} else {
-				I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
-			}
-		}
-	}
 
 	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
-- 
1.7.11.7

Re: [PATCH 8/8] drm/i915: rip out pre-DDI stuff from haswell_crtc_mode_set

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Especially getting rid of all things lvds is ... great!
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 114 +----------------------------------
>  1 file changed, 1 insertion(+), 113 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 68c0524..27fc014 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5465,23 +5465,14 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>         int pipe = intel_crtc->pipe;
>         int plane = intel_crtc->plane;
>         int num_connectors = 0;
> -       intel_clock_t clock, reduced_clock;
> -       u32 dpll = 0, fp = 0, fp2 = 0;
> -       bool ok, has_reduced_clock = false;
> -       bool is_lvds = false, is_dp = false, is_cpu_edp = false;
> +       bool is_dp = false, is_cpu_edp = false;
>         struct intel_encoder *encoder;
> -       u32 temp;
>         int ret;
>         bool dither;
>
>         for_each_encoder_on_crtc(dev, crtc, encoder) {
>                 switch (encoder->type) {
> -               case INTEL_OUTPUT_LVDS:
> -                       is_lvds = true;
> -                       break;
>                 case INTEL_OUTPUT_DISPLAYPORT:
> -                       is_dp = true;
> -                       break;
>                 case INTEL_OUTPUT_EDP:
>                         is_dp = true;
>                         if (!intel_encoder_is_pch_edp(&encoder->base))
> @@ -5512,93 +5503,15 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>         if (!intel_ddi_pll_mode_set(crtc, adjusted_mode->clock))
>                 return -EINVAL;
>
> -       if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> -               ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
> -                                            &has_reduced_clock,
> -                                            &reduced_clock);
> -               if (!ok) {
> -                       DRM_ERROR("Couldn't find PLL settings for mode!\n");
> -                       return -EINVAL;
> -               }
> -       }
> -
>         /* Ensure that the cursor is valid for the new mode before changing... */
>         intel_crtc_update_cursor(crtc, true);
>
>         /* determine panel color depth */
>         dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp, mode);

Patch doesn't apply because on my tree (based on drm-intel-nightly)
this line uses "adjusted_mode" instead of "mode". I have no idea if
this is a problem or not, but maybe it rings a bell for you.

For completeness of your patch, you may also want to remove the following lines:

Check obsoleted by your "check soulmates" patch:

        /* We are not sure yet this won't happen. */
        WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
             INTEL_PCH_TYPE(dev));

The only ibx/cpt check remaining:

                if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
                        /* For non-DP output, clear any trans DP clock
recovery
                         * setting.*/
                        I915_WRITE(TRANSDATA_M1(pipe), 0);
                        I915_WRITE(TRANSDATA_N1(pipe), 0);
                        I915_WRITE(TRANSDPLINK_M1(pipe), 0);
                        I915_WRITE(TRANSDPLINK_N1(pipe), 0);
                }


> -       if (is_lvds && dev_priv->lvds_dither)
> -               dither = true;
>
>         DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
>         drm_mode_debug_printmodeline(mode);
>
> -       if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> -               fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
> -               if (has_reduced_clock)
> -                       fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
> -                             reduced_clock.m2;
> -
> -               dpll = ironlake_compute_dpll(intel_crtc, adjusted_mode, &clock,
> -                                            fp);
> -
> -               /* CPU eDP is the only output that doesn't need a PCH PLL of its
> -                * own on pre-Haswell/LPT generation */
> -               if (!is_cpu_edp) {
> -                       struct intel_pch_pll *pll;
> -
> -                       pll = intel_get_pch_pll(intel_crtc, dpll, fp);
> -                       if (pll == NULL) {
> -                               DRM_DEBUG_DRIVER("failed to find PLL for pipe %d\n",
> -                                                pipe);
> -                               return -EINVAL;
> -                       }
> -               } else
> -                       intel_put_pch_pll(intel_crtc);
> -
> -               /* The LVDS pin pair needs to be on before the DPLLs are
> -                * enabled.  This is an exception to the general rule that
> -                * mode_set doesn't turn things on.
> -                */
> -               if (is_lvds) {
> -                       temp = I915_READ(PCH_LVDS);
> -                       temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> -                       if (HAS_PCH_CPT(dev)) {
> -                               temp &= ~PORT_TRANS_SEL_MASK;
> -                               temp |= PORT_TRANS_SEL_CPT(pipe);
> -                       } else {
> -                               if (pipe == 1)
> -                                       temp |= LVDS_PIPEB_SELECT;
> -                               else
> -                                       temp &= ~LVDS_PIPEB_SELECT;
> -                       }
> -
> -                       /* set the corresponsding LVDS_BORDER bit */
> -                       temp |= dev_priv->lvds_border_bits;
> -                       /* Set the B0-B3 data pairs corresponding to whether
> -                        * we're going to set the DPLLs for dual-channel mode or
> -                        * not.
> -                        */
> -                       if (clock.p2 == 7)
> -                               temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
> -                       else
> -                               temp &= ~(LVDS_B0B3_POWER_UP |
> -                                         LVDS_CLKB_POWER_UP);
> -
> -                       /* It would be nice to set 24 vs 18-bit mode
> -                        * (LVDS_A3_POWER_UP) appropriately here, but we need to
> -                        * look more thoroughly into how panels behave in the
> -                        * two modes.
> -                        */
> -                       temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
> -                       if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
> -                               temp |= LVDS_HSYNC_POLARITY;
> -                       if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
> -                               temp |= LVDS_VSYNC_POLARITY;
> -                       I915_WRITE(PCH_LVDS, temp);
> -               }
> -       }
> -
>         if (is_dp && !is_cpu_edp) {
>                 intel_dp_set_m_n(crtc, mode, adjusted_mode);
>         } else {
> @@ -5613,31 +5526,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>         }
>
>         intel_crtc->lowfreq_avail = false;
> -       if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> -               if (intel_crtc->pch_pll) {
> -                       I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> -
> -                       /* Wait for the clocks to stabilize. */
> -                       POSTING_READ(intel_crtc->pch_pll->pll_reg);
> -                       udelay(150);
> -
> -                       /* The pixel multiplier can only be updated once the
> -                        * DPLL is enabled and the clocks are stable.
> -                        *
> -                        * So write it again.
> -                        */
> -                       I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> -               }
> -
> -               if (intel_crtc->pch_pll) {
> -                       if (is_lvds && has_reduced_clock && i915_powersave) {
> -                               I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
> -                               intel_crtc->lowfreq_avail = true;
> -                       } else {
> -                               I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
> -                       }
> -               }
> -       }
>
>         intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 8/8] drm/i915: rip out pre-DDI stuff from haswell_crtc_mode_set

[Daniel Vetter]

On Fri, Nov 16, 2012 at 7:28 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Hi
>
> 2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> Especially getting rid of all things lvds is ... great!
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>         intel_crtc_update_cursor(crtc, true);
>>
>>         /* determine panel color depth */
>>         dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp, mode);
>
> Patch doesn't apply because on my tree (based on drm-intel-nightly)
> this line uses "adjusted_mode" instead of "mode". I have no idea if
> this is a problem or not, but maybe it rings a bell for you.

Yeah, that's just Jani's dp 6bpc fix getting a bit in the way. Will fix up.

> For completeness of your patch, you may also want to remove the following lines:
>
> Check obsoleted by your "check soulmates" patch:
>
>         /* We are not sure yet this won't happen. */
>         WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
>              INTEL_PCH_TYPE(dev));

Will remove.

> The only ibx/cpt check remaining:
>
>                 if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>                         /* For non-DP output, clear any trans DP clock
> recovery
>                          * setting.*/
>                         I915_WRITE(TRANSDATA_M1(pipe), 0);
>                         I915_WRITE(TRANSDATA_N1(pipe), 0);
>                         I915_WRITE(TRANSDPLINK_M1(pipe), 0);
>                         I915_WRITE(TRANSDPLINK_N1(pipe), 0);

This will die in one of the dp patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/8] lvds cleanup

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Hi all,
>
> This is the first cleanup from my next stab at reworking the modeset code, with
> the ultimate goal that we can compute the entire configuration (fdi config, pll
> config, sharing of global resources) up-front, before touching the hw at all.
> Together with some neat hw state readout this should make fastboot much more
> solid, and obviously it's a requirement to properly implement the check mode of
> atomic modeset.
>
> Here I move some of the lvds stuff out of line, simple to better see through the
> jungle. The newly-added pre_pll_enable callback might be unnecessary in the end,
> since I think we should also move the pll enabling into the crtc_enable callback
> and out of ->mode_set. Also, we need some notion of exclusive pch_pll (which the
> lvds port needs to obey the modeset sequence) and stop disabling pch plls
> unconditionally, since they might be in use by another active pipe. But that is
> all stuff on top, once the entire clock handling rework settles.
>
> For context, my current wip (iow: where I am stuck atm ...):
>
> http://cgit.freedesktop.org/~danvet/drm/log/?h=modeset-rework
>
> Comments, flames and test reports highly welcome.

Since you're already touching LVDS, can I also volunteer you to take a
look at the LVDS_CTL register description on our documentation and
implement all the workarounds listed there? A quick look shows we are
missing at least bit 31 in cpt/ppt.

>
> Cheers, Daniel
>
> Daniel Vetter (8):
>   drm/i915: add encoder->pre_pll_enable callback
>   drm/i915: replace ad-hoc dual-link lvds checks
>   drm/i915: move is_dual_link_lvds to intel_lvds.c
>   drm/i915: track is_dual_link in intel_lvds
>   drm/i915: add intel_lvds->reg
>   drm/i915: move intel_update_lvds to intel_lvds->pre_pll_enable
>   drm/i915: enable intel_lvds->pre_pll_enable for ilk+, too
>   drm/i915: rip out pre-DDI stuff from haswell_crtc_mode_set
>
>  drivers/gpu/drm/i915/intel_display.c | 287 +++--------------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_lvds.c    | 161 +++++++++++++++++---
>  3 files changed, 164 insertions(+), 286 deletions(-)
>
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 0/8] lvds cleanup

[Daniel Vetter]

On Fri, Nov 16, 2012 at 02:09:05PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Hi all,
> >
> > This is the first cleanup from my next stab at reworking the modeset code, with
> > the ultimate goal that we can compute the entire configuration (fdi config, pll
> > config, sharing of global resources) up-front, before touching the hw at all.
> > Together with some neat hw state readout this should make fastboot much more
> > solid, and obviously it's a requirement to properly implement the check mode of
> > atomic modeset.
> >
> > Here I move some of the lvds stuff out of line, simple to better see through the
> > jungle. The newly-added pre_pll_enable callback might be unnecessary in the end,
> > since I think we should also move the pll enabling into the crtc_enable callback
> > and out of ->mode_set. Also, we need some notion of exclusive pch_pll (which the
> > lvds port needs to obey the modeset sequence) and stop disabling pch plls
> > unconditionally, since they might be in use by another active pipe. But that is
> > all stuff on top, once the entire clock handling rework settles.
> >
> > For context, my current wip (iow: where I am stuck atm ...):
> >
> > http://cgit.freedesktop.org/~danvet/drm/log/?h=modeset-rework
> >
> > Comments, flames and test reports highly welcome.
> 
> Since you're already touching LVDS, can I also volunteer you to take a
> look at the LVDS_CTL register description on our documentation and
> implement all the workarounds listed there? A quick look shows we are
> missing at least bit 31 in cpt/ppt.

tbh I'm not sure what we're supposed to do with that w/a: Since the clock
gating bits need to be set until we first enable the lvds in dual_link
mode, I think this is the BIOS' job. So would you be ok if I just add a
check that those bits are set when enabling the lvds output. And if that's
not the case, print a debug message? We can think harder about this once
we have an lvds panel with a black screen ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch