Re: [PATCH] drm: HDMI pixel replication modes now hactive of 720 for pixel replication

Re: [PATCH] drm: HDMI pixel replication modes now hactive of 720 for pixel replication

[Ville Syrjälä]

On Tue, Jul 29, 2014 at 02:58:23PM -0700, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> CEA SD interlaced modes use a horizontal 720 pixels that are pixel replicated to 1440. The current driver reports 1440 pixel to the OS and does not set pixel replicated modes.
> 
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c        |   68 ++++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_hdmi.c |   13 +++++++
>  2 files changed, 47 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index dfa9769..5233f4c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -632,26 +632,26 @@ static const struct drm_display_mode edid_cea_modes[] = {
>  		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC |
>  			DRM_MODE_FLAG_INTERLACE),
>  	  .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },
> -	/* 6 - 1440x480i@60Hz */
> -	{ DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
> +	/* 6 - 720(1440)x480i@60Hz */
> +	{ DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 27000, 720, 1478,

I think the best thing here might be to halve all the horizontal
timings (and the clock), and maybe do it with an explicit /2 to
remind people that this is a bit special, and also the spec lists
the doubled timings, so it's maybe a bit easier to compare with the
spec that way.

So perhaps something like this:
DRM_MODE(..., 27000/2, 1440/2, 1478/2, ...

<snip>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 2422413..f8cdf7f 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -918,6 +918,19 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  			intel_hdmi->color_range = 0;
>  	}
>  
> +	/* Adjust pipe timings for pixel doubled modes */
> +	if ((adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)) {
> +		adjusted_mode->hsync_start /= 2;
> +		adjusted_mode->hsync_end /= 2;
> +		adjusted_mode->htotal /= 2;
> +
> +		drm_mode_set_crtcinfo(adjusted_mode, 0);
> +
> +		/* Set 2x pixel double on pipe */
> +		pipe_config->pixel_multiplier = 2;
> +		pipe_config->port_clock = adjusted_mode->crtc_clock;
> +	}

If you halve the timings already in drm_edid.c all of this would be
reduced to just:

if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
	pipe_config->pixel_multiplier = 2;


> +
>  	if (intel_hdmi->color_range)
>  		pipe_config->limited_color_range = true;
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: HDMI pixel replication modes now hactive of 720 for pixel replication

[Clint Taylor]

On 08/12/2014 04:07 AM, Ville Syrjälä wrote:
> On Tue, Jul 29, 2014 at 02:58:23PM -0700, clinton.a.taylor@intel.com wrote:
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> CEA SD interlaced modes use a horizontal 720 pixels that are pixel replicated to 1440. The current driver reports 1440 pixel to the OS and does not set pixel replicated modes.
>>
>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c        |   68 ++++++++++++++++++-------------------
>>   drivers/gpu/drm/i915/intel_hdmi.c |   13 +++++++
>>   2 files changed, 47 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index dfa9769..5233f4c 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -632,26 +632,26 @@ static const struct drm_display_mode edid_cea_modes[] = {
>>   		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC |
>>   			DRM_MODE_FLAG_INTERLACE),
>>   	  .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },
>> -	/* 6 - 1440x480i@60Hz */
>> -	{ DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
>> +	/* 6 - 720(1440)x480i@60Hz */
>> +	{ DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 27000, 720, 1478,
>
> I think the best thing here might be to halve all the horizontal
> timings (and the clock), and maybe do it with an explicit /2 to
> remind people that this is a bit special, and also the spec lists
> the doubled timings, so it's maybe a bit easier to compare with the
> spec that way.
>
> So perhaps something like this:
> DRM_MODE(..., 27000/2, 1440/2, 1478/2, ...
>
> <snip>
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 2422413..f8cdf7f 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -918,6 +918,19 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   			intel_hdmi->color_range = 0;
>>   	}
>>
>> +	/* Adjust pipe timings for pixel doubled modes */
>> +	if ((adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)) {
>> +		adjusted_mode->hsync_start /= 2;
>> +		adjusted_mode->hsync_end /= 2;
>> +		adjusted_mode->htotal /= 2;
>> +
>> +		drm_mode_set_crtcinfo(adjusted_mode, 0);
>> +
>> +		/* Set 2x pixel double on pipe */
>> +		pipe_config->pixel_multiplier = 2;
>> +		pipe_config->port_clock = adjusted_mode->crtc_clock;
>> +	}
>
> If you halve the timings already in drm_edid.c all of this would be
> reduced to just:
>
> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> 	pipe_config->pixel_multiplier = 2;
>
>
>> +
>>   	if (intel_hdmi->color_range)
>>   		pipe_config->limited_color_range = true;
>>
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Sorry, I missed this comment completely before I submitted the second 
patch to dri-devel. I will be making changes to the second patch only 
based on the feedback so far.

Thanks,
Clint