[PATCH] drm/i915/bios: Copy the whole MIPI sequence block

[PATCH] drm/i915/bios: Copy the whole MIPI sequence block

[Ville Syrjala]

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+.
+	 */
+	if (section_id == BDB_MIPI_SEQUENCE && *(const u8 *)block >= 3)
+		block_size += 5;
+
 	entry = kzalloc(struct_size(entry, data, max(min_size, block_size) + 3),
 			GFP_KERNEL);
 	if (!entry) {
-- 
2.35.1

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

[Jani Nikula]

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>

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.

>  	entry = kzalloc(struct_size(entry, data, max(min_size, block_size) + 3),
>  			GFP_KERNEL);
>  	if (!entry) {

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Ville Syrjälä]

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