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 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
>>

-- 
Jani Nikula, Intel