| Message ID | 450b5883a7783d0967c3da8ce853af0c2da8082f.1718719962.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/display: platform identification with display->is.<PLATFORM> | expand |
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@intel.com> > --- > .../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. 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? > +} > + > 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 >
On Wed, 19 Jun 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> 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@intel.com> >> --- >> .../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. > > 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". 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 >>
On Thu, Jun 20, 2024 at 04:05:46PM +0300, Jani Nikula wrote: > On Wed, 19 Jun 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> 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@intel.com> > >> --- > >> .../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@intel.com> > > 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
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]; +} + +static void merge_display_is(struct intel_display_is *dst, + const struct intel_display_is *src) +{ + mem_or(dst, src, sizeof(*dst)); +} + 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)
Facilitate using display->is.HASWELL etc. for identifying platforms and subplatforms. Merge platform and subplatform members together. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- .../gpu/drm/i915/display/intel_display_core.h | 3 +++ .../drm/i915/display/intel_display_device.c | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+)