[Intel-gfx] [PATCH 3/3] drm/i915/i915: assume vbt is 4-byte aligned into oprom

[Lucas De Marchi]

On Fri, Nov 22, 2019 at 03:55:32PM +0200, Ville Syrjälä wrote:
>On Thu, Nov 21, 2019 at 10:54:29AM -0800, Lucas De Marchi wrote:
>> On Thu, Nov 21, 2019 at 03:09:03PM +0200, Jani Nikula wrote:
>> >On Wed, 20 Nov 2019, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
>> >> The unaligned ioread32() will make us read byte by byte looking for the
>> >> vbt. We could just as well have done a ioread8() + a shift and avoid the
>> >> extra confusion on how we are looking for "$VBT".
>> >>
>> >> However when using ACPI it's guaranteed the VBT is 4-byte aligned
>> >> per spec, so we can probably assume it here as well.
>> >>
>> >> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_bios.c | 19 ++++++-------------
>> >>  1 file changed, 6 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> index aa9b182efee5..6bf57b1ad056 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> @@ -1902,27 +1902,20 @@ static struct vbt_header *oprom_get_vbt(struct drm_i915_private *dev_priv)
>> >>  	void __iomem *p = NULL, *oprom;
>> >>  	struct vbt_header *vbt;
>> >>  	u16 vbt_size;
>> >> -	size_t i, size;
>> >> +	size_t size;
>> >>
>> >>  	oprom = pci_map_rom(pdev, &size);
>> >>  	if (!oprom)
>> >>  		return NULL;
>> >>
>> >>  	/* Scour memory looking for the VBT signature. */
>> >> -	for (i = 0; i + 4 < size; i++) {
>> >> -		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
>> >> -			continue;
>> >> -
>> >> -		p = oprom + i;
>> >> -		size -= i;
>> >> -		break;
>> >> -	}
>> >> -
>> >> -	if (!p)
>> >> -		goto err_unmap_oprom;
>> >> +	for (p = oprom; size >= 4; p += 4, size -= 4)
>> >> +		if (ioread32(p) == *((const u32 *)"$VBT"))
>> >> +			break;
>> >
>> >I think the original is easier to read. You only really need to change
>> >"i++" to "i += 4" and be done with it.
>>
>> I really liked this version much more... shorter and with only one control
>> variable rather than keeping the control in 3 different places (i, size
>> and p).
>
>I think I'm with Jani here. Generally not a huge fan of pointer
>arithmetic, and having just one variable modified by the loop is
>more customary so usually doesn't require me to read more than

we were previously modifying 3: i, p and size. And additionally using
oprom.... versus we only care about p inside the loop, which points to
whatever we just read... and we we keep updates on size to control the
stop condition.

>once. This thing I had to read a few times to make sure I
>understood what it's doing.

Ok, 2 against 1. I will respin this.

Lucas De Marchi

>
>-- 
>Ville Syrjälä
>Intel