| Message ID | 20161225101928.7618-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 25 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: > If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may > use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation > for a vbt in mailbox #4 for this. > > This fixes the driver not finding the VBT on a jumper ezpad mini3 > cherrytrail tablet. Thanks for the patch. I think you're onto something, but I don't think the patch is quite correct. That said, I'm not sure myself yet what would be. ;) Without the change, does intel_bios_is_valid_vbt() return true anyway? I.e. do you get "Found valid VBT in ACPI OpRegion (Mailbox #4)\n" in log? If not, which of the debug messages in intel_bios_is_valid_vbt() do you get? In the latter case, I suspect you'll end up with failure in intel_bios.c with either "No MIPI config BDB found" or "No MIPI Sequence found, parsing complete\n". BR, Jani. > > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note even with this fixed the panel still does not work with 4.9, > but it does with drm-intel-next-queued :) I believe the missing bit in > 4.9 is the "drm/915: Parsing the missed out DTD fields from the VBT" > commit, but I've not verified this. > --- > drivers/gpu/drm/i915/intel_opregion.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c > index f4429f6..eff35ae 100644 > --- a/drivers/gpu/drm/i915/intel_opregion.c > +++ b/drivers/gpu/drm/i915/intel_opregion.c > @@ -982,7 +982,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) > opregion->vbt_size = vbt_size; > } else { > vbt = base + OPREGION_VBT_OFFSET; > - vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; > + vbt_size = (mboxes & MBOX_ASLE_EXT) ? > + OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE; > + vbt_size -= OPREGION_VBT_OFFSET; > if (intel_bios_is_valid_vbt(vbt, vbt_size)) { > DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n"); > opregion->vbt = vbt;
Hi, On 27-12-16 11:58, Jani Nikula wrote: > On Sun, 25 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: >> If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may >> use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation >> for a vbt in mailbox #4 for this. >> >> This fixes the driver not finding the VBT on a jumper ezpad mini3 >> cherrytrail tablet. > > Thanks for the patch. I think you're onto something, but I don't think > the patch is quite correct. That said, I'm not sure myself yet what > would be. ;) > > Without the change, does intel_bios_is_valid_vbt() return true anyway? No. > I.e. do you get "Found valid VBT in ACPI OpRegion (Mailbox #4)\n" in > log? No. > If not, which of the debug messages in intel_bios_is_valid_vbt() do > you get? I get "BDB incomplete", which is why I wrote this patch and believe this patch is the right solution. With this patch everything works, > In the latter case, I suspect you'll end up with failure in intel_bios.c > with either "No MIPI config BDB found" or "No MIPI Sequence found, > parsing complete\n". I don't remember the exact error, other then getting the "BDB incomplete" error, and the i915 driver not listing the DSI connector under /sys/class drm. What makes you say: "but I don't think the patch is quite correct" why should the code still keep the OPREGION_ASLE_EXT start as end of the mailbox #4 vbt if there is the ASLE extenstion is not used ? Regards, Hans >> Cc: stable@vger.kernel.org >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Note even with this fixed the panel still does not work with 4.9, >> but it does with drm-intel-next-queued :) I believe the missing bit in >> 4.9 is the "drm/915: Parsing the missed out DTD fields from the VBT" >> commit, but I've not verified this. >> --- >> drivers/gpu/drm/i915/intel_opregion.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c >> index f4429f6..eff35ae 100644 >> --- a/drivers/gpu/drm/i915/intel_opregion.c >> +++ b/drivers/gpu/drm/i915/intel_opregion.c >> @@ -982,7 +982,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >> opregion->vbt_size = vbt_size; >> } else { >> vbt = base + OPREGION_VBT_OFFSET; >> - vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; >> + vbt_size = (mboxes & MBOX_ASLE_EXT) ? >> + OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE; >> + vbt_size -= OPREGION_VBT_OFFSET; >> if (intel_bios_is_valid_vbt(vbt, vbt_size)) { >> DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n"); >> opregion->vbt = vbt; >
Hi, On 31-12-16 17:00, Hans de Goede wrote: > Hi, > > On 27-12-16 11:58, Jani Nikula wrote: >> On Sun, 25 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: >>> If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may >>> use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation >>> for a vbt in mailbox #4 for this. >>> >>> This fixes the driver not finding the VBT on a jumper ezpad mini3 >>> cherrytrail tablet. >> >> Thanks for the patch. I think you're onto something, but I don't think >> the patch is quite correct. That said, I'm not sure myself yet what >> would be. ;) >> >> Without the change, does intel_bios_is_valid_vbt() return true anyway? > > No. > >> I.e. do you get "Found valid VBT in ACPI OpRegion (Mailbox #4)\n" in >> log? > > No. > >> If not, which of the debug messages in intel_bios_is_valid_vbt() do >> you get? > > I get "BDB incomplete", which is why I wrote this patch and believe > this patch is the right solution. With this patch everything works, > >> In the latter case, I suspect you'll end up with failure in intel_bios.c >> with either "No MIPI config BDB found" or "No MIPI Sequence found, >> parsing complete\n". > > I don't remember the exact error, other then getting the > "BDB incomplete" error, and the i915 driver not listing the DSI connector > under /sys/class drm. > > What makes you say: "but I don't think the patch is quite correct" why > should the code still keep the OPREGION_ASLE_EXT start as end of the > mailbox #4 vbt if there is the ASLE extenstion is not used ? Ping, any progress on this ? I still believe my original patch is correct. Eitherway I would like to see a fix for this, be it this fix or something else, as a fix is necessary to get the LCD panel to work on jumper ezpad mini3 tablets. Regards, Hans >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> Note even with this fixed the panel still does not work with 4.9, >>> but it does with drm-intel-next-queued :) I believe the missing bit in >>> 4.9 is the "drm/915: Parsing the missed out DTD fields from the VBT" >>> commit, but I've not verified this. >>> --- >>> drivers/gpu/drm/i915/intel_opregion.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c >>> index f4429f6..eff35ae 100644 >>> --- a/drivers/gpu/drm/i915/intel_opregion.c >>> +++ b/drivers/gpu/drm/i915/intel_opregion.c >>> @@ -982,7 +982,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >>> opregion->vbt_size = vbt_size; >>> } else { >>> vbt = base + OPREGION_VBT_OFFSET; >>> - vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; >>> + vbt_size = (mboxes & MBOX_ASLE_EXT) ? >>> + OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE; >>> + vbt_size -= OPREGION_VBT_OFFSET; >>> if (intel_bios_is_valid_vbt(vbt, vbt_size)) { >>> DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n"); >>> opregion->vbt = vbt; >>
On Fri, 20 Jan 2017, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 31-12-16 17:00, Hans de Goede wrote: >> Hi, >> >> On 27-12-16 11:58, Jani Nikula wrote: >>> On Sun, 25 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: >>>> If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may >>>> use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation >>>> for a vbt in mailbox #4 for this. >>>> >>>> This fixes the driver not finding the VBT on a jumper ezpad mini3 >>>> cherrytrail tablet. >>> >>> Thanks for the patch. I think you're onto something, but I don't think >>> the patch is quite correct. That said, I'm not sure myself yet what >>> would be. ;) >>> >>> Without the change, does intel_bios_is_valid_vbt() return true anyway? >> >> No. >> >>> I.e. do you get "Found valid VBT in ACPI OpRegion (Mailbox #4)\n" in >>> log? >> >> No. >> >>> If not, which of the debug messages in intel_bios_is_valid_vbt() do >>> you get? >> >> I get "BDB incomplete", which is why I wrote this patch and believe >> this patch is the right solution. With this patch everything works, >> >>> In the latter case, I suspect you'll end up with failure in intel_bios.c >>> with either "No MIPI config BDB found" or "No MIPI Sequence found, >>> parsing complete\n". >> >> I don't remember the exact error, other then getting the >> "BDB incomplete" error, and the i915 driver not listing the DSI connector >> under /sys/class drm. >> >> What makes you say: "but I don't think the patch is quite correct" why >> should the code still keep the OPREGION_ASLE_EXT start as end of the >> mailbox #4 vbt if there is the ASLE extenstion is not used ? > > Ping, any progress on this ? I still believe my original patch is > correct. Eitherway I would like to see a fix for this, be it this fix > or something else, as a fix is necessary to get the LCD panel to work > on jumper ezpad mini3 tablets. I'm sorry. Going through the opregion specs is not something I cherish, so I've been postponing this... I'll try to check later today. BR, Jani. > > Regards, > > Hans > > > >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> Note even with this fixed the panel still does not work with 4.9, >>>> but it does with drm-intel-next-queued :) I believe the missing bit in >>>> 4.9 is the "drm/915: Parsing the missed out DTD fields from the VBT" >>>> commit, but I've not verified this. >>>> --- >>>> drivers/gpu/drm/i915/intel_opregion.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c >>>> index f4429f6..eff35ae 100644 >>>> --- a/drivers/gpu/drm/i915/intel_opregion.c >>>> +++ b/drivers/gpu/drm/i915/intel_opregion.c >>>> @@ -982,7 +982,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >>>> opregion->vbt_size = vbt_size; >>>> } else { >>>> vbt = base + OPREGION_VBT_OFFSET; >>>> - vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; >>>> + vbt_size = (mboxes & MBOX_ASLE_EXT) ? >>>> + OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE; >>>> + vbt_size -= OPREGION_VBT_OFFSET; >>>> if (intel_bios_is_valid_vbt(vbt, vbt_size)) { >>>> DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n"); >>>> opregion->vbt = vbt; >>>
On Fri, 20 Jan 2017, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 31-12-16 17:00, Hans de Goede wrote: >> Hi, >> >> On 27-12-16 11:58, Jani Nikula wrote: >>> On Sun, 25 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: >>>> If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may >>>> use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation >>>> for a vbt in mailbox #4 for this. >>>> >>>> This fixes the driver not finding the VBT on a jumper ezpad mini3 >>>> cherrytrail tablet. >>> >>> Thanks for the patch. I think you're onto something, but I don't think >>> the patch is quite correct. That said, I'm not sure myself yet what >>> would be. ;) >>> >>> Without the change, does intel_bios_is_valid_vbt() return true anyway? >> >> No. >> >>> I.e. do you get "Found valid VBT in ACPI OpRegion (Mailbox #4)\n" in >>> log? >> >> No. >> >>> If not, which of the debug messages in intel_bios_is_valid_vbt() do >>> you get? >> >> I get "BDB incomplete", which is why I wrote this patch and believe >> this patch is the right solution. With this patch everything works, >> >>> In the latter case, I suspect you'll end up with failure in intel_bios.c >>> with either "No MIPI config BDB found" or "No MIPI Sequence found, >>> parsing complete\n". >> >> I don't remember the exact error, other then getting the >> "BDB incomplete" error, and the i915 driver not listing the DSI connector >> under /sys/class drm. >> >> What makes you say: "but I don't think the patch is quite correct" why >> should the code still keep the OPREGION_ASLE_EXT start as end of the >> mailbox #4 vbt if there is the ASLE extenstion is not used ? > > Ping, any progress on this ? I still believe my original patch is > correct. Eitherway I would like to see a fix for this, be it this fix > or something else, as a fix is necessary to get the LCD panel to work > on jumper ezpad mini3 tablets. Please send me /sys/kernel/debug/dri/0/i915_opregion on that machine. Perusing the opregion spec (which I regret I can't share with you), I found this: * On mailboxes in general, "Mail-box locations are fixed and should always be allocated irrespective of the support for a given mail box is available or not." * On opregion->asle->rvda, "This is mainly used when VBT size exceeds 6KB and can't be stored in Mailbox4." It isn't clear to me whether ->rvda was used or not. The opregion dump should shed light on this. You can of course check that by adding debug prints in the code too. * On mailbox 4 (the VBT), "Holds a maximum of 6KB sized Raw VBT data (not VBIOS image) from VBIOS image." Clearly the patch is against the spec. Let's see if the opregion you have there is against the spec too, and proceed from there... BR, Jani. > > Regards, > > Hans > > > >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> Note even with this fixed the panel still does not work with 4.9, >>>> but it does with drm-intel-next-queued :) I believe the missing bit in >>>> 4.9 is the "drm/915: Parsing the missed out DTD fields from the VBT" >>>> commit, but I've not verified this. >>>> --- >>>> drivers/gpu/drm/i915/intel_opregion.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c >>>> index f4429f6..eff35ae 100644 >>>> --- a/drivers/gpu/drm/i915/intel_opregion.c >>>> +++ b/drivers/gpu/drm/i915/intel_opregion.c >>>> @@ -982,7 +982,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >>>> opregion->vbt_size = vbt_size; >>>> } else { >>>> vbt = base + OPREGION_VBT_OFFSET; >>>> - vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; >>>> + vbt_size = (mboxes & MBOX_ASLE_EXT) ? >>>> + OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE; >>>> + vbt_size -= OPREGION_VBT_OFFSET; >>>> if (intel_bios_is_valid_vbt(vbt, vbt_size)) { >>>> DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n"); >>>> opregion->vbt = vbt; >>>
Hi, On 23-01-17 11:36, Jani Nikula wrote: > On Fri, 20 Jan 2017, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 31-12-16 17:00, Hans de Goede wrote: >>> Hi, >>> >>> On 27-12-16 11:58, Jani Nikula wrote: >>>> On Sun, 25 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may >>>>> use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation >>>>> for a vbt in mailbox #4 for this. >>>>> >>>>> This fixes the driver not finding the VBT on a jumper ezpad mini3 >>>>> cherrytrail tablet. >>>> >>>> Thanks for the patch. I think you're onto something, but I don't think >>>> the patch is quite correct. That said, I'm not sure myself yet what >>>> would be. ;) >>>> >>>> Without the change, does intel_bios_is_valid_vbt() return true anyway? >>> >>> No. >>> >>>> I.e. do you get "Found valid VBT in ACPI OpRegion (Mailbox #4)\n" in >>>> log? >>> >>> No. >>> >>>> If not, which of the debug messages in intel_bios_is_valid_vbt() do >>>> you get? >>> >>> I get "BDB incomplete", which is why I wrote this patch and believe >>> this patch is the right solution. With this patch everything works, >>> >>>> In the latter case, I suspect you'll end up with failure in intel_bios.c >>>> with either "No MIPI config BDB found" or "No MIPI Sequence found, >>>> parsing complete\n". >>> >>> I don't remember the exact error, other then getting the >>> "BDB incomplete" error, and the i915 driver not listing the DSI connector >>> under /sys/class drm. >>> >>> What makes you say: "but I don't think the patch is quite correct" why >>> should the code still keep the OPREGION_ASLE_EXT start as end of the >>> mailbox #4 vbt if there is the ASLE extenstion is not used ? >> >> Ping, any progress on this ? I still believe my original patch is >> correct. Eitherway I would like to see a fix for this, be it this fix >> or something else, as a fix is necessary to get the LCD panel to work >> on jumper ezpad mini3 tablets. > > Please send me /sys/kernel/debug/dri/0/i915_opregion on that machine. > > Perusing the opregion spec (which I regret I can't share with you), I > found this: > > * On mailboxes in general, "Mail-box locations are fixed and should > always be allocated irrespective of the support for a given mail box > is available or not." > > * On opregion->asle->rvda, "This is mainly used when VBT size exceeds > 6KB and can't be stored in Mailbox4." It isn't clear to me whether > ->rvda was used or not. The opregion dump should shed light on > this. You can of course check that by adding debug prints in the code > too. > > * On mailbox 4 (the VBT), "Holds a maximum of 6KB sized Raw VBT data > (not VBIOS image) from VBIOS image." > > Clearly the patch is against the spec. Let's see if the opregion you > have there is against the spec too, and proceed from there... Ok, I've attached a dump of the opregion on the tablet in question. Regards, Hans
On Mon, 23 Jan 2017, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 23-01-17 11:36, Jani Nikula wrote: >> On Fri, 20 Jan 2017, Hans de Goede <hdegoede@redhat.com> wrote: >>> Hi, >>> >>> On 31-12-16 17:00, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 27-12-16 11:58, Jani Nikula wrote: >>>>> On Sun, 25 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>> If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may >>>>>> use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation >>>>>> for a vbt in mailbox #4 for this. >>>>>> >>>>>> This fixes the driver not finding the VBT on a jumper ezpad mini3 >>>>>> cherrytrail tablet. >>>>> >>>>> Thanks for the patch. I think you're onto something, but I don't think >>>>> the patch is quite correct. That said, I'm not sure myself yet what >>>>> would be. ;) >>>>> >>>>> Without the change, does intel_bios_is_valid_vbt() return true anyway? >>>> >>>> No. >>>> >>>>> I.e. do you get "Found valid VBT in ACPI OpRegion (Mailbox #4)\n" in >>>>> log? >>>> >>>> No. >>>> >>>>> If not, which of the debug messages in intel_bios_is_valid_vbt() do >>>>> you get? >>>> >>>> I get "BDB incomplete", which is why I wrote this patch and believe >>>> this patch is the right solution. With this patch everything works, >>>> >>>>> In the latter case, I suspect you'll end up with failure in intel_bios.c >>>>> with either "No MIPI config BDB found" or "No MIPI Sequence found, >>>>> parsing complete\n". >>>> >>>> I don't remember the exact error, other then getting the >>>> "BDB incomplete" error, and the i915 driver not listing the DSI connector >>>> under /sys/class drm. >>>> >>>> What makes you say: "but I don't think the patch is quite correct" why >>>> should the code still keep the OPREGION_ASLE_EXT start as end of the >>>> mailbox #4 vbt if there is the ASLE extenstion is not used ? >>> >>> Ping, any progress on this ? I still believe my original patch is >>> correct. Eitherway I would like to see a fix for this, be it this fix >>> or something else, as a fix is necessary to get the LCD panel to work >>> on jumper ezpad mini3 tablets. >> >> Please send me /sys/kernel/debug/dri/0/i915_opregion on that machine. >> >> Perusing the opregion spec (which I regret I can't share with you), I >> found this: >> >> * On mailboxes in general, "Mail-box locations are fixed and should >> always be allocated irrespective of the support for a given mail box >> is available or not." >> >> * On opregion->asle->rvda, "This is mainly used when VBT size exceeds >> 6KB and can't be stored in Mailbox4." It isn't clear to me whether >> ->rvda was used or not. The opregion dump should shed light on >> this. You can of course check that by adding debug prints in the code >> too. >> >> * On mailbox 4 (the VBT), "Holds a maximum of 6KB sized Raw VBT data >> (not VBIOS image) from VBIOS image." >> >> Clearly the patch is against the spec. Let's see if the opregion you >> have there is against the spec too, and proceed from there... > > Ok, I've attached a dump of the opregion on the tablet in question. Thanks. rvda is not used, VBT is in in the mailbox, and reports size 0x1843, i.e. 67 bytes over the mailbox size of 6 kB. *facepalm*. I'll try to ask around to see if someone knows what they were thinking. BR, Jani. > > Regards, > > Hans >
Hi Jani, So any further update on this ? I've just got a report from an user that my fix also fixes the LCD panel on his (different model, ACER SW5_017) machine not lighting up: https://bugzilla.kernel.org/show_bug.cgi?id=155241#c56 Although my fix my be in contradiction with the VBT spec, I do not see how it can really hurt, it just allows the VBT to extend into an unused part of the opregion. Something which windows 10 apparently allows, otherwise we would not have multiple models shipping with this problem, so it seems best to just allow this in Linux too. Regards, Hans
On Sat, 04 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote: > Hi Jani, > > So any further update on this ? I've just got a report from > an user that my fix also fixes the LCD panel on his (different > model, ACER SW5_017) machine not lighting up: > > https://bugzilla.kernel.org/show_bug.cgi?id=155241#c56 > > Although my fix my be in contradiction with the VBT spec, > I do not see how it can really hurt, it just allows the > VBT to extend into an unused part of the opregion. > > Something which windows 10 apparently allows, otherwise > we would not have multiple models shipping with this > problem, so it seems best to just allow this in Linux > too. Suffice it to say I'm pretty frustrated with this. Let's get your patch merged, I'll reply to it with a couple of specific comments. BR, Jani.
On Sun, 25 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: > If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may > use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation > for a vbt in mailbox #4 for this. > > This fixes the driver not finding the VBT on a jumper ezpad mini3 > cherrytrail tablet. > > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note even with this fixed the panel still does not work with 4.9, > but it does with drm-intel-next-queued :) I believe the missing bit in > 4.9 is the "drm/915: Parsing the missed out DTD fields from the VBT" > commit, but I've not verified this. > --- > drivers/gpu/drm/i915/intel_opregion.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c > index f4429f6..eff35ae 100644 > --- a/drivers/gpu/drm/i915/intel_opregion.c > +++ b/drivers/gpu/drm/i915/intel_opregion.c > @@ -982,7 +982,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) > opregion->vbt_size = vbt_size; > } else { > vbt = base + OPREGION_VBT_OFFSET; > - vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; > + vbt_size = (mboxes & MBOX_ASLE_EXT) ? > + OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE; > + vbt_size -= OPREGION_VBT_OFFSET; First, I want a big fat warning comment about what's going on here. Otherwise someone's bound to "fix" this later on. Second, per the spec, the ASLE ext mailbox is 1k in size, and there's a 1k reserved region at the end. We probably shouldn't allow VBT to extend over there. But hey, per the spec we also shouldn't allow VBT to extend over mailbox #5 either. So if you can't be bothered with that, neither will I. BR, Jani. > if (intel_bios_is_valid_vbt(vbt, vbt_size)) { > DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n"); > opregion->vbt = vbt;
Hi, On 06-02-17 08:04, Jani Nikula wrote: > On Sun, 25 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: >> If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may >> use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation >> for a vbt in mailbox #4 for this. >> >> This fixes the driver not finding the VBT on a jumper ezpad mini3 >> cherrytrail tablet. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Note even with this fixed the panel still does not work with 4.9, >> but it does with drm-intel-next-queued :) I believe the missing bit in >> 4.9 is the "drm/915: Parsing the missed out DTD fields from the VBT" >> commit, but I've not verified this. >> --- >> drivers/gpu/drm/i915/intel_opregion.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c >> index f4429f6..eff35ae 100644 >> --- a/drivers/gpu/drm/i915/intel_opregion.c >> +++ b/drivers/gpu/drm/i915/intel_opregion.c >> @@ -982,7 +982,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >> opregion->vbt_size = vbt_size; >> } else { >> vbt = base + OPREGION_VBT_OFFSET; >> - vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; >> + vbt_size = (mboxes & MBOX_ASLE_EXT) ? >> + OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE; >> + vbt_size -= OPREGION_VBT_OFFSET; > > First, I want a big fat warning comment about what's going on > here. Otherwise someone's bound to "fix" this later on. Done for v2, which I will send shortly. > Second, per the spec, the ASLE ext mailbox is 1k in size, and there's a > 1k reserved region at the end. We probably shouldn't allow VBT to extend > over there. But hey, per the spec we also shouldn't allow VBT to extend > over mailbox #5 either. So if you can't be bothered with that, neither > will I. Hmm, that makes no sense, OPREGION_SIZE is 8192 bytes or 0x2000, OPREGION_ASLE_EXT_OFFSET is 0x1C00 or 7168 bytes, if there is 1k reserved after the ASLE ext mailbox then there is exactly 0 bytes available for the ASLE ext mailbox or OPREGION_SIZE should be 9216 not 8192 (which I don't think so). Maybe the last 1k is either the ASLE ext mailbox OR reserved ? Anyways leaving this bit as is for now. Regards, Hans
On Fri, 10 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote: > On 06-02-17 08:04, Jani Nikula wrote: >> First, I want a big fat warning comment about what's going on >> here. Otherwise someone's bound to "fix" this later on. > > Done for v2, which I will send shortly. Thanks. >> Second, per the spec, the ASLE ext mailbox is 1k in size, and there's a >> 1k reserved region at the end. We probably shouldn't allow VBT to extend >> over there. But hey, per the spec we also shouldn't allow VBT to extend >> over mailbox #5 either. So if you can't be bothered with that, neither >> will I. > > Hmm, that makes no sense, OPREGION_SIZE is 8192 bytes or 0x2000, > OPREGION_ASLE_EXT_OFFSET is 0x1C00 or 7168 bytes, if there is 1k > reserved after the ASLE ext mailbox then there is exactly 0 bytes > available for the ASLE ext mailbox or OPREGION_SIZE should be 9216 > not 8192 (which I don't think so). Maybe the last 1k is either > the ASLE ext mailbox OR reserved ? You're quite right, that makes no sense. :) Looks like we account for the reserved part at the end of struct opregion_asle_ext, but the spec isn't quite clear whether the reserved part is actually part of the mailbox or not. I'm not quite sure how I managed to confuse myself to turn that into 1k. Sorry. BR, Jani.
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index f4429f6..eff35ae 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -982,7 +982,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) opregion->vbt_size = vbt_size; } else { vbt = base + OPREGION_VBT_OFFSET; - vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; + vbt_size = (mboxes & MBOX_ASLE_EXT) ? + OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE; + vbt_size -= OPREGION_VBT_OFFSET; if (intel_bios_is_valid_vbt(vbt, vbt_size)) { DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n"); opregion->vbt = vbt;
If there is no OPREGION_ASLE_EXT then a VBT stored in mailbox #4 may use the ASLE_EXT parts of the opregion. Adjust the vbt_size calculation for a vbt in mailbox #4 for this. This fixes the driver not finding the VBT on a jumper ezpad mini3 cherrytrail tablet. Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Note even with this fixed the panel still does not work with 4.9, but it does with drm-intel-next-queued :) I believe the missing bit in 4.9 is the "drm/915: Parsing the missed out DTD fields from the VBT" commit, but I've not verified this. --- drivers/gpu/drm/i915/intel_opregion.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)