[PATCH v3 23/27] drm/msm/dpu: rework dpu_plane_atomic_check()
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Tue Feb 7 17:59:31 UTC 2023
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 at 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))
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list