On Thu, Jun 20, 2024 at 04:05:46PM +0300, Jani Nikula wrote: > On Wed, 19 Jun 2024, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote: > > On Tue, Jun 18, 2024 at 05:22:55PM +0300, Jani Nikula wrote: > >> Facilitate using display->is.HASWELL etc. for identifying platforms and > >> subplatforms. Merge platform and subplatform members together. > >> > >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> > >> --- > >> .../gpu/drm/i915/display/intel_display_core.h | 3 +++ > >> .../drm/i915/display/intel_display_device.c | 19 +++++++++++++++++++ > >> 2 files changed, 22 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > >> index 7715fc329057..35bea92893af 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > >> @@ -286,6 +286,9 @@ struct intel_display { > >> /* drm device backpointer */ > >> struct drm_device *drm; > >> > >> + /* Platform identification */ > >> + struct intel_display_is is; > >> + > >> /* Display functions */ > >> struct { > >> /* Top level crtc-ish functions */ > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c > >> index 0c275d85bd30..954caea38005 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display_device.c > >> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c > >> @@ -1269,8 +1269,25 @@ find_subplatform_desc(struct pci_dev *pdev, const struct platform_desc *desc) > >> return NULL; > >> } > >> > >> +static void mem_or(void *_dst, const void *_src, size_t size) > >> +{ > >> + const u8 *src = _src; > >> + u8 *dst = _dst; > >> + size_t i; > >> + > >> + for (i = 0; i < size; i++) > >> + dst[i] |= src[i]; > > > > I confess that here I got a bit lost. But I believe it is just a matter of > > adding a few comments in the code or perhaps adjusting function names... > > > > If my coffee is working well still, what we are doing here is ensuring that: > > > > is.HASWELL returns true regardless the subplatform this is coming from... > > like is.HASWELL_ULT or is.HASWELL_ULX. > > > > But since you are only doing dst |= src and not doing src |= dst and also > > not calling this function for different subplatforms, then individually > > is.HASWELL_ULT is false for ULX platform and vice-versa. > > The subplatform stuff here only ever applies to one subplatform, not > multiple. The "ULX is also ULT" is not handled yet, and maybe I'd like > to handle that in some special way, because I like the simplicity of > only having one subplatform in effect at a time. fully agree! > > > > > Perhaps the name 'merge' is not a good one? > > > >> +} > >> + > >> +static void merge_display_is(struct intel_display_is *dst, > >> + const struct intel_display_is *src) > >> +{ > >> + mem_or(dst, src, sizeof(*dst)); > > > > and/or perhaps we don't need this extra indirection here? > > I added the extra indirection only because "mem_or" is something that > could exist as a generic thing. "just do bitwise OR from src to dst". yeap, it makes sense. I'm not 100% convinced of the the 'merge_display_is name, but I honestly have no other (better or worse) suggestion, so Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > BR, > Jani. > > > > >> +} > >> + > >> void intel_display_device_probe(struct drm_i915_private *i915) > >> { > >> + struct intel_display *display = &i915->display; > >> struct pci_dev *pdev = to_pci_dev(i915->drm.dev); > >> const struct intel_display_device_info *info; > >> struct intel_display_ip_ver ip_ver = {}; > >> @@ -1308,11 +1325,13 @@ void intel_display_device_probe(struct drm_i915_private *i915) > >> > >> drm_WARN_ON(&i915->drm, !desc->platform || !desc->name); > >> DISPLAY_RUNTIME_INFO(i915)->platform = desc->platform; > >> + display->is = desc->is; > >> > >> subdesc = find_subplatform_desc(pdev, desc); > >> if (subdesc) { > >> drm_WARN_ON(&i915->drm, !subdesc->subplatform || !subdesc->name); > >> DISPLAY_RUNTIME_INFO(i915)->subplatform = subdesc->subplatform; > >> + merge_display_is(&display->is, &subdesc->is); > >> } > >> > >> if (ip_ver.ver || ip_ver.rel || ip_ver.step) > >> -- > >> 2.39.2 > >> > > -- > Jani Nikula, Intel