Re: [PATCH] drm/i915/bios: store child devices in a list

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

On Thu, Nov 07, 2019 at 10:22:10AM +0200, Jani Nikula wrote:
> On Wed, 06 Nov 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Nov 06, 2019 at 06:45:31PM +0200, Jani Nikula wrote:
> >> Using the array is getting clumsy. Make things a bit more dynamic.
> >> 
> >> In code, start migrating towards calling the new struct child_device
> >> "child" and the VBT-originating struct child_device_config "config".
> >> 
> >> Remove early returns on not having child devices when the end result
> >> after "iterating" the empty list would be the same.
> >> 
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> 
> >> ---
> >> 
> >> The end goal: allow more meta information to be added to the new
> >> child_device struct, independent of DDI port info being used or not on
> >> the platform, and eventually migrate ddi_port_info to it as well,
> >> unifying the stuff across platforms.
> >> 
> >> Currently it's not easily possible to associate for example the DSC
> >> compression data to the child device for non-DDI platforms or for DSI
> >> outputs. This lets us add the compression data (pointer) to struct
> >> child_device.
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_bios.c | 203 ++++++++++------------
> >>  drivers/gpu/drm/i915/i915_drv.h           |   3 +-
> >>  2 files changed, 97 insertions(+), 109 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> index a03f56b7b4ef..025074862ab0 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> @@ -58,6 +58,12 @@
> >>   * that.
> >>   */
> >>  
> >> +/* Wrapper for VBT child device config */
> >> +struct child_device {
> >> +	struct child_device_config config;
> >> +	struct list_head node;
> >
> > The wrapper is a bit unfortunate. I don't suppose we could just shove
> > the list head into the existing struct and adjust what needs adjusting?
> 
> The existing struct is used for serialization and the size is checked
> against what's in vbt etc. I might also add stuff in the wrapper struct,
> at least intermediately, so it's kind of useful. I don't really think
> the wrapper is all that bad.
> 
> >
> >> +};
> >> +
> >>  #define	SLAVE_ADDR1	0x70
> >>  #define	SLAVE_ADDR2	0x72
> >>  
> >> @@ -449,8 +455,9 @@ static void
> >>  parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
> >>  {
> >>  	struct sdvo_device_mapping *mapping;
> >> -	const struct child_device_config *child;
> >> -	int i, count = 0;
> >> +	const struct child_device_config *config;
> >
> > This thing could at least can live inside the loop. Though the rename is
> > also a bit unfortunate, leading to a needlessly large diff. Avoiding the
> > wrapper struct would also avoid that. I guess another option would
> > be to select a different name for the wrapper pointer here and keep the
> > original name for the actual thing.
> 
> The main problem with avoiding the rename is to come up with a better
> name for the wrapper structure. :) Child and config seemed apt, but I do
> understand the downsides. I'd just like to have names that we can use
> througout. Maybe we can stick to child for struct child_device_config,
> but what do we call the whole wrapper struct and local vars?
> Suggestions?

I suck at naming things. If no better name comes up I guess we could at
least split the rename to a separate patch. Would be easier to see
what's actually changing with the introduction of the list.

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