| Message ID | 20240826104132.966597-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/display: use old bpp as a base when modeset is not allowed | expand |
This patch works properly. Thanks! Tested-by: Shawn C Lee <shawn.c.lee@intel.com> > >We are currently observing failure on refresh rate change on VRR setup if full modeset is not allowed. This is caused by the mismatch in bpp configured by GOP and bpp value calculated by our driver. Changing bpp to value calculated by our driver would require full mode set. > >We don't have mechanism to communicate current bpp to userspace -> Userspace can't request to use current bpp. Changing bpp means full modeset. This becomes a problem when userspace haven't allowed full mode set. > >Complete solution here would mean adding mechanism to communicate current bpp to userspace. User space should use this bpp to avoid changing bpp if it wants to avoid full mode set. > >Tackle this for now in our driver by using existing bpp if full modeset is not allowed. > >Signed-off-by: Jouni Högander <jouni.hogander@intel.com> >--- > drivers/gpu/drm/i915/display/intel_display.c | 33 ++++++++++++++------ > 1 file changed, 23 insertions(+), 10 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >index 9049b9a1209d8..7b805998b280a 100644 >--- a/drivers/gpu/drm/i915/display/intel_display.c >+++ b/drivers/gpu/drm/i915/display/intel_display.c >@@ -4385,21 +4385,34 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state, > struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >- struct intel_crtc_state *crtc_state = >+ struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); >+ struct intel_crtc_state *old_crtc_state = >+ intel_atomic_get_old_crtc_state(state, crtc); > struct drm_connector *connector; > struct drm_connector_state *connector_state; > int bpp, i; > >- if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || >- IS_CHERRYVIEW(dev_priv))) >- bpp = 10*3; >- else if (DISPLAY_VER(dev_priv) >= 5) >- bpp = 12*3; >- else >- bpp = 8*3; >+ /* >+ * TODO: We don't have mechanism to communicate current bpp to >+ * userspace -> Userspace can't request to use current bpp. Changing bpp >+ * means full modeset. This becomes a problem when userspace wants to >+ * avoid full modeset. Tackle this on our driver by using existing bpp >+ * if full modeset is not allowed. >+ */ >+ if (!state->base.allow_modeset) { >+ bpp = old_crtc_state->pipe_bpp; >+ } else { >+ if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || >+ IS_CHERRYVIEW(dev_priv))) >+ bpp = 10 * 3; >+ else if (DISPLAY_VER(dev_priv) >= 5) >+ bpp = 12 * 3; >+ else >+ bpp = 8 * 3; >+ } > >- crtc_state->pipe_bpp = bpp; >+ new_crtc_state->pipe_bpp = bpp; > > /* Clamp display bpp to connector max bpp */ > for_each_new_connector_in_state(&state->base, connector, connector_state, i) { @@ -4408,7 +4421,7 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state, > if (connector_state->crtc != &crtc->base) > continue; > >- ret = compute_sink_pipe_bpp(connector_state, crtc_state); >+ ret = compute_sink_pipe_bpp(connector_state, new_crtc_state); > if (ret) > return ret; > } >-- >2.34.1 >
Hey, We shouldn't have code acting differently whether modesets are allowed, I think I'm missing some context here? Cheers, ~Marten Den 2024-08-26 kl. 12:41, skrev Jouni Högander: > We are currently observing failure on refresh rate change on VRR setup if > full modeset is not allowed. This is caused by the mismatch in bpp > configured by GOP and bpp value calculated by our driver. Changing bpp to > value calculated by our driver would require full mode set. > > We don't have mechanism to communicate current bpp to userspace -> > Userspace can't request to use current bpp. Changing bpp means full > modeset. This becomes a problem when userspace haven't allowed full mode > set. > > Complete solution here would mean adding mechanism to communicate current > bpp to userspace. User space should use this bpp to avoid changing bpp if > it wants to avoid full mode set. > > Tackle this for now in our driver by using existing bpp if full modeset is > not allowed. > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 33 ++++++++++++++------ > 1 file changed, 23 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 9049b9a1209d8..7b805998b280a 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -4385,21 +4385,34 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state, > struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - struct intel_crtc_state *crtc_state = > + struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > + struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > struct drm_connector *connector; > struct drm_connector_state *connector_state; > int bpp, i; > > - if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || > - IS_CHERRYVIEW(dev_priv))) > - bpp = 10*3; > - else if (DISPLAY_VER(dev_priv) >= 5) > - bpp = 12*3; > - else > - bpp = 8*3; > + /* > + * TODO: We don't have mechanism to communicate current bpp to > + * userspace -> Userspace can't request to use current bpp. Changing bpp > + * means full modeset. This becomes a problem when userspace wants to > + * avoid full modeset. Tackle this on our driver by using existing bpp > + * if full modeset is not allowed. > + */ > + if (!state->base.allow_modeset) { > + bpp = old_crtc_state->pipe_bpp; > + } else { > + if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || > + IS_CHERRYVIEW(dev_priv))) > + bpp = 10 * 3; > + else if (DISPLAY_VER(dev_priv) >= 5) > + bpp = 12 * 3; > + else > + bpp = 8 * 3; > + } > > - crtc_state->pipe_bpp = bpp; > + new_crtc_state->pipe_bpp = bpp; > > /* Clamp display bpp to connector max bpp */ > for_each_new_connector_in_state(&state->base, connector, connector_state, i) { > @@ -4408,7 +4421,7 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state, > if (connector_state->crtc != &crtc->base) > continue; > > - ret = compute_sink_pipe_bpp(connector_state, crtc_state); > + ret = compute_sink_pipe_bpp(connector_state, new_crtc_state); > if (ret) > return ret; > }
On Tue, 27 Aug 2024, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Hey, > > We shouldn't have code acting differently whether modesets are allowed, > I think I'm missing some context here? Yeah. Since GOP is mentioned, is this really about state readout instead? BR, Jani. > > Cheers, > ~Marten > > Den 2024-08-26 kl. 12:41, skrev Jouni Högander: >> We are currently observing failure on refresh rate change on VRR setup if >> full modeset is not allowed. This is caused by the mismatch in bpp >> configured by GOP and bpp value calculated by our driver. Changing bpp to >> value calculated by our driver would require full mode set. >> >> We don't have mechanism to communicate current bpp to userspace -> >> Userspace can't request to use current bpp. Changing bpp means full >> modeset. This becomes a problem when userspace haven't allowed full mode >> set. >> >> Complete solution here would mean adding mechanism to communicate current >> bpp to userspace. User space should use this bpp to avoid changing bpp if >> it wants to avoid full mode set. >> >> Tackle this for now in our driver by using existing bpp if full modeset is >> not allowed. >> >> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 33 ++++++++++++++------ >> 1 file changed, 23 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index 9049b9a1209d8..7b805998b280a 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -4385,21 +4385,34 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state, >> struct intel_crtc *crtc) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> - struct intel_crtc_state *crtc_state = >> + struct intel_crtc_state *new_crtc_state = >> intel_atomic_get_new_crtc_state(state, crtc); >> + struct intel_crtc_state *old_crtc_state = >> + intel_atomic_get_old_crtc_state(state, crtc); >> struct drm_connector *connector; >> struct drm_connector_state *connector_state; >> int bpp, i; >> >> - if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || >> - IS_CHERRYVIEW(dev_priv))) >> - bpp = 10*3; >> - else if (DISPLAY_VER(dev_priv) >= 5) >> - bpp = 12*3; >> - else >> - bpp = 8*3; >> + /* >> + * TODO: We don't have mechanism to communicate current bpp to >> + * userspace -> Userspace can't request to use current bpp. Changing bpp >> + * means full modeset. This becomes a problem when userspace wants to >> + * avoid full modeset. Tackle this on our driver by using existing bpp >> + * if full modeset is not allowed. >> + */ >> + if (!state->base.allow_modeset) { >> + bpp = old_crtc_state->pipe_bpp; >> + } else { >> + if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || >> + IS_CHERRYVIEW(dev_priv))) >> + bpp = 10 * 3; >> + else if (DISPLAY_VER(dev_priv) >= 5) >> + bpp = 12 * 3; >> + else >> + bpp = 8 * 3; >> + } >> >> - crtc_state->pipe_bpp = bpp; >> + new_crtc_state->pipe_bpp = bpp; >> >> /* Clamp display bpp to connector max bpp */ >> for_each_new_connector_in_state(&state->base, connector, connector_state, i) { >> @@ -4408,7 +4421,7 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state, >> if (connector_state->crtc != &crtc->base) >> continue; >> >> - ret = compute_sink_pipe_bpp(connector_state, crtc_state); >> + ret = compute_sink_pipe_bpp(connector_state, new_crtc_state); >> if (ret) >> return ret; >> } >
On Tue, 2024-08-27 at 13:29 +0300, Jani Nikula wrote: > On Tue, 27 Aug 2024, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: > > Hey, > > > > We shouldn't have code acting differently whether modesets are > > allowed, > > I think I'm missing some context here? > > Yeah. Since GOP is mentioned, is this really about state readout > instead? We already have bpp configured by GOP correctly read. I don't think we want to stick into that always? The patch is targeted for case where userspace thinks changing refresh rate without modeset is possible as panel supports VRR. Our compute_config ends up to full modeset due to differing bpp and preventing changing refresh rate unless full mode set is allowed. BR, Jouni Högander > > BR, > Jani. > > > > > > Cheers, > > ~Marten > > > > Den 2024-08-26 kl. 12:41, skrev Jouni Högander: > > > We are currently observing failure on refresh rate change on VRR > > > setup if > > > full modeset is not allowed. This is caused by the mismatch in > > > bpp > > > configured by GOP and bpp value calculated by our driver. > > > Changing bpp to > > > value calculated by our driver would require full mode set. > > > > > > We don't have mechanism to communicate current bpp to userspace - > > > > > > > Userspace can't request to use current bpp. Changing bpp means > > > full > > > modeset. This becomes a problem when userspace haven't allowed > > > full mode > > > set. > > > > > > Complete solution here would mean adding mechanism to communicate > > > current > > > bpp to userspace. User space should use this bpp to avoid > > > changing bpp if > > > it wants to avoid full mode set. > > > > > > Tackle this for now in our driver by using existing bpp if full > > > modeset is > > > not allowed. > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 33 > > > ++++++++++++++------ > > > 1 file changed, 23 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 9049b9a1209d8..7b805998b280a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -4385,21 +4385,34 @@ compute_baseline_pipe_bpp(struct > > > intel_atomic_state *state, > > > struct intel_crtc *crtc) > > > { > > > struct drm_i915_private *dev_priv = to_i915(crtc- > > > >base.dev); > > > - struct intel_crtc_state *crtc_state = > > > + struct intel_crtc_state *new_crtc_state = > > > intel_atomic_get_new_crtc_state(state, crtc); > > > + struct intel_crtc_state *old_crtc_state = > > > + intel_atomic_get_old_crtc_state(state, crtc); > > > struct drm_connector *connector; > > > struct drm_connector_state *connector_state; > > > int bpp, i; > > > > > > - if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || > > > - IS_CHERRYVIEW(dev_priv))) > > > - bpp = 10*3; > > > - else if (DISPLAY_VER(dev_priv) >= 5) > > > - bpp = 12*3; > > > - else > > > - bpp = 8*3; > > > + /* > > > + * TODO: We don't have mechanism to communicate current > > > bpp to > > > + * userspace -> Userspace can't request to use current > > > bpp. Changing bpp > > > + * means full modeset. This becomes a problem when > > > userspace wants to > > > + * avoid full modeset. Tackle this on our driver by using > > > existing bpp > > > + * if full modeset is not allowed. > > > + */ > > > + if (!state->base.allow_modeset) { > > > + bpp = old_crtc_state->pipe_bpp; > > > + } else { > > > + if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) > > > || > > > + IS_CHERRYVIEW(dev_priv))) > > > + bpp = 10 * 3; > > > + else if (DISPLAY_VER(dev_priv) >= 5) > > > + bpp = 12 * 3; > > > + else > > > + bpp = 8 * 3; > > > + } > > > > > > - crtc_state->pipe_bpp = bpp; > > > + new_crtc_state->pipe_bpp = bpp; > > > > > > /* Clamp display bpp to connector max bpp */ > > > for_each_new_connector_in_state(&state->base, connector, > > > connector_state, i) { > > > @@ -4408,7 +4421,7 @@ compute_baseline_pipe_bpp(struct > > > intel_atomic_state *state, > > > if (connector_state->crtc != &crtc->base) > > > continue; > > > > > > - ret = compute_sink_pipe_bpp(connector_state, > > > crtc_state); > > > + ret = compute_sink_pipe_bpp(connector_state, > > > new_crtc_state); > > > if (ret) > > > return ret; > > > } > > >
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 9049b9a1209d8..7b805998b280a 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -4385,21 +4385,34 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state, struct intel_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - struct intel_crtc_state *crtc_state = + struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); + struct intel_crtc_state *old_crtc_state = + intel_atomic_get_old_crtc_state(state, crtc); struct drm_connector *connector; struct drm_connector_state *connector_state; int bpp, i; - if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || - IS_CHERRYVIEW(dev_priv))) - bpp = 10*3; - else if (DISPLAY_VER(dev_priv) >= 5) - bpp = 12*3; - else - bpp = 8*3; + /* + * TODO: We don't have mechanism to communicate current bpp to + * userspace -> Userspace can't request to use current bpp. Changing bpp + * means full modeset. This becomes a problem when userspace wants to + * avoid full modeset. Tackle this on our driver by using existing bpp + * if full modeset is not allowed. + */ + if (!state->base.allow_modeset) { + bpp = old_crtc_state->pipe_bpp; + } else { + if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || + IS_CHERRYVIEW(dev_priv))) + bpp = 10 * 3; + else if (DISPLAY_VER(dev_priv) >= 5) + bpp = 12 * 3; + else + bpp = 8 * 3; + } - crtc_state->pipe_bpp = bpp; + new_crtc_state->pipe_bpp = bpp; /* Clamp display bpp to connector max bpp */ for_each_new_connector_in_state(&state->base, connector, connector_state, i) { @@ -4408,7 +4421,7 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state, if (connector_state->crtc != &crtc->base) continue; - ret = compute_sink_pipe_bpp(connector_state, crtc_state); + ret = compute_sink_pipe_bpp(connector_state, new_crtc_state); if (ret) return ret; }
We are currently observing failure on refresh rate change on VRR setup if full modeset is not allowed. This is caused by the mismatch in bpp configured by GOP and bpp value calculated by our driver. Changing bpp to value calculated by our driver would require full mode set. We don't have mechanism to communicate current bpp to userspace -> Userspace can't request to use current bpp. Changing bpp means full modeset. This becomes a problem when userspace haven't allowed full mode set. Complete solution here would mean adding mechanism to communicate current bpp to userspace. User space should use this bpp to avoid changing bpp if it wants to avoid full mode set. Tackle this for now in our driver by using existing bpp if full modeset is not allowed. Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 33 ++++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-)