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
