Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845

[Jordan Crouse]

On Tue, Feb 13, 2018 at 02:18:13PM -0500, Sean Paul wrote:
> Hi dri-devel,
> Qualcomm has been working for the past few weeks on forward porting their
> downstream drm driver from 4.14 to mainline. Please consider this PR as a
> request for review, rather than an attempt at mainlining the code as it
> currently stands. The goal is get this driver in shape over the next coming
> months.
> 
> In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
> will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things look
> good, I'll send another pull 4realz.
> 
> To get the ball rolling, I've done some review on the new connector code, my
> comments are below.
> 
> Thanks in advance for your constructive feedback :)
> 
> Sean
> 
> [1]- git://people.freedesktop.org/~seanpaul/dpu-staging
> 
> Review feedback:
> 
> - Solve the splash screen handling (or remove it)
> - Simplify devicetree binding (remove register offsets)
> feedback from reviewing sde_connector.c:
> - Rationalize backlight implementation in sde_connector (display_count static)
> - Sort out the dsi event passing between dsi/encoder/connector (move to 
> encoder)
> - include/uapi/drm/msm_drm_pp.h needs opensource userspace (or removal)
> - connector->state access violations reading/writing mode_info
> - s/sde_rect/drm_rect/
> - sde_kms_info usage needs to be replaced with formal data structures (not 
>   stringified keypairs)
> - sde_connector_ops needs to be trimmed, duplicates connector helpers, info
>   hooks circumvent state, and other hooks should be stored in state or
>   prepopulated (get_dst_format)
> - sde_connector_get_dpms unused
> - esd status check should migrate to encoder from connector
> - backlight should be handled in panel drivers, not in the generic 
> connector/dsi
>   encoder
> - sde_connector_helper_bridge_disable is called from encoder and calls back 
> into
>   set_power encoder function. if backlight, and esd status are removed,
>   pre_kickoff can probably go away
> - sde_connector_clk_ctrl is another example of encoder->connector->encoder 
> call
> - RETIRE_FENCE connector property should be removed, opting for the native
>   atomic fences
> - ROI (regions of interest) should be expressed per-plane instead of 
> connector.
>   there is work ongoing to support dirty_rects per-plane by Deepak Singh Rawat
>    and Lukasz Spintzyk 
> - Uma Shankar  has proposed HDR source metadata
>   properties on the list, we should pivot to those instead of hand-rolling 
> them
>   in the sde driver
> - Convert HDCP implementation to upstream Content Protection property
> - Merge dsi and dsi_staging into one driver
> - Writeback connector has been proposed by ARM (Liviu Dudau and Brian 
> Starkey),
>   we should work with their proposal instead of rolling OUT_FB ourselves
> - sde_connector_set_property should be replaced with atomic helper
> - dsi hotplug can probably be punted to the panel driver
> - dpms should switch to enable/disable (or at least use the atomic helpers)
> - dsi mode handling should also defer to the panel driver
> - SDE_WB_CONFIG ioctl should be removed in favor of the existing ioctl to add
>   user-defined modes
> - dp implementation should use the existing dp helpers wherever possible
> - lots of duplicated structures in dsi_defs.h that can be replaced with 
> existing
>   drm structs
> - mode_valid should be split up and implemented directly in connector/encoder 
> as
>   appropriate
> - sde_connector->aspace seems like it's unused?
> 
> 
> The following changes since commit 9afe236df559d0dc6818f64e728a3f931a0a2231:
> 
>   drm/msm/dsi: Fix potential NULL pointer dereference in msm_dsi_modeset_init 
> (2018-02-12 10:25:15 -0500)
> 
> are available in the Git repository at:
> 
>   git://people.freedesktop.org/~seanpaul/dpu-staging for-next-compiles
> 
> for you to fetch changes up to 672005da148f82021a62d4fa658728e19f13097e:
> 
>   ARM: dts: msm: add device tree changes for SDM845 (2018-02-13 13:14:43 
> -0500)
> 
> 
> Jeykumar Sankaran (9):
>   dt-bindings: msm/dsi: Add mdp transfer time to msm dsi binding
>   dt-bindings: msm/disp: Add bindings for Snapdragon 845 DPU
>   drm: Core changes
>   drm/msm: add DPU DRM driver to support SDM845
>   drm/msm: Change mdp_get_format arguments
>   drm/msm: Core msm changes
>   drm/msm: Add DSI Staging driver
>   drm/msm: Add DisplayPort support
>   ARM: dts: msm: add device tree changes for SDM845
> 
> Manasi Navare (1):
>   drm/dp: Add HBR3 support in existing DRM DP helpers
> 
> Rob Clark (1):
>   drm/msm: rename mdp->disp
> 
>  drivers/gpu/drm/msm/dpu_io_util.c  |  507 ++
>  include/linux/dpu_io_util.h|  113 +

This looks like it was borrowed from some older downstream code - its mostly a
wrapper for iomem and 

Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845

[Rob Clark]

On Tue, Feb 13, 2018 at 7:02 PM, Jordan Crouse  wrote:
> On Tue, Feb 13, 2018 at 02:18:13PM -0500, Sean Paul wrote:
>> Hi dri-devel,
>> Qualcomm has been working for the past few weeks on forward porting their
>> downstream drm driver from 4.14 to mainline. Please consider this PR as a
>> request for review, rather than an attempt at mainlining the code as it
>> currently stands. The goal is get this driver in shape over the next coming
>> months.
>>
>> In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
>> will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things 
>> look
>> good, I'll send another pull 4realz.
>>
>>  drivers/gpu/drm/msm/msm_rd.c   |   58 +-
>
> These changes are somewhat overzealous pointer verification, If
> struct file * and/or struct inode * are NULL in a fops function your system is
> already having a bad day. The only check that might be somewhat reasonable is
> for gpu->funcs->get_param but that hook is defined on all targets so I don't
> think it is unreasonable to assume that it should be there (and if it isn't 
> the
> unit test sure better catch it). We can safely drop these changes.
>

thanks, this was something I didn't notice (or test) yet.. a couple of
the other 'elfring fixes' I encountered (I guess there are probably
some coding standards or static analysis behind this?) actually broke
things and I have already reverted in the course of unbreaking
display/gpu on db410c..

But yeah, ->get_param() is mandatory in the ioctl path (which is
something you don't need debugfs access for).. for the paranoid
probably the better thing would be a WARN_ON(!gpu->get_param) in gpu
init path.

BR,
-R
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845

[Jordan Crouse]

On Tue, Feb 13, 2018 at 02:18:13PM -0500, Sean Paul wrote:
> Hi dri-devel,
> Qualcomm has been working for the past few weeks on forward porting their
> downstream drm driver from 4.14 to mainline. Please consider this PR as a
> request for review, rather than an attempt at mainlining the code as it
> currently stands. The goal is get this driver in shape over the next coming
> months.
> 
> In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
> will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things look
> good, I'll send another pull 4realz.
> 
>  drivers/gpu/drm/msm/msm_rd.c   |   58 +-

These changes are somewhat overzealous pointer verification, If
struct file * and/or struct inode * are NULL in a fops function your system is
already having a bad day. The only check that might be somewhat reasonable is
for gpu->funcs->get_param but that hook is defined on all targets so I don't
think it is unreasonable to assume that it should be there (and if it isn't the
unit test sure better catch it). We can safely drop these changes.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel