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

[Manasi Navare]

On Wed, Jul 17, 2019 at 11:24:33PM -0700, Ser, Simon wrote:
> 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.

The idea of using tile_ prefix is so that it aligns with the tile fields in the
kernel and these are the exact names that get printed in the dmesg so makes
it more intuitive if we stick to these names.

Manasi

> 
> > +} 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__ */