| Message ID | 1413787036-19114-3-git-send-email-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Mon, Oct 20, 2014 at 8:37 AM, Dave Airlie <airlied@gmail.com> wrote: > From: Dave Airlie <airlied@redhat.com> > > A tile group is an identifier shared by a single monitor, > DisplayID topology has 8 bytes we can use for this, just > use those for now until something else comes up in the > future. We assign these to an idr and use the idr to > tell userspace what connectors are in the same tile group. > > DisplayID v1.3 says the serial number must be unique for > displays from the same manufacturer. > > Signed-off-by: Dave Airlie <airlied@redhat.com> > --- > drivers/gpu/drm/drm_crtc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_crtc.h | 16 ++++++++++++ > 2 files changed, 77 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 90e7730..dfccc34 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -5037,6 +5037,7 @@ void drm_mode_config_init(struct drm_device *dev) > INIT_LIST_HEAD(&dev->mode_config.property_blob_list); > INIT_LIST_HEAD(&dev->mode_config.plane_list); > idr_init(&dev->mode_config.crtc_idr); > + idr_init(&dev->mode_config.tile_idr); You are missing a call to idr_destroy() in drm_mode_config_cleanup(). The IDR caches are not cleaned up automatically when it drops to size 0... Thanks David > > drm_modeset_lock_all(dev); > drm_mode_create_standard_connector_properties(dev); > @@ -5146,3 +5147,63 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, > supported_rotations); > } > EXPORT_SYMBOL(drm_mode_create_rotation_property); > + > +static void drm_tile_group_free(struct kref *kref) > +{ > + struct drm_tile_group *tg = container_of(kref, struct drm_tile_group, refcount); > + struct drm_device *dev = tg->dev; > + mutex_lock(&dev->mode_config.idr_mutex); > + idr_remove(&dev->mode_config.tile_idr, tg->id); > + mutex_lock(&dev->mode_config.idr_mutex); > + kfree(tg); > +} > + > +void drm_mode_put_tile_group(struct drm_device *dev, > + struct drm_tile_group *tg) > +{ > + kref_put(&tg->refcount, drm_tile_group_free); > +} > + > +struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev, > + char topology[8]) > +{ > + struct drm_tile_group *tg; > + int id; > + mutex_lock(&dev->mode_config.idr_mutex); > + idr_for_each_entry(&dev->mode_config.tile_idr, tg, id) { > + if (!memcmp(tg->group_data, topology, 8)) { > + kref_get(&tg->refcount); > + mutex_unlock(&dev->mode_config.idr_mutex); > + return tg; > + } > + } > + mutex_unlock(&dev->mode_config.idr_mutex); > + return NULL; > +} > + > +struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, > + char topology[8]) > +{ > + struct drm_tile_group *tg; > + int ret; > + > + tg = kzalloc(sizeof(*tg), GFP_KERNEL); > + if (!tg) > + return ERR_PTR(-ENOMEM); > + > + kref_init(&tg->refcount); > + memcpy(tg->group_data, topology, 8); > + tg->dev = dev; > + > + mutex_lock(&dev->mode_config.idr_mutex); > + ret = idr_alloc(&dev->mode_config.tile_idr, tg, 1, 0, GFP_KERNEL); > + if (ret >= 0) { > + tg->id = ret; > + } else { > + kfree(tg); > + tg = NULL; > + } > + > + mutex_unlock(&dev->mode_config.idr_mutex); > + return tg; > +} > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index f1105d0..afaec4b 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -136,6 +136,14 @@ struct drm_display_info { > u8 cea_rev; > }; > > +/* data corresponds to displayid vend/prod/serial */ > +struct drm_tile_group { > + struct kref refcount; > + struct drm_device *dev; > + int id; > + u8 group_data[8]; > +}; > + > struct drm_framebuffer_funcs { > /* note: use drm_framebuffer_remove() */ > void (*destroy)(struct drm_framebuffer *framebuffer); > @@ -770,6 +778,7 @@ struct drm_mode_config { > struct drm_modeset_acquire_ctx *acquire_ctx; /* for legacy _lock_all() / _unlock_all() */ > struct mutex idr_mutex; /* for IDR management */ > struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */ > + struct idr tile_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */ > /* this is limited to one for now */ > > > @@ -1106,6 +1115,13 @@ extern void drm_set_preferred_mode(struct drm_connector *connector, > extern int drm_edid_header_is_valid(const u8 *raw_edid); > extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid); > extern bool drm_edid_is_valid(struct edid *edid); > + > +extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, > + char topology[8]); > +extern struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev, > + char topology[8]); > +extern void drm_mode_put_tile_group(struct drm_device *dev, > + struct drm_tile_group *tg); > struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, > int hsize, int vsize, int fresh, > bool rb); > -- > 2.1.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Oct 20, 2014 at 04:37:12PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@redhat.com> > > A tile group is an identifier shared by a single monitor, > DisplayID topology has 8 bytes we can use for this, just > use those for now until something else comes up in the > future. We assign these to an idr and use the idr to > tell userspace what connectors are in the same tile group. > > DisplayID v1.3 says the serial number must be unique for > displays from the same manufacturer. > > Signed-off-by: Dave Airlie <airlied@redhat.com> > --- > drivers/gpu/drm/drm_crtc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_crtc.h | 16 ++++++++++++ > 2 files changed, 77 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 90e7730..dfccc34 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -5037,6 +5037,7 @@ void drm_mode_config_init(struct drm_device *dev) > INIT_LIST_HEAD(&dev->mode_config.property_blob_list); > INIT_LIST_HEAD(&dev->mode_config.plane_list); > idr_init(&dev->mode_config.crtc_idr); > + idr_init(&dev->mode_config.tile_idr); > > drm_modeset_lock_all(dev); > drm_mode_create_standard_connector_properties(dev); > @@ -5146,3 +5147,63 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, > supported_rotations); > } > EXPORT_SYMBOL(drm_mode_create_rotation_property); > + > +static void drm_tile_group_free(struct kref *kref) > +{ > + struct drm_tile_group *tg = container_of(kref, struct drm_tile_group, refcount); > + struct drm_device *dev = tg->dev; > + mutex_lock(&dev->mode_config.idr_mutex); > + idr_remove(&dev->mode_config.tile_idr, tg->id); > + mutex_lock(&dev->mode_config.idr_mutex); > + kfree(tg); > +} > + > +void drm_mode_put_tile_group(struct drm_device *dev, > + struct drm_tile_group *tg) > +{ > + kref_put(&tg->refcount, drm_tile_group_free); > +} > + > +struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev, > + char topology[8]) > +{ > + struct drm_tile_group *tg; > + int id; > + mutex_lock(&dev->mode_config.idr_mutex); > + idr_for_each_entry(&dev->mode_config.tile_idr, tg, id) { > + if (!memcmp(tg->group_data, topology, 8)) { > + kref_get(&tg->refcount); Don't you need a kref_get_unless_zero here since we only destroy the idr entry after the refcount dropped to zero? Or is there some magic thing that prevents this like another mutex (in which case some mutex assert in get/put would be good)? And kerneldoc for the non-exported functions please, preferrably with some overview DOC: section to pull it all together. -Daniel > + mutex_unlock(&dev->mode_config.idr_mutex); > + return tg; > + } > + } > + mutex_unlock(&dev->mode_config.idr_mutex); > + return NULL; > +} > + > +struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, > + char topology[8]) > +{ > + struct drm_tile_group *tg; > + int ret; > + > + tg = kzalloc(sizeof(*tg), GFP_KERNEL); > + if (!tg) > + return ERR_PTR(-ENOMEM); > + > + kref_init(&tg->refcount); > + memcpy(tg->group_data, topology, 8); > + tg->dev = dev; > + > + mutex_lock(&dev->mode_config.idr_mutex); > + ret = idr_alloc(&dev->mode_config.tile_idr, tg, 1, 0, GFP_KERNEL); > + if (ret >= 0) { > + tg->id = ret; > + } else { > + kfree(tg); > + tg = NULL; > + } > + > + mutex_unlock(&dev->mode_config.idr_mutex); > + return tg; > +} > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index f1105d0..afaec4b 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -136,6 +136,14 @@ struct drm_display_info { > u8 cea_rev; > }; > > +/* data corresponds to displayid vend/prod/serial */ > +struct drm_tile_group { > + struct kref refcount; > + struct drm_device *dev; > + int id; > + u8 group_data[8]; > +}; > + > struct drm_framebuffer_funcs { > /* note: use drm_framebuffer_remove() */ > void (*destroy)(struct drm_framebuffer *framebuffer); > @@ -770,6 +778,7 @@ struct drm_mode_config { > struct drm_modeset_acquire_ctx *acquire_ctx; /* for legacy _lock_all() / _unlock_all() */ > struct mutex idr_mutex; /* for IDR management */ > struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */ > + struct idr tile_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */ > /* this is limited to one for now */ > > > @@ -1106,6 +1115,13 @@ extern void drm_set_preferred_mode(struct drm_connector *connector, > extern int drm_edid_header_is_valid(const u8 *raw_edid); > extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid); > extern bool drm_edid_is_valid(struct edid *edid); > + > +extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, > + char topology[8]); > +extern struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev, > + char topology[8]); > +extern void drm_mode_put_tile_group(struct drm_device *dev, > + struct drm_tile_group *tg); > struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, > int hsize, int vsize, int fresh, > bool rb); > -- > 2.1.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> Don't you need a kref_get_unless_zero here since we only destroy the idr > entry after the refcount dropped to zero? Or is there some magic thing > that prevents this like another mutex (in which case some mutex assert in > get/put would be good)? This does all happen under mode_config.mutex but I think I'd rather use get_unless_zero. > > And kerneldoc for the non-exported functions please, preferrably with some > overview DOC: section to pull it all together. I'm posting v2, advice on kerneldoc required, so the functions end up in the right place, I don't really wnat to add a new C file for this. Dave.
On Wed, Oct 22, 2014 at 12:23:30PM +1000, Dave Airlie wrote: > > And kerneldoc for the non-exported functions please, preferrably with some > > overview DOC: section to pull it all together. > > I'm posting v2, advice on kerneldoc required, so the functions end up > in the right place, I don't really wnat to add a new C file for this. Argh sorry that was a boilerplate reply that escated ;-) I've sent the same one to pretty much all the recent i915 patch series where this is the new rule. I don't think drm internal docs make a lot of sense as long as all the driver stuff is nicely documented. Some barrier to entry for core drm might actually be useful ... Cheers, Daniel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 90e7730..dfccc34 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5037,6 +5037,7 @@ void drm_mode_config_init(struct drm_device *dev) INIT_LIST_HEAD(&dev->mode_config.property_blob_list); INIT_LIST_HEAD(&dev->mode_config.plane_list); idr_init(&dev->mode_config.crtc_idr); + idr_init(&dev->mode_config.tile_idr); drm_modeset_lock_all(dev); drm_mode_create_standard_connector_properties(dev); @@ -5146,3 +5147,63 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, supported_rotations); } EXPORT_SYMBOL(drm_mode_create_rotation_property); + +static void drm_tile_group_free(struct kref *kref) +{ + struct drm_tile_group *tg = container_of(kref, struct drm_tile_group, refcount); + struct drm_device *dev = tg->dev; + mutex_lock(&dev->mode_config.idr_mutex); + idr_remove(&dev->mode_config.tile_idr, tg->id); + mutex_lock(&dev->mode_config.idr_mutex); + kfree(tg); +} + +void drm_mode_put_tile_group(struct drm_device *dev, + struct drm_tile_group *tg) +{ + kref_put(&tg->refcount, drm_tile_group_free); +} + +struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev, + char topology[8]) +{ + struct drm_tile_group *tg; + int id; + mutex_lock(&dev->mode_config.idr_mutex); + idr_for_each_entry(&dev->mode_config.tile_idr, tg, id) { + if (!memcmp(tg->group_data, topology, 8)) { + kref_get(&tg->refcount); + mutex_unlock(&dev->mode_config.idr_mutex); + return tg; + } + } + mutex_unlock(&dev->mode_config.idr_mutex); + return NULL; +} + +struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, + char topology[8]) +{ + struct drm_tile_group *tg; + int ret; + + tg = kzalloc(sizeof(*tg), GFP_KERNEL); + if (!tg) + return ERR_PTR(-ENOMEM); + + kref_init(&tg->refcount); + memcpy(tg->group_data, topology, 8); + tg->dev = dev; + + mutex_lock(&dev->mode_config.idr_mutex); + ret = idr_alloc(&dev->mode_config.tile_idr, tg, 1, 0, GFP_KERNEL); + if (ret >= 0) { + tg->id = ret; + } else { + kfree(tg); + tg = NULL; + } + + mutex_unlock(&dev->mode_config.idr_mutex); + return tg; +} diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index f1105d0..afaec4b 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -136,6 +136,14 @@ struct drm_display_info { u8 cea_rev; }; +/* data corresponds to displayid vend/prod/serial */ +struct drm_tile_group { + struct kref refcount; + struct drm_device *dev; + int id; + u8 group_data[8]; +}; + struct drm_framebuffer_funcs { /* note: use drm_framebuffer_remove() */ void (*destroy)(struct drm_framebuffer *framebuffer); @@ -770,6 +778,7 @@ struct drm_mode_config { struct drm_modeset_acquire_ctx *acquire_ctx; /* for legacy _lock_all() / _unlock_all() */ struct mutex idr_mutex; /* for IDR management */ struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */ + struct idr tile_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */ /* this is limited to one for now */ @@ -1106,6 +1115,13 @@ extern void drm_set_preferred_mode(struct drm_connector *connector, extern int drm_edid_header_is_valid(const u8 *raw_edid); extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid); extern bool drm_edid_is_valid(struct edid *edid); + +extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, + char topology[8]); +extern struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev, + char topology[8]); +extern void drm_mode_put_tile_group(struct drm_device *dev, + struct drm_tile_group *tg); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh, bool rb);