Re: [PATCH v7 12/19] drm/imx: Add i.MX8qxp Display Controller KMS

[Liu Ying <victor.liu@nxp.com>]

On 12/27/2024, Dmitry Baryshkov wrote:
> On Wed, 25 Dec 2024 at 09:18, Liu Ying <victor.liu@nxp.com> wrote:
>>
>> On 12/23/2024, Dmitry Baryshkov wrote:
>>> On Mon, Dec 23, 2024 at 02:41:40PM +0800, Liu Ying wrote:
>>>> i.MX8qxp Display Controller(DC) is comprised of three main components that
>>>> include a blit engine for 2D graphics accelerations, display controller for
>>>> display output processing, as well as a command sequencer.  Add kernel
>>>> mode setting support for the display controller part with two CRTCs and
>>>> two primary planes(backed by FetchLayer and FetchWarp respectively).  The
>>>> registers of the display controller are accessed without command sequencer
>>>> involved, instead just by using CPU.  The command sequencer is supposed to
>>>> be used by the blit engine.
>>>>
>>>> Reviewed-by: Maxime Ripard <mripard@kernel.org>
>>>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>>>> ---
>>>> v7:
>>>> * Drop using typeof in macros and explicitly define variable types. (Dmitry)
>>>> * Add a comment for disable_irq_nosync() to explain why _nosync. (Dmitry)
>>>> * Inline dc_crtc_check_clock(). (Dmitry)
>>>> * Use global drm_dc->pe. (Dmitry)
>>>> * Drop dc_crtc_disable_at_unbind(). (Dmitry)
>>>> * Add kernel doc for struct dc_{crtc,drm_device,plane}. (Dmitry)
>>>> * Define common IRQ handlers separately for each IRQs. (Dmitry)
>>>> * Rebase this patch upon next-20241220, so drop date entry from drm_driver
>>>>   and correctly include drm/clients/drm_client_setup.h.
>>>> * Collect Maxime's R-b tag.
>>>>
>>>> v6:
>>>> * No change.
>>>>
>>>> v5:
>>>> * Replace .remove_new with .remove in dc-drv.c. (Uwe)
>>>>
>>>> v4:
>>>> * Move dc_fg_displaymode(), dc_fg_panic_displaymode() and dc_lb_blendcontrol()
>>>>   function calls from KMS routine to initialization stage. (Dmitry)
>>>> * Drop dc-crtc.h and dc-plane.h header files and move relevant defines to
>>>>   appropriate .h header files or .c source files. (Dmitry)
>>>> * Drop futile "else" clause from dc_crtc_common_irq_handler(). (Dmitry)
>>>> * Drop dc_drm->pe_rpm_count. (Dmitry)
>>>> * Drop DC_{CRTCS,ENCODERS,PRIMARYS} macros and only use DC_DISPLAYS. (Dmitry)
>>>> * Drop drmm_kcalloc() function call to allocate an array for storing IRQs.
>>>>   Instead, put it in struct dc_crtc.  (Dmitry)
>>>> * Call devm_request_irq() to request IRQs, instead of using drmm action.
>>>>   (Dmitry)
>>>> * Call devm_drm_of_get_bridge() to find the next bridge. (Dmitry)
>>>> * Select DRM_CLIENT_SELECTION due to rebase.
>>>> * Select the missing DRM_DISPLAY_HELPER and DRM_BRIDGE_CONNECTOR.
>>>> * Use DRM_FBDEV_DMA_DRIVER_OPS due to rebase.
>>>> * Replace drm_fbdev_dma_setup() with drm_client_setup_with_fourcc() due to
>>>>   rebase.
>>>> * Replace drmm_add_action_or_reset() with devm_add_action_or_reset() to
>>>>   register dc_drm_component_unbind_all() action.
>>>> * Request interrupts in dc_crtc_post_init() after encoder initialization to
>>>>   make sure next bridge is found first.
>>>>
>>>> v3:
>>>> * No change.
>>>>
>>>> v2:
>>>> * Find next bridge from TCon's port.
>>>> * Drop drm/drm_module.h include from dc-drv.c.
>>>>
>>>>  drivers/gpu/drm/imx/dc/Kconfig    |   5 +
>>>>  drivers/gpu/drm/imx/dc/Makefile   |   5 +-
>>>>  drivers/gpu/drm/imx/dc/dc-crtc.c  | 556 ++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/imx/dc/dc-de.h    |   3 +
>>>>  drivers/gpu/drm/imx/dc/dc-drv.c   | 238 +++++++++++++
>>>>  drivers/gpu/drm/imx/dc/dc-drv.h   |  22 ++
>>>>  drivers/gpu/drm/imx/dc/dc-kms.c   | 143 ++++++++
>>>>  drivers/gpu/drm/imx/dc/dc-kms.h   | 131 +++++++
>>>>  drivers/gpu/drm/imx/dc/dc-plane.c | 241 +++++++++++++
>>>>  9 files changed, 1342 insertions(+), 2 deletions(-)
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.c
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.c
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.h
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.c
>>>>
>>>
>>>
>>>> +
>>>> +static int dc_plane_check_no_off_screen(struct drm_plane_state *state,
>>>> +                                    struct drm_crtc_state *crtc_state)
>>>> +{
>>>> +    if (state->dst.x1 < 0 || state->dst.y1 < 0 ||
>>>> +        state->dst.x2 > crtc_state->adjusted_mode.hdisplay ||
>>>> +        state->dst.y2 > crtc_state->adjusted_mode.vdisplay) {
>>>> +            dc_plane_dbg(state->plane, "no off screen\n");
>>>> +            return -EINVAL;
>>>> +    }
>>>
>>> Nit: doesn't drm_atomic_helper_check_plane_state() ensure in this already?
>>
>> The can_position argument is set to true, though it is a bit unusual
>> for primary plane.  If can_position is false, I think this should be
>> ensured already by drm_atomic_helper_check_plane_state().
> 
> The function still calls drm_rect_clip_scaled() to clip the
> plane_state->dst, does it not?

You are right. I'll drop dc_plane_check_no_off_screen() in next version.

> 
>>> With that in mind
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> Thanks.
> 
> 
> 

-- 
Regards,
Liu Ying