| Message ID | 1555977388-14203-2-git-send-email-sunpeng.li@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/dp: Use non-cyclic idr | expand |
On Mon, Apr 22, 2019 at 07:56:27PM -0400, sunpeng.li@amd.com wrote: > From: Leo Li <sunpeng.li@amd.com> > > To give identifiable attributes to MST DP aux devices, we can use the > MST relative address. Expose this function for later use. > > Signed-off-by: Leo Li <sunpeng.li@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- > include/drm/drm_dp_mst_helper.h | 4 ++++ > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 2ab16c9..86ff8e2 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8 *guid) > } > } > > -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb, > +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb, > int pnum, > char *proppath, > size_t proppath_size) I was wondering if we need to export this but looks like both drm_dp_mst_topology.o and drm_dp_aux_dev.o both end up in drm_kms_helper.ko, so the answer is no. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, > if (created && !port->input) { > char proppath[255]; > > - build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); > + drm_dp_build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); > port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, port, proppath); > if (!port->connector) { > /* remove it from the port list */ > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index 371cc28..81c8d79 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, > int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, > int pbn); > > +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb, > + int pnum, > + char *proppath, > + size_t proppath_size); > > int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); > > -- > 2.7.4
Closer, but are we sure we want to use the MST prop path for this? Why not add a sysfs attribute with the corresponding DRM connector name instead since the connector itself will have a path property. That way we can associate aux devices for eDP and DP devices with their corresponding connectors as well On Mon, 2019-04-22 at 19:56 -0400, sunpeng.li@amd.com wrote: > From: Leo Li <sunpeng.li@amd.com> > > To give identifiable attributes to MST DP aux devices, we can use the > MST relative address. Expose this function for later use. > > Signed-off-by: Leo Li <sunpeng.li@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- > include/drm/drm_dp_mst_helper.h | 4 ++++ > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 2ab16c9..86ff8e2 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct > drm_dp_mst_branch *mstb, u8 *guid) > } > } > > -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb, > +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb, > int pnum, > char *proppath, > size_t proppath_size) > @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch > *mstb, > if (created && !port->input) { > char proppath[255]; > > - build_mst_prop_path(mstb, port->port_num, proppath, > sizeof(proppath)); > + drm_dp_build_mst_prop_path(mstb, port->port_num, proppath, > sizeof(proppath)); > port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, > port, proppath); > if (!port->connector) { > /* remove it from the port list */ > diff --git a/include/drm/drm_dp_mst_helper.h > b/include/drm/drm_dp_mst_helper.h > index 371cc28..81c8d79 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct > drm_dp_mst_topology_mgr *mgr, > int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, > int pbn); > > +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb, > + int pnum, > + char *proppath, > + size_t proppath_size); > > int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); >
On 2019-04-24 1:26 p.m., Lyude Paul wrote: > Closer, but are we sure we want to use the MST prop path for this? Why not add > a sysfs attribute with the corresponding DRM connector name instead since the > connector itself will have a path property. That way we can associate aux > devices for eDP and DP devices with their corresponding connectors as well I thought about that as well, but I hit a wall when trying to get the SST connector from the aux device. Perhaps there's a simpler way that I'm overlooking? It's easier for MST, since the mst_port can be obtained via container_of dp_aux. port->connector would then give what we want. For SST though, each driver calls drm_aux_register() with an aux struct that they've initialized. I'm not sure how I can reliably get the drm_connector from that. Maybe drm_dp_aux should have a back reference to the connector? FWICT all drivers using drm_aux_register() should be able to provide the associated connector when calling it. Leo > > On Mon, 2019-04-22 at 19:56 -0400, sunpeng.li@amd.com wrote: >> From: Leo Li <sunpeng.li@amd.com> >> >> To give identifiable attributes to MST DP aux devices, we can use the >> MST relative address. Expose this function for later use. >> >> Signed-off-by: Leo Li <sunpeng.li@amd.com> >> --- >> drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- >> include/drm/drm_dp_mst_helper.h | 4 ++++ >> 2 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c >> b/drivers/gpu/drm/drm_dp_mst_topology.c >> index 2ab16c9..86ff8e2 100644 >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c >> @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct >> drm_dp_mst_branch *mstb, u8 *guid) >> } >> } >> >> -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb, >> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb, >> int pnum, >> char *proppath, >> size_t proppath_size) >> @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch >> *mstb, >> if (created && !port->input) { >> char proppath[255]; >> >> - build_mst_prop_path(mstb, port->port_num, proppath, >> sizeof(proppath)); >> + drm_dp_build_mst_prop_path(mstb, port->port_num, proppath, >> sizeof(proppath)); >> port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, >> port, proppath); >> if (!port->connector) { >> /* remove it from the port list */ >> diff --git a/include/drm/drm_dp_mst_helper.h >> b/include/drm/drm_dp_mst_helper.h >> index 371cc28..81c8d79 100644 >> --- a/include/drm/drm_dp_mst_helper.h >> +++ b/include/drm/drm_dp_mst_helper.h >> @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct >> drm_dp_mst_topology_mgr *mgr, >> int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, >> int pbn); >> >> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb, >> + int pnum, >> + char *proppath, >> + size_t proppath_size); >> >> int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); >>
On Wed, Apr 24, 2019 at 08:40:30PM +0000, Li, Sun peng (Leo) wrote: > > > > On 2019-04-24 1:26 p.m., Lyude Paul wrote: > > Closer, but are we sure we want to use the MST prop path for this? Why not add > > a sysfs attribute with the corresponding DRM connector name instead since the > > connector itself will have a path property. That way we can associate aux > > devices for eDP and DP devices with their corresponding connectors as well > > I thought about that as well, but I hit a wall when trying to get the > SST connector from the aux device. Perhaps there's a simpler way that > I'm overlooking? > > It's easier for MST, since the mst_port can be obtained via container_of > dp_aux. port->connector would then give what we want. > > For SST though, each driver calls drm_aux_register() with an aux struct > that they've initialized. I'm not sure how I can reliably get the > drm_connector from that. On i915 the aux is a child of the connector, so no extra attributes/links needed. Maybe other drivers should/could follow that apporach as well?
On Wed, 2019-04-24 at 23:52 +0300, Ville Syrjälä wrote: > On Wed, Apr 24, 2019 at 08:40:30PM +0000, Li, Sun peng (Leo) wrote: > > > > > > On 2019-04-24 1:26 p.m., Lyude Paul wrote: > > > Closer, but are we sure we want to use the MST prop path for this? Why > > > not add > > > a sysfs attribute with the corresponding DRM connector name instead > > > since the > > > connector itself will have a path property. That way we can associate > > > aux > > > devices for eDP and DP devices with their corresponding connectors as > > > well > > > > I thought about that as well, but I hit a wall when trying to get the > > SST connector from the aux device. Perhaps there's a simpler way that > > I'm overlooking? > > > > It's easier for MST, since the mst_port can be obtained via container_of > > dp_aux. port->connector would then give what we want. > > > > For SST though, each driver calls drm_aux_register() with an aux struct > > that they've initialized. I'm not sure how I can reliably get the > > drm_connector from that. > > On i915 the aux is a child of the connector, so no extra > attributes/links needed. Maybe other drivers should/could > follow that apporach as well? ooo, good point. Yeah that seems like it would be worth a shot since it'd be a little nicer then just adding more sysfs attributes. But otherwise if that doesn't work, adding a connector parameter to drm_dp_aux_register() should be fine. >
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 2ab16c9..86ff8e2 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8 *guid) } } -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb, +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb, int pnum, char *proppath, size_t proppath_size) @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, if (created && !port->input) { char proppath[255]; - build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); + drm_dp_build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, port, proppath); if (!port->connector) { /* remove it from the port list */ diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 371cc28..81c8d79 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, int pbn); +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb, + int pnum, + char *proppath, + size_t proppath_size); int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);