Message-ID: <ca2827b5-9f6f-164b-6b3f-3f01898d3202@tuxedocomputers.com>
Date:   Wed, 30 Jun 2021 11:42:10 +0200
From:   Werner Sembach <wse@...edocomputers.com>
To:     Pekka Paalanen <ppaalanen@...il.com>
Cc:     amd-gfx@...ts.freedesktop.org, tzimmermann@...e.de,
        intel-gfx@...ts.freedesktop.org, sunpeng.li@....com,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        airlied@...ux.ie, rodrigo.vivi@...el.com,
        alexander.deucher@....com, christian.koenig@....com
Subject: Re: [PATCH v4 03/17] drm/uAPI: Add "active bpc" as feedback channel
 for "max bpc" drm property


Am 30.06.21 um 10:21 schrieb Pekka Paalanen:
> On Tue, 29 Jun 2021 13:02:05 +0200
> Werner Sembach <wse@...edocomputers.com> wrote:
>
>> Am 28.06.21 um 19:03 schrieb Werner Sembach:
>>> Am 18.06.21 um 11:11 schrieb Werner Sembach:
>>>> Add a new general drm property "active bpc" which can be used by graphic
>>>> drivers to report the applied bit depth per pixel back to userspace.
>>>>
>>>> While "max bpc" can be used to change the color depth, there was no way to
>>>> check which one actually got used. While in theory the driver chooses the
>>>> best/highest color depth within the max bpc setting a user might not be
>>>> fully aware what his hardware is or isn't capable off. This is meant as a
>>>> quick way to double check the setup.
>>>>
>>>> In the future, automatic color calibration for screens might also depend on
>>>> this information being available.
>>>>
>>>> Signed-off-by: Werner Sembach <wse@...edocomputers.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_connector.c | 51 +++++++++++++++++++++++++++++++++
>>>>   include/drm/drm_connector.h     |  8 ++++++
>>>>   2 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>> index da39e7ff6965..943f6b61053b 100644
>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>> @@ -1197,6 +1197,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>>>>    *	drm_connector_attach_max_bpc_property() to create and attach the
>>>>    *	property to the connector during initialization.
>>>>    *
>>>> + * active bpc:
>>>> + *	This read-only range property tells userspace the pixel color bit depth
>>>> + *	actually used by the hardware display engine on "the cable" on a
>>>> + *	connector. The chosen value depends on hardware capabilities, both
>>>> + *	display engine and connected monitor, and the "max bpc" property.
>>>> + *	Drivers shall use drm_connector_attach_active_bpc_property() to install
>>>> + *	this property.
>>>> + *
>>> Regarding "on the cable" and dithering: As far as I can tell, what the dithering option does, is setting a hardware
>>> register here:
>>>
>>> - https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/intel_display.c#L4534
>>>
>>> - https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/intel_display.c#L4571
>>>
>>> So dithering seems to be calculated by fixed purpose hardware/firmware outside of the driver?
>>>
>>> The Intel driver does not seem to set a target bpc/bpp for this hardware so I guess it defaults to 6 or 8 bpc?
>> Never mind it does. This switch-case does affect the dithering output:
>> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/intel_display.c#L4537
> Hi,
>
> I obviously do not know the intel driver or hardware at all, but
> to me that just looks like translating from bits per pixel to bits per
> channel in RGB mapping?
No, if i understand the documentation correctly: Writing bit depth here 
with dithering enabled sets the dithering target bpc.
>
>> As found in this documentation p.548:
>> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-lkf-vol02c-commandreference-registers-part2.pdf
>>
>> So max bpc and active bpc are affecting/affected by the bpc after dithering.
> By definition, if the cable carries N bpc, then dithering does not
> change that. The cable still carries N bpc, but due to spatial or
> temporal dithering, the *observed* color resolution may or may not be
> higher than the cable bpc.
Yes, and max bpc and active bpc tell the cable bpc ist not the 
*observed* bpc.
>
> Of course, if the cable bpc is 8, and dithering targets 6 bpc, then 2
> LSB on the cable are always zero, right?
I would assume that in this case only 6 bpc are actually send? Isn't the 
whole thing of dithering that you can't send, for example, 8 bpc?
>
> Maybe one would want to do that if the monitor has a 6 bit panel and it
> simply ignored the 2 LSB, and the cable cannot go down to 6 bpc.

Is there dithering actually doing this? aka is my assumption above wrong?

AMD code that confused me before, is hinting that you might be right: 
https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c#L826

there is a set_clamp depth and a separate DCP_SPATIAL_DITHER_DEPTH_30BPP

>
> So, what does "max bpc" mean right now?
>
> It seems like dither on/off is insufficient information, one would also
> need to control the dithering target bpc. I suppose the driver has a
> policy on how it chooses the target bpc, but what is that policy? Is
> the dither target bpc the cable bpc or the sink bpc?
>
> Needless to say, I'm quite confused.

... We need someone who knows what dithering on intel and amd gpu 
actually means.

But I don't want this to become a blocker for this patchset, because if 
there is no dithering, which seems to be the norm, the active bpc 
property is already really usefull as it is. So add a note to the docs 
that the value might be invalid when dithering is active for now?

>
>
> Thanks,
> pq
>
>>> Similar things happen on amd. Here the output dither depth seems to be written to a fixed value however:
>>>
>>> - https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c#L828
>>>
>>> - https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c#L769
>>>
>>> Does anyone know about a resource where I can read up on the used registers and what this hardware actually does?
>> Searching now for a similar register reference for AMD GPUs.
>>> My proposal for now: "max bpc" affects what happens before dither, so I would keep "active bpc" the same and add another
>>> drm property "dither active: true/false". No additional property to control dither, as amdgpu does have one already
>>> (which isn't always active?) and Intel driver does only seem prepared for dithering at 6bpc (albeit I don't know why to
>>> dither at 6bpc and what depth to dither to?).
>>>   
>>>>    * Connectors also have one standardized atomic property:
>>>>    *
>>>>    * CRTC_ID:
>>>> @@ -2152,6 +2160,49 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>>>>   }
>>>>   EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
>>>>   
>>>> +/**
>>>> + * drm_connector_attach_active_bpc_property - attach "active bpc" property
>>>> + * @connector: connector to attach active bpc property on.
>>>> + * @min: The minimum bit depth supported by the connector.
>>>> + * @max: The maximum bit depth supported by the connector.
>>>> + *
>>>> + * This is used to check the applied bit depth on a connector.
>>>> + *
>>>> + * Returns:
>>>> + * Zero on success, negative errno on failure.
>>>> + */
>>>> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max)
>>>> +{
>>>> +	struct drm_device *dev = connector->dev;
>>>> +	struct drm_property *prop;
>>>> +
>>>> +	if (!connector->active_bpc_property) {
>>>> +		prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "active bpc",
>>>> +						 min, max);
>>>> +		if (!prop)
>>>> +			return -ENOMEM;
>>>> +
>>>> +		connector->active_bpc_property = prop;
>>>> +		drm_object_attach_property(&connector->base, prop, 0);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
>>>> +
>>>> +/**
>>>> + * drm_connector_set_active_bpc_property - sets the active bits per color property for a connector
>>>> + * @connector: drm connector
>>>> + * @active_bpc: bits per color for the connector currently active on "the cable"
>>>> + *
>>>> + * Should be used by atomic drivers to update the active bits per color over a connector.
>>>> + */
>>>> +void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc)
>>>> +{
>>>> +	drm_object_property_set_value(&connector->base, connector->active_bpc_property, active_bpc);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_connector_set_active_bpc_property);
>>>> +
>>>>   /**
>>>>    * drm_connector_attach_hdr_output_metadata_property - attach "HDR_OUTPUT_METADA" property
>>>>    * @connector: connector to attach the property on.
>>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>>> index 714d1a01c065..eee86de62a5f 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -1380,6 +1380,12 @@ struct drm_connector {
>>>>   	 */
>>>>   	struct drm_property *max_bpc_property;
>>>>   
>>>> +	/**
>>>> +	 * @active_bpc_property: Default connector property for the active bpc
>>>> +	 * to be driven out of the connector.
>>>> +	 */
>>>> +	struct drm_property *active_bpc_property;
>>>> +
>>>>   #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>>>>   #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>>>>   #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
>>>> @@ -1702,6 +1708,8 @@ int drm_connector_set_panel_orientation_with_quirk(
>>>>   	int width, int height);
>>>>   int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>>>>   					  int min, int max);
>>>> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max);
>>>> +void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc);
>>>>   
>>>>   /**
>>>>    * struct drm_tile_group - Tile group metadata
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@...ts.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Powered by blists - more mailing lists