Re: [PATCH v1 06/14] drm/msm/dp: add display compression related struct

[Neil Armstrong <neil.armstrong@linaro.org>]

On 23/01/2023 19:24, Kuogee Hsieh wrote:
> Add display compression related struct to support variant compression
> mechanism. However, DSC is the only one supported at this moment.
> VDC may be added later.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_panel.h | 42 ++++++++++++++++++
>   drivers/gpu/drm/msm/msm_drv.h     | 89 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 131 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> index 1153e88..4c45d51 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -21,12 +21,54 @@ struct edid;
>   #define DP_DOWNSTREAM_PORTS		4
>   #define DP_DOWNSTREAM_CAP_SIZE		4
>   
> +
> +#define DP_PANEL_CAPS_DSC	BIT(0)
> +
> +enum dp_output_format {
> +	DP_OUTPUT_FORMAT_RGB,
> +	DP_OUTPUT_FORMAT_YCBCR420,
> +	DP_OUTPUT_FORMAT_YCBCR422,
> +	DP_OUTPUT_FORMAT_YCBCR444,
> +	DP_OUTPUT_FORMAT_INVALID,
> +};
> +
> +
> +struct dp_panel_info {
> +	u32 h_active;
> +	u32 v_active;
> +	u32 h_back_porch;
> +	u32 h_front_porch;
> +	u32 h_sync_width;
> +	u32 h_active_low;
> +	u32 v_back_porch;
> +	u32 v_front_porch;
> +	u32 v_sync_width;
> +	u32 v_active_low;
> +	u32 h_skew;
> +	u32 refresh_rate;
> +	u32 pixel_clk_khz;
> +	u32 bpp;
> +	bool widebus_en;
> +	struct msm_compression_info comp_info;
> +	s64 dsc_overhead_fp;
> +};
> +
>   struct dp_display_mode {
>   	struct drm_display_mode drm_mode;
> +	struct dp_panel_info timing;
>   	u32 capabilities;
> +	s64 fec_overhead_fp;
> +	s64 dsc_overhead_fp;
>   	u32 bpp;
>   	u32 h_active_low;
>   	u32 v_active_low;
> +	/**
> +	 * @output_format:
> +	 *
> +	 * This is used to indicate DP output format.
> +	 * The output format can be read from drm_mode.
> +	 */
> +	enum dp_output_format output_format;
>   };
>   
>   struct dp_panel_in {
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 9f0c184..f155803 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -1,6 +1,7 @@
>   /* SPDX-License-Identifier: GPL-2.0-only */
>   /*
>    * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023. Qualcomm Innovation Center, Inc. All rights reserved
>    * Copyright (C) 2013 Red Hat
>    * Author: Rob Clark <robdclark@gmail.com>
>    */
> @@ -70,6 +71,16 @@ enum msm_dp_controller {
>   #define MAX_H_TILES_PER_DISPLAY 2
>   
>   /**
> + * enum msm_display_compression_type - compression method used for pixel stream
> + * @MSM_DISPLAY_COMPRESSION_NONE:     Pixel data is not compressed
> + * @MSM_DISPLAY_COMPRESSION_DSC:      DSC compresison is used
> + */
> +enum msm_display_compression_type {
> +	MSM_DISPLAY_COMPRESSION_NONE,
> +	MSM_DISPLAY_COMPRESSION_DSC,
> +};
> +
> +/**
>    * enum msm_event_wait - type of HW events to wait for
>    * @MSM_ENC_COMMIT_DONE - wait for the driver to flush the registers to HW
>    * @MSM_ENC_TX_COMPLETE - wait for the HW to transfer the frame to panel
> @@ -82,6 +93,84 @@ enum msm_event_wait {
>   };
>   
>   /**
> + * struct msm_display_dsc_info - defines dsc configuration
> + * @config                   DSC encoder configuration
> + * @scr_rev:                 DSC revision.
> + * @initial_lines:           Number of initial lines stored in encoder.
> + * @pkt_per_line:            Number of packets per line.
> + * @bytes_in_slice:          Number of bytes in slice.
> + * @eol_byte_num:            Valid bytes at the end of line.
> + * @bytes_per_pkt            Number of bytes in DSI packet
> + * @pclk_per_line:           Compressed width.
> + * @slice_last_group_size:   Size of last group in pixels.
> + * @slice_per_pkt:           Number of slices per packet.
> + * @num_active_ss_per_enc:   Number of active soft slices per encoder.
> + * @source_color_space:      Source color space of DSC encoder
> + * @chroma_format:           Chroma_format of DSC encoder.
> + * @det_thresh_flatness:     Flatness threshold.
> + * @extra_width:             Extra width required in timing calculations.
> + * @pps_delay_ms:            Post PPS command delay in milliseconds.
> + * @dsc_4hsmerge_en:         Using DSC 4HS merge topology
> + * @dsc_4hsmerge_padding     4HS merge DSC pair padding value in bytes
> + * @dsc_4hsmerge_alignment   4HS merge DSC alignment value in bytes
> + * @half_panel_pu            True for single and dual dsc encoders if partial
> + *                           update sets the roi width to half of mode width
> + *                           False in all other cases
> + */
> +struct msm_display_dsc_info {
> +	struct drm_dsc_config drm_dsc;
> +	u8 scr_rev;
> +
> +	int initial_lines;
> +	int pkt_per_line;
> +	int bytes_in_slice;
> +	int bytes_per_pkt;
> +	int eol_byte_num;
> +	int pclk_per_line;
> +	int slice_last_group_size;
> +	int slice_per_pkt;
> +	int num_active_ss_per_enc;
> +	int source_color_space;
> +	int chroma_format;
> +	int det_thresh_flatness;
> +	u32 extra_width;
> +	u32 pps_delay_ms;
> +	bool dsc_4hsmerge_en;
> +	u32 dsc_4hsmerge_padding;
> +	u32 dsc_4hsmerge_alignment;
> +	bool half_panel_pu;
> +};
> +
> +/*
> + * conver from struct drm_dsc_config to struct msm_display_dsc_info
> + */
> +#define to_msm_dsc_info(dsc) container_of((dsc), struct msm_display_dsc_info, drm_dsc)

This is weird, you consider dsc struct is in control of dpu, but no with DSC it's part
of the DSI panel struct.

msm_display_dsc_info->drm_dsc should be a pointer.

If some values caculated in msm_display_dsc_info are *really* needed, then a new structure
should be added, but as a companion of the dsm dsc pointer, not containing it.

> +
> +/**
> + * Bits/pixel target >> 4  (removing the fractional bits)
> + * returns the integer bpp value from the drm_dsc_config struct
> + */
> +#define DSC_BPP(config) ((config).bits_per_pixel >> 4)
> +
> +/**
> + * struct msm_compression_info - defined panel compression
> + * @enabled:          enabled/disabled
> + * @comp_type:        type of compression supported
> + * @comp_ratio:       compression ratio
> + * @src_bpp:          bits per pixel before compression
> + * @tgt_bpp:          bits per pixel after compression
> + * @msm_dsc_info:     msm dsc info if the compression supported is DSC
> + */
> +struct msm_compression_info {
> +	bool enabled;

I would rather use comp_type = MSM_DISPLAY_COMPRESSION_NONE instead of having a supplementary bool....

> +	enum msm_display_compression_type comp_type;
> +	u32 comp_ratio;
> +	u32 src_bpp;
> +	u32 tgt_bpp;

Those are never used outside of dp, so get them out.

> +	struct msm_display_dsc_info msm_dsc_info;
> +};
> +
> +/**
>    * struct msm_display_topology - defines a display topology pipeline
>    * @num_lm:       number of layer mixers used
>    * @num_intf:     number of interfaces the panel is mounted on