| Message ID | 20230430235732.3341119-7-dmitry.baryshkov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: simplify DPU encoder init | expand |
On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: > There is little sense to get intf index just to call dpu_rm_get_intf() > on it. Move dpu_rm_get_intf() call to dpu_encoder_get_intf() function. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++------------ > 1 file changed, 8 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 507ff3f88c67..b35e92c658ad 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1259,22 +1259,23 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, > mutex_unlock(&dpu_enc->enc_lock); > } > > -static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog, > +static struct dpu_hw_intf *dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog, > + struct dpu_rm *dpu_rm, > enum dpu_intf_type type, u32 controller_id) > { > int i = 0; > > if (type == INTF_WB) > - return INTF_MAX; > + return NULL; > > for (i = 0; i < catalog->intf_count; i++) { > if (catalog->intf[i].type == type > && catalog->intf[i].controller_id == controller_id) { > - return catalog->intf[i].id; > + return dpu_rm_get_intf(dpu_rm, catalog->intf[i].id); > } Why has the for loop been retained in this function but not for writeback? is there any difference? Doesnt looks like there needs to be. > } > > - return INTF_MAX; > + return NULL; > } > > void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, > @@ -2244,7 +2245,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right > */ > u32 controller_id = disp_info->h_tile_instance[i]; > - enum dpu_intf intf_idx; > > if (disp_info->num_of_h_tiles > 1) { > if (i == 0) > @@ -2258,12 +2258,9 @@ 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); > > - intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, > - disp_info->intf_type, > - controller_id); > - > - if (intf_idx >= INTF_0 && intf_idx < INTF_MAX) > - phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, intf_idx); > + phys_params.hw_intf = dpu_encoder_get_intf(dpu_kms->catalog, &dpu_kms->rm, > + disp_info->intf_type, > + controller_id); > > if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX) > phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, controller_id); > @@ -2287,7 +2284,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n"); > break; > } > - unnecessary change? > } > > mutex_unlock(&dpu_enc->enc_lock);
On 03/05/2023 02:57, Abhinav Kumar wrote: > > > On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: >> There is little sense to get intf index just to call dpu_rm_get_intf() >> on it. Move dpu_rm_get_intf() call to dpu_encoder_get_intf() function. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++------------ >> 1 file changed, 8 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 507ff3f88c67..b35e92c658ad 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -1259,22 +1259,23 @@ static void >> dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, >> mutex_unlock(&dpu_enc->enc_lock); >> } >> -static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg >> *catalog, >> +static struct dpu_hw_intf *dpu_encoder_get_intf(const struct >> dpu_mdss_cfg *catalog, >> + struct dpu_rm *dpu_rm, >> enum dpu_intf_type type, u32 controller_id) >> { >> int i = 0; >> if (type == INTF_WB) >> - return INTF_MAX; >> + return NULL; >> for (i = 0; i < catalog->intf_count; i++) { >> if (catalog->intf[i].type == type >> && catalog->intf[i].controller_id == controller_id) { >> - return catalog->intf[i].id; >> + return dpu_rm_get_intf(dpu_rm, catalog->intf[i].id); >> } > > Why has the for loop been retained in this function but not for > writeback? is there any difference? Doesnt looks like there needs to be. For writeback we always return controller_id (WB_2). For interfaces we have to map type+controller_id to the INTF instance. > >> } >> - return INTF_MAX; >> + return NULL; >> } >> void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, >> @@ -2244,7 +2245,6 @@ static int dpu_encoder_setup_display(struct >> dpu_encoder_virt *dpu_enc, >> * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right >> */ >> u32 controller_id = disp_info->h_tile_instance[i]; >> - enum dpu_intf intf_idx; >> if (disp_info->num_of_h_tiles > 1) { >> if (i == 0) >> @@ -2258,12 +2258,9 @@ 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); >> - intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, >> - disp_info->intf_type, >> - controller_id); >> - >> - if (intf_idx >= INTF_0 && intf_idx < INTF_MAX) >> - phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, >> intf_idx); >> + phys_params.hw_intf = dpu_encoder_get_intf(dpu_kms->catalog, >> &dpu_kms->rm, >> + disp_info->intf_type, >> + controller_id); >> if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX) >> phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, >> controller_id); >> @@ -2287,7 +2284,6 @@ static int dpu_encoder_setup_display(struct >> dpu_encoder_virt *dpu_enc, >> DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n"); >> break; >> } >> - > unnecessary change? ack, it sneaked in. I'll drop it for v2. >> } >> mutex_unlock(&dpu_enc->enc_lock);
On 5/2/2023 4:58 PM, Dmitry Baryshkov wrote: > On 03/05/2023 02:57, Abhinav Kumar wrote: >> >> >> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: >>> There is little sense to get intf index just to call dpu_rm_get_intf() >>> on it. Move dpu_rm_get_intf() call to dpu_encoder_get_intf() function. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++------------ >>> 1 file changed, 8 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 507ff3f88c67..b35e92c658ad 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -1259,22 +1259,23 @@ static void >>> dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, >>> mutex_unlock(&dpu_enc->enc_lock); >>> } >>> -static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg >>> *catalog, >>> +static struct dpu_hw_intf *dpu_encoder_get_intf(const struct >>> dpu_mdss_cfg *catalog, >>> + struct dpu_rm *dpu_rm, >>> enum dpu_intf_type type, u32 controller_id) >>> { >>> int i = 0; >>> if (type == INTF_WB) >>> - return INTF_MAX; >>> + return NULL; >>> for (i = 0; i < catalog->intf_count; i++) { >>> if (catalog->intf[i].type == type >>> && catalog->intf[i].controller_id == controller_id) { >>> - return catalog->intf[i].id; >>> + return dpu_rm_get_intf(dpu_rm, catalog->intf[i].id); >>> } >> >> Why has the for loop been retained in this function but not for >> writeback? is there any difference? Doesnt looks like there needs to be. > > For writeback we always return controller_id (WB_2). For interfaces we > have to map type+controller_id to the INTF instance. Ah correct, got it now. With that minor comment fixed from below, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > >> >>> } >>> - return INTF_MAX; >>> + return NULL; >>> } >>> void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, >>> @@ -2244,7 +2245,6 @@ static int dpu_encoder_setup_display(struct >>> dpu_encoder_virt *dpu_enc, >>> * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right >>> */ >>> u32 controller_id = disp_info->h_tile_instance[i]; >>> - enum dpu_intf intf_idx; >>> if (disp_info->num_of_h_tiles > 1) { >>> if (i == 0) >>> @@ -2258,12 +2258,9 @@ 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); >>> - intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, >>> - disp_info->intf_type, >>> - controller_id); >>> - >>> - if (intf_idx >= INTF_0 && intf_idx < INTF_MAX) >>> - phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, >>> intf_idx); >>> + phys_params.hw_intf = dpu_encoder_get_intf(dpu_kms->catalog, >>> &dpu_kms->rm, >>> + disp_info->intf_type, >>> + controller_id); >>> if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX) >>> phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, >>> controller_id); >>> @@ -2287,7 +2284,6 @@ static int dpu_encoder_setup_display(struct >>> dpu_encoder_virt *dpu_enc, >>> DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n"); >>> break; >>> } >>> - >> unnecessary change? > > > ack, it sneaked in. I'll drop it for v2. > >>> } >>> mutex_unlock(&dpu_enc->enc_lock); >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 507ff3f88c67..b35e92c658ad 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1259,22 +1259,23 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, mutex_unlock(&dpu_enc->enc_lock); } -static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog, +static struct dpu_hw_intf *dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog, + struct dpu_rm *dpu_rm, enum dpu_intf_type type, u32 controller_id) { int i = 0; if (type == INTF_WB) - return INTF_MAX; + return NULL; for (i = 0; i < catalog->intf_count; i++) { if (catalog->intf[i].type == type && catalog->intf[i].controller_id == controller_id) { - return catalog->intf[i].id; + return dpu_rm_get_intf(dpu_rm, catalog->intf[i].id); } } - return INTF_MAX; + return NULL; } void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, @@ -2244,7 +2245,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right */ u32 controller_id = disp_info->h_tile_instance[i]; - enum dpu_intf intf_idx; if (disp_info->num_of_h_tiles > 1) { if (i == 0) @@ -2258,12 +2258,9 @@ 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); - intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, - disp_info->intf_type, - controller_id); - - if (intf_idx >= INTF_0 && intf_idx < INTF_MAX) - phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, intf_idx); + phys_params.hw_intf = dpu_encoder_get_intf(dpu_kms->catalog, &dpu_kms->rm, + disp_info->intf_type, + controller_id); if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX) phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, controller_id); @@ -2287,7 +2284,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n"); break; } - } mutex_unlock(&dpu_enc->enc_lock);
There is little sense to get intf index just to call dpu_rm_get_intf() on it. Move dpu_rm_get_intf() call to dpu_encoder_get_intf() function. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)