[igt-dev] [PATCH] lib/igt_kms.c igt_kms.h: tile property parser

[Ser, Simon]

Hi,

Here are a few comments.

On Wed, 2019-07-17 at 11:10 -0700, Madhumitha Tolakanahalli Pradeep wrote:
> The tile property parser parses the connector tile property obtained
> from connector's Display ID block and set per connector.
> 
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Signed-off-by: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep at intel.com>
> ---
>  lib/igt_kms.c | 26 ++++++++++++++++++++++++++
>  lib/igt_kms.h | 11 +++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 175e71c3..846314de 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -4511,3 +4511,29 @@ bool igt_display_has_format_mod(igt_display_t *display, uint32_t format,
>  
>  	return false;
>  }
> +
> +/**
> + * igt_parse_connector_tile_blob:
> + * @blob: pointer to the connector's tile properties
> + * @tile: pointer to tile structure that is populated by the function
> + *
> + * Parses the connector tile blob to extract the tile information
> + *
> + */
> +
> +void igt_parse_connector_tile_blob(drmModePropertyBlobPtr blob,
> +				igt_tile_info_t * tile)

Style: align with as many tabs as possible, then fill the rest with
spaces.

> +{
> +	char * blob_data = blob->data;

Style: no space after star.

    char *thing = stuff;

Applies to the whole patch.

> +	if(blob) {

Style: space after if keyword.

> +		tile->tile_group_id = atoi(strtok(blob_data, ":"));

This segfaults if the field doesn't exist. It would be nicer to
igt_assert that strtok doesn't return NULL. Maybe with a wrapper
function? e.g.

    static int parse_next_tile_field(char *data)

Also this doesn't check for integer overflows, but not sure it's worth
the hassle.

> +		tile->tile_is_single_monitor = atoi(strtok(NULL, ":"));
> +		tile->num_h_tile = atoi(strtok(NULL, ":"));
> +		tile->num_v_tile = atoi(strtok(NULL, ":"));
> +		tile->tile_h_loc = atoi(strtok(NULL, ":"));
> +		tile->tile_v_loc = atoi(strtok(NULL, ":"));
> +		tile->tile_h_size = atoi(strtok(NULL, ":"));
> +		tile->tile_v_size = atoi(strtok(NULL, ":"));
> +	}
> +}
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 0486737b..08483505 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -387,6 +387,14 @@ struct igt_display {
>  	int format_mod_count;
>  };
>  
> +typedef struct {
> +	int tile_group_id;
> +	bool tile_is_single_monitor;
> +	uint8_t num_h_tile, num_v_tile;
> +	uint8_t tile_h_loc, tile_v_loc;
> +	uint16_t tile_h_size, tile_v_size;

I wouldn't repeat the tile_ prefix for all of these fields, it's
already clear that it's about the tile from the type name.

> +} igt_tile_info_t;

I'm personally not a fan of typedefs, but it seems like the rest of IGT
doesn't care, so it's probably fine.

>  void igt_display_require(igt_display_t *display, int drm_fd);
>  void igt_display_fini(igt_display_t *display);
>  void igt_display_reset(igt_display_t *display);
> @@ -835,4 +843,7 @@ static inline bool igt_vblank_before(uint32_t a, uint32_t b)
>  	return igt_vblank_after(b, a);
>  }
>  
> +void igt_parse_connector_tile_blob(drmModePropertyBlobPtr blob,
> +			igt_tile_info_t * tile);
> +
>  #endif /* __IGT_KMS_H__ */