Re: [Freedreno] [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces
On Fri, 22 Apr 2022 at 21:18, Abhinav Kumar wrote:
>
> Hi Dmitry
>
> On 4/22/2022 3:37 AM, Dmitry Baryshkov wrote:
> > On Fri, 22 Apr 2022 at 04:59, Abhinav Kumar
> > wrote:
> >>
> >>
> >>
> >> On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
> >>> On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar
> >>> wrote:
>
> Hi Dmitry
>
> Thanks for the review.
>
> One question below.
>
> On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
> > On 21/04/2022 23:48, Abhinav Kumar wrote:
> >> Using intf_idx even for writeback interfaces is confusing
> >> because intf_idx is of type enum dpu_intf but the index used
> >> for writeback is of type enum dpu_wb.
> >>
> >> In addition, this makes it easier to separately check the
> >> availability of the two as its possible that there are boards
> >> which don't have any physical display connected but can still
> >> work in writeback mode.
> >>
> >> Signed-off-by: Abhinav Kumar
> >
> > Looks good, two minor issues bellow.
> >
> > With them fixed, I'd even squash this patch into the corresponding patch
> > of the previous patchset.
> >
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 62
> >> +---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ++
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +-
> >> 3 files changed, 40 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> index 9c12841..054d7e4 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -962,7 +962,6 @@ static void
> >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >> struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
> >> int num_lm, num_ctl, num_pp, num_dsc;
> >> unsigned int dsc_mask = 0;
> >> -enum dpu_hw_blk_type blk_type;
> >> int i;
> >> if (!drm_enc) {
> >> @@ -1044,17 +1043,11 @@ static void
> >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >> phys->hw_pp = dpu_enc->hw_pp[i];
> >> phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
> >> -if (dpu_encoder_get_intf_mode(_enc->base) ==
> >> INTF_MODE_WB_LINE)
> >> -blk_type = DPU_HW_BLK_WB;
> >> -else
> >> -blk_type = DPU_HW_BLK_INTF;
> >> +if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
> >> +phys->hw_intf = dpu_rm_get_intf(_kms->rm,
> >> phys->intf_idx);
> >> -if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
> >> -if (blk_type == DPU_HW_BLK_INTF)
> >> -phys->hw_intf = dpu_rm_get_intf(_kms->rm,
> >> phys->intf_idx);
> >> -else if (blk_type == DPU_HW_BLK_WB)
> >> -phys->hw_wb = dpu_rm_get_wb(_kms->rm,
> >> phys->intf_idx);
> >> -}
> >> +if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
> >> +phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
> >
> > We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
>
> So there is an error if
>
> 1) Neither wb NOR intf are present
> 2) Both wb AND intf are present for the physical encoder?
>
> The second check is okay for now to add but considering concurrent
> writeback then that wouldn't assumption be wrong since both physical and
> wb interfaces can go with the same encoder?
> >>>
> >>> To the same encoder, but not to the same physical encoder. Here we
> >>> check the phys_enc parameters.
> >>
> >> Ok got it, let me re-spin this RFC with patches 2 & 3 squashed.
> >> Get the acks on them.
> >>
> >> Then will absorb into WB series and re-post it.
> >
> > Sounds like a good plan!
> >
> >>
> >>>
>
> >
> >> if (!phys->hw_intf && !phys->hw_wb) {
> >> DPU_ERROR_ENC(dpu_enc,
> >> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
> >> drm_encoder *drm_enc)
> >> mutex_unlock(_enc->enc_lock);
> >> }
> >> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
> >> *catalog,
> >> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg
> >> *catalog,
> >> enum dpu_intf_type type, u32 controller_id)
> >> {
> >> int i = 0;
> >> @@ -1213,16 +1206,28 @@ static enum dpu_intf
> >> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
> >> return catalog->intf[i].id;
> >> }
> >> }
> >> -} else {
> >> -for (i = 0; i < catalog->wb_count; i++) {
> >> -
Re: [Freedreno] [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces
Hi Dmitry
On 4/22/2022 3:37 AM, Dmitry Baryshkov wrote:
On Fri, 22 Apr 2022 at 04:59, Abhinav Kumar wrote:
On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar wrote:
Hi Dmitry
Thanks for the review.
One question below.
On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
On 21/04/2022 23:48, Abhinav Kumar wrote:
Using intf_idx even for writeback interfaces is confusing
because intf_idx is of type enum dpu_intf but the index used
for writeback is of type enum dpu_wb.
In addition, this makes it easier to separately check the
availability of the two as its possible that there are boards
which don't have any physical display connected but can still
work in writeback mode.
Signed-off-by: Abhinav Kumar
Looks good, two minor issues bellow.
With them fixed, I'd even squash this patch into the corresponding patch
of the previous patchset.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 62
+---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +-
3 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c12841..054d7e4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -962,7 +962,6 @@ static void
dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
int num_lm, num_ctl, num_pp, num_dsc;
unsigned int dsc_mask = 0;
-enum dpu_hw_blk_type blk_type;
int i;
if (!drm_enc) {
@@ -1044,17 +1043,11 @@ static void
dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
phys->hw_pp = dpu_enc->hw_pp[i];
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
-if (dpu_encoder_get_intf_mode(_enc->base) ==
INTF_MODE_WB_LINE)
-blk_type = DPU_HW_BLK_WB;
-else
-blk_type = DPU_HW_BLK_INTF;
+if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
+phys->hw_intf = dpu_rm_get_intf(_kms->rm,
phys->intf_idx);
-if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
-if (blk_type == DPU_HW_BLK_INTF)
-phys->hw_intf = dpu_rm_get_intf(_kms->rm,
phys->intf_idx);
-else if (blk_type == DPU_HW_BLK_WB)
-phys->hw_wb = dpu_rm_get_wb(_kms->rm,
phys->intf_idx);
-}
+if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
+phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
So there is an error if
1) Neither wb NOR intf are present
2) Both wb AND intf are present for the physical encoder?
The second check is okay for now to add but considering concurrent
writeback then that wouldn't assumption be wrong since both physical and
wb interfaces can go with the same encoder?
To the same encoder, but not to the same physical encoder. Here we
check the phys_enc parameters.
Ok got it, let me re-spin this RFC with patches 2 & 3 squashed.
Get the acks on them.
Then will absorb into WB series and re-post it.
Sounds like a good plan!
if (!phys->hw_intf && !phys->hw_wb) {
DPU_ERROR_ENC(dpu_enc,
@@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
mutex_unlock(_enc->enc_lock);
}
-static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
*catalog,
+static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
enum dpu_intf_type type, u32 controller_id)
{
int i = 0;
@@ -1213,16 +1206,28 @@ static enum dpu_intf
dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
return catalog->intf[i].id;
}
}
-} else {
-for (i = 0; i < catalog->wb_count; i++) {
-if (catalog->wb[i].id == controller_id)
-return catalog->wb[i].id;
-}
}
return INTF_MAX;
}
+static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
+enum dpu_intf_type type, u32 controller_id)
+{
+int i = 0;
+
+if (type != INTF_WB)
+goto end;
+
+for (i = 0; i < catalog->wb_count; i++) {
+if (catalog->wb[i].id == controller_id)
+return catalog->wb[i].id;
+}
+
+end:
+return WB_MAX;
I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
ack, i guess in that case even the places checking the return value of
this function need to be changed.
Yes, of course.
INTF_NONE/WB_NONE is not of enum dpu_intf or enum dpu_wb, its of type
enum dpu_intf_mode
Do we want to add them to dpu_intf/dpu_wb with a -1 value OR leave it
as-it-is.
+}
+
static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
struct
Re: [Freedreno] [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces
On Fri, 22 Apr 2022 at 04:59, Abhinav Kumar wrote:
>
>
>
> On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
> > On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar
> > wrote:
> >>
> >> Hi Dmitry
> >>
> >> Thanks for the review.
> >>
> >> One question below.
> >>
> >> On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
> >>> On 21/04/2022 23:48, Abhinav Kumar wrote:
> Using intf_idx even for writeback interfaces is confusing
> because intf_idx is of type enum dpu_intf but the index used
> for writeback is of type enum dpu_wb.
>
> In addition, this makes it easier to separately check the
> availability of the two as its possible that there are boards
> which don't have any physical display connected but can still
> work in writeback mode.
>
> Signed-off-by: Abhinav Kumar
> >>>
> >>> Looks good, two minor issues bellow.
> >>>
> >>> With them fixed, I'd even squash this patch into the corresponding patch
> >>> of the previous patchset.
> >>>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 62
> +---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +-
> 3 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c12841..054d7e4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -962,7 +962,6 @@ static void
> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
> int num_lm, num_ctl, num_pp, num_dsc;
> unsigned int dsc_mask = 0;
> -enum dpu_hw_blk_type blk_type;
> int i;
> if (!drm_enc) {
> @@ -1044,17 +1043,11 @@ static void
> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> phys->hw_pp = dpu_enc->hw_pp[i];
> phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
> -if (dpu_encoder_get_intf_mode(_enc->base) ==
> INTF_MODE_WB_LINE)
> -blk_type = DPU_HW_BLK_WB;
> -else
> -blk_type = DPU_HW_BLK_INTF;
> +if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
> +phys->hw_intf = dpu_rm_get_intf(_kms->rm,
> phys->intf_idx);
> -if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
> -if (blk_type == DPU_HW_BLK_INTF)
> -phys->hw_intf = dpu_rm_get_intf(_kms->rm,
> phys->intf_idx);
> -else if (blk_type == DPU_HW_BLK_WB)
> -phys->hw_wb = dpu_rm_get_wb(_kms->rm,
> phys->intf_idx);
> -}
> +if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
> +phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
> >>>
> >>> We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
> >>
> >> So there is an error if
> >>
> >> 1) Neither wb NOR intf are present
> >> 2) Both wb AND intf are present for the physical encoder?
> >>
> >> The second check is okay for now to add but considering concurrent
> >> writeback then that wouldn't assumption be wrong since both physical and
> >> wb interfaces can go with the same encoder?
> >
> > To the same encoder, but not to the same physical encoder. Here we
> > check the phys_enc parameters.
>
> Ok got it, let me re-spin this RFC with patches 2 & 3 squashed.
> Get the acks on them.
>
> Then will absorb into WB series and re-post it.
Sounds like a good plan!
>
> >
> >>
> >>>
> if (!phys->hw_intf && !phys->hw_wb) {
> DPU_ERROR_ENC(dpu_enc,
> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
> mutex_unlock(_enc->enc_lock);
> }
> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
> *catalog,
> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> enum dpu_intf_type type, u32 controller_id)
> {
> int i = 0;
> @@ -1213,16 +1206,28 @@ static enum dpu_intf
> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
> return catalog->intf[i].id;
> }
> }
> -} else {
> -for (i = 0; i < catalog->wb_count; i++) {
> -if (catalog->wb[i].id == controller_id)
> -return catalog->wb[i].id;
> -}
> }
> return INTF_MAX;
> }
> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
> +enum dpu_intf_type type, u32 controller_id)
> +{
> +int i = 0;
> +
> +if (type != INTF_WB)
> +goto end;
>
Re: [Freedreno] [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces
On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar wrote:
Hi Dmitry
Thanks for the review.
One question below.
On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
On 21/04/2022 23:48, Abhinav Kumar wrote:
Using intf_idx even for writeback interfaces is confusing
because intf_idx is of type enum dpu_intf but the index used
for writeback is of type enum dpu_wb.
In addition, this makes it easier to separately check the
availability of the two as its possible that there are boards
which don't have any physical display connected but can still
work in writeback mode.
Signed-off-by: Abhinav Kumar
Looks good, two minor issues bellow.
With them fixed, I'd even squash this patch into the corresponding patch
of the previous patchset.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 62
+---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +-
3 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c12841..054d7e4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -962,7 +962,6 @@ static void
dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
int num_lm, num_ctl, num_pp, num_dsc;
unsigned int dsc_mask = 0;
-enum dpu_hw_blk_type blk_type;
int i;
if (!drm_enc) {
@@ -1044,17 +1043,11 @@ static void
dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
phys->hw_pp = dpu_enc->hw_pp[i];
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
-if (dpu_encoder_get_intf_mode(_enc->base) ==
INTF_MODE_WB_LINE)
-blk_type = DPU_HW_BLK_WB;
-else
-blk_type = DPU_HW_BLK_INTF;
+if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
+phys->hw_intf = dpu_rm_get_intf(_kms->rm,
phys->intf_idx);
-if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
-if (blk_type == DPU_HW_BLK_INTF)
-phys->hw_intf = dpu_rm_get_intf(_kms->rm,
phys->intf_idx);
-else if (blk_type == DPU_HW_BLK_WB)
-phys->hw_wb = dpu_rm_get_wb(_kms->rm,
phys->intf_idx);
-}
+if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
+phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
So there is an error if
1) Neither wb NOR intf are present
2) Both wb AND intf are present for the physical encoder?
The second check is okay for now to add but considering concurrent
writeback then that wouldn't assumption be wrong since both physical and
wb interfaces can go with the same encoder?
To the same encoder, but not to the same physical encoder. Here we
check the phys_enc parameters.
Ok got it, let me re-spin this RFC with patches 2 & 3 squashed.
Get the acks on them.
Then will absorb into WB series and re-post it.
if (!phys->hw_intf && !phys->hw_wb) {
DPU_ERROR_ENC(dpu_enc,
@@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
mutex_unlock(_enc->enc_lock);
}
-static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
*catalog,
+static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
enum dpu_intf_type type, u32 controller_id)
{
int i = 0;
@@ -1213,16 +1206,28 @@ static enum dpu_intf
dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
return catalog->intf[i].id;
}
}
-} else {
-for (i = 0; i < catalog->wb_count; i++) {
-if (catalog->wb[i].id == controller_id)
-return catalog->wb[i].id;
-}
}
return INTF_MAX;
}
+static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
+enum dpu_intf_type type, u32 controller_id)
+{
+int i = 0;
+
+if (type != INTF_WB)
+goto end;
+
+for (i = 0; i < catalog->wb_count; i++) {
+if (catalog->wb[i].id == controller_id)
+return catalog->wb[i].id;
+}
+
+end:
+return WB_MAX;
I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
ack, i guess in that case even the places checking the return value of
this function need to be changed.
Yes, of course.
+}
+
static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
{
@@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
i, controller_id, phys_params.split_role);
-/*
- * FIXME: have separate intf_idx and wb_idx to avoid using
- * enum
Re: [Freedreno] [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces
On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar wrote:
>
> Hi Dmitry
>
> Thanks for the review.
>
> One question below.
>
> On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
> > On 21/04/2022 23:48, Abhinav Kumar wrote:
> >> Using intf_idx even for writeback interfaces is confusing
> >> because intf_idx is of type enum dpu_intf but the index used
> >> for writeback is of type enum dpu_wb.
> >>
> >> In addition, this makes it easier to separately check the
> >> availability of the two as its possible that there are boards
> >> which don't have any physical display connected but can still
> >> work in writeback mode.
> >>
> >> Signed-off-by: Abhinav Kumar
> >
> > Looks good, two minor issues bellow.
> >
> > With them fixed, I'd even squash this patch into the corresponding patch
> > of the previous patchset.
> >
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 62
> >> +---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ++
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +-
> >> 3 files changed, 40 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> index 9c12841..054d7e4 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -962,7 +962,6 @@ static void
> >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >> struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
> >> int num_lm, num_ctl, num_pp, num_dsc;
> >> unsigned int dsc_mask = 0;
> >> -enum dpu_hw_blk_type blk_type;
> >> int i;
> >> if (!drm_enc) {
> >> @@ -1044,17 +1043,11 @@ static void
> >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >> phys->hw_pp = dpu_enc->hw_pp[i];
> >> phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
> >> -if (dpu_encoder_get_intf_mode(_enc->base) ==
> >> INTF_MODE_WB_LINE)
> >> -blk_type = DPU_HW_BLK_WB;
> >> -else
> >> -blk_type = DPU_HW_BLK_INTF;
> >> +if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
> >> +phys->hw_intf = dpu_rm_get_intf(_kms->rm,
> >> phys->intf_idx);
> >> -if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
> >> -if (blk_type == DPU_HW_BLK_INTF)
> >> -phys->hw_intf = dpu_rm_get_intf(_kms->rm,
> >> phys->intf_idx);
> >> -else if (blk_type == DPU_HW_BLK_WB)
> >> -phys->hw_wb = dpu_rm_get_wb(_kms->rm,
> >> phys->intf_idx);
> >> -}
> >> +if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
> >> +phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
> >
> > We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
>
> So there is an error if
>
> 1) Neither wb NOR intf are present
> 2) Both wb AND intf are present for the physical encoder?
>
> The second check is okay for now to add but considering concurrent
> writeback then that wouldn't assumption be wrong since both physical and
> wb interfaces can go with the same encoder?
To the same encoder, but not to the same physical encoder. Here we
check the phys_enc parameters.
>
> >
> >> if (!phys->hw_intf && !phys->hw_wb) {
> >> DPU_ERROR_ENC(dpu_enc,
> >> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
> >> drm_encoder *drm_enc)
> >> mutex_unlock(_enc->enc_lock);
> >> }
> >> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
> >> *catalog,
> >> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> >> enum dpu_intf_type type, u32 controller_id)
> >> {
> >> int i = 0;
> >> @@ -1213,16 +1206,28 @@ static enum dpu_intf
> >> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
> >> return catalog->intf[i].id;
> >> }
> >> }
> >> -} else {
> >> -for (i = 0; i < catalog->wb_count; i++) {
> >> -if (catalog->wb[i].id == controller_id)
> >> -return catalog->wb[i].id;
> >> -}
> >> }
> >> return INTF_MAX;
> >> }
> >> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
> >> +enum dpu_intf_type type, u32 controller_id)
> >> +{
> >> +int i = 0;
> >> +
> >> +if (type != INTF_WB)
> >> +goto end;
> >> +
> >> +for (i = 0; i < catalog->wb_count; i++) {
> >> +if (catalog->wb[i].id == controller_id)
> >> +return catalog->wb[i].id;
> >> +}
> >> +
> >> +end:
> >> +return WB_MAX;
> >
> > I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
> ack, i guess in that case even the places checking the return value of
> this function need to be changed.
Yes, of course.
> >
> >> +}
> >> +
> >> static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> >> struct dpu_encoder_phys *phy_enc)
> >>
Re: [Freedreno] [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces
Hi Dmitry
Thanks for the review.
One question below.
On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
On 21/04/2022 23:48, Abhinav Kumar wrote:
Using intf_idx even for writeback interfaces is confusing
because intf_idx is of type enum dpu_intf but the index used
for writeback is of type enum dpu_wb.
In addition, this makes it easier to separately check the
availability of the two as its possible that there are boards
which don't have any physical display connected but can still
work in writeback mode.
Signed-off-by: Abhinav Kumar
Looks good, two minor issues bellow.
With them fixed, I'd even squash this patch into the corresponding patch
of the previous patchset.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 62
+---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +-
3 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c12841..054d7e4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -962,7 +962,6 @@ static void
dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
int num_lm, num_ctl, num_pp, num_dsc;
unsigned int dsc_mask = 0;
- enum dpu_hw_blk_type blk_type;
int i;
if (!drm_enc) {
@@ -1044,17 +1043,11 @@ static void
dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
phys->hw_pp = dpu_enc->hw_pp[i];
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
- if (dpu_encoder_get_intf_mode(_enc->base) ==
INTF_MODE_WB_LINE)
- blk_type = DPU_HW_BLK_WB;
- else
- blk_type = DPU_HW_BLK_INTF;
+ if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
+ phys->hw_intf = dpu_rm_get_intf(_kms->rm,
phys->intf_idx);
- if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
- if (blk_type == DPU_HW_BLK_INTF)
- phys->hw_intf = dpu_rm_get_intf(_kms->rm,
phys->intf_idx);
- else if (blk_type == DPU_HW_BLK_WB)
- phys->hw_wb = dpu_rm_get_wb(_kms->rm,
phys->intf_idx);
- }
+ if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
+ phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
So there is an error if
1) Neither wb NOR intf are present
2) Both wb AND intf are present for the physical encoder?
The second check is okay for now to add but considering concurrent
writeback then that wouldn't assumption be wrong since both physical and
wb interfaces can go with the same encoder?
if (!phys->hw_intf && !phys->hw_wb) {
DPU_ERROR_ENC(dpu_enc,
@@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
mutex_unlock(_enc->enc_lock);
}
-static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
*catalog,
+static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
enum dpu_intf_type type, u32 controller_id)
{
int i = 0;
@@ -1213,16 +1206,28 @@ static enum dpu_intf
dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
return catalog->intf[i].id;
}
}
- } else {
- for (i = 0; i < catalog->wb_count; i++) {
- if (catalog->wb[i].id == controller_id)
- return catalog->wb[i].id;
- }
}
return INTF_MAX;
}
+static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
+ enum dpu_intf_type type, u32 controller_id)
+{
+ int i = 0;
+
+ if (type != INTF_WB)
+ goto end;
+
+ for (i = 0; i < catalog->wb_count; i++) {
+ if (catalog->wb[i].id == controller_id)
+ return catalog->wb[i].id;
+ }
+
+end:
+ return WB_MAX;
I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
ack, i guess in that case even the places checking the return value of
this function need to be changed.
+}
+
static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
{
@@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
i, controller_id, phys_params.split_role);
- /*
- * FIXME: have separate intf_idx and wb_idx to avoid using
- * enum dpu_intf type for wb_idx and also to be able to
- * not bail out when there is no intf for boards which dont
- * have a display connected to them.
- * Having a valid wb_idx but not a intf_idx can be a valid
- * combination moving forward.
- */
- phys_params.intf_idx =
dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
+
Re: [Freedreno] [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces
On 21/04/2022 23:48, Abhinav Kumar wrote:
Using intf_idx even for writeback interfaces is confusing
because intf_idx is of type enum dpu_intf but the index used
for writeback is of type enum dpu_wb.
In addition, this makes it easier to separately check the
availability of the two as its possible that there are boards
which don't have any physical display connected but can still
work in writeback mode.
Signed-off-by: Abhinav Kumar
Looks good, two minor issues bellow.
With them fixed, I'd even squash this patch into the corresponding patch
of the previous patchset.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 62 +---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +-
3 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c12841..054d7e4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -962,7 +962,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct
drm_encoder *drm_enc,
struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
int num_lm, num_ctl, num_pp, num_dsc;
unsigned int dsc_mask = 0;
- enum dpu_hw_blk_type blk_type;
int i;
if (!drm_enc) {
@@ -1044,17 +1043,11 @@ static void dpu_encoder_virt_atomic_mode_set(struct
drm_encoder *drm_enc,
phys->hw_pp = dpu_enc->hw_pp[i];
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
- if (dpu_encoder_get_intf_mode(_enc->base) == INTF_MODE_WB_LINE)
- blk_type = DPU_HW_BLK_WB;
- else
- blk_type = DPU_HW_BLK_INTF;
+ if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
+ phys->hw_intf = dpu_rm_get_intf(_kms->rm,
phys->intf_idx);
- if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
- if (blk_type == DPU_HW_BLK_INTF)
- phys->hw_intf = dpu_rm_get_intf(_kms->rm,
phys->intf_idx);
- else if (blk_type == DPU_HW_BLK_WB)
- phys->hw_wb = dpu_rm_get_wb(_kms->rm,
phys->intf_idx);
- }
+ if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
+ phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
if (!phys->hw_intf && !phys->hw_wb) {
DPU_ERROR_ENC(dpu_enc,
@@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
mutex_unlock(_enc->enc_lock);
}
-static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
+static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
enum dpu_intf_type type, u32 controller_id)
{
int i = 0;
@@ -1213,16 +1206,28 @@ static enum dpu_intf dpu_encoder_get_intf_or_wb(struct
dpu_mdss_cfg *catalog,
return catalog->intf[i].id;
}
}
- } else {
- for (i = 0; i < catalog->wb_count; i++) {
- if (catalog->wb[i].id == controller_id)
- return catalog->wb[i].id;
- }
}
return INTF_MAX;
}
+static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
+ enum dpu_intf_type type, u32 controller_id)
+{
+ int i = 0;
+
+ if (type != INTF_WB)
+ goto end;
+
+ for (i = 0; i < catalog->wb_count; i++) {
+ if (catalog->wb[i].id == controller_id)
+ return catalog->wb[i].id;
+ }
+
+end:
+ return WB_MAX;
I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
+}
+
static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
{
@@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
i, controller_id, phys_params.split_role);
- /*
-* FIXME: have separate intf_idx and wb_idx to avoid using
-* enum dpu_intf type for wb_idx and also to be able to
-* not bail out when there is no intf for boards which dont
-* have a display connected to them.
-* Having a valid wb_idx but not a intf_idx can be a valid
-* combination moving forward.
-*/
- phys_params.intf_idx =
dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
+ phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
