| Message ID | 450b5883a7783d0967c3da8ce853af0c2da8082f.1718719962.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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
On Tue, Jun 18, 2024 at 05:22:55PM GMT, 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) why are we not using linux/bitmap.h that has support for bitfields with multiple words and instead rolling our own? Lucas De Marchi >+{ >+ 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) >-- >2.39.2 >
On Thu, 27 Jun 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Tue, Jun 18, 2024 at 05:22:55PM GMT, 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) > > why are we not using linux/bitmap.h that has support for bitfields with > multiple words and instead rolling our own? Because this is primarily about named struct members, and the bitfields and ORing them together is just an implementation detail. I could use bitmap_or(), but I'd have to rely on bitmap implementation details to get it all precisely correct. I would not be able to trivially use DECLARE_BITMAP() for this. Using a union can get tricky: struct intel_display_is { union { struct { INTEL_DISPLAY_PLATFORMS(MEMBER); }; DECLARE_BITMAP(raw, NUM_PLATFORMS); }; }; I don't know if that even works. Can't used named structs, otherwise it defeats the purpose. BR, Jani. > > Lucas De Marchi > >>+{ >>+ 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) >>-- >>2.39.2 >>
On Thu, Jun 27, 2024 at 09:47:17PM GMT, Jani Nikula wrote: >On Thu, 27 Jun 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> On Tue, Jun 18, 2024 at 05:22:55PM GMT, 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) >> >> why are we not using linux/bitmap.h that has support for bitfields with >> multiple words and instead rolling our own? > >Because this is primarily about named struct members, and the bitfields >and ORing them together is just an implementation detail. > >I could use bitmap_or(), but I'd have to rely on bitmap implementation >details to get it all precisely correct. I would not be able to >trivially use DECLARE_BITMAP() for this. > >Using a union can get tricky: > >struct intel_display_is { > union { > struct { > INTEL_DISPLAY_PLATFORMS(MEMBER); > }; > DECLARE_BITMAP(raw, NUM_PLATFORMS); > }; >}; > >I don't know if that even works. Can't used named structs, otherwise it >defeats the purpose. a union like that seems good to me Lucas De Marchi > >BR, >Jani. > >> >> Lucas De Marchi >> >>>+{ >>>+ 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) >>>-- >>>2.39.2 >>> > >-- >Jani Nikula, Intel
On Thu, 27 Jun 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Thu, Jun 27, 2024 at 09:47:17PM GMT, Jani Nikula wrote: >>On Thu, 27 Jun 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>> On Tue, Jun 18, 2024 at 05:22:55PM GMT, 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) >>> >>> why are we not using linux/bitmap.h that has support for bitfields with >>> multiple words and instead rolling our own? >> >>Because this is primarily about named struct members, and the bitfields >>and ORing them together is just an implementation detail. >> >>I could use bitmap_or(), but I'd have to rely on bitmap implementation >>details to get it all precisely correct. I would not be able to >>trivially use DECLARE_BITMAP() for this. >> >>Using a union can get tricky: >> >>struct intel_display_is { >> union { >> struct { >> INTEL_DISPLAY_PLATFORMS(MEMBER); >> }; >> DECLARE_BITMAP(raw, NUM_PLATFORMS); >> }; >>}; >> >>I don't know if that even works. Can't used named structs, otherwise it >>defeats the purpose. > > a union like that seems good to me With the platform enum removed in patch 6, where do we get the number of platforms and subplatforms? BR, Jani. > > Lucas De Marchi > > >> >>BR, >>Jani. >> >>> >>> Lucas De Marchi >>> >>>>+{ >>>>+ 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) >>>>-- >>>>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(+)