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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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@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
once. This thing I had to read a few times to make sure I
understood what it's doing.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx