subject:"\[PATCH 05\/14\] drm\/msm\/dpu\: enable master\-slave encoders explicitly"

Re: [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

2018-08-31 Thread Sean Paul

On Fri, Aug 31, 2018 at 12:16:46PM -0700, Jeykumar Sankaran wrote:
> On 2018-08-30 09:24, Sean Paul wrote:
> > On Tue, Aug 28, 2018 at 05:39:54PM -0700, Jeykumar Sankaran wrote:
> > > Identify slave-master encoders during initialization and enable
> > > the encoders explicitly as the current logic has redundant and
> > > ambiguous loops.
> > 
> > Please include a "Changes in vX" section so I don't need to figure it
> > out
> > myself.
> > 
> Since I split these clean up changes into a new series, I was not sure
> how to handle the versioning. With "changes in v4" log, Should I also
> change the prefix labeling to [PATCH v4 05/14]? A couple of changes
> are also introduced in this series.

Yeah, sometimes it's best just to choose an arbitrarily high version if multiple
series' are coming together. In this case it's more simple since we're just
adding patches to the series, so v4 would have worked. As far as the new 
patches,
just add:

Changed in v4:
 - Introduced patch (split from "blah blah")
or
Changed in v4:
 - Introduced patch (suggested by Sean)

Sean

> 
> Thanks,
> Jeykumar S.
> 
> > > 
> > > Signed-off-by: Jeykumar Sankaran 
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 46
> > ++---
> > >  1 file changed, 15 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 991b22c..b223bd2 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -180,6 +180,7 @@ struct dpu_encoder_virt {
> > >   unsigned int num_phys_encs;
> > >   struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
> > >   struct dpu_encoder_phys *cur_master;
> > > + struct dpu_encoder_phys *cur_slave;
> > >   struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
> > > 
> > >   bool intfs_swapped;
> > > @@ -1141,7 +1142,8 @@ void dpu_encoder_virt_restore(struct drm_encoder
> > *drm_enc)
> > >  static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
> > >  {
> > >   struct dpu_encoder_virt *dpu_enc = NULL;
> > > - int i, ret = 0;
> > > + struct dpu_encoder_phys *phys  = NULL;
> > > + int ret = 0;
> > >   struct drm_display_mode *cur_mode = NULL;
> > > 
> > >   if (!drm_enc) {
> > > @@ -1154,21 +1156,14 @@ static void dpu_encoder_virt_enable(struct
> > drm_encoder *drm_enc)
> > >   trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
> > >cur_mode->vdisplay);
> > > 
> > > - dpu_enc->cur_master = NULL;
> > > - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > > - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> > > + /* always enable slave encoder before master */
> > > + phys = dpu_enc->cur_slave;
> > 
> > I think this variable makes things less readable. It made sense in the
> > loop since
> > you're indented an extra level and they're obfuscated anyways, but since
> > they're
> > explicit now, could you please just use dpu_enc->cur_slave/master
> > directly?
> > 
> > With this nit and the added changelog in the commit addressed,
> > 
> > Reviewed-by: Sean Paul 
> > 
> > 
> > > + if (phys && phys->ops.enable)
> > > + phys->ops.enable(phys);
> > > 
> > > - if (phys && phys->ops.is_master &&
> > phys->ops.is_master(phys)) {
> > > - DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n",
> > i);
> > > - dpu_enc->cur_master = phys;
> > > - break;
> > > - }
> > > - }
> > > -
> > > - if (!dpu_enc->cur_master) {
> > > - DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
> > > - return;
> > > - }
> > > + phys = dpu_enc->cur_master;
> > > + if (phys && phys->ops.enable)
> > > + phys->ops.enable(phys);
> > > 
> > >   ret = dpu_encoder_resource_control(drm_enc,
> > DPU_ENC_RC_EVENT_KICKOFF);
> > >   if (ret) {
> > > @@ -1177,21 +1172,6 @@ static void dpu_encoder_virt_enable(struct
> > drm_encoder *drm_enc)
> > >   return;
> > >   }
> > > 
> > > - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > > - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> > > -
> > > - if (!phys)
> > > - continue;
> > > -
> > > - if (phys != dpu_enc->cur_master) {
> > > - if (phys->ops.enable)
> > > - phys->ops.enable(phys);
> > > - }
> > > - }
> > > -
> > > - if (dpu_enc->cur_master->ops.enable)
> > > - dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
> > > -
> > >   _dpu_encoder_virt_enable_helper(drm_enc);
> > >  }
> > > 
> > > @@ -2062,6 +2042,11 @@ static int dpu_encoder_virt_add_phys_encs(
> > >   ++dpu_enc->num_phys_encs;
> > >   }
> > > 
> > > + if (params->split_role == ENC_ROLE_SLAVE)
> > > + dpu_enc->cur_slave = enc;
> > > + else
> > > + dpu_enc->cur_master = enc;
> > > +
> > >   return 0;
> > >  }
> > > 
> > > @@ -2228,7 +2213,6 @@ int

Re: [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

2018-08-31 Thread Jeykumar Sankaran


On 2018-08-30 09:24, Sean Paul wrote:

On Tue, Aug 28, 2018 at 05:39:54PM -0700, Jeykumar Sankaran wrote:

Identify slave-master encoders during initialization and enable
the encoders explicitly as the current logic has redundant and
ambiguous loops.


Please include a "Changes in vX" section so I don't need to figure it 
out

myself.


Since I split these clean up changes into a new series, I was not sure
how to handle the versioning. With "changes in v4" log, Should I also
change the prefix labeling to [PATCH v4 05/14]? A couple of changes
are also introduced in this series.

Thanks,
Jeykumar S.



Signed-off-by: Jeykumar Sankaran 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 46

++---

 1 file changed, 15 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

index 991b22c..b223bd2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -180,6 +180,7 @@ struct dpu_encoder_virt {
unsigned int num_phys_encs;
struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
struct dpu_encoder_phys *cur_master;
+   struct dpu_encoder_phys *cur_slave;
struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];

bool intfs_swapped;
@@ -1141,7 +1142,8 @@ void dpu_encoder_virt_restore(struct drm_encoder

*drm_enc)

 static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
 {
struct dpu_encoder_virt *dpu_enc = NULL;
-   int i, ret = 0;
+   struct dpu_encoder_phys *phys  = NULL;
+   int ret = 0;
struct drm_display_mode *cur_mode = NULL;

if (!drm_enc) {
@@ -1154,21 +1156,14 @@ static void dpu_encoder_virt_enable(struct

drm_encoder *drm_enc)

trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
 cur_mode->vdisplay);

-   dpu_enc->cur_master = NULL;
-   for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-   struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+   /* always enable slave encoder before master */
+   phys = dpu_enc->cur_slave;


I think this variable makes things less readable. It made sense in the
loop since
you're indented an extra level and they're obfuscated anyways, but 
since

they're
explicit now, could you please just use dpu_enc->cur_slave/master
directly?

With this nit and the added changelog in the commit addressed,

Reviewed-by: Sean Paul 



+   if (phys && phys->ops.enable)
+   phys->ops.enable(phys);

-   if (phys && phys->ops.is_master &&

phys->ops.is_master(phys)) {

-   DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n",

i);

-   dpu_enc->cur_master = phys;
-   break;
-   }
-   }
-
-   if (!dpu_enc->cur_master) {
-   DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
-   return;
-   }
+   phys = dpu_enc->cur_master;
+   if (phys && phys->ops.enable)
+   phys->ops.enable(phys);

ret = dpu_encoder_resource_control(drm_enc,

DPU_ENC_RC_EVENT_KICKOFF);

if (ret) {
@@ -1177,21 +1172,6 @@ static void dpu_encoder_virt_enable(struct

drm_encoder *drm_enc)

return;
}

-   for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-   struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
-   if (!phys)
-   continue;
-
-   if (phys != dpu_enc->cur_master) {
-   if (phys->ops.enable)
-   phys->ops.enable(phys);
-   }
-   }
-
-   if (dpu_enc->cur_master->ops.enable)
-   dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
-
_dpu_encoder_virt_enable_helper(drm_enc);
 }

@@ -2062,6 +2042,11 @@ static int dpu_encoder_virt_add_phys_encs(
++dpu_enc->num_phys_encs;
}

+   if (params->split_role == ENC_ROLE_SLAVE)
+   dpu_enc->cur_slave = enc;
+   else
+   dpu_enc->cur_master = enc;
+
return 0;
 }

@@ -2228,7 +2213,6 @@ int dpu_encoder_setup(struct drm_device *dev,

struct drm_encoder *enc,

if (ret)
goto fail;

-   dpu_enc->cur_master = NULL;
spin_lock_init(_enc->enc_spinlock);

atomic_set(_enc->frame_done_timeout, 0);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

a Linux Foundation Collaborative Project



--
Jeykumar S
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

2018-08-30 Thread Sean Paul

On Tue, Aug 28, 2018 at 05:39:54PM -0700, Jeykumar Sankaran wrote:
> Identify slave-master encoders during initialization and enable
> the encoders explicitly as the current logic has redundant and
> ambiguous loops.

Please include a "Changes in vX" section so I don't need to figure it out 
myself.

> 
> Signed-off-by: Jeykumar Sankaran 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 46 
> ++---
>  1 file changed, 15 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 991b22c..b223bd2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -180,6 +180,7 @@ struct dpu_encoder_virt {
>   unsigned int num_phys_encs;
>   struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
>   struct dpu_encoder_phys *cur_master;
> + struct dpu_encoder_phys *cur_slave;
>   struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
>  
>   bool intfs_swapped;
> @@ -1141,7 +1142,8 @@ void dpu_encoder_virt_restore(struct drm_encoder 
> *drm_enc)
>  static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
>  {
>   struct dpu_encoder_virt *dpu_enc = NULL;
> - int i, ret = 0;
> + struct dpu_encoder_phys *phys  = NULL;
> + int ret = 0;
>   struct drm_display_mode *cur_mode = NULL;
>  
>   if (!drm_enc) {
> @@ -1154,21 +1156,14 @@ static void dpu_encoder_virt_enable(struct 
> drm_encoder *drm_enc)
>   trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
>cur_mode->vdisplay);
>  
> - dpu_enc->cur_master = NULL;
> - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> + /* always enable slave encoder before master */
> + phys = dpu_enc->cur_slave;

I think this variable makes things less readable. It made sense in the loop 
since
you're indented an extra level and they're obfuscated anyways, but since they're
explicit now, could you please just use dpu_enc->cur_slave/master directly?

With this nit and the added changelog in the commit addressed,

Reviewed-by: Sean Paul 


> + if (phys && phys->ops.enable)
> + phys->ops.enable(phys);
>  
> - if (phys && phys->ops.is_master && phys->ops.is_master(phys)) {
> - DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n", i);
> - dpu_enc->cur_master = phys;
> - break;
> - }
> - }
> -
> - if (!dpu_enc->cur_master) {
> - DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
> - return;
> - }
> + phys = dpu_enc->cur_master;
> + if (phys && phys->ops.enable)
> + phys->ops.enable(phys);
>  
>   ret = dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_KICKOFF);
>   if (ret) {
> @@ -1177,21 +1172,6 @@ static void dpu_encoder_virt_enable(struct drm_encoder 
> *drm_enc)
>   return;
>   }
>  
> - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> -
> - if (!phys)
> - continue;
> -
> - if (phys != dpu_enc->cur_master) {
> - if (phys->ops.enable)
> - phys->ops.enable(phys);
> - }
> - }
> -
> - if (dpu_enc->cur_master->ops.enable)
> - dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
> -
>   _dpu_encoder_virt_enable_helper(drm_enc);
>  }
>  
> @@ -2062,6 +2042,11 @@ static int dpu_encoder_virt_add_phys_encs(
>   ++dpu_enc->num_phys_encs;
>   }
>  
> + if (params->split_role == ENC_ROLE_SLAVE)
> + dpu_enc->cur_slave = enc;
> + else
> + dpu_enc->cur_master = enc;
> +
>   return 0;
>  }
>  
> @@ -2228,7 +2213,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct 
> drm_encoder *enc,
>   if (ret)
>   goto fail;
>  
> - dpu_enc->cur_master = NULL;
>   spin_lock_init(_enc->enc_spinlock);
>  
>   atomic_set(_enc->frame_done_timeout, 0);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

2018-08-28 Thread Jeykumar Sankaran

Identify slave-master encoders during initialization and enable
the encoders explicitly as the current logic has redundant and
ambiguous loops.

Signed-off-by: Jeykumar Sankaran 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 46 ++---
 1 file changed, 15 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 991b22c..b223bd2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -180,6 +180,7 @@ struct dpu_encoder_virt {
unsigned int num_phys_encs;
struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
struct dpu_encoder_phys *cur_master;
+   struct dpu_encoder_phys *cur_slave;
struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
 
bool intfs_swapped;
@@ -1141,7 +1142,8 @@ void dpu_encoder_virt_restore(struct drm_encoder *drm_enc)
 static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
 {
struct dpu_encoder_virt *dpu_enc = NULL;
-   int i, ret = 0;
+   struct dpu_encoder_phys *phys  = NULL;
+   int ret = 0;
struct drm_display_mode *cur_mode = NULL;
 
if (!drm_enc) {
@@ -1154,21 +1156,14 @@ static void dpu_encoder_virt_enable(struct drm_encoder 
*drm_enc)
trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
 cur_mode->vdisplay);
 
-   dpu_enc->cur_master = NULL;
-   for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-   struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+   /* always enable slave encoder before master */
+   phys = dpu_enc->cur_slave;
+   if (phys && phys->ops.enable)
+   phys->ops.enable(phys);
 
-   if (phys && phys->ops.is_master && phys->ops.is_master(phys)) {
-   DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n", i);
-   dpu_enc->cur_master = phys;
-   break;
-   }
-   }
-
-   if (!dpu_enc->cur_master) {
-   DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
-   return;
-   }
+   phys = dpu_enc->cur_master;
+   if (phys && phys->ops.enable)
+   phys->ops.enable(phys);
 
ret = dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_KICKOFF);
if (ret) {
@@ -1177,21 +1172,6 @@ static void dpu_encoder_virt_enable(struct drm_encoder 
*drm_enc)
return;
}
 
-   for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-   struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
-   if (!phys)
-   continue;
-
-   if (phys != dpu_enc->cur_master) {
-   if (phys->ops.enable)
-   phys->ops.enable(phys);
-   }
-   }
-
-   if (dpu_enc->cur_master->ops.enable)
-   dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
-
_dpu_encoder_virt_enable_helper(drm_enc);
 }
 
@@ -2062,6 +2042,11 @@ static int dpu_encoder_virt_add_phys_encs(
++dpu_enc->num_phys_encs;
}
 
+   if (params->split_role == ENC_ROLE_SLAVE)
+   dpu_enc->cur_slave = enc;
+   else
+   dpu_enc->cur_master = enc;
+
return 0;
 }
 
@@ -2228,7 +2213,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct 
drm_encoder *enc,
if (ret)
goto fail;
 
-   dpu_enc->cur_master = NULL;
spin_lock_init(_enc->enc_spinlock);
 
atomic_set(_enc->frame_done_timeout, 0);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

Re: [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

Re: [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

[PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

4 matches

Site Navigation

Mail list logo

Footer information