| Message ID | 20221229191856.3508092-6-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: wide planes support | expand |
On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: > The pipe's layout is not cached, corresponding data structure is zeroed > out each time in the dpu_plane_sspp_atomic_update(), right before the > call to _dpu_plane_set_scanout() -> dpu_format_populate_layout(). > > Drop plane_addr comparison against previous layout and corresponding > EAGAIN handling. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> The change itself LGTM, hence But, shouldnt we add this EAGAIN validation or in other words fix this rather than drop this? Like I wrote in the review last time, this makes sure to fail the commit if the same addr is being programmed. > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 10 +--------- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +--- > 2 files changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c > index d95540309d4d..ec1001e10f4f 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c > @@ -918,8 +918,7 @@ int dpu_format_populate_layout( > struct drm_framebuffer *fb, > struct dpu_hw_fmt_layout *layout) > { > - uint32_t plane_addr[DPU_MAX_PLANES]; > - int i, ret; > + int ret; > > if (!fb || !layout) { > DRM_ERROR("invalid arguments\n"); > @@ -940,9 +939,6 @@ int dpu_format_populate_layout( > if (ret) > return ret; > > - for (i = 0; i < DPU_MAX_PLANES; ++i) > - plane_addr[i] = layout->plane_addr[i]; > - > /* Populate the addresses given the fb */ > if (DPU_FORMAT_IS_UBWC(layout->format) || > DPU_FORMAT_IS_TILE(layout->format)) > @@ -950,10 +946,6 @@ int dpu_format_populate_layout( > else > ret = _dpu_format_populate_addrs_linear(aspace, fb, layout); > > - /* check if anything changed */ > - if (!ret && !memcmp(plane_addr, layout->plane_addr, sizeof(plane_addr))) > - ret = -EAGAIN; > - > return ret; > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index cdde7b9ec882..43fb8e00ada6 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -476,9 +476,7 @@ static void _dpu_plane_set_scanout(struct drm_plane *plane, > int ret; > > ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); > - if (ret == -EAGAIN) > - DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n"); > - else if (ret) > + if (ret) > DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); > else if (pdpu->pipe_hw->ops.setup_sourceaddress) { > trace_dpu_plane_set_scanout(pdpu->pipe_hw->idx,
On Fri, 27 Jan 2023 at 02:52, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: > > The pipe's layout is not cached, corresponding data structure is zeroed > > out each time in the dpu_plane_sspp_atomic_update(), right before the > > call to _dpu_plane_set_scanout() -> dpu_format_populate_layout(). > > > > Drop plane_addr comparison against previous layout and corresponding > > EAGAIN handling. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > The change itself LGTM, hence > > But, shouldnt we add this EAGAIN validation or in other words fix this > rather than drop this? What for? Does it really save us anything? What's the price of re-programming the SSPP_SRC0_ADDR registers? > > Like I wrote in the review last time, this makes sure to fail the commit > if the same addr is being programmed. First, there is nothing wrong with committing the same source addr. For example setting the atomic property incurs an internal drm_atomic_commit() with no change to addresses at all. And then, this doesn't make atomic_commit fail. Instead it just shortcuts a call to SSPP->setup_sourceaddress. > > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 10 +--------- > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +--- > > 2 files changed, 2 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c > > index d95540309d4d..ec1001e10f4f 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c > > @@ -918,8 +918,7 @@ int dpu_format_populate_layout( > > struct drm_framebuffer *fb, > > struct dpu_hw_fmt_layout *layout) > > { > > - uint32_t plane_addr[DPU_MAX_PLANES]; > > - int i, ret; > > + int ret; > > > > if (!fb || !layout) { > > DRM_ERROR("invalid arguments\n"); > > @@ -940,9 +939,6 @@ int dpu_format_populate_layout( > > if (ret) > > return ret; > > > > - for (i = 0; i < DPU_MAX_PLANES; ++i) > > - plane_addr[i] = layout->plane_addr[i]; > > - > > /* Populate the addresses given the fb */ > > if (DPU_FORMAT_IS_UBWC(layout->format) || > > DPU_FORMAT_IS_TILE(layout->format)) > > @@ -950,10 +946,6 @@ int dpu_format_populate_layout( > > else > > ret = _dpu_format_populate_addrs_linear(aspace, fb, layout); > > > > - /* check if anything changed */ > > - if (!ret && !memcmp(plane_addr, layout->plane_addr, sizeof(plane_addr))) > > - ret = -EAGAIN; > > - > > return ret; > > } > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > index cdde7b9ec882..43fb8e00ada6 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > @@ -476,9 +476,7 @@ static void _dpu_plane_set_scanout(struct drm_plane *plane, > > int ret; > > > > ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); > > - if (ret == -EAGAIN) > > - DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n"); > > - else if (ret) > > + if (ret) > > DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); > > else if (pdpu->pipe_hw->ops.setup_sourceaddress) { > > trace_dpu_plane_set_scanout(pdpu->pipe_hw->idx,
On 1/26/2023 10:05 PM, Dmitry Baryshkov wrote: > On Fri, 27 Jan 2023 at 02:52, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: >>> The pipe's layout is not cached, corresponding data structure is zeroed >>> out each time in the dpu_plane_sspp_atomic_update(), right before the >>> call to _dpu_plane_set_scanout() -> dpu_format_populate_layout(). >>> >>> Drop plane_addr comparison against previous layout and corresponding >>> EAGAIN handling. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> >> The change itself LGTM, hence >> >> But, shouldnt we add this EAGAIN validation or in other words fix this >> rather than drop this? > > What for? Does it really save us anything? What's the price of > re-programming the SSPP_SRC0_ADDR registers? > There are 4 Src registers being programmed per sspp. With number of layers going up this will be 4x. So lets say there are 5 layers and only one of their address has changed, we need to reprogram only 4 regs but now will reprogram 20. Thats why i thought this is a good optimization. But still, that is a separate change so I am fine if this goes in first as its just removing dead code anyway. Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> >> Like I wrote in the review last time, this makes sure to fail the commit >> if the same addr is being programmed. > > First, there is nothing wrong with committing the same source addr. > For example setting the atomic property incurs an internal > drm_atomic_commit() with no change to addresses at all. > And then, this doesn't make atomic_commit fail. Instead it just > shortcuts a call to SSPP->setup_sourceaddress. > Ack, yes it wont fail the commit but will skip programming the new address. >> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 10 +--------- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +--- >>> 2 files changed, 2 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c >>> index d95540309d4d..ec1001e10f4f 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c >>> @@ -918,8 +918,7 @@ int dpu_format_populate_layout( >>> struct drm_framebuffer *fb, >>> struct dpu_hw_fmt_layout *layout) >>> { >>> - uint32_t plane_addr[DPU_MAX_PLANES]; >>> - int i, ret; >>> + int ret; >>> >>> if (!fb || !layout) { >>> DRM_ERROR("invalid arguments\n"); >>> @@ -940,9 +939,6 @@ int dpu_format_populate_layout( >>> if (ret) >>> return ret; >>> >>> - for (i = 0; i < DPU_MAX_PLANES; ++i) >>> - plane_addr[i] = layout->plane_addr[i]; >>> - >>> /* Populate the addresses given the fb */ >>> if (DPU_FORMAT_IS_UBWC(layout->format) || >>> DPU_FORMAT_IS_TILE(layout->format)) >>> @@ -950,10 +946,6 @@ int dpu_format_populate_layout( >>> else >>> ret = _dpu_format_populate_addrs_linear(aspace, fb, layout); >>> >>> - /* check if anything changed */ >>> - if (!ret && !memcmp(plane_addr, layout->plane_addr, sizeof(plane_addr))) >>> - ret = -EAGAIN; >>> - >>> return ret; >>> } >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> index cdde7b9ec882..43fb8e00ada6 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> @@ -476,9 +476,7 @@ static void _dpu_plane_set_scanout(struct drm_plane *plane, >>> int ret; >>> >>> ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); >>> - if (ret == -EAGAIN) >>> - DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n"); >>> - else if (ret) >>> + if (ret) >>> DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); >>> else if (pdpu->pipe_hw->ops.setup_sourceaddress) { >>> trace_dpu_plane_set_scanout(pdpu->pipe_hw->idx, > > >
On 28/01/2023 01:59, Abhinav Kumar wrote: > > > On 1/26/2023 10:05 PM, Dmitry Baryshkov wrote: >> On Fri, 27 Jan 2023 at 02:52, Abhinav Kumar >> <quic_abhinavk@quicinc.com> wrote: >>> >>> >>> >>> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: >>>> The pipe's layout is not cached, corresponding data structure is zeroed >>>> out each time in the dpu_plane_sspp_atomic_update(), right before the >>>> call to _dpu_plane_set_scanout() -> dpu_format_populate_layout(). >>>> >>>> Drop plane_addr comparison against previous layout and corresponding >>>> EAGAIN handling. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> >>> The change itself LGTM, hence >>> >>> But, shouldnt we add this EAGAIN validation or in other words fix this >>> rather than drop this? >> >> What for? Does it really save us anything? What's the price of >> re-programming the SSPP_SRC0_ADDR registers? >> > There are 4 Src registers being programmed per sspp. > > With number of layers going up this will be 4x. > > So lets say there are 5 layers and only one of their address has > changed, we need to reprogram only 4 regs but now will reprogram 20. I think this was the original intention for this change, however the implementation ended up being written in a way when this condition doesn't trigger at all. > > Thats why i thought this is a good optimization. > > But still, that is a separate change so I am fine if this goes in first > as its just removing dead code anyway. > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On 2/3/2023 6:16 AM, Dmitry Baryshkov wrote: > On 28/01/2023 01:59, Abhinav Kumar wrote: >> >> >> On 1/26/2023 10:05 PM, Dmitry Baryshkov wrote: >>> On Fri, 27 Jan 2023 at 02:52, Abhinav Kumar >>> <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: >>>>> The pipe's layout is not cached, corresponding data structure is >>>>> zeroed >>>>> out each time in the dpu_plane_sspp_atomic_update(), right before the >>>>> call to _dpu_plane_set_scanout() -> dpu_format_populate_layout(). >>>>> >>>>> Drop plane_addr comparison against previous layout and corresponding >>>>> EAGAIN handling. >>>>> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> >>>> The change itself LGTM, hence >>>> >>>> But, shouldnt we add this EAGAIN validation or in other words fix this >>>> rather than drop this? >>> >>> What for? Does it really save us anything? What's the price of >>> re-programming the SSPP_SRC0_ADDR registers? >>> >> There are 4 Src registers being programmed per sspp. >> >> With number of layers going up this will be 4x. >> >> So lets say there are 5 layers and only one of their address has >> changed, we need to reprogram only 4 regs but now will reprogram 20. > > I think this was the original intention for this change, however the > implementation ended up being written in a way when this condition > doesn't trigger at all. > Yes, and thats why I wrote that we should fix this rather than drop this. >> >> Thats why i thought this is a good optimization. >> >> But still, that is a separate change so I am fine if this goes in >> first as its just removing dead code anyway. >> >> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c index d95540309d4d..ec1001e10f4f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c @@ -918,8 +918,7 @@ int dpu_format_populate_layout( struct drm_framebuffer *fb, struct dpu_hw_fmt_layout *layout) { - uint32_t plane_addr[DPU_MAX_PLANES]; - int i, ret; + int ret; if (!fb || !layout) { DRM_ERROR("invalid arguments\n"); @@ -940,9 +939,6 @@ int dpu_format_populate_layout( if (ret) return ret; - for (i = 0; i < DPU_MAX_PLANES; ++i) - plane_addr[i] = layout->plane_addr[i]; - /* Populate the addresses given the fb */ if (DPU_FORMAT_IS_UBWC(layout->format) || DPU_FORMAT_IS_TILE(layout->format)) @@ -950,10 +946,6 @@ int dpu_format_populate_layout( else ret = _dpu_format_populate_addrs_linear(aspace, fb, layout); - /* check if anything changed */ - if (!ret && !memcmp(plane_addr, layout->plane_addr, sizeof(plane_addr))) - ret = -EAGAIN; - return ret; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index cdde7b9ec882..43fb8e00ada6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -476,9 +476,7 @@ static void _dpu_plane_set_scanout(struct drm_plane *plane, int ret; ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); - if (ret == -EAGAIN) - DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n"); - else if (ret) + if (ret) DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); else if (pdpu->pipe_hw->ops.setup_sourceaddress) { trace_dpu_plane_set_scanout(pdpu->pipe_hw->idx,
The pipe's layout is not cached, corresponding data structure is zeroed out each time in the dpu_plane_sspp_atomic_update(), right before the call to _dpu_plane_set_scanout() -> dpu_format_populate_layout(). Drop plane_addr comparison against previous layout and corresponding EAGAIN handling. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 10 +--------- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +--- 2 files changed, 2 insertions(+), 12 deletions(-)