[PATCH/RFC 1/5] drm/edid: Reorganise the DisplayID parsing code
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Sep 10 11:54:16 UTC 2019
On Wed, Aug 21, 2019 at 09:50:01PM +0300, Laurent Pinchart wrote:
> This change started as an attempt to remove the forward declaration of
> validate_displayid(), and ended up reorganising the DisplayID parsing
> code to fix serial intertwined issues.
>
> The drm_parse_display_id() function, which parses a DisplayID block, is
> made aware of whether the DisplayID comes from an EDID extension block
> or is direct DisplayID data. This is a layering violation, this should
> be handled in the caller. Similarly, the validate_displayid() function
> should not take an offset in the data buffer, it should also receive a
> pointer to the DisplayID data.
>
> In order to simplify the callers of drm_find_displayid_extension(), the
> function is modified to return a pointer to the DisplayID data, not to
> the EDID extension block. The pointer can then be used directly for
> validation and parsing, without the need to add an offset.
>
> For consistency reasons the validate_displayid() function is renamed to
> drm_displayid_is_valid() and modified to return a bool, and the
> drm_parse_display_id() renamed to drm_parse_displayid().
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> drivers/gpu/drm/drm_edid.c | 104 ++++++++++++++++++-------------------
> 1 file changed, 52 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 82a4ceed3fcf..7c6bc5183b60 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1342,7 +1342,6 @@ MODULE_PARM_DESC(edid_fixup,
>
> static void drm_get_displayid(struct drm_connector *connector,
> struct edid *edid);
> -static int validate_displayid(u8 *displayid, int length, int idx);
>
> static int drm_edid_block_checksum(const u8 *raw_edid)
> {
> @@ -2927,17 +2926,49 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
> return edid_ext;
> }
>
> +static bool drm_displayid_is_valid(u8 *displayid, unsigned int length)
const* would be nice.
same in many other places in the series.
> +{
> + struct displayid_hdr *base;
> + u8 csum = 0;
> + int i;
> +
> + base = (struct displayid_hdr *)displayid;
Can be done when declaring the variable.
> +
> + DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
> + base->rev, base->bytes, base->prod_id, base->ext_count);
> +
> + if (base->bytes + 5 > length)
> + return false;
> +
> + for (i = 0; i <= base->bytes + 5; i++)
> + csum += displayid[i];
> +
> + if (csum) {
> + DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);
> + return false;
> + }
> +
> + return true;
> +}
>
> static u8 *drm_find_displayid_extension(const struct edid *edid)
> {
> - return drm_find_edid_extension(edid, DISPLAYID_EXT);
> + u8 *ext = drm_find_edid_extension(edid, DISPLAYID_EXT);
> +
> + if (!ext)
> + return NULL;
> +
> + /*
> + * Skip the EDID extension block tag number to return the DisplayID
> + * data.
> + */
> + return ext + 1;
> }
>
> static u8 *drm_find_cea_extension(const struct edid *edid)
> {
> - int ret;
> - int idx = 1;
> - int length = EDID_LENGTH;
> + int idx;
> + int length = EDID_LENGTH - 1;
> struct displayid_block *block;
> u8 *cea;
> u8 *displayid;
> @@ -2952,11 +2983,10 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
> if (!displayid)
> return NULL;
>
> - ret = validate_displayid(displayid, length, idx);
> - if (ret)
> + if (!drm_displayid_is_valid(displayid, length))
> return NULL;
>
> - idx += sizeof(struct displayid_hdr);
> + idx = sizeof(struct displayid_hdr);
It's a bit silly/fragile that everyone has to do this. I'd rather
eliminate this idx initialzation from the callers of
for_each_displayid_db() entirely.
> for_each_displayid_db(displayid, block, idx, length) {
> if (block->tag == DATA_BLOCK_CTA) {
> cea = (u8 *)block;
This here is also borked. We should validate the size of the
CEA ext block somewhere.
> @@ -4711,29 +4741,6 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> return quirks;
> }
>
> -static int validate_displayid(u8 *displayid, int length, int idx)
> -{
> - int i;
> - u8 csum = 0;
> - struct displayid_hdr *base;
> -
> - base = (struct displayid_hdr *)&displayid[idx];
> -
> - DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
> - base->rev, base->bytes, base->prod_id, base->ext_count);
> -
> - if (base->bytes + 5 > length - idx)
> - return -EINVAL;
> - for (i = idx; i <= base->bytes + 5; i++) {
> - csum += displayid[i];
> - }
> - if (csum) {
> - DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);
> - return -EINVAL;
> - }
> - return 0;
> -}
> -
> static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *dev,
> struct displayid_detailed_timings_1 *timings)
> {
> @@ -4809,9 +4816,8 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
> struct edid *edid)
> {
> u8 *displayid;
> - int ret;
> - int idx = 1;
> - int length = EDID_LENGTH;
> + int idx;
> + int length = EDID_LENGTH - 1;
> struct displayid_block *block;
> int num_modes = 0;
>
> @@ -4819,11 +4825,10 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
> if (!displayid)
> return 0;
>
> - ret = validate_displayid(displayid, length, idx);
> - if (ret)
> + if (!drm_displayid_is_valid(displayid, length))
> return 0;
>
> - idx += sizeof(struct displayid_hdr);
> + idx = sizeof(struct displayid_hdr);
> for_each_displayid_db(displayid, block, idx, length) {
> switch (block->tag) {
> case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
> @@ -5424,23 +5429,17 @@ static int drm_parse_tiled_block(struct drm_connector *connector,
> return 0;
> }
>
> -static int drm_parse_display_id(struct drm_connector *connector,
> - u8 *displayid, int length,
> - bool is_edid_extension)
> +static int drm_parse_displayid(struct drm_connector *connector,
> + u8 *displayid, int length)
> {
> - /* if this is an EDID extension the first byte will be 0x70 */
> - int idx = 0;
> struct displayid_block *block;
> + int idx;
> int ret;
>
> - if (is_edid_extension)
> - idx = 1;
> + if (!drm_displayid_is_valid(displayid, length))
> + return -EINVAL;
>
> - ret = validate_displayid(displayid, length, idx);
> - if (ret)
> - return ret;
> -
> - idx += sizeof(struct displayid_hdr);
> + idx = sizeof(struct displayid_hdr);
> for_each_displayid_db(displayid, block, idx, length) {
> DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n",
> block->tag, block->rev, block->num_bytes);
> @@ -5468,8 +5467,9 @@ static int drm_parse_display_id(struct drm_connector *connector,
> static void drm_get_displayid(struct drm_connector *connector,
> struct edid *edid)
> {
> - void *displayid = NULL;
> + void *displayid;
> int ret;
> +
> connector->has_tile = false;
> displayid = drm_find_displayid_extension(edid);
> if (!displayid) {
> @@ -5477,16 +5477,16 @@ static void drm_get_displayid(struct drm_connector *connector,
> goto out_drop_ref;
> }
>
> - ret = drm_parse_display_id(connector, displayid, EDID_LENGTH, true);
> + ret = drm_parse_displayid(connector, displayid, EDID_LENGTH - 1);
> if (ret < 0)
> goto out_drop_ref;
> if (!connector->has_tile)
> goto out_drop_ref;
> return;
> +
> out_drop_ref:
> if (connector->tile_group) {
> drm_mode_put_tile_group(connector->dev, connector->tile_group);
> connector->tile_group = NULL;
> }
> - return;
> }
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
More information about the dri-devel
mailing list