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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
