Re: [PATCH 3/8] drm: Add drm_for_each_connector_encoder_ids()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 27, 2018 at 11:11:57AM +0200, Daniel Vetter wrote:
> On Wed, Jun 27, 2018 at 11:08:48AM +0200, Daniel Vetter wrote:
> > On Tue, Jun 26, 2018 at 08:47:09PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Add a convenience macro for iterating connector->encoder_ids[].
> > > Isolates the users from the implementation details.
> > > 
> > > Also use ARRAY_SIZE() when populating the array to avoid spreading
> > > knowledge about the array size all over.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_connector.c    | 22 ++++++++++------------
> > >  drivers/gpu/drm/drm_fb_helper.c    |  6 +++---
> > >  drivers/gpu/drm/drm_probe_helper.c |  9 +++++----
> > >  include/drm/drm_connector.h        | 11 +++++++++++
> > >  4 files changed, 29 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 2f9ebddd178e..c43646cb8145 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -321,7 +321,7 @@ int drm_mode_connector_attach_encoder(struct drm_connector *connector,
> > >  	if (WARN_ON(connector->encoder))
> > >  		return -EINVAL;
> > >  
> > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > +	for (i = 0; i < ARRAY_SIZE(connector->encoder_ids); i++) {
> > >  		if (connector->encoder_ids[i] == 0) {
> > >  			connector->encoder_ids[i] = encoder->base.id;
> > >  			return 0;
> > > @@ -1693,6 +1693,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > >  	int encoders_count = 0;
> > >  	int ret = 0;
> > >  	int copied = 0;
> > > +	u32 encoder_id;
> > >  	int i;
> > >  	struct drm_mode_modeinfo u_mode;
> > >  	struct drm_mode_modeinfo __user *mode_ptr;
> > > @@ -1708,22 +1709,19 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > >  	if (!connector)
> > >  		return -ENOENT;
> > >  
> > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> > > -		if (connector->encoder_ids[i] != 0)
> > > -			encoders_count++;
> > > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i)
> > > +		encoders_count++;
> > >  
> > >  	if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
> > >  		copied = 0;
> > >  		encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
> > > -		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > -			if (connector->encoder_ids[i] != 0) {
> > > -				if (put_user(connector->encoder_ids[i],
> > > -					     encoder_ptr + copied)) {
> > > -					ret = -EFAULT;
> > > -					goto out;
> > > -				}
> > > -				copied++;
> > > +
> > > +		drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > > +			if (put_user(encoder_id, encoder_ptr + copied)) {
> > > +				ret = -EFAULT;
> > > +				goto out;
> > >  			}
> > > +			copied++;
> > >  		}
> > >  	}
> > >  	out_resp->count_encoders = encoders_count;
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 61c39cd75a27..e086b08748f4 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -2326,12 +2326,12 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
> > >  static bool connector_crtc_ok(struct drm_connector *connector,
> > >  			      struct drm_crtc *crtc)
> > >  {
> > > +	u32 encoder_id;
> > >  	int i;
> > >  
> > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > >  		struct drm_encoder *encoder =
> > > -			drm_encoder_find(connector->dev, NULL,
> > > -					 connector->encoder_ids[i]);
> > > +			drm_encoder_find(connector->dev, NULL, encoder_id);
> > >  
> > >  		if (encoder->possible_crtcs & drm_crtc_mask(crtc))
> > >  			return true;
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index 527743394150..0239f76c52fb 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -88,9 +88,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
> > >  			    struct drm_connector *connector)
> > >  {
> > >  	struct drm_device *dev = connector->dev;
> > > -	uint32_t *ids = connector->encoder_ids;
> > >  	enum drm_mode_status ret = MODE_OK;
> > > -	unsigned int i;
> > > +	u32 encoder_id;
> > > +	int i;
> > >  
> > >  	/* Step 1: Validate against connector */
> > >  	ret = drm_connector_mode_valid(connector, mode);
> > > @@ -98,8 +98,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
> > >  		return ret;
> > >  
> > >  	/* Step 2: Validate against encoders and crtcs */
> > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > -		struct drm_encoder *encoder = drm_encoder_find(dev, NULL, ids[i]);
> > > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > > +		struct drm_encoder *encoder =
> > > +			drm_encoder_find(dev, NULL, encoder_id);
> > >  		struct drm_crtc *crtc;
> > >  
> > >  		if (!encoder)
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 14ab58ade87f..4a0363f7dd8c 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -1193,4 +1193,15 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter);
> > >  #define drm_for_each_connector_iter(connector, iter) \
> > >  	while ((connector = drm_connector_list_iter_next(iter)))
> > >  
> > > +/**
> > > + * drm_for_each_connector_encoder_ids - iterate connector encoder_ids[]
> > 
> > &drm_connector.encoder_ids would give you a (fairly direct) link. Just a
> > nitpick.
> > 
> > Wrt the api: I think including the drm_encoder_find would be useful here -
> > it's what 2/3 users want anyway, and the ioctl can easily look up the id
> > again by following encoder->base.id. And then maybe call the function a
> > bit differently, I think usually we go with
> > $parent_object_for_each_$iterated_thing, so
> > drm_connector_for_each_encoder(). There's also the
> > for_each_$thing_in_$parent_object pattern (used for atomic states), but I
> > think that's less fitting here.
> > 
> > Apologies for the interface bikeshedding ...
> 
> Looking at all the patches it's 15 places that want the drm_encoder_find
> integrated vs. 4 that don't want it (without further refactoring). So even
> more reasons for drm_connector_for_each_encoder imo.

I had that same thought initially, but then forgot about it for
some reason. It does indeed lead to an even neater result.

Although it should be somewhat obvious that there can only be one
currently used encoder for a connector, I think we may want to name
this eg. drm_connector_for_each_possible_encoder() to make it super
clear what we're talking about.

> -Daniel
> 
> > -Daniel
> > 
> > > + * @connector: &struct drm_connector pointer used as cursor
> > > + * @encoder_id: the encoder ID
> > > + * @__i: int iteration cursor, for macro-internal use
> > > + */
> > > +#define drm_for_each_connector_encoder_ids(connector, encoder_id, __i) \
> > > +	for ((__i) = 0; (__i) < ARRAY_SIZE((connector)->encoder_ids) && \
> > > +		     (connector)->encoder_ids[(__i)] != 0; (__i)++) \
> > > +		for_each_if((encoder_id) = (connector)->encoder_ids[(__i)])
> > > +
> > >  #endif
> > > -- 
> > > 2.16.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel