| Message ID | 20230203182132.1307834-24-dmitry.baryshkov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,01/27] drm/msm/dpu: rename struct dpu_hw_pipe(_cfg) to dpu_hw_sspp(_cfg) | expand |
On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: > Split pipe-dependent code from dpu_plane_atomic_check() into the > separate function dpu_plane_atomic_check_pipe(). This is one of Same comment as prev patch, dpu_plane_atomic_check_pipe() ---> dpu_plane_atomic_check_sspp() > preparational steps to add r_pipe support. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 88 ++++++++++++++--------- > 1 file changed, 53 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index f94e132733f3..e69499490d39 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -903,6 +903,51 @@ static int dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, > return 0; > } > > +static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, > + struct dpu_sw_pipe *pipe, > + struct dpu_hw_sspp_cfg *pipe_cfg, pipe_cfg --> sspp_cfg Also, had a question. For function parameters spreading across multiple lines do we want to align to one guideline? Is it going to be two tabs more than the prev line or aligning it with the opening brace of prev line? I am seeing a mix in the prev patch of the series and this one. > + const struct dpu_format *fmt) > +{ > + uint32_t min_src_size; > + > + min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; > + > + if (DPU_FORMAT_IS_YUV(fmt) && > + (!(pipe->sspp->cap->features & DPU_SSPP_SCALER) || > + !(pipe->sspp->cap->features & DPU_SSPP_CSC_ANY))) { > + DPU_DEBUG_PLANE(pdpu, > + "plane doesn't have scaler/csc for yuv\n"); > + return -EINVAL; > + } > + > + /* check src bounds */ > + if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || > + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { > + DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", > + DRM_RECT_ARG(&pipe_cfg->src_rect)); > + return -E2BIG; > + } > + > + /* valid yuv image */ > + if (DPU_FORMAT_IS_YUV(fmt) && > + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & 0x1 || > + drm_rect_width(&pipe_cfg->src_rect) & 0x1 || > + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { > + DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", > + DRM_RECT_ARG(&pipe_cfg->src_rect)); > + return -EINVAL; > + } > + > + /* min dst support */ > + if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { > + DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", > + DRM_RECT_ARG(&pipe_cfg->dst_rect)); > + return -EINVAL; > + } > + > + return 0; > +} > + > static int dpu_plane_atomic_check(struct drm_plane *plane, > struct drm_atomic_state *state) > { > @@ -915,7 +960,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > const struct dpu_format *fmt; > struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; > struct drm_rect fb_rect = { 0 }; > - uint32_t min_src_size, max_linewidth; > + uint32_t max_linewidth; > unsigned int rotation; > uint32_t supported_rotations; > const struct dpu_sspp_cfg *pipe_hw_caps = pstate->pipe.sspp->cap; > @@ -970,46 +1015,19 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > max_linewidth = pdpu->catalog->caps->max_linewidth; > > - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > - > - min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; > - > - if (DPU_FORMAT_IS_YUV(fmt) && > - (!(pipe_hw_caps->features & DPU_SSPP_SCALER) || > - !(pipe_hw_caps->features & DPU_SSPP_CSC_ANY))) { > - DPU_DEBUG_PLANE(pdpu, > - "plane doesn't have scaler/csc for yuv\n"); > - return -EINVAL; > - > - /* check src bounds */ > - } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || > - drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { > - DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", > - DRM_RECT_ARG(&pipe_cfg->src_rect)); > - return -E2BIG; > - > - /* valid yuv image */ > - } else if (DPU_FORMAT_IS_YUV(fmt) && > - (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & 0x1 || > - drm_rect_width(&pipe_cfg->src_rect) & 0x1 || > - drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { > - DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", > - DRM_RECT_ARG(&pipe_cfg->src_rect)); > - return -EINVAL; > - > - /* min dst support */ > - } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { > - DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", > - DRM_RECT_ARG(&pipe_cfg->dst_rect)); > - return -EINVAL; > - > /* check decimated source width */ > - } else if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { > + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { > DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n", > DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); > return -E2BIG; > } > > + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > + > + ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, fmt); > + if (ret) > + return ret; > + > supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0; > > if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION))
On 07/02/2023 19:49, Abhinav Kumar wrote: > > > On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: >> Split pipe-dependent code from dpu_plane_atomic_check() into the >> separate function dpu_plane_atomic_check_pipe(). This is one of > > Same comment as prev patch, dpu_plane_atomic_check_pipe() ---> > dpu_plane_atomic_check_sspp() No, it does what it does: it checks one software pipe configuration. > >> preparational steps to add r_pipe support. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 88 ++++++++++++++--------- >> 1 file changed, 53 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> index f94e132733f3..e69499490d39 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> @@ -903,6 +903,51 @@ static int dpu_plane_check_inline_rotation(struct >> dpu_plane *pdpu, >> return 0; >> } >> +static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, >> + struct dpu_sw_pipe *pipe, >> + struct dpu_hw_sspp_cfg *pipe_cfg, > > pipe_cfg --> sspp_cfg > > Also, had a question. For function parameters spreading across multiple > lines do we want to align to one guideline? Is it going to be two tabs > more than the prev line or aligning it with the opening brace of prev line? > > I am seeing a mix in the prev patch of the series and this one. I usually tend to keep the existing indent when adding new args and shifting to open brace when adding new functions. Maybe I failed a question in this patch, I'll doublecheck it. > >> + const struct dpu_format *fmt) >> +{ >> + uint32_t min_src_size; >> + >> + min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >> + >> + if (DPU_FORMAT_IS_YUV(fmt) && >> + (!(pipe->sspp->cap->features & DPU_SSPP_SCALER) || >> + !(pipe->sspp->cap->features & DPU_SSPP_CSC_ANY))) { >> + DPU_DEBUG_PLANE(pdpu, >> + "plane doesn't have scaler/csc for yuv\n"); >> + return -EINVAL; >> + } >> + >> + /* check src bounds */ >> + if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >> + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >> + DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >> + return -E2BIG; >> + } >> + >> + /* valid yuv image */ >> + if (DPU_FORMAT_IS_YUV(fmt) && >> + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & >> 0x1 || >> + drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >> + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >> + DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", >> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >> + return -EINVAL; >> + } >> + >> + /* min dst support */ >> + if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >> + DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", >> + DRM_RECT_ARG(&pipe_cfg->dst_rect)); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> static int dpu_plane_atomic_check(struct drm_plane *plane, >> struct drm_atomic_state *state) >> { >> @@ -915,7 +960,7 @@ static int dpu_plane_atomic_check(struct drm_plane >> *plane, >> const struct dpu_format *fmt; >> struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; >> struct drm_rect fb_rect = { 0 }; >> - uint32_t min_src_size, max_linewidth; >> + uint32_t max_linewidth; >> unsigned int rotation; >> uint32_t supported_rotations; >> const struct dpu_sspp_cfg *pipe_hw_caps = pstate->pipe.sspp->cap; >> @@ -970,46 +1015,19 @@ static int dpu_plane_atomic_check(struct >> drm_plane *plane, >> max_linewidth = pdpu->catalog->caps->max_linewidth; >> - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >> - >> - min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >> - >> - if (DPU_FORMAT_IS_YUV(fmt) && >> - (!(pipe_hw_caps->features & DPU_SSPP_SCALER) || >> - !(pipe_hw_caps->features & DPU_SSPP_CSC_ANY))) { >> - DPU_DEBUG_PLANE(pdpu, >> - "plane doesn't have scaler/csc for yuv\n"); >> - return -EINVAL; >> - >> - /* check src bounds */ >> - } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >> - drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >> - DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >> - return -E2BIG; >> - >> - /* valid yuv image */ >> - } else if (DPU_FORMAT_IS_YUV(fmt) && >> - (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & >> 0x1 || >> - drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >> - drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >> - DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", >> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >> - return -EINVAL; >> - >> - /* min dst support */ >> - } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >> - DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", >> - DRM_RECT_ARG(&pipe_cfg->dst_rect)); >> - return -EINVAL; >> - >> /* check decimated source width */ >> - } else if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { >> + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { >> DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n", >> DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); >> return -E2BIG; >> } >> + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >> + >> + ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, >> fmt); >> + if (ret) >> + return ret; >> + >> supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0; >> if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION))
On 2/7/2023 9:51 AM, Dmitry Baryshkov wrote: > On 07/02/2023 19:49, Abhinav Kumar wrote: >> >> >> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: >>> Split pipe-dependent code from dpu_plane_atomic_check() into the >>> separate function dpu_plane_atomic_check_pipe(). This is one of >> >> Same comment as prev patch, dpu_plane_atomic_check_pipe() ---> >> dpu_plane_atomic_check_sspp() > > No, it does what it does: it checks one software pipe configuration. The concept of software pipe is only to encapsulate the hw_sspp along with its params +struct dpu_sw_pipe { + struct dpu_hw_sspp *sspp; + enum dpu_sspp_multirect_index multirect_index; + enum dpu_sspp_multirect_mode multirect_mode; +}; So its a very thin differentiation there. > >> >>> preparational steps to add r_pipe support. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 88 ++++++++++++++--------- >>> 1 file changed, 53 insertions(+), 35 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> index f94e132733f3..e69499490d39 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> @@ -903,6 +903,51 @@ static int >>> dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, >>> return 0; >>> } >>> +static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, >>> + struct dpu_sw_pipe *pipe, >>> + struct dpu_hw_sspp_cfg *pipe_cfg, >> >> pipe_cfg --> sspp_cfg >> >> Also, had a question. For function parameters spreading across >> multiple lines do we want to align to one guideline? Is it going to be >> two tabs more than the prev line or aligning it with the opening brace >> of prev line? >> >> I am seeing a mix in the prev patch of the series and this one. > > I usually tend to keep the existing indent when adding new args and > shifting to open brace when adding new functions. Maybe I failed a > question in this patch, I'll doublecheck it. > >> >>> + const struct dpu_format *fmt) >>> +{ >>> + uint32_t min_src_size; >>> + >>> + min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >>> + >>> + if (DPU_FORMAT_IS_YUV(fmt) && >>> + (!(pipe->sspp->cap->features & DPU_SSPP_SCALER) || >>> + !(pipe->sspp->cap->features & DPU_SSPP_CSC_ANY))) { >>> + DPU_DEBUG_PLANE(pdpu, >>> + "plane doesn't have scaler/csc for yuv\n"); >>> + return -EINVAL; >>> + } >>> + >>> + /* check src bounds */ >>> + if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >>> + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >>> + DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >>> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >>> + return -E2BIG; >>> + } >>> + >>> + /* valid yuv image */ >>> + if (DPU_FORMAT_IS_YUV(fmt) && >>> + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & >>> 0x1 || >>> + drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >>> + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >>> + DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", >>> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >>> + return -EINVAL; >>> + } >>> + >>> + /* min dst support */ >>> + if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >>> + DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", >>> + DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static int dpu_plane_atomic_check(struct drm_plane *plane, >>> struct drm_atomic_state *state) >>> { >>> @@ -915,7 +960,7 @@ static int dpu_plane_atomic_check(struct >>> drm_plane *plane, >>> const struct dpu_format *fmt; >>> struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; >>> struct drm_rect fb_rect = { 0 }; >>> - uint32_t min_src_size, max_linewidth; >>> + uint32_t max_linewidth; >>> unsigned int rotation; >>> uint32_t supported_rotations; >>> const struct dpu_sspp_cfg *pipe_hw_caps = pstate->pipe.sspp->cap; >>> @@ -970,46 +1015,19 @@ static int dpu_plane_atomic_check(struct >>> drm_plane *plane, >>> max_linewidth = pdpu->catalog->caps->max_linewidth; >>> - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>> - >>> - min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >>> - >>> - if (DPU_FORMAT_IS_YUV(fmt) && >>> - (!(pipe_hw_caps->features & DPU_SSPP_SCALER) || >>> - !(pipe_hw_caps->features & DPU_SSPP_CSC_ANY))) { >>> - DPU_DEBUG_PLANE(pdpu, >>> - "plane doesn't have scaler/csc for yuv\n"); >>> - return -EINVAL; >>> - >>> - /* check src bounds */ >>> - } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >>> - drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >>> - DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >>> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >>> - return -E2BIG; >>> - >>> - /* valid yuv image */ >>> - } else if (DPU_FORMAT_IS_YUV(fmt) && >>> - (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & >>> 0x1 || >>> - drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >>> - drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >>> - DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", >>> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >>> - return -EINVAL; >>> - >>> - /* min dst support */ >>> - } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >>> - DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", >>> - DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>> - return -EINVAL; >>> - >>> /* check decimated source width */ >>> - } else if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { >>> + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { >>> DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " >>> line:%u\n", >>> DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); >>> return -E2BIG; >>> } >>> + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>> + >>> + ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, >>> fmt); >>> + if (ret) >>> + return ret; >>> + >>> supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0; >>> if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION)) >
On 07/02/2023 19:57, Abhinav Kumar wrote: > > > On 2/7/2023 9:51 AM, Dmitry Baryshkov wrote: >> On 07/02/2023 19:49, Abhinav Kumar wrote: >>> >>> >>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: >>>> Split pipe-dependent code from dpu_plane_atomic_check() into the >>>> separate function dpu_plane_atomic_check_pipe(). This is one of >>> >>> Same comment as prev patch, dpu_plane_atomic_check_pipe() ---> >>> dpu_plane_atomic_check_sspp() >> >> No, it does what it does: it checks one software pipe configuration. > > The concept of software pipe is only to encapsulate the hw_sspp along > with its params > > +struct dpu_sw_pipe { > + struct dpu_hw_sspp *sspp; > + enum dpu_sspp_multirect_index multirect_index; > + enum dpu_sspp_multirect_mode multirect_mode; > +}; > > So its a very thin differentiation there. SSPP is a hardware thing, while the sw_pipe is a software instance. It can employ an SSPP or a half of SSPP. And if it employs a half of SSPP (rec1) than the name dpu_plane_atomic_check_sspp() doesn't make sense: we are not checking the SSPP itself (or its configuration). We are checking the parameters of SW pipe. > >> >>> >>>> preparational steps to add r_pipe support. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 88 >>>> ++++++++++++++--------- >>>> 1 file changed, 53 insertions(+), 35 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>> index f94e132733f3..e69499490d39 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>> @@ -903,6 +903,51 @@ static int >>>> dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, >>>> return 0; >>>> } >>>> +static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, >>>> + struct dpu_sw_pipe *pipe, >>>> + struct dpu_hw_sspp_cfg *pipe_cfg, >>> >>> pipe_cfg --> sspp_cfg >>> >>> Also, had a question. For function parameters spreading across >>> multiple lines do we want to align to one guideline? Is it going to >>> be two tabs more than the prev line or aligning it with the opening >>> brace of prev line? >>> >>> I am seeing a mix in the prev patch of the series and this one. >> >> I usually tend to keep the existing indent when adding new args and >> shifting to open brace when adding new functions. Maybe I failed a >> question in this patch, I'll doublecheck it. >> >>> >>>> + const struct dpu_format *fmt) >>>> +{ >>>> + uint32_t min_src_size; >>>> + >>>> + min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >>>> + >>>> + if (DPU_FORMAT_IS_YUV(fmt) && >>>> + (!(pipe->sspp->cap->features & DPU_SSPP_SCALER) || >>>> + !(pipe->sspp->cap->features & DPU_SSPP_CSC_ANY))) { >>>> + DPU_DEBUG_PLANE(pdpu, >>>> + "plane doesn't have scaler/csc for yuv\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + /* check src bounds */ >>>> + if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >>>> + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >>>> + DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >>>> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>> + return -E2BIG; >>>> + } >>>> + >>>> + /* valid yuv image */ >>>> + if (DPU_FORMAT_IS_YUV(fmt) && >>>> + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & >>>> 0x1 || >>>> + drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >>>> + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >>>> + DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", >>>> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + /* min dst support */ >>>> + if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >>>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >>>> + DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", >>>> + DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int dpu_plane_atomic_check(struct drm_plane *plane, >>>> struct drm_atomic_state *state) >>>> { >>>> @@ -915,7 +960,7 @@ static int dpu_plane_atomic_check(struct >>>> drm_plane *plane, >>>> const struct dpu_format *fmt; >>>> struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; >>>> struct drm_rect fb_rect = { 0 }; >>>> - uint32_t min_src_size, max_linewidth; >>>> + uint32_t max_linewidth; >>>> unsigned int rotation; >>>> uint32_t supported_rotations; >>>> const struct dpu_sspp_cfg *pipe_hw_caps = pstate->pipe.sspp->cap; >>>> @@ -970,46 +1015,19 @@ static int dpu_plane_atomic_check(struct >>>> drm_plane *plane, >>>> max_linewidth = pdpu->catalog->caps->max_linewidth; >>>> - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>>> - >>>> - min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >>>> - >>>> - if (DPU_FORMAT_IS_YUV(fmt) && >>>> - (!(pipe_hw_caps->features & DPU_SSPP_SCALER) || >>>> - !(pipe_hw_caps->features & DPU_SSPP_CSC_ANY))) { >>>> - DPU_DEBUG_PLANE(pdpu, >>>> - "plane doesn't have scaler/csc for yuv\n"); >>>> - return -EINVAL; >>>> - >>>> - /* check src bounds */ >>>> - } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >>>> - drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >>>> - DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >>>> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>> - return -E2BIG; >>>> - >>>> - /* valid yuv image */ >>>> - } else if (DPU_FORMAT_IS_YUV(fmt) && >>>> - (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & >>>> 0x1 || >>>> - drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >>>> - drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >>>> - DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", >>>> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>> - return -EINVAL; >>>> - >>>> - /* min dst support */ >>>> - } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >>>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >>>> - DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", >>>> - DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>>> - return -EINVAL; >>>> - >>>> /* check decimated source width */ >>>> - } else if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { >>>> + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { >>>> DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " >>>> line:%u\n", >>>> DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); >>>> return -E2BIG; >>>> } >>>> + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>>> + >>>> + ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, >>>> pipe_cfg, fmt); >>>> + if (ret) >>>> + return ret; >>>> + >>>> supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0; >>>> if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION)) >>
On 2/7/2023 9:59 AM, Dmitry Baryshkov wrote: > On 07/02/2023 19:57, Abhinav Kumar wrote: >> >> >> On 2/7/2023 9:51 AM, Dmitry Baryshkov wrote: >>> On 07/02/2023 19:49, Abhinav Kumar wrote: >>>> >>>> >>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: >>>>> Split pipe-dependent code from dpu_plane_atomic_check() into the >>>>> separate function dpu_plane_atomic_check_pipe(). This is one of >>>> >>>> Same comment as prev patch, dpu_plane_atomic_check_pipe() ---> >>>> dpu_plane_atomic_check_sspp() >>> >>> No, it does what it does: it checks one software pipe configuration. >> >> The concept of software pipe is only to encapsulate the hw_sspp along >> with its params >> >> +struct dpu_sw_pipe { >> + struct dpu_hw_sspp *sspp; >> + enum dpu_sspp_multirect_index multirect_index; >> + enum dpu_sspp_multirect_mode multirect_mode; >> +}; >> >> So its a very thin differentiation there. > > SSPP is a hardware thing, while the sw_pipe is a software instance. It > can employ an SSPP or a half of SSPP. And if it employs a half of SSPP > (rec1) than the name dpu_plane_atomic_check_sspp() doesn't make sense: > we are not checking the SSPP itself (or its configuration). We are > checking the parameters of SW pipe. > Ok, I need to check in the next couple of patches how you are handling the same dpu_sw_pipe for different rects of the same sspp. struct dpu_plane_state { struct drm_plane_state base; struct msm_gem_address_space *aspace; - struct dpu_hw_sspp *hw_sspp; + struct dpu_sw_pipe pipe; enum dpu_stage stage; bool needs_qos_remap; - uint32_t multirect_index; So far what I saw was dpu_plane_state ---> dpu_sw_pipe ---> dpu_hw_sppp In this chain, I couldnt get how the two rects of the DMA SSPP are differentiated, perhaps there is something in the next few changes. >> >>> >>>> >>>>> preparational steps to add r_pipe support. >>>>> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 88 >>>>> ++++++++++++++--------- >>>>> 1 file changed, 53 insertions(+), 35 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>> index f94e132733f3..e69499490d39 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>> @@ -903,6 +903,51 @@ static int >>>>> dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, >>>>> return 0; >>>>> } >>>>> +static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, >>>>> + struct dpu_sw_pipe *pipe, >>>>> + struct dpu_hw_sspp_cfg *pipe_cfg, >>>> >>>> pipe_cfg --> sspp_cfg >>>> >>>> Also, had a question. For function parameters spreading across >>>> multiple lines do we want to align to one guideline? Is it going to >>>> be two tabs more than the prev line or aligning it with the opening >>>> brace of prev line? >>>> >>>> I am seeing a mix in the prev patch of the series and this one. >>> >>> I usually tend to keep the existing indent when adding new args and >>> shifting to open brace when adding new functions. Maybe I failed a >>> question in this patch, I'll doublecheck it. >>> >>>> >>>>> + const struct dpu_format *fmt) >>>>> +{ >>>>> + uint32_t min_src_size; >>>>> + >>>>> + min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >>>>> + >>>>> + if (DPU_FORMAT_IS_YUV(fmt) && >>>>> + (!(pipe->sspp->cap->features & DPU_SSPP_SCALER) || >>>>> + !(pipe->sspp->cap->features & DPU_SSPP_CSC_ANY))) { >>>>> + DPU_DEBUG_PLANE(pdpu, >>>>> + "plane doesn't have scaler/csc for yuv\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + /* check src bounds */ >>>>> + if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >>>>> + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >>>>> + DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >>>>> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>>> + return -E2BIG; >>>>> + } >>>>> + >>>>> + /* valid yuv image */ >>>>> + if (DPU_FORMAT_IS_YUV(fmt) && >>>>> + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & >>>>> 0x1 || >>>>> + drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >>>>> + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >>>>> + DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT >>>>> "\n", >>>>> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + /* min dst support */ >>>>> + if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >>>>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >>>>> + DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", >>>>> + DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static int dpu_plane_atomic_check(struct drm_plane *plane, >>>>> struct drm_atomic_state *state) >>>>> { >>>>> @@ -915,7 +960,7 @@ static int dpu_plane_atomic_check(struct >>>>> drm_plane *plane, >>>>> const struct dpu_format *fmt; >>>>> struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; >>>>> struct drm_rect fb_rect = { 0 }; >>>>> - uint32_t min_src_size, max_linewidth; >>>>> + uint32_t max_linewidth; >>>>> unsigned int rotation; >>>>> uint32_t supported_rotations; >>>>> const struct dpu_sspp_cfg *pipe_hw_caps = >>>>> pstate->pipe.sspp->cap; >>>>> @@ -970,46 +1015,19 @@ static int dpu_plane_atomic_check(struct >>>>> drm_plane *plane, >>>>> max_linewidth = pdpu->catalog->caps->max_linewidth; >>>>> - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>>>> - >>>>> - min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >>>>> - >>>>> - if (DPU_FORMAT_IS_YUV(fmt) && >>>>> - (!(pipe_hw_caps->features & DPU_SSPP_SCALER) || >>>>> - !(pipe_hw_caps->features & DPU_SSPP_CSC_ANY))) { >>>>> - DPU_DEBUG_PLANE(pdpu, >>>>> - "plane doesn't have scaler/csc for yuv\n"); >>>>> - return -EINVAL; >>>>> - >>>>> - /* check src bounds */ >>>>> - } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >>>>> - drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >>>>> - DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >>>>> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>>> - return -E2BIG; >>>>> - >>>>> - /* valid yuv image */ >>>>> - } else if (DPU_FORMAT_IS_YUV(fmt) && >>>>> - (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & >>>>> 0x1 || >>>>> - drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >>>>> - drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >>>>> - DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT >>>>> "\n", >>>>> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>>> - return -EINVAL; >>>>> - >>>>> - /* min dst support */ >>>>> - } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >>>>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >>>>> - DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", >>>>> - DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>>>> - return -EINVAL; >>>>> - >>>>> /* check decimated source width */ >>>>> - } else if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { >>>>> + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { >>>>> DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " >>>>> line:%u\n", >>>>> DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); >>>>> return -E2BIG; >>>>> } >>>>> + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>>>> + >>>>> + ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, >>>>> pipe_cfg, fmt); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0; >>>>> if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION)) >>> >
On 07/02/2023 20:08, Abhinav Kumar wrote: > > > On 2/7/2023 9:59 AM, Dmitry Baryshkov wrote: >> On 07/02/2023 19:57, Abhinav Kumar wrote: >>> >>> >>> On 2/7/2023 9:51 AM, Dmitry Baryshkov wrote: >>>> On 07/02/2023 19:49, Abhinav Kumar wrote: >>>>> >>>>> >>>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: >>>>>> Split pipe-dependent code from dpu_plane_atomic_check() into the >>>>>> separate function dpu_plane_atomic_check_pipe(). This is one of >>>>> >>>>> Same comment as prev patch, dpu_plane_atomic_check_pipe() ---> >>>>> dpu_plane_atomic_check_sspp() >>>> >>>> No, it does what it does: it checks one software pipe configuration. >>> >>> The concept of software pipe is only to encapsulate the hw_sspp along >>> with its params >>> >>> +struct dpu_sw_pipe { >>> + struct dpu_hw_sspp *sspp; >>> + enum dpu_sspp_multirect_index multirect_index; >>> + enum dpu_sspp_multirect_mode multirect_mode; >>> +}; >>> >>> So its a very thin differentiation there. >> >> SSPP is a hardware thing, while the sw_pipe is a software instance. It >> can employ an SSPP or a half of SSPP. And if it employs a half of SSPP >> (rec1) than the name dpu_plane_atomic_check_sspp() doesn't make sense: >> we are not checking the SSPP itself (or its configuration). We are >> checking the parameters of SW pipe. >> > > Ok, I need to check in the next couple of patches how you are handling > the same dpu_sw_pipe for different rects of the same sspp. rec0 and rec1 are two different instances of dpu_sw_pipe > > struct dpu_plane_state { > struct drm_plane_state base; > struct msm_gem_address_space *aspace; > - struct dpu_hw_sspp *hw_sspp; > + struct dpu_sw_pipe pipe; > enum dpu_stage stage; > bool needs_qos_remap; > - uint32_t multirect_index; > > So far what I saw was dpu_plane_state ---> dpu_sw_pipe ---> dpu_hw_sppp > > In this chain, I couldnt get how the two rects of the DMA SSPP are > differentiated, perhaps there is something in the next few changes. > >>> >>>> >>>>> >>>>>> preparational steps to add r_pipe support. >>>>>> >>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>> --- >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 88 >>>>>> ++++++++++++++--------- >>>>>> 1 file changed, 53 insertions(+), 35 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>>> index f94e132733f3..e69499490d39 100644 >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>>> @@ -903,6 +903,51 @@ static int >>>>>> dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, >>>>>> return 0; >>>>>> } >>>>>> +static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, >>>>>> + struct dpu_sw_pipe *pipe, >>>>>> + struct dpu_hw_sspp_cfg *pipe_cfg, >>>>> >>>>> pipe_cfg --> sspp_cfg >>>>> >>>>> Also, had a question. For function parameters spreading across >>>>> multiple lines do we want to align to one guideline? Is it going to >>>>> be two tabs more than the prev line or aligning it with the opening >>>>> brace of prev line? >>>>> >>>>> I am seeing a mix in the prev patch of the series and this one. >>>> >>>> I usually tend to keep the existing indent when adding new args and >>>> shifting to open brace when adding new functions. Maybe I failed a >>>> question in this patch, I'll doublecheck it. >>>> >>>>> >>>>>> + const struct dpu_format *fmt) >>>>>> +{ >>>>>> + uint32_t min_src_size; >>>>>> + >>>>>> + min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >>>>>> + >>>>>> + if (DPU_FORMAT_IS_YUV(fmt) && >>>>>> + (!(pipe->sspp->cap->features & DPU_SSPP_SCALER) || >>>>>> + !(pipe->sspp->cap->features & DPU_SSPP_CSC_ANY))) { >>>>>> + DPU_DEBUG_PLANE(pdpu, >>>>>> + "plane doesn't have scaler/csc for yuv\n"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + /* check src bounds */ >>>>>> + if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >>>>>> + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >>>>>> + DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >>>>>> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>>>> + return -E2BIG; >>>>>> + } >>>>>> + >>>>>> + /* valid yuv image */ >>>>>> + if (DPU_FORMAT_IS_YUV(fmt) && >>>>>> + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 >>>>>> & 0x1 || >>>>>> + drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >>>>>> + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >>>>>> + DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT >>>>>> "\n", >>>>>> + DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + /* min dst support */ >>>>>> + if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >>>>>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >>>>>> + DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT >>>>>> "\n", >>>>>> + DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static int dpu_plane_atomic_check(struct drm_plane *plane, >>>>>> struct drm_atomic_state *state) >>>>>> { >>>>>> @@ -915,7 +960,7 @@ static int dpu_plane_atomic_check(struct >>>>>> drm_plane *plane, >>>>>> const struct dpu_format *fmt; >>>>>> struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; >>>>>> struct drm_rect fb_rect = { 0 }; >>>>>> - uint32_t min_src_size, max_linewidth; >>>>>> + uint32_t max_linewidth; >>>>>> unsigned int rotation; >>>>>> uint32_t supported_rotations; >>>>>> const struct dpu_sspp_cfg *pipe_hw_caps = >>>>>> pstate->pipe.sspp->cap; >>>>>> @@ -970,46 +1015,19 @@ static int dpu_plane_atomic_check(struct >>>>>> drm_plane *plane, >>>>>> max_linewidth = pdpu->catalog->caps->max_linewidth; >>>>>> - fmt = >>>>>> to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>>>>> - >>>>>> - min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; >>>>>> - >>>>>> - if (DPU_FORMAT_IS_YUV(fmt) && >>>>>> - (!(pipe_hw_caps->features & DPU_SSPP_SCALER) || >>>>>> - !(pipe_hw_caps->features & DPU_SSPP_CSC_ANY))) { >>>>>> - DPU_DEBUG_PLANE(pdpu, >>>>>> - "plane doesn't have scaler/csc for yuv\n"); >>>>>> - return -EINVAL; >>>>>> - >>>>>> - /* check src bounds */ >>>>>> - } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >>>>>> - drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >>>>>> - DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >>>>>> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>>>> - return -E2BIG; >>>>>> - >>>>>> - /* valid yuv image */ >>>>>> - } else if (DPU_FORMAT_IS_YUV(fmt) && >>>>>> - (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 >>>>>> & 0x1 || >>>>>> - drm_rect_width(&pipe_cfg->src_rect) & 0x1 || >>>>>> - drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { >>>>>> - DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT >>>>>> "\n", >>>>>> - DRM_RECT_ARG(&pipe_cfg->src_rect)); >>>>>> - return -EINVAL; >>>>>> - >>>>>> - /* min dst support */ >>>>>> - } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || >>>>>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { >>>>>> - DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT >>>>>> "\n", >>>>>> - DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>>>>> - return -EINVAL; >>>>>> - >>>>>> /* check decimated source width */ >>>>>> - } else if (drm_rect_width(&pipe_cfg->src_rect) > >>>>>> max_linewidth) { >>>>>> + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { >>>>>> DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " >>>>>> line:%u\n", >>>>>> DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); >>>>>> return -E2BIG; >>>>>> } >>>>>> + fmt = >>>>>> to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>>>>> + >>>>>> + ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, >>>>>> pipe_cfg, fmt); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> supported_rotations = DRM_MODE_REFLECT_MASK | >>>>>> DRM_MODE_ROTATE_0; >>>>>> if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION)) >>>> >>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index f94e132733f3..e69499490d39 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -903,6 +903,51 @@ static int dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, return 0; } +static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, + struct dpu_sw_pipe *pipe, + struct dpu_hw_sspp_cfg *pipe_cfg, + const struct dpu_format *fmt) +{ + uint32_t min_src_size; + + min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; + + if (DPU_FORMAT_IS_YUV(fmt) && + (!(pipe->sspp->cap->features & DPU_SSPP_SCALER) || + !(pipe->sspp->cap->features & DPU_SSPP_CSC_ANY))) { + DPU_DEBUG_PLANE(pdpu, + "plane doesn't have scaler/csc for yuv\n"); + return -EINVAL; + } + + /* check src bounds */ + if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { + DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", + DRM_RECT_ARG(&pipe_cfg->src_rect)); + return -E2BIG; + } + + /* valid yuv image */ + if (DPU_FORMAT_IS_YUV(fmt) && + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & 0x1 || + drm_rect_width(&pipe_cfg->src_rect) & 0x1 || + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { + DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", + DRM_RECT_ARG(&pipe_cfg->src_rect)); + return -EINVAL; + } + + /* min dst support */ + if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { + DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", + DRM_RECT_ARG(&pipe_cfg->dst_rect)); + return -EINVAL; + } + + return 0; +} + static int dpu_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state) { @@ -915,7 +960,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, const struct dpu_format *fmt; struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; struct drm_rect fb_rect = { 0 }; - uint32_t min_src_size, max_linewidth; + uint32_t max_linewidth; unsigned int rotation; uint32_t supported_rotations; const struct dpu_sspp_cfg *pipe_hw_caps = pstate->pipe.sspp->cap; @@ -970,46 +1015,19 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, max_linewidth = pdpu->catalog->caps->max_linewidth; - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); - - min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1; - - if (DPU_FORMAT_IS_YUV(fmt) && - (!(pipe_hw_caps->features & DPU_SSPP_SCALER) || - !(pipe_hw_caps->features & DPU_SSPP_CSC_ANY))) { - DPU_DEBUG_PLANE(pdpu, - "plane doesn't have scaler/csc for yuv\n"); - return -EINVAL; - - /* check src bounds */ - } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || - drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { - DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", - DRM_RECT_ARG(&pipe_cfg->src_rect)); - return -E2BIG; - - /* valid yuv image */ - } else if (DPU_FORMAT_IS_YUV(fmt) && - (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & 0x1 || - drm_rect_width(&pipe_cfg->src_rect) & 0x1 || - drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { - DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", - DRM_RECT_ARG(&pipe_cfg->src_rect)); - return -EINVAL; - - /* min dst support */ - } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { - DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", - DRM_RECT_ARG(&pipe_cfg->dst_rect)); - return -EINVAL; - /* check decimated source width */ - } else if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n", DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); return -E2BIG; } + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); + + ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, fmt); + if (ret) + return ret; + supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0; if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION))
Split pipe-dependent code from dpu_plane_atomic_check() into the separate function dpu_plane_atomic_check_pipe(). This is one of preparational steps to add r_pipe support. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 88 ++++++++++++++--------- 1 file changed, 53 insertions(+), 35 deletions(-)