[PATCH v3 23/27] drm/msm/dpu: rework dpu_plane_atomic_check()
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Tue Feb 7 19:51:41 UTC 2023
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 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