Re: [Intel-gfx] [PATCH] drm/i915/bios: Copy the whole MIPI sequence block

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

On Tue, Aug 30, 2022 at 11:27:27AM +0300, Jani Nikula wrote:
> On Mon, 29 Aug 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Turns out the MIPI sequence block version number and
> > new block size fields are considered part of the block
> > header and are not included in the reported new block size
> > field itself. Bump up the block size appropriately so that
> > we'll copy over the last five bytes of the block as well.
> >
> > For this particular machine those last five bytes included
> > parts of the GPIO op for the backlight on sequence, causing
> > the backlight no longer to turn back on:
> >
> >  		Sequence 6 - MIPI_SEQ_BACKLIGHT_ON
> >  			Delay: 20000 us
> > -			GPIO index 0, number 0, set 0 (0x00)
> > +			GPIO index 1, number 70, set 1 (0x01)
> >
> > Cc: stable@vger.kernel.org
> > Fixes: e163cfb4c96d ("drm/i915/bios: Make copies of VBT data blocks")
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6652
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 81d6cfbd2615..d493d04f4049 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -479,6 +479,13 @@ init_bdb_block(struct drm_i915_private *i915,
> >  
> >  	block_size = get_blocksize(block);
> >  
> > +	/*
> > +	 * Version number and new block size are considered
> > +	 * part of the header for MIPI sequenece block v3+.
> > +	 */
> 
> Quoth Bspec, "This field specifies the size of this entire data
> structure excluding this header." Okay.
> 
> > +	if (section_id == BDB_MIPI_SEQUENCE && *(const u8 *)block >= 3)
> > +		block_size += 5;
> > +
> 
> Since we need to look at the header later, we can't just use data +
> 5. Okay.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks.

> 
> I just don't get the whole sequence block u32 size thing, because the
> vbt header and bdb header still have u16, and they're supposed to cover
> the entire vbt and all bdb data blocks. And this is what we check
> anyway.

Yeah, very much snafu.

> 
> >  	entry = kzalloc(struct_size(entry, data, max(min_size, block_size) + 3),
> >  			GFP_KERNEL);
> >  	if (!entry) {
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel