| Message ID | cover.1613580193.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/i915/bios: vbt child device rework | expand |
On Wed, Feb 17, 2021 at 07:03:30PM +0200, Jani Nikula wrote: >I see the parsing and caching of child device data into >i915->vbt.ddi_port_info[] slightly problematic. We keep adding data to >it, and it just duplicates information. Start moving towards a single >point of truth, and getting the information directly from the child >device data. > >One obstacle has been that init_vbt_missing_defaults() only initializes >ddi_port_info, without child devices. As the same problem arose in a >patch from Lucas, I thought it was time to start creating fake child >devices to unify the code. > >There are a bunch of cleanups and refactoring here. Patches 1-5 are >enough to fix Lucas' patch I think. Patch 10 does what Lucas was after, >just in a different way and as a byproduct of something else. The later humn... but we'd still need the patches in that series to cleanup the calls to intel_ddi_init() from intel_display.c. Or did I miss anything? I did a quick scan through the patches and left a few comments. Overall it looks good to me, but I need to dedicate a little more time to give a proper r-b. I will do soon if no one beats me to it. thanks Lucas De Marchi >patches in the series are more to show the direction, and seek >validation for that direction. > >Naming is also a question mark. All of these are a bit questionable: >intel_bios_encoder_data, devdata, intel_bios_encoder_supports_*, etc. > >BR, >Jani. > > >[1] http://patchwork.freedesktop.org/patch/msgid/20210213190511.1017088-2-lucas.demarchi@intel.com > > > >Cc: Lucas De Marchi <lucas.demarchi@intel.com> >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >Jani Nikula (12): > drm/i915/bios: mass convert dev_priv to i915 > drm/i915/bios: store bdb version in i915 > drm/i915/bios: limit default outputs by platform on missing VBT > drm/i915/bios: limit default outputs to ports A through F > drm/i915/bios: create fake child devices on missing VBT > drm/i915/bios: rename display_device_data to intel_bios_encoder_data > drm/i915/bios: add i915 backpointer to intel_bios_encoder_data > drm/i915/vbt: add helper functions to check output support > drm/i915/bios: save a higher level pointer in ddi_vbt_port_info[] > drm/i915/bios: start using the intel_bios_encoder_data directly > drm/i915/bios: start using intel_bios_encoder_data for Type-C USB and > TBT > drm/i915/bios: add intel_bios_encoder_data to encoder, use for iboost > > drivers/gpu/drm/i915/display/intel_bios.c | 1021 +++++++++-------- > drivers/gpu/drm/i915/display/intel_bios.h | 17 +- > drivers/gpu/drm/i915/display/intel_ddi.c | 28 +- > .../drm/i915/display/intel_display_types.h | 3 + > drivers/gpu/drm/i915/i915_drv.h | 9 +- > 5 files changed, 584 insertions(+), 494 deletions(-) > >-- >2.20.1 >
On Wed, 17 Feb 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Wed, Feb 17, 2021 at 07:03:30PM +0200, Jani Nikula wrote: >>I see the parsing and caching of child device data into >>i915->vbt.ddi_port_info[] slightly problematic. We keep adding data to >>it, and it just duplicates information. Start moving towards a single >>point of truth, and getting the information directly from the child >>device data. >> >>One obstacle has been that init_vbt_missing_defaults() only initializes >>ddi_port_info, without child devices. As the same problem arose in a >>patch from Lucas, I thought it was time to start creating fake child >>devices to unify the code. >> >>There are a bunch of cleanups and refactoring here. Patches 1-5 are >>enough to fix Lucas' patch I think. Patch 10 does what Lucas was after, >>just in a different way and as a byproduct of something else. The later > > humn... but we'd still need the patches in that series to cleanup > the calls to intel_ddi_init() from intel_display.c. Or did I miss > anything? Nah, my bad. This makes it possible to do what you do there. BR, Jani. > > I did a quick scan through the patches and left a few comments. Overall > it looks good to me, but I need to dedicate a little more time to give a > proper r-b. I will do soon if no one beats me to it. > > > thanks > Lucas De Marchi > >>patches in the series are more to show the direction, and seek >>validation for that direction. >> >>Naming is also a question mark. All of these are a bit questionable: >>intel_bios_encoder_data, devdata, intel_bios_encoder_supports_*, etc. >> >>BR, >>Jani. >> >> >>[1] http://patchwork.freedesktop.org/patch/msgid/20210213190511.1017088-2-lucas.demarchi@intel.com >> >> >> >>Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> >>Jani Nikula (12): >> drm/i915/bios: mass convert dev_priv to i915 >> drm/i915/bios: store bdb version in i915 >> drm/i915/bios: limit default outputs by platform on missing VBT >> drm/i915/bios: limit default outputs to ports A through F >> drm/i915/bios: create fake child devices on missing VBT >> drm/i915/bios: rename display_device_data to intel_bios_encoder_data >> drm/i915/bios: add i915 backpointer to intel_bios_encoder_data >> drm/i915/vbt: add helper functions to check output support >> drm/i915/bios: save a higher level pointer in ddi_vbt_port_info[] >> drm/i915/bios: start using the intel_bios_encoder_data directly >> drm/i915/bios: start using intel_bios_encoder_data for Type-C USB and >> TBT >> drm/i915/bios: add intel_bios_encoder_data to encoder, use for iboost >> >> drivers/gpu/drm/i915/display/intel_bios.c | 1021 +++++++++-------- >> drivers/gpu/drm/i915/display/intel_bios.h | 17 +- >> drivers/gpu/drm/i915/display/intel_ddi.c | 28 +- >> .../drm/i915/display/intel_display_types.h | 3 + >> drivers/gpu/drm/i915/i915_drv.h | 9 +- >> 5 files changed, 584 insertions(+), 494 deletions(-) >> >>-- >>2.20.1 >>
I see the parsing and caching of child device data into i915->vbt.ddi_port_info[] slightly problematic. We keep adding data to it, and it just duplicates information. Start moving towards a single point of truth, and getting the information directly from the child device data. One obstacle has been that init_vbt_missing_defaults() only initializes ddi_port_info, without child devices. As the same problem arose in a patch from Lucas, I thought it was time to start creating fake child devices to unify the code. There are a bunch of cleanups and refactoring here. Patches 1-5 are enough to fix Lucas' patch I think. Patch 10 does what Lucas was after, just in a different way and as a byproduct of something else. The later patches in the series are more to show the direction, and seek validation for that direction. Naming is also a question mark. All of these are a bit questionable: intel_bios_encoder_data, devdata, intel_bios_encoder_supports_*, etc. BR, Jani. [1] http://patchwork.freedesktop.org/patch/msgid/20210213190511.1017088-2-lucas.demarchi@intel.com Cc: Lucas De Marchi <lucas.demarchi@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Jani Nikula (12): drm/i915/bios: mass convert dev_priv to i915 drm/i915/bios: store bdb version in i915 drm/i915/bios: limit default outputs by platform on missing VBT drm/i915/bios: limit default outputs to ports A through F drm/i915/bios: create fake child devices on missing VBT drm/i915/bios: rename display_device_data to intel_bios_encoder_data drm/i915/bios: add i915 backpointer to intel_bios_encoder_data drm/i915/vbt: add helper functions to check output support drm/i915/bios: save a higher level pointer in ddi_vbt_port_info[] drm/i915/bios: start using the intel_bios_encoder_data directly drm/i915/bios: start using intel_bios_encoder_data for Type-C USB and TBT drm/i915/bios: add intel_bios_encoder_data to encoder, use for iboost drivers/gpu/drm/i915/display/intel_bios.c | 1021 +++++++++-------- drivers/gpu/drm/i915/display/intel_bios.h | 17 +- drivers/gpu/drm/i915/display/intel_ddi.c | 28 +- .../drm/i915/display/intel_display_types.h | 3 + drivers/gpu/drm/i915/i915_drv.h | 9 +- 5 files changed, 584 insertions(+), 494 deletions(-)