Re: [PATCH v6 1/2] drm: Add connector property to limit max bpc
On Mon, Sep 17, 2018 at 09:52:08PM -0700, Radhakrishna Sripada wrote:
> At times 12bpc HDMI cannot be driven due to faulty cables, dongles
> level shifters etc. To workaround them we may need to drive the output
> at a lower bpc. Currently the user space does not have a way to limit
> the bpc. The default bpc to be programmed is decided by the driver and
> is run against connector limitations.
>
> Creating a new connector property "max bpc" in order to limit the bpc.
> xrandr can make use of this connector property to make sure that bpc does
> not exceed the configured value. This property can be used by userspace to
> set the bpc.
>
> V2: Initialize max_bpc to satisfy kms_properties
> V3: Move the property to drm_connector
> V4: Split drm and i915 components(Ville)
> V5: Make the property per connector(Ville)
> V6: Compare the requested bpc to connector bpc(Daniel)
> Move the attach_property function to core(Ville)
>
> Cc: Ville Syrjälä
> Cc: Daniel Vetter
> Cc: Kishore Kadiyala
> Cc: Rodrigo Vivi
> Cc: Manasi Navare
> Cc: Stanislav Lisovskiy
> Signed-off-by: Radhakrishna Sripada
> ---
> drivers/gpu/drm/drm_atomic.c| 24
> drivers/gpu/drm/drm_atomic_helper.c | 4
> drivers/gpu/drm/drm_atomic_uapi.c | 4
> drivers/gpu/drm/drm_connector.c | 34 ++
> include/drm/drm_connector.h | 20
> 5 files changed, 86 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7ada75919756..fa9996deb132 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -390,6 +390,7 @@ static int drm_atomic_connector_check(struct
> drm_connector *connector,
> {
> struct drm_crtc_state *crtc_state;
> struct drm_writeback_job *writeback_job = state->writeback_job;
> + struct drm_display_info *info = >display_info;
>
> if ((connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) ||
> !writeback_job)
> return 0;
> @@ -400,6 +401,29 @@ static int drm_atomic_connector_check(struct
> drm_connector *connector,
> return -EINVAL;
> }
>
> + if (connector->max_bpc_property) {
> + if (info->bpc != 0 && info->bpc < state->max_requested_bpc) {
> + /* Don't use an invalid EDID bpc value */
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] Clamping requested"
> + " display bpp (was %d) to EDID
> reported"
> + "max of %d\n", connector->base.id,
> + connector->name,
> + state->max_requested_bpc, info->bpc);
> + state->max_bpc = info->bpc;
> + } else if (info->bpc != 0) {
> + state->max_bpc = state->max_requested_bpc;
> + } else {
> + /* Clamp bpc to 8 on screens witout EDID 1.4 */
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] Clamping requested"
> + " display bpp (was %d) to default
> limit"
> + " of 24\n", connector->base.id,
> + connector->name,
> state->max_requested_bpc);
> + state->max_bpc = 8;
> + }
> + } else {
> + state->max_bpc = info->bpc ? info->bpc : 8;
> + }
Something like
max_bpc = info->bpc ?: 8;
if (max_bpc_prop)
max_bpc = min(max_bpc, max_requested_bpc);
for short?
Not quite sure the 8bpc fallback should be here. Maybe it should be in
the edid code instead? Though I guess we want something for edidless
displays too so maybe this is fine.
> +
> if (state->crtc)
> crtc_state = drm_atomic_get_existing_crtc_state(state->state,
> state->crtc);
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 3cf1aa132778..d9ce8077fb6a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -639,6 +639,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> if (old_connector_state->link_status !=
> new_connector_state->link_status)
> new_crtc_state->connectors_changed = true;
> +
> + if (old_connector_state->max_requested_bpc !=
> + new_connector_state->max_requested_bpc)
> + new_crtc_state->connectors_changed = true;
> }
>
> if (funcs->atomic_check)
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index d5b7f315098c..86ac33922b09 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -740,6
[PATCH v6 1/2] drm: Add connector property to limit max bpc
At times 12bpc HDMI cannot be driven due to faulty cables, dongles
level shifters etc. To workaround them we may need to drive the output
at a lower bpc. Currently the user space does not have a way to limit
the bpc. The default bpc to be programmed is decided by the driver and
is run against connector limitations.
Creating a new connector property "max bpc" in order to limit the bpc.
xrandr can make use of this connector property to make sure that bpc does
not exceed the configured value. This property can be used by userspace to
set the bpc.
V2: Initialize max_bpc to satisfy kms_properties
V3: Move the property to drm_connector
V4: Split drm and i915 components(Ville)
V5: Make the property per connector(Ville)
V6: Compare the requested bpc to connector bpc(Daniel)
Move the attach_property function to core(Ville)
Cc: Ville Syrjälä
Cc: Daniel Vetter
Cc: Kishore Kadiyala
Cc: Rodrigo Vivi
Cc: Manasi Navare
Cc: Stanislav Lisovskiy
Signed-off-by: Radhakrishna Sripada
---
drivers/gpu/drm/drm_atomic.c| 24
drivers/gpu/drm/drm_atomic_helper.c | 4
drivers/gpu/drm/drm_atomic_uapi.c | 4
drivers/gpu/drm/drm_connector.c | 34 ++
include/drm/drm_connector.h | 20
5 files changed, 86 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7ada75919756..fa9996deb132 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -390,6 +390,7 @@ static int drm_atomic_connector_check(struct drm_connector
*connector,
{
struct drm_crtc_state *crtc_state;
struct drm_writeback_job *writeback_job = state->writeback_job;
+ struct drm_display_info *info = >display_info;
if ((connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) ||
!writeback_job)
return 0;
@@ -400,6 +401,29 @@ static int drm_atomic_connector_check(struct drm_connector
*connector,
return -EINVAL;
}
+ if (connector->max_bpc_property) {
+ if (info->bpc != 0 && info->bpc < state->max_requested_bpc) {
+ /* Don't use an invalid EDID bpc value */
+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] Clamping requested"
+" display bpp (was %d) to EDID
reported"
+"max of %d\n", connector->base.id,
+connector->name,
+state->max_requested_bpc, info->bpc);
+ state->max_bpc = info->bpc;
+ } else if (info->bpc != 0) {
+ state->max_bpc = state->max_requested_bpc;
+ } else {
+ /* Clamp bpc to 8 on screens witout EDID 1.4 */
+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] Clamping requested"
+" display bpp (was %d) to default
limit"
+" of 24\n", connector->base.id,
+connector->name,
state->max_requested_bpc);
+ state->max_bpc = 8;
+ }
+ } else {
+ state->max_bpc = info->bpc ? info->bpc : 8;
+ }
+
if (state->crtc)
crtc_state = drm_atomic_get_existing_crtc_state(state->state,
state->crtc);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 3cf1aa132778..d9ce8077fb6a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -639,6 +639,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
if (old_connector_state->link_status !=
new_connector_state->link_status)
new_crtc_state->connectors_changed = true;
+
+ if (old_connector_state->max_requested_bpc !=
+ new_connector_state->max_requested_bpc)
+ new_crtc_state->connectors_changed = true;
}
if (funcs->atomic_check)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index d5b7f315098c..86ac33922b09 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -740,6 +740,8 @@ static int drm_atomic_connector_set_property(struct
drm_connector *connector,
return set_out_fence_for_connector(state->state, connector,
fence_ptr);
+ } else if (property == connector->max_bpc_property) {
+ state->max_requested_bpc = val;
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
