On Fri, 24 Jun 2022 at 20:15, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
>
> With current implementation, communication between interface driver and
> upper mdss encoder layer are implemented through function calls. This
> increase code complexity. Since struct msm_display_info contains msm
> generic display information, it can be expended to contains more useful
> information, such as widebus and dcs, in future to serve as communication
> channel purpose between interface driver and upper mdss encoder layer so
> that existing function calls can be eliminated.
> This patch more struct msm_display_info to msm_drv.h to be visible by
> whole msm scope.
NAK.
The msm_display_info contains information used by (and useful to) DPU
only, it is not 'msm generic' info. For this reason it has been moved
from msm_drv.h to dpu_encoder.h inIn commit b7420739f112 ("drm/msm:
move struct msm_display_info to dpu driver") . Neither mdp5 nor mdp4
are going to use this structure.
At some point I thought too that we might be able to create a set of
data and functions to describe encoder backends (dsi, hdmi, dp). This
has failed for me. After musing over the msm_drv.h part containing
functions published by the backends, I could not end up with a set of
them being good enough. The only common part seems to be the
modeset_init, snapshot and (once DP gets the DSC interface)
get_dsc_config. The rest is backend-specific.
I would suggest returning to this topic if/once DSI gets wide bus
support or DP starts using bonded interfaces. Before that I don't
foresee that common data structure would simplify things rather than
complicating them.
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 --------------------
> drivers/gpu/drm/msm/msm_drv.h | 19 +++++++++++++++++++
> 2 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 781d41c..6b604c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -19,26 +19,6 @@
> #define IDLE_TIMEOUT (66 - 16/2)
>
> /**
> - * struct msm_display_info - defines display properties
> - * @intf_type: DRM_MODE_ENCODER_ type
> - * @capabilities: Bitmask of display flags
> - * @num_of_h_tiles: Number of horizontal tiles in case of split interface
> - * @h_tile_instance: Controller instance used per tile. Number of elements is
> - * based on num_of_h_tiles
> - * @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is
> - * used instead of panel TE in cmd mode panels
> - * @dsc: DSC configuration data for DSC-enabled displays
> - */
> -struct msm_display_info {
> - int intf_type;
> - uint32_t capabilities;
> - uint32_t num_of_h_tiles;
> - uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> - bool is_te_using_watchdog_timer;
> - struct msm_display_dsc_config *dsc;
> -};
> -
> -/**
> * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
> * @encoder: encoder pointer
> * @crtc: crtc pointer
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index fdbaad5..f9c263b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -106,11 +106,30 @@ struct msm_drm_thread {
> struct kthread_worker *worker;
> };
>
> +<<<<<<< HEAD
Not to mention that this patch is broken per se.
> /* DSC config */
> struct msm_display_dsc_config {
> struct drm_dsc_config *drm;
> };
>
> +/**
> + * struct msm_display_info - defines display properties
> + * @intf_type: DRM_MODE_ENCODER_ type
> + * @capabilities: Bitmask of display flags
> + * @num_of_h_tiles: Number of horizontal tiles in case of split interface
> + * @h_tile_instance: Controller instance used per tile. Number of elements is
> + * based on num_of_h_tiles
> + * @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is
> + * used instead of panel TE in cmd mode panels
> + */
> +struct msm_display_info {
> + int intf_type;
> + uint32_t capabilities;
> + uint32_t num_of_h_tiles;
> + uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> + bool is_te_using_watchdog_timer;
> +};
> +
> struct msm_drm_private {
>
> struct drm_device *dev;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
With best wishes
Dmitry
