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

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

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

-- 
Jani Nikula, Intel