mbox series

[0/3] i915 private writeback framework

Message ID 20220502054219.2083162-1-suraj.kandpal@intel.com (mailing list archive)
Headers show
Series i915 private writeback framework | expand

Message

Kandpal, Suraj May 2, 2022, 5:42 a.m. UTC
A patch series was floated in the drm mailing list which aimed to change
the drm_connector and drm_encoder fields to pointer in the
drm_connector_writeback structure, this received a huge pushback from
the community but since i915 expects each connector present in the
drm_device list to be a intel_connector but drm_writeback framework.
[1] https://patchwork.kernel.org/project/dri-devel/patch/20220202081702.22119-1-suraj.kandpal@intel.com/
[2] https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-suraj.kandpal@intel.com/
This forces us to use a drm_connector which is not embedded in
intel_connector the current drm_writeback framework becomes very
unfeasible to us as it would mean a lot of checks at a lot of places
to take into account the above issue.Since no one had an issue with
encoder field being changed into a pointer it was decided to break the
connector and encoder pointer changes into two different series.The
encoder field changes is currently being worked upon by Abhinav Kumar
[3]https://patchwork.kernel.org/project/dri-devel/list/?series=633565
In the meantime for i915 to start using the writeback functionality we
came up with a interim solution to own writeback pipeline bypassing one
provided by drm which is what these patches do.
Note: these are temp patches till we figure out how we can either change
drm core writeback to work with our intel_connector structure or find a
different solution which allows us to work with the current

Suraj Kandpal (3):
  drm/i915: Creating writeback pipeline to bypass drm_writeback
    framework
  drm/i915: Define WD trancoder for i915
  drm/i915: Enabling WD Transcoder

 drivers/gpu/drm/i915/Makefile                 |   2 +
 drivers/gpu/drm/i915/display/intel_acpi.c     |   1 +
 drivers/gpu/drm/i915/display/intel_display.c  |  89 +-
 drivers/gpu/drm/i915/display/intel_display.h  |  15 +
 .../drm/i915/display/intel_display_types.h    |  18 +
 drivers/gpu/drm/i915/display/intel_dpll.c     |   3 +
 drivers/gpu/drm/i915/display/intel_opregion.c |   3 +
 .../gpu/drm/i915/display/intel_wb_connector.c | 296 ++++++
 .../gpu/drm/i915/display/intel_wb_connector.h |  99 ++
 drivers/gpu/drm/i915/display/intel_wd.c       | 978 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_wd.h       |  82 ++
 drivers/gpu/drm/i915/i915_drv.h               |   5 +
 drivers/gpu/drm/i915/i915_irq.c               |   8 +-
 drivers/gpu/drm/i915/i915_pci.c               |   7 +-
 drivers/gpu/drm/i915/i915_reg.h               | 139 +++
 15 files changed, 1742 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_wb_connector.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_wb_connector.h
 create mode 100644 drivers/gpu/drm/i915/display/intel_wd.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_wd.h

Comments

Daniel Vetter May 4, 2022, 9:43 a.m. UTC | #1
On Mon, May 02, 2022 at 11:12:16AM +0530, Suraj Kandpal wrote:
> A patch series was floated in the drm mailing list which aimed to change
> the drm_connector and drm_encoder fields to pointer in the
> drm_connector_writeback structure, this received a huge pushback from
> the community but since i915 expects each connector present in the
> drm_device list to be a intel_connector but drm_writeback framework.
> [1] https://patchwork.kernel.org/project/dri-devel/patch/20220202081702.22119-1-suraj.kandpal@intel.com/
> [2] https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-suraj.kandpal@intel.com/
> This forces us to use a drm_connector which is not embedded in
> intel_connector the current drm_writeback framework becomes very
> unfeasible to us as it would mean a lot of checks at a lot of places
> to take into account the above issue.Since no one had an issue with
> encoder field being changed into a pointer it was decided to break the
> connector and encoder pointer changes into two different series.The
> encoder field changes is currently being worked upon by Abhinav Kumar
> [3]https://patchwork.kernel.org/project/dri-devel/list/?series=633565
> In the meantime for i915 to start using the writeback functionality we
> came up with a interim solution to own writeback pipeline bypassing one
> provided by drm which is what these patches do.
> Note: these are temp patches till we figure out how we can either change
> drm core writeback to work with our intel_connector structure or find a
> different solution which allows us to work with the current

I'm assuming this is just fyi to keep development moving and not being
planned for merging?
-Daniel

> 
> Suraj Kandpal (3):
>   drm/i915: Creating writeback pipeline to bypass drm_writeback
>     framework
>   drm/i915: Define WD trancoder for i915
>   drm/i915: Enabling WD Transcoder
> 
>  drivers/gpu/drm/i915/Makefile                 |   2 +
>  drivers/gpu/drm/i915/display/intel_acpi.c     |   1 +
>  drivers/gpu/drm/i915/display/intel_display.c  |  89 +-
>  drivers/gpu/drm/i915/display/intel_display.h  |  15 +
>  .../drm/i915/display/intel_display_types.h    |  18 +
>  drivers/gpu/drm/i915/display/intel_dpll.c     |   3 +
>  drivers/gpu/drm/i915/display/intel_opregion.c |   3 +
>  .../gpu/drm/i915/display/intel_wb_connector.c | 296 ++++++
>  .../gpu/drm/i915/display/intel_wb_connector.h |  99 ++
>  drivers/gpu/drm/i915/display/intel_wd.c       | 978 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_wd.h       |  82 ++
>  drivers/gpu/drm/i915/i915_drv.h               |   5 +
>  drivers/gpu/drm/i915/i915_irq.c               |   8 +-
>  drivers/gpu/drm/i915/i915_pci.c               |   7 +-
>  drivers/gpu/drm/i915/i915_reg.h               | 139 +++
>  15 files changed, 1742 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_wb_connector.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_wb_connector.h
>  create mode 100644 drivers/gpu/drm/i915/display/intel_wd.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_wd.h
> 
> -- 
> 2.35.1
>
Kandpal, Suraj May 4, 2022, 9:52 a.m. UTC | #2
Hi Daniel,

> > A patch series was floated in the drm mailing list which aimed to
> > change the drm_connector and drm_encoder fields to pointer in the
> > drm_connector_writeback structure, this received a huge pushback from
> > the community but since i915 expects each connector present in the
> > drm_device list to be a intel_connector but drm_writeback framework.
> > [1]
> > https://patchwork.kernel.org/project/dri-devel/patch/20220202081702.22
> > 119-1-suraj.kandpal@intel.com/ [2]
> > https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22
> > 261-6-suraj.kandpal@intel.com/ This forces us to use a drm_connector
> > which is not embedded in intel_connector the current drm_writeback
> > framework becomes very unfeasible to us as it would mean a lot of
> > checks at a lot of places to take into account the above issue.Since
> > no one had an issue with encoder field being changed into a pointer it
> > was decided to break the connector and encoder pointer changes into
> > two different series.The encoder field changes is currently being
> > worked upon by Abhinav Kumar
> > [3]https://patchwork.kernel.org/project/dri-devel/list/?series=633565
> > In the meantime for i915 to start using the writeback functionality we
> > came up with a interim solution to own writeback pipeline bypassing
> > one provided by drm which is what these patches do.
> > Note: these are temp patches till we figure out how we can either
> > change drm core writeback to work with our intel_connector structure
> > or find a different solution which allows us to work with the current
> 
> I'm assuming this is just fyi to keep development moving and not being
> planned for merging?
Yes we do plan to get it merged as a proper implementation that uses drm-core
will require significant time and to unblock the writeback functionality these interim
series of patches have been floated.

Regards
Suraj Kandpal
Daniel Vetter May 4, 2022, 9:57 a.m. UTC | #3
On Wed, May 04, 2022 at 09:52:34AM +0000, Kandpal, Suraj wrote:
> Hi Daniel,
> 
> > > A patch series was floated in the drm mailing list which aimed to
> > > change the drm_connector and drm_encoder fields to pointer in the
> > > drm_connector_writeback structure, this received a huge pushback from
> > > the community but since i915 expects each connector present in the
> > > drm_device list to be a intel_connector but drm_writeback framework.
> > > [1]
> > > https://patchwork.kernel.org/project/dri-devel/patch/20220202081702.22
> > > 119-1-suraj.kandpal@intel.com/ [2]
> > > https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22
> > > 261-6-suraj.kandpal@intel.com/ This forces us to use a drm_connector
> > > which is not embedded in intel_connector the current drm_writeback
> > > framework becomes very unfeasible to us as it would mean a lot of
> > > checks at a lot of places to take into account the above issue.Since
> > > no one had an issue with encoder field being changed into a pointer it
> > > was decided to break the connector and encoder pointer changes into
> > > two different series.The encoder field changes is currently being
> > > worked upon by Abhinav Kumar
> > > [3]https://patchwork.kernel.org/project/dri-devel/list/?series=633565
> > > In the meantime for i915 to start using the writeback functionality we
> > > came up with a interim solution to own writeback pipeline bypassing
> > > one provided by drm which is what these patches do.
> > > Note: these are temp patches till we figure out how we can either
> > > change drm core writeback to work with our intel_connector structure
> > > or find a different solution which allows us to work with the current
> > 
> > I'm assuming this is just fyi to keep development moving and not being
> > planned for merging?
> Yes we do plan to get it merged as a proper implementation that uses drm-core
> will require significant time and to unblock the writeback functionality these interim
> series of patches have been floated.

No, I really don't think merging some interim hack is the way to go.

It's not display, but on the gem side there was this huge exception about
"hey we need to merge guc scheduler and it's totally ready but it's not
done properly like it should have been for upstream". And surprise it
wasn't ready and took a year just to get the hack job complete, and now no
one seems to be working on doing things properly. So I'm not eager at all
to ack more hack jobs for i915.

Do it right before merging to upstream please.
-Daniel