On Fri, Oct 2, 2015 at 3:35 AM, Hans de Goede <hdegoede at redhat.com> wrote:> Hi, > > On 02-10-15 09:26, Ilia Mirkin wrote: >> >> On Fri, Oct 2, 2015 at 3:18 AM, Hans de Goede <hdegoede at redhat.com> wrote: >>> >>> Hi, >>> >>> On 02-10-15 05:41, Ilia Mirkin wrote: >>> >>> <nothing> >>> >>> As someone who has recently started following nouveau I must say that >>> it would greatly help me (and likely others) if patches likes this would >>> come with a somewhat more descriptive commit message. >> >> >> Duly noted. I normally try to say a bit, but have gotten lazy of late. >> Ben's pretty terse too :) How about something like >> >> """ >> Currently OF bios load fails for a few reasons: >> - checksum failure >> - bios size too small >> - no PCIR header >> - bios length not a multiple of 4 >> >> In this change, we resolve all of the above by ignoring any checksum >> failures, and faking the PCIR data when loading from OF. >> """ > > > Much better, I must say that now that you spell it out that the > ignoring of the checksum sort of worries me, but I guess the > checksum is part of the missing PCIR header ?Nope, the two things are unrelated. The PCIR section is normally embedded in the vbios and tells you how long the vbios is, and allows you to have multiple elements appended at the end (which nouveau has recently acquired some support for). The checksum failures just happen, pretty sure that the OF-sourced vbioses just don't have a checksum byte at all. And they cause nouveau to try to load a vbios from a different source, which almost invariably fails to actually parse properly. (Why? Because you should just use the one from OF, duh!) -ilia
Hi, On 02-10-15 09:38, Ilia Mirkin wrote:> On Fri, Oct 2, 2015 at 3:35 AM, Hans de Goede <hdegoede at redhat.com> wrote: >> Hi, >> >> On 02-10-15 09:26, Ilia Mirkin wrote: >>> >>> On Fri, Oct 2, 2015 at 3:18 AM, Hans de Goede <hdegoede at redhat.com> wrote: >>>> >>>> Hi, >>>> >>>> On 02-10-15 05:41, Ilia Mirkin wrote: >>>> >>>> <nothing> >>>> >>>> As someone who has recently started following nouveau I must say that >>>> it would greatly help me (and likely others) if patches likes this would >>>> come with a somewhat more descriptive commit message. >>> >>> >>> Duly noted. I normally try to say a bit, but have gotten lazy of late. >>> Ben's pretty terse too :) How about something like >>> >>> """ >>> Currently OF bios load fails for a few reasons: >>> - checksum failure >>> - bios size too small >>> - no PCIR header >>> - bios length not a multiple of 4 >>> >>> In this change, we resolve all of the above by ignoring any checksum >>> failures, and faking the PCIR data when loading from OF. >>> """ >> >> >> Much better, I must say that now that you spell it out that the >> ignoring of the checksum sort of worries me, but I guess the >> checksum is part of the missing PCIR header ? > > Nope, the two things are unrelated. The PCIR section is normally > embedded in the vbios and tells you how long the vbios is, and allows > you to have multiple elements appended at the end (which nouveau has > recently acquired some support for). The checksum failures just > happen,> "pretty sure that the OF-sourced vbioses just don't have a > checksum byte at all."You may want to add something along those lines to the commit message too... Regards, Hans
On Fri, Oct 2, 2015 at 3:39 AM, Hans de Goede <hdegoede at redhat.com> wrote:> Hi, > > > On 02-10-15 09:38, Ilia Mirkin wrote: >> >> On Fri, Oct 2, 2015 at 3:35 AM, Hans de Goede <hdegoede at redhat.com> wrote: >>> >>> Hi, >>> >>> On 02-10-15 09:26, Ilia Mirkin wrote: >>>> >>>> >>>> On Fri, Oct 2, 2015 at 3:18 AM, Hans de Goede <hdegoede at redhat.com> >>>> wrote: >>>>> >>>>> >>>>> Hi, >>>>> >>>>> On 02-10-15 05:41, Ilia Mirkin wrote: >>>>> >>>>> <nothing> >>>>> >>>>> As someone who has recently started following nouveau I must say that >>>>> it would greatly help me (and likely others) if patches likes this >>>>> would >>>>> come with a somewhat more descriptive commit message. >>>> >>>> >>>> >>>> Duly noted. I normally try to say a bit, but have gotten lazy of late. >>>> Ben's pretty terse too :) How about something like >>>> >>>> """ >>>> Currently OF bios load fails for a few reasons: >>>> - checksum failure >>>> - bios size too small >>>> - no PCIR header >>>> - bios length not a multiple of 4 >>>> >>>> In this change, we resolve all of the above by ignoring any checksum >>>> failures, and faking the PCIR data when loading from OF. >>>> """ >>> >>> >>> >>> Much better, I must say that now that you spell it out that the >>> ignoring of the checksum sort of worries me, but I guess the >>> checksum is part of the missing PCIR header ? >> >> >> Nope, the two things are unrelated. The PCIR section is normally >> embedded in the vbios and tells you how long the vbios is, and allows >> you to have multiple elements appended at the end (which nouveau has >> recently acquired some support for). The checksum failures just >> happen, > > >> "pretty sure that the OF-sourced vbioses just don't have a >> checksum byte at all." > > > You may want to add something along those lines to the commit message > too...Ben, I see you already committed my change to your tree, could you amend the commit message to read something like """ Currently OF bios load fails for a few reasons: - checksum failure - bios size too small - no PCIR header - bios length not a multiple of 4 In this change, we resolve all of the above by ignoring any checksum failures (since OF VBIOS tends not to have a checksum), and faking the PCIR data when loading from OF. """ I think Hans has a very valid point about the terseness of nouveau commit messages... I'm often a bit annoyed at the terseness of yours esp for larger changes, but apparently I go and do the same thing myself :) Thanks, -ilia