Re: [PATCH RFC v2 5/8] drm/bridge: dw-hdmi: support dynamically get input/out color info

[Andrzej Hajda <a.hajda@samsung.com>]

On 30.11.2018 14:42, Neil Armstrong wrote:
> From: Zheng Yang <zhengyang@rock-chips.com>
>
> To get input/output bus_format/enc_format dynamically, this patch
> introduce following funstion in plat_data:
> 	- get_input_bus_format
> 	- get_output_bus_format
> 	- get_enc_in_encoding
> 	- get_enc_out_encoding


It seems fishy. On one side description says about dynamic resolution of
formats and encodings.

On the other side these functions as only argument takes platform_data
which should be rather static.

Where is this "dynamic" thing? The only usage of these callbacks I have
found in next patches is also not dynamic, the functions just return
some static value.

Moreover function takes void* argument, which is again something
suspicious, why cannot you pass know structure?

And finally encoding usually should depend on display mode, it should
not depend only static data.


What kind of problems do you want to solve here?


Regards

Andrzej



>
> Signed-off-by: Zheng Yang <zhengyang@rock-chips.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 28 +++++++++++++++++------
>  include/drm/bridge/dw_hdmi.h              |  5 ++++
>  2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 4a9a24e854db..bd564ffdf18b 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1810,6 +1810,7 @@ static void hdmi_disable_overflow_interrupts(struct dw_hdmi *hdmi)
>  static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  {
>  	int ret;
> +	void *data = hdmi->plat_data->phy_data;
>  
>  	hdmi_disable_overflow_interrupts(hdmi);
>  
> @@ -1821,10 +1822,13 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  		dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
>  	}
>  
> -	if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> -	    (hdmi->vic == 21) || (hdmi->vic == 22) ||
> -	    (hdmi->vic == 2) || (hdmi->vic == 3) ||
> -	    (hdmi->vic == 17) || (hdmi->vic == 18))
> +	if (hdmi->plat_data->get_enc_out_encoding)
> +		hdmi->hdmi_data.enc_out_encoding =
> +			hdmi->plat_data->get_enc_out_encoding(data);
> +	else if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> +		 (hdmi->vic == 21) || (hdmi->vic == 22) ||
> +		 (hdmi->vic == 2) || (hdmi->vic == 3) ||
> +		 (hdmi->vic == 17) || (hdmi->vic == 18))
>  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_601;
>  	else
>  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_709;
> @@ -1833,21 +1837,31 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  	hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;
>  
>  	/* TOFIX: Get input format from plat data or fallback to RGB888 */
> -	if (hdmi->plat_data->input_bus_format)
> +	if (hdmi->plat_data->get_input_bus_format)
> +		hdmi->hdmi_data.enc_in_bus_format =
> +			hdmi->plat_data->get_input_bus_format(data);
> +	else if (hdmi->plat_data->input_bus_format)
>  		hdmi->hdmi_data.enc_in_bus_format =
>  			hdmi->plat_data->input_bus_format;
>  	else
>  		hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  
>  	/* TOFIX: Get input encoding from plat data or fallback to none */
> -	if (hdmi->plat_data->input_bus_encoding)
> +	if (hdmi->plat_data->get_enc_in_encoding)
> +		hdmi->hdmi_data.enc_in_encoding =
> +			hdmi->plat_data->get_enc_in_encoding(data);
> +	else if (hdmi->plat_data->input_bus_encoding)
>  		hdmi->hdmi_data.enc_in_encoding =
>  			hdmi->plat_data->input_bus_encoding;
>  	else
>  		hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT;
>  
>  	/* TOFIX: Default to RGB888 output format */
> -	hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +	if (hdmi->plat_data->get_output_bus_format)
> +		hdmi->hdmi_data.enc_out_bus_format =
> +			hdmi->plat_data->get_output_bus_format(data);
> +	else
> +		hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  
>  	hdmi->hdmi_data.pix_repet_factor = 0;
>  	hdmi->hdmi_data.hdcp_enable = 0;
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 7a02744ce0bc..2e797f782c51 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -142,6 +142,11 @@ struct dw_hdmi_plat_data {
>  	int (*configure_phy)(struct dw_hdmi *hdmi,
>  			     const struct dw_hdmi_plat_data *pdata,
>  			     unsigned long mpixelclock);
> +
> +	unsigned long (*get_input_bus_format)(void *data);
> +	unsigned long (*get_output_bus_format)(void *data);
> +	unsigned long (*get_enc_in_encoding)(void *data);
> +	unsigned long (*get_enc_out_encoding)(void *data);
>  };
>  
>  struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,