Re: [PATCH 5/6] drm/i915/display: add "is" member to struct intel_display

[Jani Nikula <jani.nikula@intel.com>]

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

-- 
Jani Nikula, Intel