* [PATCH 2/5] drm/i915: WARN if the DP aux read is too big
2013-09-12 16:58 [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+ Paulo Zanoni
@ 2013-09-12 16:58 ` Paulo Zanoni
2013-09-12 17:15 ` Chris Wilson
2013-09-12 16:58 ` [PATCH 3/5] drm/i915: check for more ASLC interrupts Paulo Zanoni
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Paulo Zanoni @ 2013-09-12 16:58 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
So far we control all the reads an none of them exceeds the current
limit of 20 bytes, but we never think about this when reviewing
patches, so add a big WARN. In case we ever hit that WARN, we whould
change the size of the reply array.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 20e468c..f30b691 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -577,6 +577,8 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
msg_bytes = 4;
reply_bytes = recv_bytes + 1;
+ if (WARN_ON(reply_bytes > sizeof(reply)))
+ return -E2BIG;
for (;;) {
ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] drm/i915: WARN if the DP aux read is too big
2013-09-12 16:58 ` [PATCH 2/5] drm/i915: WARN if the DP aux read is too big Paulo Zanoni
@ 2013-09-12 17:15 ` Chris Wilson
2013-09-12 21:03 ` [PATCH 2/5] drm/i915: fix intel_dp_aux_native_read's reply array size Paulo Zanoni
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2013-09-12 17:15 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Thu, Sep 12, 2013 at 01:58:18PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> So far we control all the reads an none of them exceeds the current
> limit of 20 bytes, but we never think about this when reviewing
> patches, so add a big WARN. In case we ever hit that WARN, we whould
> change the size of the reply array.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 20e468c..f30b691 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -577,6 +577,8 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
>
> msg_bytes = 4;
> reply_bytes = recv_bytes + 1;
> + if (WARN_ON(reply_bytes > sizeof(reply)))
> + return -E2BIG;
Meh, this would be a programming mistake, and coule be eliminated
entirely with:
uint8_t reply[recv_bytes+1];
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/5] drm/i915: fix intel_dp_aux_native_read's reply array size
2013-09-12 17:15 ` Chris Wilson
@ 2013-09-12 21:03 ` Paulo Zanoni
2013-09-13 9:21 ` Jani Nikula
0 siblings, 1 reply; 20+ messages in thread
From: Paulo Zanoni @ 2013-09-12 21:03 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
So far we control all the reads an none of them exceeds the current
limit of 20 bytes, but we never think about this when reviewing
patches, so we may at some point in the future overflow the buffer.
My initial patch just added a WARN in case we were about to overflow
the buffer, but Chris suggested to make the size of the array dynamic.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 20e468c..bf0b260 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -564,7 +564,7 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
{
uint8_t msg[4];
int msg_bytes;
- uint8_t reply[20];
+ uint8_t reply[recv_bytes + 1];
int reply_bytes;
uint8_t ack;
int ret;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] drm/i915: fix intel_dp_aux_native_read's reply array size
2013-09-12 21:03 ` [PATCH 2/5] drm/i915: fix intel_dp_aux_native_read's reply array size Paulo Zanoni
@ 2013-09-13 9:21 ` Jani Nikula
2013-09-13 12:53 ` Damien Lespiau
0 siblings, 1 reply; 20+ messages in thread
From: Jani Nikula @ 2013-09-13 9:21 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On Fri, 13 Sep 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> So far we control all the reads an none of them exceeds the current
> limit of 20 bytes, but we never think about this when reviewing
> patches, so we may at some point in the future overflow the buffer.
>
> My initial patch just added a WARN in case we were about to overflow
> the buffer, but Chris suggested to make the size of the array dynamic.
VLA on stack in kernel gives me an uneasy feeling, but I'll survive.
intel_dp_aux_native_write() has a range check and an error return for
this. Maybe make the same change there, and add a
BUG_ON({send,recv}_bytes > 0x100) to both, since that's the max?
Jani.
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 20e468c..bf0b260 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -564,7 +564,7 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
> {
> uint8_t msg[4];
> int msg_bytes;
> - uint8_t reply[20];
> + uint8_t reply[recv_bytes + 1];
> int reply_bytes;
> uint8_t ack;
> int ret;
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] drm/i915: fix intel_dp_aux_native_read's reply array size
2013-09-13 9:21 ` Jani Nikula
@ 2013-09-13 12:53 ` Damien Lespiau
2013-09-13 13:41 ` Jani Nikula
0 siblings, 1 reply; 20+ messages in thread
From: Damien Lespiau @ 2013-09-13 12:53 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, Paulo Zanoni
On Fri, Sep 13, 2013 at 12:21:21PM +0300, Jani Nikula wrote:
> On Fri, 13 Sep 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > So far we control all the reads an none of them exceeds the current
> > limit of 20 bytes, but we never think about this when reviewing
> > patches, so we may at some point in the future overflow the buffer.
> >
> > My initial patch just added a WARN in case we were about to overflow
> > the buffer, but Chris suggested to make the size of the array dynamic.
>
> VLA on stack in kernel gives me an uneasy feeling, but I'll survive.
IIRC sparse will complain about this. Might worth double checking.
--
Damien
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] drm/i915: fix intel_dp_aux_native_read's reply array size
2013-09-13 12:53 ` Damien Lespiau
@ 2013-09-13 13:41 ` Jani Nikula
2013-09-13 13:50 ` Damien Lespiau
0 siblings, 1 reply; 20+ messages in thread
From: Jani Nikula @ 2013-09-13 13:41 UTC (permalink / raw)
To: Damien Lespiau; +Cc: intel-gfx, Paulo Zanoni
On Fri, 13 Sep 2013, Damien Lespiau <damien.lespiau@intel.com> wrote:
> On Fri, Sep 13, 2013 at 12:21:21PM +0300, Jani Nikula wrote:
>> On Fri, 13 Sep 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > So far we control all the reads an none of them exceeds the current
>> > limit of 20 bytes, but we never think about this when reviewing
>> > patches, so we may at some point in the future overflow the buffer.
>> >
>> > My initial patch just added a WARN in case we were about to overflow
>> > the buffer, but Chris suggested to make the size of the array dynamic.
>>
>> VLA on stack in kernel gives me an uneasy feeling, but I'll survive.
>
> IIRC sparse will complain about this. Might worth double checking.
True.
CHECK drivers/gpu/drm/i915/intel_dp.c
drivers/gpu/drm/i915/intel_dp.c:567:34: error: bad constant expression
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] drm/i915: fix intel_dp_aux_native_read's reply array size
2013-09-13 13:41 ` Jani Nikula
@ 2013-09-13 13:50 ` Damien Lespiau
2013-09-16 19:26 ` Paulo Zanoni
0 siblings, 1 reply; 20+ messages in thread
From: Damien Lespiau @ 2013-09-13 13:50 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, Paulo Zanoni
On Fri, Sep 13, 2013 at 04:41:42PM +0300, Jani Nikula wrote:
> On Fri, 13 Sep 2013, Damien Lespiau <damien.lespiau@intel.com> wrote:
> > On Fri, Sep 13, 2013 at 12:21:21PM +0300, Jani Nikula wrote:
> >> On Fri, 13 Sep 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >
> >> > So far we control all the reads an none of them exceeds the current
> >> > limit of 20 bytes, but we never think about this when reviewing
> >> > patches, so we may at some point in the future overflow the buffer.
> >> >
> >> > My initial patch just added a WARN in case we were about to overflow
> >> > the buffer, but Chris suggested to make the size of the array dynamic.
> >>
> >> VLA on stack in kernel gives me an uneasy feeling, but I'll survive.
> >
> > IIRC sparse will complain about this. Might worth double checking.
>
> True.
>
> CHECK drivers/gpu/drm/i915/intel_dp.c
> drivers/gpu/drm/i915/intel_dp.c:567:34: error: bad constant expression
Also,
https://lkml.org/lkml/2013/3/8/486
--
Damein
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] drm/i915: fix intel_dp_aux_native_read's reply array size
2013-09-13 13:50 ` Damien Lespiau
@ 2013-09-16 19:26 ` Paulo Zanoni
0 siblings, 0 replies; 20+ messages in thread
From: Paulo Zanoni @ 2013-09-16 19:26 UTC (permalink / raw)
To: Damien Lespiau; +Cc: Intel Graphics Development, Paulo Zanoni
2013/9/13 Damien Lespiau <damien.lespiau@intel.com>:
> On Fri, Sep 13, 2013 at 04:41:42PM +0300, Jani Nikula wrote:
>> On Fri, 13 Sep 2013, Damien Lespiau <damien.lespiau@intel.com> wrote:
>> > On Fri, Sep 13, 2013 at 12:21:21PM +0300, Jani Nikula wrote:
>> >> On Fri, 13 Sep 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
>> >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> >
>> >> > So far we control all the reads an none of them exceeds the current
>> >> > limit of 20 bytes, but we never think about this when reviewing
>> >> > patches, so we may at some point in the future overflow the buffer.
>> >> >
>> >> > My initial patch just added a WARN in case we were about to overflow
>> >> > the buffer, but Chris suggested to make the size of the array dynamic.
>> >>
>> >> VLA on stack in kernel gives me an uneasy feeling, but I'll survive.
>> >
>> > IIRC sparse will complain about this. Might worth double checking.
>>
>> True.
>>
>> CHECK drivers/gpu/drm/i915/intel_dp.c
>> drivers/gpu/drm/i915/intel_dp.c:567:34: error: bad constant expression
>
> Also,
>
> https://lkml.org/lkml/2013/3/8/486
I just realized 20 bytes is really the maximum since we only have 5
data registers, so I'll bring back v1 of the patch, but with 2
additional checks: one spotted by Jani and one in intel_dp_aux_ch.
>
> --
> Damein
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/5] drm/i915: check for more ASLC interrupts
2013-09-12 16:58 [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+ Paulo Zanoni
2013-09-12 16:58 ` [PATCH 2/5] drm/i915: WARN if the DP aux read is too big Paulo Zanoni
@ 2013-09-12 16:58 ` Paulo Zanoni
2013-09-13 10:23 ` Jani Nikula
2013-09-12 16:58 ` [PATCH 4/5] drm/i915: clear opregon->lid_state after we unmap it Paulo Zanoni
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Paulo Zanoni @ 2013-09-12 16:58 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Sometimes I see the "non asle set request??" message on my Haswell
machine, so I decided to get the spec and see if some bits are missing
from the mask. We do have some bits missing from the mask, so this
patch adds them. But I still see the "non asle set request??" message
on my machine :(
Also use the proper ASLC name to indicate the registers we're talking
about.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_opregion.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index c4fb2ae..a01e0f8 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -117,12 +117,17 @@ struct opregion_asle {
#define ASLE_ARDY_READY (1 << 0)
#define ASLE_ARDY_NOT_READY (0 << 0)
-/* ASLE irq request bits */
-#define ASLE_SET_ALS_ILLUM (1 << 0)
-#define ASLE_SET_BACKLIGHT (1 << 1)
-#define ASLE_SET_PFIT (1 << 2)
-#define ASLE_SET_PWM_FREQ (1 << 3)
-#define ASLE_REQ_MSK 0xf
+/* ASLE Interrupt Command (ASLC) bits */
+#define ASLC_SET_ALS_ILLUM (1 << 0)
+#define ASLC_SET_BACKLIGHT (1 << 1)
+#define ASLC_SET_PFIT (1 << 2)
+#define ASLC_SET_PWM_FREQ (1 << 3)
+#define ASLC_SUPPORTED_ROTATION_ANGLES (1 << 4)
+#define ASLC_BUTTON_ARRAY (1 << 5)
+#define ASLC_CONVERTIBLE_INDICATOR (1 << 6)
+#define ASLC_DOCKING_INDICATOR (1 << 7)
+#define ASLC_ISCT_STATE_CHANGE (1 << 8)
+#define ASLC_REQ_MSK 0x1ff
/* response bits of ASLE irq request */
#define ASLE_ALS_ILLUM_FAILED (1<<10)
@@ -421,25 +426,31 @@ void intel_opregion_asle_intr(struct drm_device *dev)
if (!asle)
return;
- asle_req = ioread32(&asle->aslc) & ASLE_REQ_MSK;
+ asle_req = ioread32(&asle->aslc) & ASLC_REQ_MSK;
if (!asle_req) {
DRM_DEBUG_DRIVER("non asle set request??\n");
return;
}
- if (asle_req & ASLE_SET_ALS_ILLUM)
+ if (asle_req & ASLC_SET_ALS_ILLUM)
asle_stat |= asle_set_als_illum(dev, ioread32(&asle->alsi));
- if (asle_req & ASLE_SET_BACKLIGHT)
+ if (asle_req & ASLC_SET_BACKLIGHT)
asle_stat |= asle_set_backlight(dev, ioread32(&asle->bclp));
- if (asle_req & ASLE_SET_PFIT)
+ if (asle_req & ASLC_SET_PFIT)
asle_stat |= asle_set_pfit(dev, ioread32(&asle->pfit));
- if (asle_req & ASLE_SET_PWM_FREQ)
+ if (asle_req & ASLC_SET_PWM_FREQ)
asle_stat |= asle_set_pwm_freq(dev, ioread32(&asle->pfmb));
+ if (asle_req & (ASLC_SUPPORTED_ROTATION_ANGLES | ASLC_BUTTON_ARRAY |
+ ASLC_CONVERTIBLE_INDICATOR | ASLC_DOCKING_INDICATOR |
+ ASLC_ISCT_STATE_CHANGE)) {
+ DRM_DEBUG_DRIVER("ASLC interrupt not supported\n");
+ }
+
iowrite32(asle_stat, &asle->aslc);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/5] drm/i915: check for more ASLC interrupts
2013-09-12 16:58 ` [PATCH 3/5] drm/i915: check for more ASLC interrupts Paulo Zanoni
@ 2013-09-13 10:23 ` Jani Nikula
0 siblings, 0 replies; 20+ messages in thread
From: Jani Nikula @ 2013-09-13 10:23 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On Thu, 12 Sep 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Sometimes I see the "non asle set request??" message on my Haswell
> machine, so I decided to get the spec and see if some bits are missing
> from the mask. We do have some bits missing from the mask, so this
> patch adds them. But I still see the "non asle set request??" message
> on my machine :(
>
> Also use the proper ASLC name to indicate the registers we're talking
> about.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_opregion.c | 33 ++++++++++++++++++++++-----------
> 1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index c4fb2ae..a01e0f8 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -117,12 +117,17 @@ struct opregion_asle {
> #define ASLE_ARDY_READY (1 << 0)
> #define ASLE_ARDY_NOT_READY (0 << 0)
>
> -/* ASLE irq request bits */
> -#define ASLE_SET_ALS_ILLUM (1 << 0)
> -#define ASLE_SET_BACKLIGHT (1 << 1)
> -#define ASLE_SET_PFIT (1 << 2)
> -#define ASLE_SET_PWM_FREQ (1 << 3)
> -#define ASLE_REQ_MSK 0xf
> +/* ASLE Interrupt Command (ASLC) bits */
> +#define ASLC_SET_ALS_ILLUM (1 << 0)
> +#define ASLC_SET_BACKLIGHT (1 << 1)
> +#define ASLC_SET_PFIT (1 << 2)
> +#define ASLC_SET_PWM_FREQ (1 << 3)
> +#define ASLC_SUPPORTED_ROTATION_ANGLES (1 << 4)
> +#define ASLC_BUTTON_ARRAY (1 << 5)
> +#define ASLC_CONVERTIBLE_INDICATOR (1 << 6)
> +#define ASLC_DOCKING_INDICATOR (1 << 7)
> +#define ASLC_ISCT_STATE_CHANGE (1 << 8)
> +#define ASLC_REQ_MSK 0x1ff
While at it, please also s/ASLE/ASLC/ on the response bits, and add the
correspoinding response defines for the added bits above.
> /* response bits of ASLE irq request */
> #define ASLE_ALS_ILLUM_FAILED (1<<10)
> @@ -421,25 +426,31 @@ void intel_opregion_asle_intr(struct drm_device *dev)
> if (!asle)
> return;
>
> - asle_req = ioread32(&asle->aslc) & ASLE_REQ_MSK;
> + asle_req = ioread32(&asle->aslc) & ASLC_REQ_MSK;
>
> if (!asle_req) {
> DRM_DEBUG_DRIVER("non asle set request??\n");
Maybe we should log the original ioread32(&asle->aslc) in this case.
> return;
> }
>
> - if (asle_req & ASLE_SET_ALS_ILLUM)
> + if (asle_req & ASLC_SET_ALS_ILLUM)
> asle_stat |= asle_set_als_illum(dev, ioread32(&asle->alsi));
>
> - if (asle_req & ASLE_SET_BACKLIGHT)
> + if (asle_req & ASLC_SET_BACKLIGHT)
> asle_stat |= asle_set_backlight(dev, ioread32(&asle->bclp));
>
> - if (asle_req & ASLE_SET_PFIT)
> + if (asle_req & ASLC_SET_PFIT)
> asle_stat |= asle_set_pfit(dev, ioread32(&asle->pfit));
>
> - if (asle_req & ASLE_SET_PWM_FREQ)
> + if (asle_req & ASLC_SET_PWM_FREQ)
> asle_stat |= asle_set_pwm_freq(dev, ioread32(&asle->pfmb));
>
> + if (asle_req & (ASLC_SUPPORTED_ROTATION_ANGLES | ASLC_BUTTON_ARRAY |
> + ASLC_CONVERTIBLE_INDICATOR | ASLC_DOCKING_INDICATOR |
> + ASLC_ISCT_STATE_CHANGE)) {
> + DRM_DEBUG_DRIVER("ASLC interrupt not supported\n");
Should we report failure for these, similar to what everything other
than the backlight request do?
BR,
Jani.
> + }
> +
> iowrite32(asle_stat, &asle->aslc);
> }
>
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/5] drm/i915: clear opregon->lid_state after we unmap it
2013-09-12 16:58 [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+ Paulo Zanoni
2013-09-12 16:58 ` [PATCH 2/5] drm/i915: WARN if the DP aux read is too big Paulo Zanoni
2013-09-12 16:58 ` [PATCH 3/5] drm/i915: check for more ASLC interrupts Paulo Zanoni
@ 2013-09-12 16:58 ` Paulo Zanoni
2013-09-13 13:53 ` Rodrigo Vivi
2013-09-12 16:58 ` [PATCH 5/5] drm/i915: check for errors on i915_drm_thaw Paulo Zanoni
2013-09-12 17:36 ` [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+ Ville Syrjälä
4 siblings, 1 reply; 20+ messages in thread
From: Paulo Zanoni @ 2013-09-12 16:58 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
We don't seem to be using the pointer after it's unmapped, so this
patch doesn't fix any bug I can reproduce.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_opregion.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index a01e0f8..dd88c08 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -668,6 +668,7 @@ void intel_opregion_fini(struct drm_device *dev)
opregion->swsci = NULL;
opregion->asle = NULL;
opregion->vbt = NULL;
+ opregion->lid_state = NULL;
}
static void swsci_setup(struct drm_device *dev)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] drm/i915: clear opregon->lid_state after we unmap it
2013-09-12 16:58 ` [PATCH 4/5] drm/i915: clear opregon->lid_state after we unmap it Paulo Zanoni
@ 2013-09-13 13:53 ` Rodrigo Vivi
0 siblings, 0 replies; 20+ messages in thread
From: Rodrigo Vivi @ 2013-09-13 13:53 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
I think this is just the right thing to do, so:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
On Thu, Sep 12, 2013 at 1:58 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We don't seem to be using the pointer after it's unmapped, so this
> patch doesn't fix any bug I can reproduce.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_opregion.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index a01e0f8..dd88c08 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -668,6 +668,7 @@ void intel_opregion_fini(struct drm_device *dev)
> opregion->swsci = NULL;
> opregion->asle = NULL;
> opregion->vbt = NULL;
> + opregion->lid_state = NULL;
> }
>
> static void swsci_setup(struct drm_device *dev)
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/5] drm/i915: check for errors on i915_drm_thaw
2013-09-12 16:58 [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+ Paulo Zanoni
` (2 preceding siblings ...)
2013-09-12 16:58 ` [PATCH 4/5] drm/i915: clear opregon->lid_state after we unmap it Paulo Zanoni
@ 2013-09-12 16:58 ` Paulo Zanoni
2013-09-12 17:10 ` Chris Wilson
2013-09-12 17:36 ` [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+ Ville Syrjälä
4 siblings, 1 reply; 20+ messages in thread
From: Paulo Zanoni @ 2013-09-12 16:58 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Before this patch, i915_drm_thaw would always return 0. After this
patch, it returns the error code from __i915_drm_thaw(), since it's
the only sub-function that can return an error code.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec690ca..f6dc6b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -650,7 +650,7 @@ static int i915_drm_thaw(struct drm_device *dev)
mutex_unlock(&dev->struct_mutex);
}
- __i915_drm_thaw(dev);
+ error = __i915_drm_thaw(dev);
return error;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] drm/i915: check for errors on i915_drm_thaw
2013-09-12 16:58 ` [PATCH 5/5] drm/i915: check for errors on i915_drm_thaw Paulo Zanoni
@ 2013-09-12 17:10 ` Chris Wilson
2013-09-12 21:06 ` [PATCH 6/5] drm/i915: move more code to __i915_drm_thaw Paulo Zanoni
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2013-09-12 17:10 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Thu, Sep 12, 2013 at 01:58:21PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Before this patch, i915_drm_thaw would always return 0. After this
> patch, it returns the error code from __i915_drm_thaw(), since it's
> the only sub-function that can return an error code.
You could go a step further here and pass a parameter to
__i915_drm_thaw() to unify the two thaw paths.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/5] drm/i915: move more code to __i915_drm_thaw
2013-09-12 17:10 ` Chris Wilson
@ 2013-09-12 21:06 ` Paulo Zanoni
2013-09-12 21:44 ` Chris Wilson
0 siblings, 1 reply; 20+ messages in thread
From: Paulo Zanoni @ 2013-09-12 21:06 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Both callers had code to sanitize the uncore and restore the GTT
mappings just before calling __i915_drm_thaw, so Chris suggested I
should unify the code.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 39 ++++++++++++++-------------------------
1 file changed, 14 insertions(+), 25 deletions(-)
I'm not sure if this is really what Chris had in mind, but it's an idea...
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f6dc6b8..75e7550 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -576,11 +576,20 @@ static void intel_resume_hotplug(struct drm_device *dev)
drm_helper_hpd_irq_event(dev);
}
-static int __i915_drm_thaw(struct drm_device *dev)
+static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
{
struct drm_i915_private *dev_priv = dev->dev_private;
int error = 0;
+ intel_uncore_sanitize(dev);
+
+ if (drm_core_check_feature(dev, DRIVER_MODESET) &&
+ restore_gtt_mappings) {
+ mutex_lock(&dev->struct_mutex);
+ i915_gem_restore_gtt_mappings(dev);
+ mutex_unlock(&dev->struct_mutex);
+ }
+
i915_restore_state(dev);
intel_opregion_setup(dev);
@@ -640,19 +649,7 @@ static int __i915_drm_thaw(struct drm_device *dev)
static int i915_drm_thaw(struct drm_device *dev)
{
- int error = 0;
-
- intel_uncore_sanitize(dev);
-
- if (drm_core_check_feature(dev, DRIVER_MODESET)) {
- mutex_lock(&dev->struct_mutex);
- i915_gem_restore_gtt_mappings(dev);
- mutex_unlock(&dev->struct_mutex);
- }
-
- error = __i915_drm_thaw(dev);
-
- return error;
+ return __i915_drm_thaw(dev, true);
}
int i915_resume(struct drm_device *dev)
@@ -668,20 +665,12 @@ int i915_resume(struct drm_device *dev)
pci_set_master(dev->pdev);
- intel_uncore_sanitize(dev);
-
/*
* Platforms with opregion should have sane BIOS, older ones (gen3 and
- * earlier) need this since the BIOS might clear all our scratch PTEs.
+ * earlier) need to restore the GTT mappings since the BIOS might clear
+ * all our scratch PTEs.
*/
- if (drm_core_check_feature(dev, DRIVER_MODESET) &&
- !dev_priv->opregion.header) {
- mutex_lock(&dev->struct_mutex);
- i915_gem_restore_gtt_mappings(dev);
- mutex_unlock(&dev->struct_mutex);
- }
-
- ret = __i915_drm_thaw(dev);
+ ret = __i915_drm_thaw(dev, !dev_priv->opregion.header);
if (ret)
return ret;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 6/5] drm/i915: move more code to __i915_drm_thaw
2013-09-12 21:06 ` [PATCH 6/5] drm/i915: move more code to __i915_drm_thaw Paulo Zanoni
@ 2013-09-12 21:44 ` Chris Wilson
2013-09-13 9:40 ` Daniel Vetter
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2013-09-12 21:44 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Thu, Sep 12, 2013 at 06:06:43PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Both callers had code to sanitize the uncore and restore the GTT
> mappings just before calling __i915_drm_thaw, so Chris suggested I
> should unify the code.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Slightly more tricky to prove would be that neither i915_restore_state()
nor intel_opregion_setup depends upon the GTT (when modesetting is
enabled) and so coalesce the check+lock into the main MODESET block.
I think it's okay, there is a general ordering issue where we may point
a few registers at the GTT before it is set, but I don't think they are
used until afterwards. However, it is uncertain enough that I wouldn't
reorder the code without the set of init flags Daniel keeps muttering on
about to assert that we don't use partially restored state.
If Jesse wants to shave off a couple of microseconds from the resume
time by doing so, he is more than welcome. :)
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] drm/i915: move more code to __i915_drm_thaw
2013-09-12 21:44 ` Chris Wilson
@ 2013-09-13 9:40 ` Daniel Vetter
0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2013-09-13 9:40 UTC (permalink / raw)
To: Chris Wilson, Paulo Zanoni, intel-gfx, Paulo Zanoni
On Thu, Sep 12, 2013 at 10:44:39PM +0100, Chris Wilson wrote:
> On Thu, Sep 12, 2013 at 06:06:43PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Both callers had code to sanitize the uncore and restore the GTT
> > mappings just before calling __i915_drm_thaw, so Chris suggested I
> > should unify the code.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Slightly more tricky to prove would be that neither i915_restore_state()
> nor intel_opregion_setup depends upon the GTT (when modesetting is
> enabled) and so coalesce the check+lock into the main MODESET block.
> I think it's okay, there is a general ordering issue where we may point
> a few registers at the GTT before it is set, but I don't think they are
> used until afterwards. However, it is uncertain enough that I wouldn't
> reorder the code without the set of init flags Daniel keeps muttering on
> about to assert that we don't use partially restored state.
>
> If Jesse wants to shave off a couple of microseconds from the resume
> time by doing so, he is more than welcome. :)
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+
2013-09-12 16:58 [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+ Paulo Zanoni
` (3 preceding siblings ...)
2013-09-12 16:58 ` [PATCH 5/5] drm/i915: check for errors on i915_drm_thaw Paulo Zanoni
@ 2013-09-12 17:36 ` Ville Syrjälä
2013-09-13 9:41 ` Daniel Vetter
4 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2013-09-12 17:36 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Thu, Sep 12, 2013 at 01:58:17PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Because this PCI config register doesn't exist on Gen5+.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Yep. Can't see it in configdb. Actually it's only listed there for CTG
and CLN, which I guess makes sense as non-mobile platforms probably
don't have backlights. For BLB and ELK it's listed as reserved and RO,
so I guess it doesn't hurt to poke it on them. If I'm wrong we could
probably slap on an IS_MOBILE() check there too. Older stuff doesn't
exist in configdb, so that's as far back as I can go.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_suspend.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 70db618..3538370 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -340,7 +340,9 @@ int i915_save_state(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> int i;
>
> - pci_read_config_byte(dev->pdev, LBB, &dev_priv->regfile.saveLBB);
> + if (INTEL_INFO(dev)->gen <= 4)
> + pci_read_config_byte(dev->pdev, LBB,
> + &dev_priv->regfile.saveLBB);
>
> mutex_lock(&dev->struct_mutex);
>
> @@ -390,7 +392,9 @@ int i915_restore_state(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> int i;
>
> - pci_write_config_byte(dev->pdev, LBB, dev_priv->regfile.saveLBB);
> + if (INTEL_INFO(dev)->gen <= 4)
> + pci_write_config_byte(dev->pdev, LBB,
> + dev_priv->regfile.saveLBB);
>
> mutex_lock(&dev->struct_mutex);
>
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+
2013-09-12 17:36 ` [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+ Ville Syrjälä
@ 2013-09-13 9:41 ` Daniel Vetter
0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2013-09-13 9:41 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni
On Thu, Sep 12, 2013 at 08:36:18PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 12, 2013 at 01:58:17PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Because this PCI config register doesn't exist on Gen5+.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Yep. Can't see it in configdb. Actually it's only listed there for CTG
> and CLN, which I guess makes sense as non-mobile platforms probably
> don't have backlights. For BLB and ELK it's listed as reserved and RO,
> so I guess it doesn't hurt to poke it on them. If I'm wrong we could
> probably slap on an IS_MOBILE() check there too. Older stuff doesn't
> exist in configdb, so that's as far back as I can go.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 20+ messages in thread