On Thu, Nov 07, 2019 at 10:22:10AM +0200, Jani Nikula wrote:
> On Wed, 06 Nov 2019, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >>
> >> ---
> >>
> >> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
