Re: [Intel-gfx] [PATCH 1/1] drm/i915: Removing one use macro

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

On Thu, Oct 20, 2022 at 09:48:20AM +0000, Kandpal, Suraj wrote:
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> > > b/drivers/gpu/drm/i915/display/intel_display.h
> > > index 2af4a1925063..c803330a276d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > @@ -469,10 +469,6 @@ enum hpd_pin {
> > >  	list_for_each_entry((intel_encoder), &(dev)-
> > >mode_config.encoder_list, base.head) \
> > >  		for_each_if((intel_encoder)->base.crtc == (__crtc))
> > >
> > > -#define for_each_connector_on_encoder(dev, __encoder,
> > intel_connector) \
> > > -	list_for_each_entry((intel_connector), &(dev)-
> > >mode_config.connector_list, base.head) \
> > > -		for_each_if((intel_connector)->base.encoder ==
> > (__encoder))
> > > -
> > >  #define for_each_old_intel_plane_in_state(__state, plane,
> > old_plane_state, __i) \
> > >  	for ((__i) = 0; \
> > >  	     (__i) < (__state)->base.dev->mode_config.num_total_plane && \
> > > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > index cbfabd58b75a..31ce4ac4059c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > @@ -205,12 +205,22 @@ static bool intel_crtc_has_encoders(struct
> > > intel_crtc *crtc)
> > >
> > >  static struct intel_connector *intel_encoder_find_connector(struct
> > > intel_encoder *encoder)  {
> > > -	struct drm_device *dev = encoder->base.dev;
> > > +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > >  	struct intel_connector *connector;
> > > +	struct drm_connector_list_iter conn_iter;
> > > +	bool found_connector = false;
> > >
> > > -	for_each_connector_on_encoder(dev, &encoder->base, connector)
> > > -		return connector;
> > > +	drm_connector_list_iter_begin(&i915->drm, &conn_iter);
> > > +		for_each_intel_connector_iter(connector, &conn_iter) {
> > 
> > Bogus indentation.
> > 
> Hi Ville,
> 
> Will remove the bogus indentation.
> 
> > > +			if (&encoder->base == connector->base.encoder) {
> > 
> > Still using the legacy pointers so not great. But I suppse it's still better than
> > before because the horrors are now better contained in this file rather than
> > in an official looking macro.
> 
> Any suggestion on how I could make this better or should I go ahead with this
> approach for now

It's fine for now. Stopping the legacy pointer usage involved
a lot of work.

> 
> > 
> > I do have some work in some old branch to get rid of the legacy pointer
> > crap, but it'll probably need a rewrite by now...
> > 
> > > +				found_connector = true;
> > > +				break;
> > > +			}
> > > +	}
> > > +	drm_connector_list_iter_end(&conn_iter);
> > >
> > > +	if (found_connector)
> > 
> > I would make that a pointer to the connector, then you can just return it
> > directly. And as a bonus we don't use the loop iterator outside the loop
> > anymore, which is a bit questionable in general.
> 
> Ah got it .
> 
> Regards,
> Suraj Kandpal
> 
> > 
> > > +		return connector;
> > >  	return NULL;
> > >  }
> > >
> > > --
> > > 2.25.1
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel