Re: [PATCH v17 4/6] drm/bridge: dw-hdmi: handle unusable or non-configured CSC module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Neil,

> Am 07.04.2022 um 10:28 schrieb Neil Armstrong <narmstrong@xxxxxxxxxxxx>:
> 
> Hi,
> 
> On 06/04/2022 18:26, H. Nikolaus Schaller wrote:
>> From: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> The dw-hdmi integrates an optional Color Space Conversion feature used
>> to handle color-space conversions.
>> On some platforms, the CSC isn't built-in or non-functional.
>> This adds the necessary code to disable the CSC functionality
>> and limit the bus format negotiation to force using the same
>> input bus format as the output bus format.
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 100 +++++++++++++++-------
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.h |   1 +
>>  include/drm/bridge/dw_hdmi.h              |   1 +
>>  3 files changed, 71 insertions(+), 31 deletions(-)
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index f50af40e10340..b5a665c5e406e 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -158,6 +158,8 @@ struct dw_hdmi {
>>  	struct hdmi_data_info hdmi_data;
>>  	const struct dw_hdmi_plat_data *plat_data;
>>  +	bool csc_available;		/* indicates if the CSC engine is usable */
>> +
>>  	int vic;
>>    	u8 edid[HDMI_EDID_LEN];
>> @@ -1009,9 +1011,10 @@ static int is_color_space_interpolation(struct dw_hdmi *hdmi)
>>    static bool is_csc_needed(struct dw_hdmi *hdmi)
>>  {
>> -	return is_color_space_conversion(hdmi) ||
>> -	       is_color_space_decimation(hdmi) ||
>> -	       is_color_space_interpolation(hdmi);
>> +	return hdmi->csc_available &&
>> +	       (is_color_space_conversion(hdmi) ||
>> +		is_color_space_decimation(hdmi) ||
>> +		is_color_space_interpolation(hdmi));
>>  }
>>    static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
>> @@ -1064,6 +1067,9 @@ static void hdmi_video_csc(struct dw_hdmi *hdmi)
>>  	int interpolation = HDMI_CSC_CFG_INTMODE_DISABLE;
>>  	int decimation = 0;
>>  +	if (!hdmi->csc_available)
>> +		return;
>> +
>>  	/* YCC422 interpolation to 444 mode */
>>  	if (is_color_space_interpolation(hdmi))
>>  		interpolation = HDMI_CSC_CFG_INTMODE_CHROMA_INT_FORMULA1;
>> @@ -2665,6 +2671,7 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>  					u32 output_fmt,
>>  					unsigned int *num_input_fmts)
>>  {
>> +	struct dw_hdmi *hdmi = bridge->driver_private;
>>  	u32 *input_fmts;
>>  	unsigned int i = 0;
>>  @@ -2683,62 +2690,81 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>  	/* 8bit */
>>  	case MEDIA_BUS_FMT_RGB888_1X24:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>> +		if (hdmi->csc_available) {
>> +			input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>> +			input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>> +		}
>>  		break;
>>  	case MEDIA_BUS_FMT_YUV8_1X24:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>> +		if (hdmi->csc_available) {
>> +			input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>> +			input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>> +		}
>>  		break;
>>  	case MEDIA_BUS_FMT_UYVY8_1X16:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>> +		if (hdmi->csc_available) {
>> +			input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>> +			input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>> +		}
>>  		break;
>>    	/* 10bit */
>>  	case MEDIA_BUS_FMT_RGB101010_1X30:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
>> +		if (hdmi->csc_available) {
>> +			input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
>> +			input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
>> +		}
>>  		break;
>>  	case MEDIA_BUS_FMT_YUV10_1X30:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>> +		if (hdmi->csc_available) {
>> +			input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
>> +			input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>> +		}
>>  		break;
>>  	case MEDIA_BUS_FMT_UYVY10_1X20:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>> +		if (hdmi->csc_available) {
>> +			input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
>> +			input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>> +		}
>>  		break;
>>    	/* 12bit */
>>  	case MEDIA_BUS_FMT_RGB121212_1X36:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
>> +		if (hdmi->csc_available) {
>> +			input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
>> +			input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
>> +		}
>>  		break;
>>  	case MEDIA_BUS_FMT_YUV12_1X36:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
>> +		if (hdmi->csc_available) {
>> +			input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
>> +			input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
>> +		}
>>  		break;
>>  	case MEDIA_BUS_FMT_UYVY12_1X24:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
>> +		if (hdmi->csc_available) {
>> +			input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
>> +			input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
>> +		}
>>  		break;
>>    	/* 16bit */
>>  	case MEDIA_BUS_FMT_RGB161616_1X48:
>>  		input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
>> +		if (hdmi->csc_available)
>> +			input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
>>  		break;
>>  	case MEDIA_BUS_FMT_YUV16_1X48:
>> -		input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
>> -		input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
>> +		if (hdmi->csc_available)
>> +			input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
>>  		break;
>>    	/*YUV 4:2:0 */
>> @@ -2767,15 +2793,24 @@ static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
>>  {
>>  	struct dw_hdmi *hdmi = bridge->driver_private;
>>  -	hdmi->hdmi_data.enc_out_bus_format =
>> -			bridge_state->output_bus_cfg.format;
>> +	if (!hdmi->csc_available &&
>> +	    bridge_state->output_bus_cfg.format != bridge_state->input_bus_cfg.format) {
>> +		dev_warn(hdmi->dev, "different input format 0x%04x & output format 0x%04x while CSC isn't usable, fallback to safe format\n",
>> +			 bridge_state->input_bus_cfg.format,
>> +			 bridge_state->output_bus_cfg.format);
>> +		hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_FIXED;
>> +		hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_FIXED;
>> +	} else {
>> +		hdmi->hdmi_data.enc_out_bus_format =
>> +				bridge_state->output_bus_cfg.format;
>>  -	hdmi->hdmi_data.enc_in_bus_format =
>> -			bridge_state->input_bus_cfg.format;
>> +		hdmi->hdmi_data.enc_in_bus_format =
>> +				bridge_state->input_bus_cfg.format;
>>  -	dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
>> -		bridge_state->input_bus_cfg.format,
>> -		bridge_state->output_bus_cfg.format);
>> +		dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
>> +			bridge_state->input_bus_cfg.format,
>> +			bridge_state->output_bus_cfg.format);
>> +	}
>>    	return 0;
>>  }
>> @@ -3481,6 +3516,9 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>>  		hdmi->cec = platform_device_register_full(&pdevinfo);
>>  	}
>>  +	/* Get CSC useability from config0 register and permit override for platforms */
>> +	hdmi->csc_available = !plat_data->disable_csc || (config0 & HDMI_CONFIG0_CSC);
>> +
>>  	drm_bridge_add(&hdmi->bridge);
>>    	return hdmi;
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> index 1999db05bc3b2..279722e4d1898 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> @@ -541,6 +541,7 @@ enum {
>>    /* CONFIG0_ID field values */
>>  	HDMI_CONFIG0_I2S = 0x10,
>> +	HDMI_CONFIG0_CSC = 0x04,
>>  	HDMI_CONFIG0_CEC = 0x02,
>>    /* CONFIG1_ID field values */
>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>> index 2a1f85f9a8a3f..b2f689cbe864c 100644
>> --- a/include/drm/bridge/dw_hdmi.h
>> +++ b/include/drm/bridge/dw_hdmi.h
>> @@ -157,6 +157,7 @@ struct dw_hdmi_plat_data {
>>  			     unsigned long mpixelclock);
>>    	unsigned int disable_cec : 1;
>> +	unsigned int disable_csc : 1;
>>  };
>>    struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
> 
> Is this really still needed now you filter correctly the possible
> modes in patch 1 ?

I had not tried to remove them because they were needed in [PATCH v16]
but indeed they are no longer needed. Something (which I personally
don't understand) may have blocked it so far, but it is not worth
further analyses.

So we can shrink the series and no need to touch drm/bridge: dw-hdmi:
any more!

I'll now post a new v18.

BR and thanks for review,
Nikolaus





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux