| Message ID | 1349978908-7687-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2012/10/11 Daniel Vetter <daniel.vetter@ffwll.ch>: > ... since they don't apply to pre-pch platforms and could actually be > harmful. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Ok, so I checked the specs and yes, these bit definitions don't exist. The problem here is that instead of "must-be-zero", the spec says "Reserved. Software must preserve the contents of these bits" for bits 29:16 (and also some others). So maybe by setting everything to 0 instead of enabling bits 17, 18, 20-23 we could actually be breaking things? Either way, both the old and new code don't follow the specification. Maybe on non-pch-split we could try to read ADPA and erase all the bits except the "must-be-preserved" ones? > --- > drivers/gpu/drm/i915/intel_crt.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > index c42b980..46c90f5 100644 > --- a/drivers/gpu/drm/i915/intel_crt.c > +++ b/drivers/gpu/drm/i915/intel_crt.c > @@ -235,7 +235,11 @@ static void intel_crt_mode_set(struct drm_encoder *encoder, > dpll_md & ~DPLL_MD_UDI_MULTIPLIER_MASK); > } > > - adpa = ADPA_HOTPLUG_BITS; > + if (HAS_PCH_SPLIT(dev)) > + adpa = ADPA_HOTPLUG_BITS; > + else > + adpa = 0; > + > if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC) > adpa |= ADPA_HSYNC_ACTIVE_HIGH; > if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC) > -- > 1.7.11.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 12, 2012 at 4:47 AM, Paulo Zanoni <przanoni@gmail.com> wrote: > 2012/10/11 Daniel Vetter <daniel.vetter@ffwll.ch>: >> ... since they don't apply to pre-pch platforms and could actually be >> harmful. >> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Ok, so I checked the specs and yes, these bit definitions don't exist. > The problem here is that instead of "must-be-zero", the spec says > "Reserved. Software must preserve the contents of these bits" for bits > 29:16 (and also some others). So maybe by setting everything to 0 > instead of enabling bits 17, 18, 20-23 we could actually be breaking > things? Either way, both the old and new code don't follow the > specification. Indeed, I've overlooked the "must be preserved" wording, and it goes back to gen2 when the ADPA reg was added. So I've fired up all my gen2/gen3 machines, and they have all zeros in these registers. And we never write anything in there. I suspect this is simply a hint from the Bspec authors to driver writes that they might eventually use these reserved bits in future hw platforms. And if the driver preserves the bit settings, it will automatically work. I think MBZ is mostly used for bit ranges that have been used once, but are no longer implemented (e.g. bits 12:13 do something in gen2/3 but not on later hw gens). > Maybe on non-pch-split we could try to read ADPA and erase all the > bits except the "must-be-preserved" ones? See above, I think we can clear them - at least all my old hw seems to work perfectly well with all these bits being zero. -Daniel
2012/10/12 Daniel Vetter <daniel.vetter@ffwll.ch>: > On Fri, Oct 12, 2012 at 4:47 AM, Paulo Zanoni <przanoni@gmail.com> wrote: >> 2012/10/11 Daniel Vetter <daniel.vetter@ffwll.ch>: >>> ... since they don't apply to pre-pch platforms and could actually be >>> harmful. >>> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> Ok, so I checked the specs and yes, these bit definitions don't exist. >> The problem here is that instead of "must-be-zero", the spec says >> "Reserved. Software must preserve the contents of these bits" for bits >> 29:16 (and also some others). So maybe by setting everything to 0 >> instead of enabling bits 17, 18, 20-23 we could actually be breaking >> things? Either way, both the old and new code don't follow the >> specification. > > Indeed, I've overlooked the "must be preserved" wording, and it goes > back to gen2 when the ADPA reg was added. So I've fired up all my > gen2/gen3 machines, and they have all zeros in these registers. Ok, so please do a final test: try to write something to those "must-be-preserved" bits and check if the values stay or not. If after writing 1 to bits 17-18, 20-23 you read 0, then you have my Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>. I still do plan to test the 6 patches of the series on hsw later btw. > And we > never write anything in there. I suspect this is simply a hint from > the Bspec authors to driver writes that they might eventually use > these reserved bits in future hw platforms. And if the driver > preserves the bit settings, it will automatically work. I think MBZ is > mostly used for bit ranges that have been used once, but are no longer > implemented (e.g. bits 12:13 do something in gen2/3 but not on later > hw gens). > >> Maybe on non-pch-split we could try to read ADPA and erase all the >> bits except the "must-be-preserved" ones? > > See above, I think we can clear them - at least all my old hw seems to > work perfectly well with all these bits being zero. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Oct 12, 2012 at 7:17 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > Ok, so please do a final test: try to write something to those > "must-be-preserved" bits and check if the values stay or not. If after > writing 1 to bits 17-18, 20-23 you read 0, then you have my > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>. I still do plan > to test the 6 patches of the series on hsw later btw. I've tested a few bits on a few machines, and some do stick and some cause ... strange things (like a seemingly random set of other register bits also being set). I guess that's not the answer you've been looking for. I'd still prefer if we just try to clear things, worst case we can easily read out the current state of these reserved bits at boot up and restore them at resume time. But I'd really prefer if we reduce our reliance on the boot-up state from the bios as much as possible. And in this case here the only way to figure things out is to merge it (it's really early for 3.8 anyway) and see what happens. Cheers, Daniel
2012/10/12 Daniel Vetter <daniel.vetter@ffwll.ch>: > On Fri, Oct 12, 2012 at 7:17 PM, Paulo Zanoni <przanoni@gmail.com> wrote: >> Ok, so please do a final test: try to write something to those >> "must-be-preserved" bits and check if the values stay or not. If after >> writing 1 to bits 17-18, 20-23 you read 0, then you have my >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>. I still do plan >> to test the 6 patches of the series on hsw later btw. > > I've tested a few bits on a few machines, and some do stick and some > cause ... strange things (like a seemingly random set of other > register bits also being set). I guess that's not the answer you've > been looking for. I'd still prefer if we just try to clear things, > worst case we can easily read out the current state of these reserved > bits at boot up and restore them at resume time. But I'd really prefer > if we reduce our reliance on the boot-up state from the bios as much > as possible. And in this case here the only way to figure things out > is to merge it (it's really early for 3.8 anyway) and see what > happens. Well, ok then... Both the old and the new version don't exactly follow the spec, but I guess writing zero makes more sense than enabling random bits, especially because you said you checked and your machines actually have zeros on those bits. Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index c42b980..46c90f5 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -235,7 +235,11 @@ static void intel_crt_mode_set(struct drm_encoder *encoder, dpll_md & ~DPLL_MD_UDI_MULTIPLIER_MASK); } - adpa = ADPA_HOTPLUG_BITS; + if (HAS_PCH_SPLIT(dev)) + adpa = ADPA_HOTPLUG_BITS; + else + adpa = 0; + if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC) adpa |= ADPA_HSYNC_ACTIVE_HIGH; if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
... since they don't apply to pre-pch platforms and could actually be harmful. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_crt.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)