| Message ID | 20210608174320.37429-5-wse@tuxedocomputers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add "activ bpc" and "active color format" drm property | expand |
Hi On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote: > This commits implements the "active bpc" drm property for the Intel GPU driver. > > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++ > drivers/gpu/drm/i915/display/intel_dp.c | 8 ++++++-- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 +++- > drivers/gpu/drm/i915/display/intel_hdmi.c | 4 +++- > 4 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 64e9107d70f7..50c11b8770a7 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev, > { > struct intel_atomic_state *state = to_intel_atomic_state(_state); > struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_connector *connector; > + struct drm_connector_state *new_conn_state; > + int i; > int ret = 0; > > state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); > @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev, > intel_shared_dpll_swap_state(state); > intel_atomic_track_fbs(state); > > + /* Extract information from crtc to communicate it to userspace as connector properties */ > + for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) { > + struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc); > + if (crtc) { > + struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > + new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3; > + } > + else > + new_conn_state->active_bpc = 0; > + } > + This seems fairly intrusive, but also commit / commit_tail might not be the best place to put this, we want to support it at the connector level. Indeed, this will cause some issue if your HDMI output is a bridge for example, where the commit will be in an entirely different driver that has no dependency on the HDMI controller one. I think it would be best to do that assignment in atomic_check. That way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it would know what the output state would have been like. Also, all of your patches don't follow the kernel coding style. Make sure you fix the issues reported by checkpatch --strict Maxime
On Thu, Jun 10, 2021 at 02:50:36PM +0200, Maxime Ripard wrote: > Hi > > On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote: > > This commits implements the "active bpc" drm property for the Intel GPU driver. > > > > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++ > > drivers/gpu/drm/i915/display/intel_dp.c | 8 ++++++-- > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 +++- > > drivers/gpu/drm/i915/display/intel_hdmi.c | 4 +++- > > 4 files changed, 26 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 64e9107d70f7..50c11b8770a7 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev, > > { > > struct intel_atomic_state *state = to_intel_atomic_state(_state); > > struct drm_i915_private *dev_priv = to_i915(dev); > > + struct drm_connector *connector; > > + struct drm_connector_state *new_conn_state; > > + int i; > > int ret = 0; > > > > state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); > > @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev, > > intel_shared_dpll_swap_state(state); > > intel_atomic_track_fbs(state); > > > > + /* Extract information from crtc to communicate it to userspace as connector properties */ > > + for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) { > > + struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc); > > + if (crtc) { > > + struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > > + new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3; > > + } > > + else > > + new_conn_state->active_bpc = 0; > > + } > > + > > This seems fairly intrusive, but also commit / commit_tail might not be > the best place to put this, we want to support it at the connector > level. > > Indeed, this will cause some issue if your HDMI output is a bridge for > example, where the commit will be in an entirely different driver that > has no dependency on the HDMI controller one. > > I think it would be best to do that assignment in atomic_check. That > way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it > would know what the output state would have been like. DRM_MODE_ATOMIC_TEST_ONLY isn't allowed to change anything.
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 64e9107d70f7..50c11b8770a7 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev, { struct intel_atomic_state *state = to_intel_atomic_state(_state); struct drm_i915_private *dev_priv = to_i915(dev); + struct drm_connector *connector; + struct drm_connector_state *new_conn_state; + int i; int ret = 0; state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev, intel_shared_dpll_swap_state(state); intel_atomic_track_fbs(state); + /* Extract information from crtc to communicate it to userspace as connector properties */ + for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) { + struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc); + if (crtc) { + struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); + new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3; + } + else + new_conn_state->active_bpc = 0; + } + drm_atomic_state_get(&state->base); INIT_WORK(&state->base.commit_work, intel_atomic_commit_work); diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 642c60f3d9b1..67826ba976ed 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4671,10 +4671,14 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect intel_attach_force_audio_property(connector); intel_attach_broadcast_rgb_property(connector); - if (HAS_GMCH(dev_priv)) + if (HAS_GMCH(dev_priv)) { drm_connector_attach_max_bpc_property(connector, 6, 10); - else if (DISPLAY_VER(dev_priv) >= 5) + drm_connector_attach_active_bpc_property(connector, 6, 10); + } + else if (DISPLAY_VER(dev_priv) >= 5) { drm_connector_attach_max_bpc_property(connector, 6, 12); + drm_connector_attach_active_bpc_property(connector, 6, 12); + } /* Register HDMI colorspace for case of lspcon */ if (intel_bios_is_lspcon_present(dev_priv, port)) { diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 2daa3f67791e..5a1869dc2210 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -844,8 +844,10 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo */ connector->max_bpc_property = intel_dp->attached_connector->base.max_bpc_property; - if (connector->max_bpc_property) + if (connector->max_bpc_property) { drm_connector_attach_max_bpc_property(connector, 6, 12); + drm_connector_attach_active_bpc_property(connector, 6, 12); + } return connector; diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index d69f0a6dc26d..8af78b27b6ce 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2463,8 +2463,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c drm_object_attach_property(&connector->base, connector->dev->mode_config.hdr_output_metadata_property, 0); - if (!HAS_GMCH(dev_priv)) + if (!HAS_GMCH(dev_priv)) { drm_connector_attach_max_bpc_property(connector, 8, 12); + drm_connector_attach_active_bpc_property(connector, 8, 12); + } } /*
This commits implements the "active bpc" drm property for the Intel GPU driver. Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> --- drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++ drivers/gpu/drm/i915/display/intel_dp.c | 8 ++++++-- drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 +++- drivers/gpu/drm/i915/display/intel_hdmi.c | 4 +++- 4 files changed, 26 insertions(+), 4 deletions(-)