Re: [PATCH v4 2/2] drm/i915/vbt: Handle generic DTD block

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 14 Nov 2019, Matt Roper <matthew.d.roper@intel.com> wrote:
> VBT revision 229 adds a new "Generic DTD" block 58 and deprecates the
> old LFP panel mode data in block 42.  Let's start parsing this block to
> fill in the panel fixed mode on devices with a >=229 VBT.
>
> v2:
>  * Update according to the recent updates:
>     - DTD size is now 16 bits instead of 24
>     - polarity is now just a single bit for hsync and vsync and is
>       properly documented
>  * Minor checkpatch fix
>
> v3:
>  * Now that panel options are parsed separately from the previous patch,
>    move generic DTD parsing into a function parallel to
>    parse_lfp_panel_dtd.  We'll still fall back to looking at the legacy
>    LVDS timing block if the generic DTD fails.  (Jani)
>  * Don't forget to actually set lfp_lvds_vbt_mode!  (Jani)
>  * Drop "bdb_" prefix from dtd entry structure.  (Jani)
>  * Follow C99 standard for structure's flexible array member.  (Jani)
>
> Bspec: 54751
> Bspec: 20148
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c     | 86 ++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_vbt_defs.h | 31 +++++++
>  2 files changed, 115 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index d13ce0b7db8b..3b66160fd3c4 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -296,7 +296,7 @@ parse_lfp_panel_dtd(struct drm_i915_private *dev_priv,
>  
>  	dev_priv->vbt.lfp_lvds_vbt_mode = panel_fixed_mode;
>  
> -	DRM_DEBUG_KMS("Found panel mode in BIOS VBT tables:\n");
> +	DRM_DEBUG_KMS("Found panel mode in BIOS VBT legacy lfp table:\n");
>  	drm_mode_debug_printmodeline(panel_fixed_mode);
>  
>  	fp_timing = get_lvds_fp_timing(bdb, lvds_lfp_data,
> @@ -313,6 +313,81 @@ parse_lfp_panel_dtd(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static void
> +parse_generic_dtd(struct drm_i915_private *dev_priv,
> +		  const struct bdb_header *bdb)
> +{
> +	const struct bdb_generic_dtd *generic_dtd;
> +	const struct generic_dtd_entry *dtd;
> +	struct drm_display_mode *panel_fixed_mode;
> +	int num_dtd;
> +
> +	generic_dtd = find_section(bdb, BDB_GENERIC_DTD);
> +	if (!generic_dtd)
> +		return;
> +
> +	if (generic_dtd->gdtd_size < sizeof(struct generic_dtd_entry)) {
> +		DRM_ERROR("GDTD size %u is too small.\n",
> +			  generic_dtd->gdtd_size);
> +		return;
> +	} else if (generic_dtd->gdtd_size !=
> +		   sizeof(struct generic_dtd_entry)) {
> +		DRM_ERROR("Unexpected GDTD size %u\n", generic_dtd->gdtd_size);
> +		/* DTD has unknown fields, but keep going */
> +	}
> +
> +	num_dtd = (get_blocksize(generic_dtd) -
> +		   sizeof(struct bdb_generic_dtd)) / generic_dtd->gdtd_size;
> +	if (dev_priv->vbt.panel_type > num_dtd) {
> +		DRM_ERROR("Panel type %d not found in table of %d DTD's\n",
> +			  dev_priv->vbt.panel_type, num_dtd);
> +		return;
> +	}

I wonder how many times we've duplicated a version of the dozen or so
lines above... Not part of this patch, but we'll need to address this
one of these days.

> +
> +	dtd = &generic_dtd->dtd[dev_priv->vbt.panel_type];
> +
> +	panel_fixed_mode = kzalloc(sizeof(*panel_fixed_mode), GFP_KERNEL);
> +	if (!panel_fixed_mode)
> +		return;
> +
> +	panel_fixed_mode->hdisplay = dtd->hactive;
> +	panel_fixed_mode->hsync_start =
> +		panel_fixed_mode->hdisplay + dtd->hfront_porch;
> +	panel_fixed_mode->hsync_end =
> +		panel_fixed_mode->hsync_start + dtd->hsync;
> +	panel_fixed_mode->htotal = panel_fixed_mode->hsync_end;
> +
> +	panel_fixed_mode->vdisplay = dtd->vactive;
> +	panel_fixed_mode->vsync_start =
> +		panel_fixed_mode->vdisplay + dtd->vfront_porch;
> +	panel_fixed_mode->vsync_end =
> +		panel_fixed_mode->vsync_start + dtd->vsync;
> +	panel_fixed_mode->vtotal = panel_fixed_mode->vsync_end;
> +
> +	panel_fixed_mode->clock = dtd->pixel_clock;
> +	panel_fixed_mode->width_mm = dtd->width_mm;
> +	panel_fixed_mode->height_mm = dtd->height_mm;
> +
> +	panel_fixed_mode->type = DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_set_name(panel_fixed_mode);
> +
> +	if (dtd->hsync_polarity)

Nitpick, I tend to like bit flags names that say what the value means,
i.e. have "positive" in there to indicate the flag set means
positive. But we already have the same thing in the old struct. *shrug*

> +		panel_fixed_mode->flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		panel_fixed_mode->flags |= DRM_MODE_FLAG_NHSYNC;
> +
> +	if (dtd->vsync_polarity)
> +		panel_fixed_mode->flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		panel_fixed_mode->flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	DRM_DEBUG_KMS("Found panel mode in BIOS VBT generic dtd table:\n");
> +	drm_mode_debug_printmodeline(panel_fixed_mode);
> +
> +	dev_priv->vbt.lfp_lvds_vbt_mode = panel_fixed_mode;
> +}
> +
> +

Nitpick, double newlines.

>  static void
>  parse_lfp_backlight(struct drm_i915_private *dev_priv,
>  		    const struct bdb_header *bdb)
> @@ -1877,7 +1952,14 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  	parse_general_features(dev_priv, bdb);
>  	parse_general_definitions(dev_priv, bdb);
>  	parse_panel_options(dev_priv, bdb);
> -	parse_lfp_panel_dtd(dev_priv, bdb);
> +	if (bdb->version >= 229)
> +		/*
> +		 * If this fails, we'll fall back to trying to parse the
> +		 * legacy LVDS block below.
> +		 */
> +		parse_generic_dtd(dev_priv, bdb);
> +	if (!dev_priv->vbt.lfp_lvds_vbt_mode)
> +		parse_lfp_panel_dtd(dev_priv, bdb);

Maybe we should abstract that to a helper that branches out to hide it
from the top level. Can be left for follow-up.

>  	parse_lfp_backlight(dev_priv, bdb);
>  	parse_sdvo_panel_data(dev_priv, bdb);
>  	parse_driver_features(dev_priv, bdb);
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 69a7cb1fa121..41eff2eae211 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -115,6 +115,7 @@ enum bdb_block_id {
>  	BDB_MIPI_CONFIG			= 52,
>  	BDB_MIPI_SEQUENCE		= 53,
>  	BDB_COMPRESSION_PARAMETERS	= 56,
> +	BDB_GENERIC_DTD			= 58,
>  	BDB_SKIP			= 254, /* VBIOS private block, ignore */
>  };
>  
> @@ -863,4 +864,34 @@ struct bdb_compression_parameters {
>  	struct dsc_compression_parameters_entry data[16];
>  } __packed;
>  
> +/*
> + * Block 58 - Generic DTD Block
> + */
> +
> +struct generic_dtd_entry {
> +	u32 pixel_clock;
> +	u16 hactive;
> +	u16 hblank;
> +	u16 hfront_porch;
> +	u16 hsync;
> +	u16 vactive;
> +	u16 vblank;
> +	u16 vfront_porch;
> +	u16 vsync;
> +	u16 width_mm;
> +	u16 height_mm;
> +
> +	/* Flags */
> +	u8 rsvd_flags:6;
> +	u8 vsync_polarity:1;
> +	u8 hsync_polarity:1;
> +
> +	u32 rsvd:24;

This does seem to work (and I had to try to believe), but for packed
structs I think I'd feel more comfortable grouping bitfields such that
the bits fill up the type. Now this depends on the u8 and 24 bits of the
u32 to form 4 bytes. Having u32 rsvd:24 does not work independently, and
I don't want to risk anyone picking that up and cargo culting
incorrectly.

So maybe just u8 rsvd[3] for clarity?

This one I'd like fixed, I'll leave the other nitpicks to your
discretion or maybe follow-up patches, and with that,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +} __packed;
> +
> +struct bdb_generic_dtd {
> +	u16 gdtd_size;
> +	struct generic_dtd_entry dtd[];	/* up to 24 DTD's */
> +} __packed;
> +
>  #endif /* _INTEL_VBT_DEFS_H_ */

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx