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

["Ser, Simon" <simon.ser@intel.com>]

On Tue, 2019-07-30 at 17:51 -0700, Tolakanahalli Pradeep, Madhumitha wrote:
> Thank you for your comments, Simon.
> 
> I have taken care of the comments regarding style changes.
> 
> On Thu, 2019-07-18 at 07:24 +0100, 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@intel.com>
> > > Cc: Petri Latvala <petri.latvala@intel.com>
> > > Signed-off-by: Madhumitha Tolakanahalli Pradeep <
> > > madhumitha.tolakanahalli.pradeep@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)
> 
> This blob information is obtained from the kernel
> (drm/drm_connector.c). It's format is defined in
> drm_connector_set_tile_property() to be of the form of 8 integers using
> ':' as a separator. Hence, the field not existing wouldn't be a
> possibility, which is why no NULL return checks are being performed.

Generally IGT tests assume the kernel could do anything. For instance,
a driver could roll its own logic instead of using
drm_connector_set_tile_property. Or the TILE blob format could be
extended in a kernel patch, breaking backwards compatibility for a
corner case.

I agree it's not that of a big deal for this specific case. I'm fine
either way.

> > 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 naming convention of the tile properties confroms to that in the
> kernel to maintain uniformity.

Indeed, that makes sense. Better to keep it that way.

> > > +} 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__ */
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev