Re: [PATCH v4] drm/i915: Reorganize display pipe register accesses

From: "Antti Koskipää" <antti.koskipaa@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v4] drm/i915: Reorganize display pipe register accesses
Date: Tue, 28 Jan 2014 15:53:30 +0200	[thread overview]
Message-ID: <52E7B65A.7050407@linux.intel.com> (raw)
In-Reply-To: <20140127133158.GS9454@intel.com>

On 01/27/14 15:31, Ville Syrjälä wrote:
> On Mon, Jan 27, 2014 at 03:09:34PM +0200, Antti Koskipaa wrote:
>> +	.dpll_offsets = { DPLL_A_OFFSET, DPLL_B_OFFSET }, \
>> +	.dpll_md_offsets = { DPLL_A_MD_OFFSET, DPLL_B_MD_OFFSET }, \
>> +	.palette_offsets = { PALETTE_A_OFFSET, PALETTE_B_OFFSET }, \
> 
> Please drop the last comma here ...
> 
>> +
>> +
>>  static const struct intel_device_info intel_i830_info = {
>>  	.gen = 2, .is_mobile = 1, .cursor_needs_physical = 1, .num_pipes = 2,
>>  	.has_overlay = 1, .overlay_needs_physical = 1,
>>  	.ring_mask = RENDER_RING,
>> +	GEN_DEFAULT_PIPEOFFSETS
> 
> ... and place it here
> 

Done.

>> +/*
>> + * There's actually no pipe EDP. Some pipe registers have
>> + * simply shifted from the pipe to the transcoder, while
>> + * keeping their original offset. Thus we need PIPE_EDP_OFFSET
>> + * to access such registers in transcoder EDP.
>> + */
>> +#define PIPE_EDP_OFFSET	0x7f000
> 
> I just realized that you're not using this actually. But I think we need
> to use it to make *future* stuff work.
> 
> For the same reason PIPECONF() needs to use _PIPE2() macro, BCLRPAT needs
> to use _TRANSCODER2() macro. Ie. the choice of which we use needs to be
> based strictly on the offset range where the register lives. 
>  0x6xxxx -> _TRANSCODER2()
>  0x7xxxx -> _PIPE2()
> 
> PIPESRC() seems to be where it needs to be, ie. using _TRANCODER2().

Read it again. The old code has PIPECONF and BCLRPAT using the wrong
macro. This patch fixes it.

> So this also means we need the pipe_offsets[] array to have space for
> PIPE_EDP_OFFSET.

Done.

-- 
- Antti