Re: [Freedreno] [DPU PATCH 01/11] drm/msm: Skip seamless disables in crtc/encoder

[Jeykumar Sankaran]

On 2018-03-12 13:14, Sean Paul wrote:

On Fri, Mar 02, 2018 at 04:04:24PM -0800, jsa...@codeaurora.org wrote:

On 2018-02-28 11:18, Sean Paul wrote:
> Instead of duplicating whole swaths of atomic helper functions (which
> are already out-of-date), just skip the encoder/crtc disables in the
> .disable hooks.
>
> Change-Id: I7bd9183ae60624204fb1de9550656b776efc7202
> Signed-off-by: Sean Paul 


Reviewed-by: Jeykumar Sankaran 



Can you consider getting rid of these checks?


Do you mean the Change-Id? Yeah, I forgot to strip them out before
sending, I'll
make sure I clean it up before I apply.

Actually, I meant removing the seamless check flags that you are moving 
to

encode/crtc. But you can ignore that, I am planning to submit a seperate
change to remove the support from the whole pipeline.


> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c|   8 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |   8 +
>  drivers/gpu/drm/msm/msm_atomic.c| 185

+---

>  3 files changed, 17 insertions(+), 184 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 3cdf1e3d9d96..a3ab6ed2bf1d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -3393,6 +3393,7 @@ static void dpu_crtc_disable(struct drm_crtc
> *crtc)
>  {
>struct dpu_crtc *dpu_crtc;
>struct dpu_crtc_state *cstate;
> +  struct drm_display_mode *mode;
>struct drm_encoder *encoder;
>struct msm_drm_private *priv;
>unsigned long flags;
> @@ -3407,8 +3408,15 @@ static void dpu_crtc_disable(struct drm_crtc
> *crtc)
>}
>dpu_crtc = to_dpu_crtc(crtc);
>cstate = to_dpu_crtc_state(crtc->state);
> +  mode = >base.adjusted_mode;
>priv = crtc->dev->dev_private;
>
> +  if (msm_is_mode_seamless(mode) || msm_is_mode_seamless_vrr(mode)
> ||
> +  msm_is_mode_seamless_dms(mode)) {
> +  DPU_DEBUG("Seamless mode is being applied, skip
> disable\n");
> +  return;
> +  }
> +
Another topic of discussion which should be brought up with dri-devel.

May not be common in PC world, but there are a handful of mobile OEM's
using panels which supports more than one resolution. Primary use 
cases

involve "seamless" switching to optimized display resolution when
streaming content changes resolutions or rendering lossless data.


Yeah, I think we can do this under the covers if the hardware supports 
it

such
as this patch. We could probably do a better job of making this useful 
for

other
drivers, but I was really just trying to get the seamless stuff out of 
the

way
so we don't need to roll our own atomic commit.

Sean



Jeykumar S.

>DPU_DEBUG("crtc%d\n", crtc->base.id);
>
>if (dpu_kms_is_suspend_state(crtc->dev))
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3d168fa09f3f..28ceb589ee40 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2183,6 +2183,7 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
>struct dpu_encoder_virt *dpu_enc = NULL;
>struct msm_drm_private *priv;
>struct dpu_kms *dpu_kms;
> +  struct drm_display_mode *mode;
>int i = 0;
>
>if (!drm_enc) {
> @@ -2196,6 +2197,13 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
>return;
>}
>
> +  mode = _enc->crtc->state->adjusted_mode;
> +  if (msm_is_mode_seamless(mode) || msm_is_mode_seamless_vrr(mode)
> ||
> +  msm_is_mode_seamless_dms(mode)) {
> +  DPU_DEBUG("Seamless mode is being applied, skip
> disable\n");
> +  return;
> +  }
> +
>dpu_enc = to_dpu_encoder_virt(drm_enc);
>DPU_DEBUG_ENC(dpu_enc, "\n");
>
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> b/drivers/gpu/drm/msm/msm_atomic.c
> index 46536edb72ee..5cfb80345052 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -84,189 +84,6 @@ static void msm_atomic_wait_for_commit_done(
>}
>  }
>
> -static void
> -msm_disable_outputs(struct drm_device *dev, struct drm_atomic_state
> *old_state)
> -{
> -  struct drm_connector *connector;
> -  struct drm_connector_state *old_conn_state, *new_conn_state;
> -  struct drm_crtc *crtc;
> -  struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> -  int i;
> -
> -  for_each_oldnew_connector_in_state(old_state, connector,
> old_conn_state, new_conn_state, i) {
> -  const struct drm_encoder_helper_funcs *funcs;
> -  struct drm_encoder *encoder;
> -  struct drm_crtc_state *old_crtc_state;
> -  unsigned int crtc_idx;
> -
> -  /*
> -   * Shut down everything that's in the changeset and
> currently
> -   * still on. So need to check the old, saved state.
> -   */
> -  if (!old_conn_state->crtc)
> -  

Re: [Freedreno] [DPU PATCH 01/11] drm/msm: Skip seamless disables in crtc/encoder

[Sean Paul]

On Fri, Mar 02, 2018 at 04:04:24PM -0800, jsa...@codeaurora.org wrote:
> On 2018-02-28 11:18, Sean Paul wrote:
> > Instead of duplicating whole swaths of atomic helper functions (which
> > are already out-of-date), just skip the encoder/crtc disables in the
> > .disable hooks.
> > 
> > Change-Id: I7bd9183ae60624204fb1de9550656b776efc7202
> > Signed-off-by: Sean Paul 
> 
> Can you consider getting rid of these checks?

Do you mean the Change-Id? Yeah, I forgot to strip them out before sending, I'll
make sure I clean it up before I apply.

> 
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c|   8 +
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |   8 +
> >  drivers/gpu/drm/msm/msm_atomic.c| 185 +---
> >  3 files changed, 17 insertions(+), 184 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 3cdf1e3d9d96..a3ab6ed2bf1d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -3393,6 +3393,7 @@ static void dpu_crtc_disable(struct drm_crtc
> > *crtc)
> >  {
> > struct dpu_crtc *dpu_crtc;
> > struct dpu_crtc_state *cstate;
> > +   struct drm_display_mode *mode;
> > struct drm_encoder *encoder;
> > struct msm_drm_private *priv;
> > unsigned long flags;
> > @@ -3407,8 +3408,15 @@ static void dpu_crtc_disable(struct drm_crtc
> > *crtc)
> > }
> > dpu_crtc = to_dpu_crtc(crtc);
> > cstate = to_dpu_crtc_state(crtc->state);
> > +   mode = >base.adjusted_mode;
> > priv = crtc->dev->dev_private;
> > 
> > +   if (msm_is_mode_seamless(mode) || msm_is_mode_seamless_vrr(mode)
> > ||
> > +   msm_is_mode_seamless_dms(mode)) {
> > +   DPU_DEBUG("Seamless mode is being applied, skip
> > disable\n");
> > +   return;
> > +   }
> > +
> Another topic of discussion which should be brought up with dri-devel.
> 
> May not be common in PC world, but there are a handful of mobile OEM's
> using panels which supports more than one resolution. Primary use cases
> involve "seamless" switching to optimized display resolution when
> streaming content changes resolutions or rendering lossless data.

Yeah, I think we can do this under the covers if the hardware supports it such
as this patch. We could probably do a better job of making this useful for other
drivers, but I was really just trying to get the seamless stuff out of the way
so we don't need to roll our own atomic commit.

Sean

> 
> Jeykumar S.
> 
> > DPU_DEBUG("crtc%d\n", crtc->base.id);
> > 
> > if (dpu_kms_is_suspend_state(crtc->dev))
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 3d168fa09f3f..28ceb589ee40 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -2183,6 +2183,7 @@ static void dpu_encoder_virt_disable(struct
> > drm_encoder *drm_enc)
> > struct dpu_encoder_virt *dpu_enc = NULL;
> > struct msm_drm_private *priv;
> > struct dpu_kms *dpu_kms;
> > +   struct drm_display_mode *mode;
> > int i = 0;
> > 
> > if (!drm_enc) {
> > @@ -2196,6 +2197,13 @@ static void dpu_encoder_virt_disable(struct
> > drm_encoder *drm_enc)
> > return;
> > }
> > 
> > +   mode = _enc->crtc->state->adjusted_mode;
> > +   if (msm_is_mode_seamless(mode) || msm_is_mode_seamless_vrr(mode)
> > ||
> > +   msm_is_mode_seamless_dms(mode)) {
> > +   DPU_DEBUG("Seamless mode is being applied, skip
> > disable\n");
> > +   return;
> > +   }
> > +
> > dpu_enc = to_dpu_encoder_virt(drm_enc);
> > DPU_DEBUG_ENC(dpu_enc, "\n");
> > 
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> > b/drivers/gpu/drm/msm/msm_atomic.c
> > index 46536edb72ee..5cfb80345052 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -84,189 +84,6 @@ static void msm_atomic_wait_for_commit_done(
> > }
> >  }
> > 
> > -static void
> > -msm_disable_outputs(struct drm_device *dev, struct drm_atomic_state
> > *old_state)
> > -{
> > -   struct drm_connector *connector;
> > -   struct drm_connector_state *old_conn_state, *new_conn_state;
> > -   struct drm_crtc *crtc;
> > -   struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > -   int i;
> > -
> > -   for_each_oldnew_connector_in_state(old_state, connector,
> > old_conn_state, new_conn_state, i) {
> > -   const struct drm_encoder_helper_funcs *funcs;
> > -   struct drm_encoder *encoder;
> > -   struct drm_crtc_state *old_crtc_state;
> > -   unsigned int crtc_idx;
> > -
> > -   /*
> > -* Shut down everything that's in the changeset and
> > currently
> > -* still on. So need to check the old, saved state.
> > -*/
> > -   if (!old_conn_state->crtc)
> > -   continue;
> > -
> > -