Re: [PATCH 08/24] drm/i915: Prepare to split crtc state in uapi and hw state

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 08/24] drm/i915: Prepare to split crtc state in uapi and hw state
Date: Mon, 14 Oct 2019 10:20:53 +0200	[thread overview]
Message-ID: <3d425b1f-4cd1-3a3b-2140-3255ae2b0385@linux.intel.com> (raw)
In-Reply-To: <20191010144700.GG1208@intel.com>

Op 10-10-2019 om 16:47 schreef Ville Syrjälä:
> On Thu, Oct 10, 2019 at 04:21:00PM +0200, Maarten Lankhorst wrote:
>> Op 08-10-2019 om 19:06 schreef Ville Syrjälä:
>>> On Fri, Oct 04, 2019 at 01:34:58PM +0200, Maarten Lankhorst wrote:
>>>> We want to split drm_crtc_state into the user visible state
>>>> and actual hardware state. To prepare for this, we need some
>>>> ground rules what should be in each state:
>>>>
>>>> In uapi we use:
>>>> - crtc, *_changed flags, event, commit, state, mode_blob,
>>>>   (plane/connector/encoder)_mask.
>>>>
>>>> In hw state we use what's displayed in hardware:
>>>> - enable, active, (adjusted) mode, color property blobs.
>>>>
>>>> clear_intel_crtc_state and hw readout need to be updated for these rules,
>>>> which will allow us to enable 2 joined pipes.
>>> I still have hard time with reading this patch. I still think it
>>> would be easier to read if we didn't do both the "uapi" and "hw" changes
>>> at the same time.
>>>
>>> step 1.
>>> 	struct drm_crtc_state uapi;
>>> 	struct {
>>> 		// hw state
>>> 	} base;
>>>
>>> step 2. 
>>> 	s/base/hw/
>>>
>>> I think that would make it more obvious which parts of the code are
>>> looking at which state.
>> It wouldn't I think, but here's
>> a dumb change with spatch on this patch.
>>
>> //+       struct {
>> //+               bool active, enable;
>> //+               struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
>> //+               struct drm_display_mode mode, adjusted_mode;
>> //+       } hw;
>>
>> @@
>> struct intel_crtc_state *T;
>> @@
>> -T->uapi.active
>> +T->hw.active
> This doesn't really help me. There is no .uapi in upstream
> code. I would like to see just the .base->.uapi changes
> alone first so I can review which parts start to look at
> the uapi state to make sure we aren't changing too much.
> Then I'd like to to see the .base->.hw changes so that I
> convince myself we didn't miss anything in the .base->.uapi
> conversion.
>
> And all the remaining drm_crtc_state usage is going to
> make us miss something for sure, so getting rid of all that
> first would probably help.

Hey,

You are correct that there is no upstream use for uapi, but it's simply
called 'base', so it would be just a big rename patch.

For !bigjoiner, the hw and uapi state are aliases. So
for example sdvo/tv it doesn't matter that drm_crtc_state is used.

The spatch I made shows that only intel_get_load_detect_pipe and color readout
use the uapi members instead of the hw members, and there are good reasons to do so.
All other instances all use hw.

As far as I can tell, even without patch 9/24 it will work
correctly, because in intel_initial_commit() atomic_check will pull
in the slave crtc, intel_dp_mst_atomic_check() and intel_psr_fastset_force()
are only called for the master crtc.

Manual verification on the remaining users of drm_crtc_state show that there
is no issue that drm_crtc_state is used. They could be fixed but would never
be affected by bigjoiner.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx